On Tue, Jun 09, 2015 at 07:22:31PM +0200, Samuel Thibault wrote:
> Dmitry Torokhov, le Tue 09 Jun 2015 09:49:35 -0700, a écrit :
> > > > + [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.
> >
> > I can do either "num_lock - kbd-num-lock" or "numlock - kbd-numlock"
> > with slight preference to the 1st. What is your preference?
>
> I'd prefer numlock - kbd-numlock.
OK.
>
> > > > +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).
> >
> > Right, I will change it to 0 and led->max_brightness (which I will set
> > to 1).
>
> Well, that won't fix the issue I'm having, see below.
>
> > > 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?
> >
> > Yes, we want LED sysfs show correct result if state is altered via
> > EV_LED/LED_* event. Basically the led bit state is the source of truth
> > here.
>
> Ok, I understand that. But it happens that your code does not work! It
> is always 255 (or will be 1 with the modifications you mention above).
> input->ledbit is whether the LED exists or not, not its state, right?
> Did your perhaps mean input->led in input_leds_brightness_get instead of
> input->ledbit?
Yep. I'll change it.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
