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

[Shem Multinymous]

Hi Stefan,

On 10/23/06, stefan@datenfreihafen.org <stefan@datenfreihafen.org> wrote:
> This patch integrates the led and thinklight feature of ibm-acpi into the LED
> subsystem.

Great! This is very much needed.

I'm concerned about the implementation, though:

> +static void led_battery_green_set(struct led_classdev *led_cdev,
> +                                                               enum led_brightness value)
> +{
> +       if (value)
> +               led_write("2 on,");
> +       else
> +               led_write("2 off,");
> +}

Building up synthetic command strings and then parsing them is not
very appropriate for kernel code. Imagine what it will look like when
the textual proc interface is eventually removed. A better solution is
to provide clean low-level functions with a normal interface (e.g.,
"set_led(int lednum, int ledstate)"), and have both the sysfs and proc
code use those low-level functions.

  Shem