Re: [PATCH v2 18/19] platform/x86: lenovo-yogabook: Add keyboard backlight control to platform driver

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3950 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]