v3 changelog:
- stylistic changes;
- More granular init error handling allows each feature to go on even if
othe features are unavailable.
- Add support for controlling USB charge when off.
On Wed, 26 Sep 2018, Andy Shevchenko wrote:
> Thank you for an update.
> Unfortunately more work is needed.
> My review below.
Thanks for you patience.
> > + switch (r->buffer.pointer[0] & 0x27) {
> > + case 0x24:
> > + val = LED_FULL;
> > + break;
> > + case 0x22:
> > + val = LED_HALF;
> > + break;
> > + default:
> > + val = LED_OFF;
> > + }
>
> Looking into this values it looks like BIT(5) defines on/off (or
> enabled/disabled) state and lowest three bits defines the value of
> brightness.
> Correct?
This is my guess also, but the firmware only writes 0, 0x22 and 0x24. As
I only have one such laptop, and it is quite expensive, I try to avoid
writing values that I do not know to be safe. Anyway, I am not sure what
is the utility of having more granular keyboard backlight brightness
control.
> If so, can it be rewritten taking above into account?
If you mean that this structure can be used to calclate the brightness
using masking and shifting instead of a switch statement, then I do not
think this will result in clearer code.
Similarly, I guess that values other than 80 or 100 might be written to
the battery charge limit, but I don't see a rea; reason to test this.
> > + tpad_led_registered =
> > + !led_classdev_register(&pf_device->dev, &tpad_led);
>
> I would rather suggest to provide one variable where bits would define
> what parts have been registered. Same for WMI part below.
OK.
> > + result = acpi_bus_register_driver(&acpi_driver);
> > + if (result < 0) {
> > + ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n"));
> > + return -ENODEV;
> > + }
> > +
>
> > + wmi_inited = !wmi_input_setup();
>
> Hmm... So, it's not a fatal error if WMI is not initialized?
That's WMI input. If it fails to initialize, the driver may still be
used for controlling the LEDs and the other features.
--
Matan.
