Hi
Thanks for the review.
2021. január 16., szombat 21:07 keltezéssel, Andy Shevchenko írta:
> On Wed, Jan 13, 2021 at 8:25 PM Barnabás Pőcze wrote:
> >
> > On certain models it is possible to control/query the keyboard backlight
> > via the SALS/HALS ACPI methods. Add support for that, and register an LED
> > class device to expose this functionality.
> > Tested on: Lenovo YOGA 520-14IKB 80X8
>
> ...
>
> > + struct {
> > + bool initialized;
> > + struct led_classdev led;
> > + unsigned int last_brightness;
> > + } kbd_bl;
>
> Data structure without sense.
>
It makes a lot of sense to me, it groups the members which are concerned with
keyboard backlight control. Do you have any specific issues with it? And
what would you suggest instead?
> ...
>
> > +static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
> > +{
> > + unsigned long hals;
> > + int err;
> > +
> > + err = eval_hals(priv->adev->handle, &hals);
> > + if (err)
> > + return err;
> > +
> > + return test_bit(HALS_KBD_BL_STATE_BIT, &hals);
>
> On some architectures IIRC it returns long (or unsigned long). Is it a problem?
>
Potentially it is, but since it's an x86 platform driver, I failed to consider
other architectures. Nonetheless, I will fix this in the next version.
> > +}
>
> ...
>
> > +static int ideapad_kbd_bl_init(struct ideapad_private *priv)
> > +{
> > + int err;
> > +
> > + if (!priv->features.kbd_bl)
> > + return -ENODEV;
> > +
> > + err = ideapad_kbd_bl_brightness_get(priv);
>
> > + if (err < 0)
>
> Just to be sure, what positive code means here?
>
Non-zero return value is the brightness, negative return value is an error code.
I realize the name `err` is arguably not be the best, but if I were to rename it to
`brightness`, then the `led_classdev_register()` call would look odd in my
opinion, and I wanted to avoid introducing two variables. But I think I'll do
just that in the next version.
> > + return err;
> > +
> > + priv->kbd_bl.last_brightness = err;
> > +
> > + priv->kbd_bl.led.name = "platform::" LED_FUNCTION_KBD_BACKLIGHT;
> > + priv->kbd_bl.led.max_brightness = 1;
> > + priv->kbd_bl.led.brightness_get = ideapad_kbd_bl_led_cdev_brightness_get;
> > + priv->kbd_bl.led.brightness_set_blocking = ideapad_kbd_bl_led_cdev_brightness_set;
> > + priv->kbd_bl.led.flags = LED_BRIGHT_HW_CHANGED;
> > +
> > + err = led_classdev_register(&priv->platform_device->dev, &priv->kbd_bl.led);
> > + if (err)
> > + return err;
> > +
> > + priv->kbd_bl.initialized = true;
> > +
> > + return 0;
> > +}
> [...]
Thanks,
Barnabás Pőcze
