Hi
2021. január 6., szerda 19:23 keltezéssel, Hans de Goede írta:
> [...]
> > #include <acpi/video.h>
> > +#include <dt-bindings/leds/common.h>
> > #include <linux/acpi.h>
> > #include <linux/backlight.h>
> > #include <linux/bitops.h>
> > @@ -22,6 +23,7 @@
> > #include <linux/input/sparse-keymap.h>
> > #include <linux/jiffies.h>
> > #include <linux/kernel.h>
> > +#include <linux/leds.h>
> > #include <linux/module.h>
> > #include <linux/platform_device.h>
> > #include <linux/rfkill.h>
>
> I guess you need a "depends on LEDS_CLASS" in the Kconfig for this?
>
You're right, I was under the impression for some reason that leds.h defines
stub functions when LED support is disabled, it turns out that's not the case
(at least not for all functions). Interestingly, some other drivers "select"
LEDS_CLASS (e.g. thinkpad_acpi), others "depend" on it (e.g. alienware-wmi)...
I'm unsure about what should be done here.
> [...]
> > +static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
> > +{
> > + int brightness;
> > +
> > + if (!priv->kbd_bl.initialized)
> > + return;
> > +
> > + brightness = ideapad_kbd_bl_brightness_get(priv);
> > + if (brightness < 0)
> > + return;
> > +
> > + if (brightness == priv->kbd_bl.last_brightness)
> > + return;
> > +
> > + priv->kbd_bl.last_brightness = brightness;
> > +
> > + led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
> > +}
>
> So I guess that there is some hotkey combo on the laptops keyboards which
> directly changes the state of the kbd backlight "underneath" us and that
> is why this is necessary ?
>
Yes, more specifically, Fn + space on the machine I tested it.
> > +
> > +static int ideapad_kbd_bl_init(struct ideapad_private *priv)
> > +{
> > + unsigned long hals;
> > + int err;
> > +
> > + err = eval_hals(priv->adev->handle, &hals);
> > + if (err)
> > + return err;
>
> So you are checking for presence of the HALS method here, but not
> for SALS which is being used in ideapad_kbd_bl_led_cdev_brightness_set()
> and you are needlessly re-evaluating HALS here. Would it not be better
> to add a features.kbd_bl flag and set that from ideapad_check_features()
> as done for other features ? That would avoid an extra evaluation of
> the HALS method and also check for SALS.
> [...]
You're right, it would be better.
Regards,
Barnabás Pőcze
