Re: [PATCH 1/3] Input: export LEDs as class devices in sysfs

[Samuel Thibault <samuel.thibault@ens-lyon.org>]

Hello,

Dmitry Torokhov, le Mon 08 Jun 2015 14:43:08 -0700, a écrit :
> 1. Instead of making LED class devices part of the input device they are
> implemented as an input handler (and thus are completely separate from
> input core).

That's nicer indeed.  Not defining triggers per LED however does not
permit to e.g. switch two leds of a keyboard independently of what
produces input events.  I'm personally fine with it, I just wanted to
mention it since this example of usage was cited at some point in the
thread.

> +	[LED_NUML]	= { "num-lock", VT_TRIGGER("kbd-numlock") },
> +	[LED_CAPSL]	= { "caps-lock", VT_TRIGGER("kbd-capslock") },
> +	[LED_SCROLLL]	= { "scroll-lock", VT_TRIGGER("kbd-scrollock") },

I'd tend to think we'd want to harmonize the user-visible LED names and
kbd trigger names, i.e. use "numlock", "capslock" and "scrollock" in
both case (or something else, but the same for LED and trigger). In my
patch I simply used the corresponding LED or kbd macro names, but there
is probably no strong reason to this, while there is probably a good
reason to choose coherent and nice user-visible names.

> +static enum led_brightness input_leds_brightness_get(struct led_classdev *cdev)
> +{
> +	struct input_led *led = container_of(cdev, struct input_led, cdev);
> +	struct input_dev *input = led->handle->dev;
> +
> +	return test_bit(led->code, input->ledbit) ? LED_FULL : LED_OFF;

This always returns LED_FULL, whatever the current state of the LED, is
that really what we want?  Userspace will be surprised to read 255 from
sysfs whatever it writes to it (with actual proper effect on the LED).
Simply not defining input_leds_brightness_get and letting the LED core
manage the value does get a proper "brightness" sysfs file behavior, is
there a reason not to do that?

> +	int led_no = 0;

...

> +	for_each_set_bit(led_code, dev->ledbit, LED_CNT) {
> +		struct input_led *led = &leds->leds[led_no];

When reading this I wondered what value led_no was, perhaps the
initialization to 0 should be moved to right before the for_each_set_bit
loop, to make the code clearer.

Samuel