Messages in this thread | | | Date | Sun, 15 Mar 2020 09:42:30 +0100 | From | Pavel Machek <> | Subject | Re: [PATCH] leds: ariel: Add driver for status LEDs on Dell Wyse 3020 |
| |
Hi!
> This adds support for controlling the LEDs attached to the Embedded > Controller on a Dell Wyse 3020 "Ariel" board. > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Does not look bad.
> +static int ariel_led_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct ariel_led *leds; > + struct regmap *ec_ram; > + int ret; > + > + leds = devm_kcalloc(dev, 3, sizeof(*leds), GFP_KERNEL); > + if (!leds) > + return -ENOMEM; > + > + ec_ram = dev_get_regmap(dev->parent, "ec_ram"); > + if (!ec_ram) > + return -ENODEV; > + > + leds[0].ec_ram = ec_ram; > + leds[0].ec_index = EC_BLUE_LED; > + leds[0].led_cdev.name = "ariel:blue:power", > + leds[0].led_cdev.brightness_get = ariel_led_get; > + leds[0].led_cdev.brightness_set = ariel_led_set; > + leds[0].led_cdev.blink_set = ariel_blink_set; > + leds[0].led_cdev.default_trigger = "default-on";
Move common settings to a loop?
Definitely delete "ariel:" prefix.
> + leds[1].led_cdev.name = "ariel:amber:status", > + leds[2].led_cdev.name = "ariel:green:status",
Do the LEDs have some label, or some kind of common function? Calling it ":status" is not too useful...
> + ret = devm_led_classdev_register(dev, &leds[2].led_cdev); > + if (ret) > + return ret; > + > + dev_info(dev, "Dell Wyse 3020 LEDs\n"); > + return 0; > +}
Just return ret; no need to print anything into the syslog.
Thanks! Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [unhandled content-type:application/pgp-signature] | |