diff mbox series

platform/x86: x86-android-tablets: Adjust Xiaomi Pad 2 bottom bezel touch buttons LED

Message ID 20240916090255.35548-1-hdegoede@redhat.com (mailing list archive)
State Accepted, archived
Headers show
Series platform/x86: x86-android-tablets: Adjust Xiaomi Pad 2 bottom bezel touch buttons LED | expand

Commit Message

Hans de Goede Sept. 16, 2024, 9:02 a.m. UTC 
The "input-events" LED trigger used to turn on the backlight LEDs had to
be rewritten to use led_trigger_register_simple() + led_trigger_event()
to fix a serious locking issue.

This means it no longer supports using blink_brightness to set a per LED
brightness for the trigger and it no longer sets LED_CORE_SUSPENDRESUME.

Adjust the MiPad 2 bottom bezel touch buttons LED class device to match:

1. Make LED_FULL the maximum brightness to fix the LED brightness
   being very low when on.
2. Set flags = LED_CORE_SUSPENDRESUME.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/x86-android-tablets/other.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Hans de Goede Sept. 16, 2024, 9:06 a.m. UTC | #1 
Hi All,

On 9/16/24 11:02 AM, Hans de Goede wrote:
> The "input-events" LED trigger used to turn on the backlight LEDs had to
> be rewritten to use led_trigger_register_simple() + led_trigger_event()
> to fix a serious locking issue.
> 
> This means it no longer supports using blink_brightness to set a per LED
> brightness for the trigger and it no longer sets LED_CORE_SUSPENDRESUME.
> 
> Adjust the MiPad 2 bottom bezel touch buttons LED class device to match:
> 
> 1. Make LED_FULL the maximum brightness to fix the LED brightness
>    being very low when on.
> 2. Set flags = LED_CORE_SUSPENDRESUME.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I've added this to my review-hans (soon to be for-next) branch now.

Regards,

Hans

> ---
>  drivers/platform/x86/x86-android-tablets/other.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/x86-android-tablets/other.c b/drivers/platform/x86/x86-android-tablets/other.c
> index aad7114c43c3..76cce8120bd2 100644
> --- a/drivers/platform/x86/x86-android-tablets/other.c
> +++ b/drivers/platform/x86/x86-android-tablets/other.c
> @@ -670,7 +670,7 @@ static const struct software_node *ktd2026_node_group[] = {
>   * is controlled by the "pwm_soc_lpss_2" PWM output.
>   */
>  #define XIAOMI_MIPAD2_LED_PERIOD_NS		19200
> -#define XIAOMI_MIPAD2_LED_DEFAULT_DUTY		 6000 /* From Android kernel */
> +#define XIAOMI_MIPAD2_LED_MAX_DUTY		 6000 /* From Android kernel */
>  
>  static struct pwm_device *xiaomi_mipad2_led_pwm;
>  
> @@ -679,7 +679,7 @@ static int xiaomi_mipad2_brightness_set(struct led_classdev *led_cdev,
>  {
>  	struct pwm_state state = {
>  		.period = XIAOMI_MIPAD2_LED_PERIOD_NS,
> -		.duty_cycle = val,
> +		.duty_cycle = XIAOMI_MIPAD2_LED_MAX_DUTY * val / LED_FULL,
>  		/* Always set PWM enabled to avoid the pin floating */
>  		.enabled = true,
>  	};
> @@ -701,11 +701,11 @@ static int __init xiaomi_mipad2_init(struct device *dev)
>  		return -ENOMEM;
>  
>  	led_cdev->name = "mipad2:white:touch-buttons-backlight";
> -	led_cdev->max_brightness = XIAOMI_MIPAD2_LED_PERIOD_NS;
> -	/* "input-events" trigger uses blink_brightness */
> -	led_cdev->blink_brightness = XIAOMI_MIPAD2_LED_DEFAULT_DUTY;
> +	led_cdev->max_brightness = LED_FULL;
>  	led_cdev->default_trigger = "input-events";
>  	led_cdev->brightness_set_blocking = xiaomi_mipad2_brightness_set;
> +	/* Turn LED off during suspend */
> +	led_cdev->flags = LED_CORE_SUSPENDRESUME;
>  
>  	ret = devm_led_classdev_register(dev, led_cdev);
>  	if (ret)
Andy Shevchenko Sept. 16, 2024, 12:05 p.m. UTC | #2 
On Mon, Sep 16, 2024 at 11:02:55AM +0200, Hans de Goede wrote:
> The "input-events" LED trigger used to turn on the backlight LEDs had to
> be rewritten to use led_trigger_register_simple() + led_trigger_event()
> to fix a serious locking issue.
> 
> This means it no longer supports using blink_brightness to set a per LED
> brightness for the trigger and it no longer sets LED_CORE_SUSPENDRESUME.
> 
> Adjust the MiPad 2 bottom bezel touch buttons LED class device to match:
> 
> 1. Make LED_FULL the maximum brightness to fix the LED brightness
>    being very low when on.
> 2. Set flags = LED_CORE_SUSPENDRESUME.

...

>  #define XIAOMI_MIPAD2_LED_PERIOD_NS		19200
> -#define XIAOMI_MIPAD2_LED_DEFAULT_DUTY		 6000 /* From Android kernel */
> +#define XIAOMI_MIPAD2_LED_MAX_DUTY		 6000 /* From Android kernel */

Perhaps + suffix at the same time? _NS? _US?

...

> -		.duty_cycle = val,
> +		.duty_cycle = XIAOMI_MIPAD2_LED_MAX_DUTY * val / LED_FULL,

I haven't checked, just to be sure there is no overflow and 64-bit div on
32-bit machines...
Hans de Goede Sept. 16, 2024, 1:10 p.m. UTC | #3 
Hi,

On 9/16/24 2:05 PM, Andy Shevchenko wrote:
> On Mon, Sep 16, 2024 at 11:02:55AM +0200, Hans de Goede wrote:
>> The "input-events" LED trigger used to turn on the backlight LEDs had to
>> be rewritten to use led_trigger_register_simple() + led_trigger_event()
>> to fix a serious locking issue.
>>
>> This means it no longer supports using blink_brightness to set a per LED
>> brightness for the trigger and it no longer sets LED_CORE_SUSPENDRESUME.
>>
>> Adjust the MiPad 2 bottom bezel touch buttons LED class device to match:
>>
>> 1. Make LED_FULL the maximum brightness to fix the LED brightness
>>    being very low when on.
>> 2. Set flags = LED_CORE_SUSPENDRESUME.
> 
> ...
> 
>>  #define XIAOMI_MIPAD2_LED_PERIOD_NS		19200
>> -#define XIAOMI_MIPAD2_LED_DEFAULT_DUTY		 6000 /* From Android kernel */
>> +#define XIAOMI_MIPAD2_LED_MAX_DUTY		 6000 /* From Android kernel */
> 
> Perhaps + suffix at the same time? _NS? _US?

Good idea, fixed in my tree.

> 
> ...
> 
>> -		.duty_cycle = val,
>> +		.duty_cycle = XIAOMI_MIPAD2_LED_MAX_DUTY * val / LED_FULL,
> 
> I haven't checked, just to be sure there is no overflow and 64-bit div on
> 32-bit machines...

all variables are 32 bit, so this a 32 bit div. LED_FULL is 255
and 6000 * 255 = 1530000 so the maximum easily fits in 32 bits .

Regards,

Hans
diff mbox series

Patch

diff --git a/drivers/platform/x86/x86-android-tablets/other.c b/drivers/platform/x86/x86-android-tablets/other.c
index aad7114c43c3..76cce8120bd2 100644
--- a/drivers/platform/x86/x86-android-tablets/other.c
+++ b/drivers/platform/x86/x86-android-tablets/other.c
@@ -670,7 +670,7 @@  static const struct software_node *ktd2026_node_group[] = {
  * is controlled by the "pwm_soc_lpss_2" PWM output.
  */
 #define XIAOMI_MIPAD2_LED_PERIOD_NS		19200
-#define XIAOMI_MIPAD2_LED_DEFAULT_DUTY		 6000 /* From Android kernel */
+#define XIAOMI_MIPAD2_LED_MAX_DUTY		 6000 /* From Android kernel */
 
 static struct pwm_device *xiaomi_mipad2_led_pwm;
 
@@ -679,7 +679,7 @@  static int xiaomi_mipad2_brightness_set(struct led_classdev *led_cdev,
 {
 	struct pwm_state state = {
 		.period = XIAOMI_MIPAD2_LED_PERIOD_NS,
-		.duty_cycle = val,
+		.duty_cycle = XIAOMI_MIPAD2_LED_MAX_DUTY * val / LED_FULL,
 		/* Always set PWM enabled to avoid the pin floating */
 		.enabled = true,
 	};
@@ -701,11 +701,11 @@  static int __init xiaomi_mipad2_init(struct device *dev)
 		return -ENOMEM;
 
 	led_cdev->name = "mipad2:white:touch-buttons-backlight";
-	led_cdev->max_brightness = XIAOMI_MIPAD2_LED_PERIOD_NS;
-	/* "input-events" trigger uses blink_brightness */
-	led_cdev->blink_brightness = XIAOMI_MIPAD2_LED_DEFAULT_DUTY;
+	led_cdev->max_brightness = LED_FULL;
 	led_cdev->default_trigger = "input-events";
 	led_cdev->brightness_set_blocking = xiaomi_mipad2_brightness_set;
+	/* Turn LED off during suspend */
+	led_cdev->flags = LED_CORE_SUSPENDRESUME;
 
 	ret = devm_led_classdev_register(dev, led_cdev);
 	if (ret)