[ltp] Re: [Patch 2/3] ibm-acpi: LED Subsystem integration.

[Shem Multinymous]

Hi Stefan,

Nice work on these patches.
A few comments:

On 10/26/06, stefan@datenfreihafen.org <stefan@datenfreihafen.org> wrote:
> +/* led 0 */
> +static void led_power_set(struct led_classdev *led_cdev,
> +                                               enum led_brightness value)
> +{
> +       if (value)
> +               set_led(0, 1);
> +       else
> +               set_led(0, 0);
> +}
> +
> +/* led 1 */
> +static void led_battery_amber_set(struct led_classdev *led_cdev,
> +                                                               enum led_brightness value)
> +{
> +       if (value)
> +               set_led(1, 1);
> +       else
> +               set_led(1, 0);
> +}
> +
> +/* led 2 */
> +static void led_battery_green_set(struct led_classdev *led_cdev,
> +                                                               enum led_brightness value)
> +{
> +       if (value)
> +               set_led(2, 1);
> +       else
> +               set_led(2, 0);
> +}
> +
> +/* led 3 */
> +static void led_ultrabase_set(struct led_classdev *led_cdev,
> +                                                       enum led_brightness value)
> +{
> +       if (value)
> +               set_led(3, 1);
> +       else
> +               set_led(3, 0);
> +}
> +
> +/* led 4 */
> +static void led_ultrabay_set(struct led_classdev *led_cdev,
> +                                                       enum led_brightness value)
> +{
> +       if (value)
> +               set_led(4, 1);
> +       else
> +               set_led(4, 0);
> +}
> +
> +/* led 7 */
> +static void led_standby_set(struct led_classdev *led_cdev,
> +                                                       enum led_brightness value)
> +{
> +       if (value)
> +               set_led(7, 1);
> +       else
> +               set_led(7, 0);
> +}

Use a #define macro for this.


> +static struct led_classdev power_led = {
> +       .name                   = "power:green",
> +       .default_trigger        = "none",
> +       .brightness_set         = led_power_set,
> +};
> +
> +static struct led_classdev battery_amber_led = {
> +       .name                   = "battery:amber",
> +       .default_trigger        = "none",
> +       .brightness_set         = led_battery_amber_set,
> +};
> +
> +static struct led_classdev battery_green_led = {
> +       .name                   = "battery:green",
> +       .default_trigger        = "none",
> +       .brightness_set         = led_battery_green_set,
> +};
> +
> +static struct led_classdev ultrabase_led = {
> +       .name                   = "ultrabase:green",
> +       .default_trigger        = "none",
> +       .brightness_set         = led_ultrabase_set,
> +};
> +
> +static struct led_classdev ultrabay_led = {
> +       .name                   = "ultrabay:green",
> +       .default_trigger        = "none",
> +       .brightness_set         = led_ultrabay_set,
> +};
> +
> +static struct led_classdev standby_led = {
> +       .name                   = "standby:green",
> +       .default_trigger        = "none",
> +       .brightness_set         = led_standby_set,
> +};
> +
> +static struct led_classdev thinklight_led = {
> +       .name                   = "thinklight:white",
> +       .default_trigger        = "none",
> +       .brightness_set         = led_thinklight_set,
> +};

Add this to the above macro too.
There should be exactly one line per LED in all of the above, with the
rest factored out.


> +static int led_probe(struct device *parent)
> +{
> +       int ret;
> +
> +       ret = led_classdev_register(parent, &power_led);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = led_classdev_register(parent, &battery_amber_led);
> +       if (ret < 0)
> +               led_classdev_unregister(&power_led);
> +
> +       ret = led_classdev_register(parent, &battery_green_led);
> +       if (ret < 0) {
> +               led_classdev_unregister(&power_led);
> +               led_classdev_unregister(&battery_amber_led);
> +       }
> +
> +       ret = led_classdev_register(parent, &ultrabase_led);
> +       if (ret < 0) {
> +               led_classdev_unregister(&power_led);
> +               led_classdev_unregister(&battery_amber_led);
> +               led_classdev_unregister(&battery_green_led);
> +       }
> +
> +       ret = led_classdev_register(parent, &ultrabay_led);
> +       if (ret < 0) {
> +               led_classdev_unregister(&power_led);
> +               led_classdev_unregister(&battery_amber_led);
> +               led_classdev_unregister(&battery_green_led);
> +               led_classdev_unregister(&ultrabase_led);
> +       }
> +
> +       ret = led_classdev_register(parent, &standby_led);
> +       if (ret < 0) {
> +               led_classdev_unregister(&power_led);
> +               led_classdev_unregister(&battery_amber_led);
> +               led_classdev_unregister(&battery_green_led);
> +               led_classdev_unregister(&ultrabase_led);
> +               led_classdev_unregister(&ultrabay_led);
> +       }
> +
> +       ret = led_classdev_register(parent, &thinklight_led);
> +       if (ret < 0) {
> +               led_classdev_unregister(&power_led);
> +               led_classdev_unregister(&battery_amber_led);
> +               led_classdev_unregister(&battery_green_led);
> +               led_classdev_unregister(&ultrabase_led);
> +               led_classdev_unregister(&ultrabay_led);
> +               led_classdev_unregister(&standby_led);
> +       }
> +
> +       return ret;
> +}

Don't duplicate the cleanup on error. Instead, put a single cleanup
sequence at the end, with labels for each line, and use goto to jump
to the right point.
See the bottom of hdaps.c (or almost any other driver init code) for example.

  Shem