Hi Darren,
El mar., 12 de jun. de 2018 a la(s) 17:09, Darren Hart
(dvhart@xxxxxxxxxxxxx) escribió:
>
> On Tue, Jun 12, 2018 at 12:49:35PM -0600, Azael Avalos wrote:
> > Second generation keyboard backlight (type 2) laptops can switch
> > on the keyboard LED on their own via hardware/firmware, but the
> > LED subsystem is unaware of such change since the LED interface
> > was only beign created on first generation keyboard backlight
> > (type 1) laptops.
> >
> > This patch creates the LED interface for second gen keyboards
> > and calls the *_hw_changed API whenever userspace changes the
> > state of the keyboard backlight LED.
> >
> > Signed-off-by: Azael Avalos <coproscefalo@xxxxxxxxx>
> > ---
> > drivers/platform/x86/toshiba_acpi.c | 22 +++++++++++++++++++---
> > 1 file changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> > index eef76bfa5d73..0b645b19865f 100644
> > --- a/drivers/platform/x86/toshiba_acpi.c
> > +++ b/drivers/platform/x86/toshiba_acpi.c
> > @@ -1836,6 +1836,7 @@ static ssize_t kbd_backlight_mode_store(struct device *dev,
> > return ret;
> >
> > toshiba->kbd_mode = mode;
> > + toshiba_acpi->kbd_mode = mode;
>
> I found myself confused within the driver regarding changes to toshiba,
> toshiba_acpi, and dev.
>
> We have the global toshiba_acpi, and we sometimes pass around the device
> and use the function argument.
>
> But, what is going on in this case when we have two different devices?
> Are they different?
They're supposed to be identical, however, there are some variables that
are not being updated, and in this case "kbd_mode" is only updated in the
main toshiba struct and not in the global and we need it updated to access
it on the kbd backlight work queue.
My thought is that the global struct was added as a convenience to
access the contents where we cannot pass the main struct as an
argument.
I can send another patch with a variable name cleanup, so we only end up
using either dev or toshiba as the name.
>
> ...
>
> > @@ -3237,11 +3248,16 @@ static void toshiba_acpi_notify(struct acpi_device *acpi_dev, u32 event)
> > pr_info("SATA power event received %x\n", event);
> > break;
> > case 0x92: /* Keyboard backlight mode changed */
> > - toshiba_acpi->kbd_event_generated = true;
> > + dev->kbd_event_generated = true;
>
> This is an unrelated cleanup right? Just using the passed in argument
> instead of the global? No functional change? Should mention this in the
> commit message if so.
Yes, sorry about this, will add a comment to the commit message in v2.
>
> If I'm confused about what these two devices do, then some comments in
> the driver would be appropriate.
>
> --
> Darren Hart
> VMware Open Source Technology Center
--
-- El mundo apesta y vosotros apestais tambien --
