| Message ID | 20180612184935.4274-1-coproscefalo@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Darren Hart |
| Headers | show |
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@gmail.com> > --- > 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? ... > @@ -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. If I'm confused about what these two devices do, then some comments in the driver would be appropriate.
Hi Darren, El mar., 12 de jun. de 2018 a la(s) 17:09, Darren Hart (dvhart@infradead.org) 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@gmail.com> > > --- > > 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
On Wed, Jun 13, 2018 at 10:28:39AM -0600, Azael Avalos wrote: > Hi Darren, > El mar., 12 de jun. de 2018 a la(s) 17:09, Darren Hart > (dvhart@infradead.org) 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@gmail.com> > > > --- > > > 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. This sounds fragile. If for convenience, wouldn't a pointer to the same data suffice, rather than attempting to keep to copies in sync?
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; /* * Some laptop models with the second generation backlit @@ -1852,7 +1853,7 @@ static ssize_t kbd_backlight_mode_store(struct device *dev, * event via genetlink. */ if (toshiba->kbd_type == 2 && - !toshiba_acpi->kbd_event_generated) + !toshiba->kbd_event_generated) schedule_work(&kbd_bl_work); } @@ -2420,6 +2421,13 @@ static void toshiba_acpi_kbd_bl_work(struct work_struct *work) &toshiba_attr_group)) pr_err("Unable to update sysfs entries\n"); + /* Notify LED subsystem about keyboard backlight change */ + if (toshiba_acpi->kbd_type == 2 && + toshiba_acpi->kbd_mode != SCI_KBD_MODE_AUTO) + led_classdev_notify_brightness_hw_changed(&toshiba_acpi->kbd_led, + (toshiba_acpi->kbd_mode == SCI_KBD_MODE_ON) ? + LED_FULL : LED_OFF); + /* Emulate the keyboard backlight event */ acpi_bus_generate_netlink_event(acpi_dev->pnp.device_class, dev_name(&acpi_dev->dev), @@ -3119,9 +3127,12 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev) /* * Only register the LED if KBD illumination is supported * and the keyboard backlight operation mode is set to FN-Z + * or we detect a second gen keyboard backlight */ - if (dev->kbd_illum_supported && dev->kbd_mode == SCI_KBD_MODE_FNZ) { + if (dev->kbd_illum_supported && + (dev->kbd_mode == SCI_KBD_MODE_FNZ || dev->kbd_type == 2)) { dev->kbd_led.name = "toshiba::kbd_backlight"; + dev->kbd_led.flags = LED_BRIGHT_HW_CHANGED; dev->kbd_led.max_brightness = 1; dev->kbd_led.brightness_set = toshiba_kbd_backlight_set; dev->kbd_led.brightness_get = toshiba_kbd_backlight_get; @@ -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; /* Update sysfs entries */ if (sysfs_update_group(&acpi_dev->dev.kobj, &toshiba_attr_group)) pr_err("Unable to update sysfs entries\n"); + /* Notify LED subsystem about keyboard backlight change */ + if (dev->kbd_type == 2 && dev->kbd_mode != SCI_KBD_MODE_AUTO) + led_classdev_notify_brightness_hw_changed(&dev->kbd_led, + (&dev->kbd_mode == SCI_KBD_MODE_ON) ? + LED_FULL : LED_OFF); break; case 0x85: /* Unknown */ case 0x8d: /* Unknown */
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@gmail.com> --- drivers/platform/x86/toshiba_acpi.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)