| Message ID | 20230430165807.472798-19-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | platform/x86: lenovo-yogabook: Modify to also work on Android version | expand |
Hello, On Sun, Apr 30, 2023 at 06:58:06PM +0200, Hans de Goede wrote: > On the Android yb1-x90f/l models there is not ACPI method to control > the keyboard backlight brightness. Instead the second PWM controller > is exposed directly to the OS there. > > Add support for controlling keyboard backlight brightness on the Android > model by using the PWM subsystem to directly control the PWM. > > The Android model also requires explicitly turning the backlight off > on suspend, which on the Windows model was done automatically. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > - Use YB_KBD_BL_PWM_PERIOD define in yogabook_pdev_pwm_lookup[] > - Turn off keyboard backlight on suspend > --- > drivers/platform/x86/lenovo-yogabook-wmi.c | 31 +++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/lenovo-yogabook-wmi.c b/drivers/platform/x86/lenovo-yogabook-wmi.c > index 0183b75a47e8..b49109d91ec3 100644 > --- a/drivers/platform/x86/lenovo-yogabook-wmi.c > +++ b/drivers/platform/x86/lenovo-yogabook-wmi.c > @@ -19,6 +19,7 @@ > #include <linux/leds.h> > #include <linux/module.h> > #include <linux/platform_device.h> > +#include <linux/pwm.h> > #include <linux/wmi.h> > #include <linux/workqueue.h> > > @@ -26,6 +27,7 @@ > > #define YB_KBD_BL_DEFAULT 128 > #define YB_KBD_BL_MAX 255 > +#define YB_KBD_BL_PWM_PERIOD 13333 > > #define YB_PDEV_NAME "yogabook-touch-kbd-digitizer-switch" > > @@ -48,6 +50,7 @@ struct yogabook_data { > struct gpio_desc *pen_touch_event; > struct gpio_desc *kbd_bl_led_enable; > struct gpio_desc *backside_hall_gpio; > + struct pwm_device *kbd_bl_pwm; > int (*set_kbd_backlight)(struct yogabook_data *data, uint8_t level); > int pen_touch_irq; > int backside_hall_irq; > @@ -267,8 +270,11 @@ static int yogabook_suspend(struct device *dev) > struct yogabook_data *data = dev_get_drvdata(dev); > > set_bit(YB_SUSPENDED, &data->flags); > - > flush_work(&data->work); > + > + if (test_bit(YB_KBD_IS_ON, &data->flags)) > + data->set_kbd_backlight(data, 0); > + > return 0; > } > > @@ -424,8 +430,21 @@ static struct gpiod_lookup_table yogabook_pdev_gpios = { > }, > }; > > +static struct pwm_lookup yogabook_pdev_pwm_lookup[] = { > + PWM_LOOKUP_WITH_MODULE("80862289:00", 0, YB_PDEV_NAME, "kbd_bl_pwm", > + YB_KBD_BL_PWM_PERIOD, PWM_POLARITY_NORMAL, > + "pwm-lpss-platform"), > +}; > + > static int yogabook_pdev_set_kbd_backlight(struct yogabook_data *data, u8 level) > { > + struct pwm_state state = { > + .period = YB_KBD_BL_PWM_PERIOD, > + .duty_cycle = YB_KBD_BL_PWM_PERIOD * level / YB_KBD_BL_MAX, > + .enabled = level, > + }; > + > + pwm_apply_state(data->kbd_bl_pwm, &state); > gpiod_set_value(data->kbd_bl_led_enable, level ? 1 : 0); > return 0; > } > @@ -475,6 +494,16 @@ static int yogabook_pdev_probe(struct platform_device *pdev) > goto error_put_devs; > } > > + pwm_add_table(yogabook_pdev_pwm_lookup, ARRAY_SIZE(yogabook_pdev_pwm_lookup)); > + data->kbd_bl_pwm = devm_pwm_get(dev, "kbd_bl_pwm"); > + pwm_remove_table(yogabook_pdev_pwm_lookup, ARRAY_SIZE(yogabook_pdev_pwm_lookup)); Keeping the table added just long enough to call devm_pwm_get() looks very creative to me. Why don't you keep the table around while the device is available? Also note that a given table must only ever be added once using pwm_add_table(). If there happen to be two yogabook devices and .probe() is called a second time, the list of tables might be corrupted. I don't know much about x86, but I think the table belongs to where this "80862289:00" device is created. Best regards Uwe
On Thu, May 4, 2023 at 7:53 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Sun, Apr 30, 2023 at 06:58:06PM +0200, Hans de Goede wrote: ... > I don't know much about x86, but I think the table belongs to where this > "80862289:00" device is created. Just for your information, it's in drivers/acpi/acpi_lpss.c.
On Fri, May 05, 2023 at 12:07:02PM +0300, Andy Shevchenko wrote: > On Thu, May 4, 2023 at 7:53 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Sun, Apr 30, 2023 at 06:58:06PM +0200, Hans de Goede wrote: > > ... > > > I don't know much about x86, but I think the table belongs to where this > > "80862289:00" device is created. > > Just for your information, it's in drivers/acpi/acpi_lpss.c. Compared to drivers/platform/x86/lenovo-yogabook-wmi.c this file is never compiled as a module and so patch #1 would become unnecessary. That file also already has a pwm_lookup table. Best regards Uwe
On Fri, May 5, 2023 at 12:21 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Fri, May 05, 2023 at 12:07:02PM +0300, Andy Shevchenko wrote: > > On Thu, May 4, 2023 at 7:53 PM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > On Sun, Apr 30, 2023 at 06:58:06PM +0200, Hans de Goede wrote: ... > > > I don't know much about x86, but I think the table belongs to where this > > > "80862289:00" device is created. > > > > Just for your information, it's in drivers/acpi/acpi_lpss.c. > > Compared to drivers/platform/x86/lenovo-yogabook-wmi.c this file is > never compiled as a module and so patch #1 would become unnecessary. > > That file also already has a pwm_lookup table. TBH, Hans knows nowadays much better what those drivers do and why the split was done this way. If he thinks that it's needed, I will trust his word.
Hi Uwe, Andy, On 5/5/23 11:21, Uwe Kleine-König wrote: > On Fri, May 05, 2023 at 12:07:02PM +0300, Andy Shevchenko wrote: >> On Thu, May 4, 2023 at 7:53 PM Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >>> On Sun, Apr 30, 2023 at 06:58:06PM +0200, Hans de Goede wrote: >> >> ... >> >>> I don't know much about x86, but I think the table belongs to where this >>> "80862289:00" device is created. >> >> Just for your information, it's in drivers/acpi/acpi_lpss.c. > > Compared to drivers/platform/x86/lenovo-yogabook-wmi.c this file is > never compiled as a module and so patch #1 would become unnecessary. > > That file also already has a pwm_lookup table. Right, the Cherry Trail SoCs in question have 2 PWM controllers the first controller is pretty much always used to control the brightness of the LCD screen. So we have a fixed pwm_lookup table for it there using the SoC's builtin display controller's device_name() as consumer-device-name. The second PWM controller however is different it is mostly unused I'm aware of 2 cases where it is used and in both cases it is used to control the brightness of a backlight for fixed (etched into the glass) touch controls. The problem is that in these cases there will be 2 totally different consumer devices. Looking at the lookup tabel checks in pwm_get() I see that it is possible to add a lookup which matches only by dev_id. So I could use this here and this would then also be in place for when I get around to writing a driver for the second case (that I'm ware of) which needs access to the second PWM controller. So this then just leaves the question of what to name the con-id, since we won't be specifying a consumer-device-name I think it is best to keep the con_id quite generic, e.g.: "pwm_soc_lpss_2" to match with the existing: "pwm_soc_backlight" for the first PWM controller. Uwe, Andy, is using a pwm_lookup with only a con_id match on "pwm_soc_lpss_2" ok with you ? Regards, Hans
Hi, On 5/6/23 13:26, Hans de Goede wrote: > Hi Uwe, Andy, > > On 5/5/23 11:21, Uwe Kleine-König wrote: >> On Fri, May 05, 2023 at 12:07:02PM +0300, Andy Shevchenko wrote: >>> On Thu, May 4, 2023 at 7:53 PM Uwe Kleine-König >>> <u.kleine-koenig@pengutronix.de> wrote: >>>> On Sun, Apr 30, 2023 at 06:58:06PM +0200, Hans de Goede wrote: >>> >>> ... >>> >>>> I don't know much about x86, but I think the table belongs to where this >>>> "80862289:00" device is created. >>> >>> Just for your information, it's in drivers/acpi/acpi_lpss.c. >> >> Compared to drivers/platform/x86/lenovo-yogabook-wmi.c this file is >> never compiled as a module and so patch #1 would become unnecessary. >> >> That file also already has a pwm_lookup table. > > Right, the Cherry Trail SoCs in question have 2 PWM controllers > the first controller is pretty much always used to control > the brightness of the LCD screen. So we have a fixed pwm_lookup > table for it there using the SoC's builtin display controller's > device_name() as consumer-device-name. > > The second PWM controller however is different it is mostly unused > I'm aware of 2 cases where it is used and in both cases it is used > to control the brightness of a backlight for fixed (etched into the > glass) touch controls. > > The problem is that in these cases there will be 2 totally different > consumer devices. Looking at the lookup tabel checks in pwm_get() > I see that it is possible to add a lookup which matches only by > dev_id. So I could use this here and this would then also be in > place for when I get around to writing a driver for the second > case (that I'm ware of) which needs access to the second PWM controller. > > So this then just leaves the question of what to name the con-id, > since we won't be specifying a consumer-device-name I think it is > best to keep the con_id quite generic, e.g.: > > "pwm_soc_lpss_2" > > to match with the existing: > > "pwm_soc_backlight" > > for the first PWM controller. > > Uwe, Andy, is using a pwm_lookup with only a con_id match on > "pwm_soc_lpss_2" ok with you ? I've decided to go ahead and go this route. So I've modified this patch to drop the pwm_lookup_table from it and changed the con_id passed to pwm_get() to: "pwm_soc_lpss_2". And then merged patches 2-19 (1) into the pdx86/review-hans (2). I'll also submit an acpi_lpss.c patch soon to add a lookup-table entry for the "pwm_soc_lpss_2" con_id there. Regards, Hans 1) All the patches except for patch 1 which exported pwm_add_table() / pwm_remove_table() 2) Patches pending for pdx86/for-next
diff --git a/drivers/platform/x86/lenovo-yogabook-wmi.c b/drivers/platform/x86/lenovo-yogabook-wmi.c index 0183b75a47e8..b49109d91ec3 100644 --- a/drivers/platform/x86/lenovo-yogabook-wmi.c +++ b/drivers/platform/x86/lenovo-yogabook-wmi.c @@ -19,6 +19,7 @@ #include <linux/leds.h> #include <linux/module.h> #include <linux/platform_device.h> +#include <linux/pwm.h> #include <linux/wmi.h> #include <linux/workqueue.h> @@ -26,6 +27,7 @@ #define YB_KBD_BL_DEFAULT 128 #define YB_KBD_BL_MAX 255 +#define YB_KBD_BL_PWM_PERIOD 13333 #define YB_PDEV_NAME "yogabook-touch-kbd-digitizer-switch" @@ -48,6 +50,7 @@ struct yogabook_data { struct gpio_desc *pen_touch_event; struct gpio_desc *kbd_bl_led_enable; struct gpio_desc *backside_hall_gpio; + struct pwm_device *kbd_bl_pwm; int (*set_kbd_backlight)(struct yogabook_data *data, uint8_t level); int pen_touch_irq; int backside_hall_irq; @@ -267,8 +270,11 @@ static int yogabook_suspend(struct device *dev) struct yogabook_data *data = dev_get_drvdata(dev); set_bit(YB_SUSPENDED, &data->flags); - flush_work(&data->work); + + if (test_bit(YB_KBD_IS_ON, &data->flags)) + data->set_kbd_backlight(data, 0); + return 0; } @@ -424,8 +430,21 @@ static struct gpiod_lookup_table yogabook_pdev_gpios = { }, }; +static struct pwm_lookup yogabook_pdev_pwm_lookup[] = { + PWM_LOOKUP_WITH_MODULE("80862289:00", 0, YB_PDEV_NAME, "kbd_bl_pwm", + YB_KBD_BL_PWM_PERIOD, PWM_POLARITY_NORMAL, + "pwm-lpss-platform"), +}; + static int yogabook_pdev_set_kbd_backlight(struct yogabook_data *data, u8 level) { + struct pwm_state state = { + .period = YB_KBD_BL_PWM_PERIOD, + .duty_cycle = YB_KBD_BL_PWM_PERIOD * level / YB_KBD_BL_MAX, + .enabled = level, + }; + + pwm_apply_state(data->kbd_bl_pwm, &state); gpiod_set_value(data->kbd_bl_led_enable, level ? 1 : 0); return 0; } @@ -475,6 +494,16 @@ static int yogabook_pdev_probe(struct platform_device *pdev) goto error_put_devs; } + pwm_add_table(yogabook_pdev_pwm_lookup, ARRAY_SIZE(yogabook_pdev_pwm_lookup)); + data->kbd_bl_pwm = devm_pwm_get(dev, "kbd_bl_pwm"); + pwm_remove_table(yogabook_pdev_pwm_lookup, ARRAY_SIZE(yogabook_pdev_pwm_lookup)); + + if (IS_ERR(data->kbd_bl_pwm)) { + r = dev_err_probe(dev, PTR_ERR(data->kbd_bl_pwm), + "Getting keyboard backlight PWM\n"); + goto error_put_devs; + } + r = gpiod_to_irq(data->pen_touch_event); if (r < 0) { dev_err_probe(dev, r, "Getting pen_touch_event IRQ\n");
On the Android yb1-x90f/l models there is not ACPI method to control the keyboard backlight brightness. Instead the second PWM controller is exposed directly to the OS there. Add support for controlling keyboard backlight brightness on the Android model by using the PWM subsystem to directly control the PWM. The Android model also requires explicitly turning the backlight off on suspend, which on the Windows model was done automatically. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: - Use YB_KBD_BL_PWM_PERIOD define in yogabook_pdev_pwm_lookup[] - Turn off keyboard backlight on suspend --- drivers/platform/x86/lenovo-yogabook-wmi.c | 31 +++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)