[v1] platform/x86: ideapad-laptop: Set max_brightness before using it

[PATCH] platform/x86: ideapad-laptop: Set max_brightness before using it

Posted by Stuart Hayhurst 2 years, 1 month ago

max_brightness would be used in ideapad_kbd_bl_brightness_get() before it was
set, causing ideapad_kbd_bl_brightness_get() to return -EINVAL sometimes

Signed-off-by: Stuart Hayhurst <stuart.a.hayhurst@gmail.com>
---
 drivers/platform/x86/ideapad-laptop.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index ac037540acfc..88eefccb6ed2 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1425,18 +1425,17 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
 	if (WARN_ON(priv->kbd_bl.initialized))
 		return -EEXIST;
 
-	brightness = ideapad_kbd_bl_brightness_get(priv);
-	if (brightness < 0)
-		return brightness;
-
-	priv->kbd_bl.last_brightness = brightness;
-
 	if (ideapad_kbd_bl_check_tristate(priv->kbd_bl.type)) {
 		priv->kbd_bl.led.max_brightness = 2;
 	} else {
 		priv->kbd_bl.led.max_brightness = 1;
 	}
 
+	brightness = ideapad_kbd_bl_brightness_get(priv);
+	if (brightness < 0)
+		return brightness;
+
+	priv->kbd_bl.last_brightness = brightness;
 	priv->kbd_bl.led.name                    = "platform::" LED_FUNCTION_KBD_BACKLIGHT;
 	priv->kbd_bl.led.brightness_get          = ideapad_kbd_bl_led_cdev_brightness_get;
 	priv->kbd_bl.led.brightness_set_blocking = ideapad_kbd_bl_led_cdev_brightness_set;
-- 
2.42.0

Re: [PATCH] platform/x86: ideapad-laptop: Set max_brightness before using it

Posted by Ilpo Järvinen 2 years, 1 month ago

On Sun, 12 Nov 2023, Stuart Hayhurst wrote:

> max_brightness would be used in ideapad_kbd_bl_brightness_get() before it was
> set, causing ideapad_kbd_bl_brightness_get() to return -EINVAL sometimes

... is used ... is set ...

> Signed-off-by: Stuart Hayhurst <stuart.a.hayhurst@gmail.com>

This lacks Fixes: + stable cc tags.

The code change seems fine.

-- 
 i.

> ---
>  drivers/platform/x86/ideapad-laptop.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index ac037540acfc..88eefccb6ed2 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -1425,18 +1425,17 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>  	if (WARN_ON(priv->kbd_bl.initialized))
>  		return -EEXIST;
>  
> -	brightness = ideapad_kbd_bl_brightness_get(priv);
> -	if (brightness < 0)
> -		return brightness;
> -
> -	priv->kbd_bl.last_brightness = brightness;
> -
>  	if (ideapad_kbd_bl_check_tristate(priv->kbd_bl.type)) {
>  		priv->kbd_bl.led.max_brightness = 2;
>  	} else {
>  		priv->kbd_bl.led.max_brightness = 1;
>  	}
>  
> +	brightness = ideapad_kbd_bl_brightness_get(priv);
> +	if (brightness < 0)
> +		return brightness;
> +
> +	priv->kbd_bl.last_brightness = brightness;
>  	priv->kbd_bl.led.name                    = "platform::" LED_FUNCTION_KBD_BACKLIGHT;
>  	priv->kbd_bl.led.brightness_get          = ideapad_kbd_bl_led_cdev_brightness_get;
>  	priv->kbd_bl.led.brightness_set_blocking = ideapad_kbd_bl_led_cdev_brightness_set;
>

[PATCH v2] platform/x86: ideapad-laptop: Set max_brightness before using it

Posted by Stuart Hayhurst 2 years, 1 month ago

max_brightness is used in ideapad_kbd_bl_brightness_get() before it's set,
causing ideapad_kbd_bl_brightness_get() to return -EINVAL sometimes

Fixes: ecaa1867b524 ("platform/x86: ideapad-laptop: Add support for keyboard backlights using KBLC ACPI symbol")
Signed-off-by: Stuart Hayhurst <stuart.a.hayhurst@gmail.com>
Cc: stable@vger.kernel.org
---

Sorry if I messed up the stable cc bit, I have no idea how that process works

v1 -> v2:
 - Fix commit message
 - Add missing tags

---
 drivers/platform/x86/ideapad-laptop.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index ac037540acfc..88eefccb6ed2 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1425,18 +1425,17 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
 	if (WARN_ON(priv->kbd_bl.initialized))
 		return -EEXIST;
 
-	brightness = ideapad_kbd_bl_brightness_get(priv);
-	if (brightness < 0)
-		return brightness;
-
-	priv->kbd_bl.last_brightness = brightness;
-
 	if (ideapad_kbd_bl_check_tristate(priv->kbd_bl.type)) {
 		priv->kbd_bl.led.max_brightness = 2;
 	} else {
 		priv->kbd_bl.led.max_brightness = 1;
 	}
 
+	brightness = ideapad_kbd_bl_brightness_get(priv);
+	if (brightness < 0)
+		return brightness;
+
+	priv->kbd_bl.last_brightness = brightness;
 	priv->kbd_bl.led.name                    = "platform::" LED_FUNCTION_KBD_BACKLIGHT;
 	priv->kbd_bl.led.brightness_get          = ideapad_kbd_bl_led_cdev_brightness_get;
 	priv->kbd_bl.led.brightness_set_blocking = ideapad_kbd_bl_led_cdev_brightness_set;
-- 
2.42.0

Re: [PATCH v2] platform/x86: ideapad-laptop: Set max_brightness before using it

Posted by Ilpo Järvinen 2 years, 1 month ago

On Tue, 14 Nov 2023 11:38:08 +0000, Stuart Hayhurst wrote:

> max_brightness is used in ideapad_kbd_bl_brightness_get() before it's set,
> causing ideapad_kbd_bl_brightness_get() to return -EINVAL sometimes
> 
> 

Thank you for your contribution, it has been applied to my local
review-ilpo branch. Note it will show up in the public
platform-drivers-x86/review-ilpo branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/1] platform/x86: ideapad-laptop: Set max_brightness before using it
      commit: 7a3c36eef9a5d13b16aa954da54224c9c6bed339

--
 i.

Re: [PATCH v2] platform/x86: ideapad-laptop: Set max_brightness before using it

Posted by Ilpo Järvinen 2 years, 1 month ago

On Tue, 14 Nov 2023, Stuart Hayhurst wrote:

> max_brightness is used in ideapad_kbd_bl_brightness_get() before it's set,
> causing ideapad_kbd_bl_brightness_get() to return -EINVAL sometimes
> 
> Fixes: ecaa1867b524 ("platform/x86: ideapad-laptop: Add support for keyboard backlights using KBLC ACPI symbol")
> Signed-off-by: Stuart Hayhurst <stuart.a.hayhurst@gmail.com>
> Cc: stable@vger.kernel.org
> ---
> 
> Sorry if I messed up the stable cc bit, I have no idea how that process works

It looks fine now, thank you (except for the . missing at the end of the 
sentence but I can fix that when I'll later apply the patch).

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

> v1 -> v2:
>  - Fix commit message
>  - Add missing tags
> 
> ---
>  drivers/platform/x86/ideapad-laptop.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index ac037540acfc..88eefccb6ed2 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -1425,18 +1425,17 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>  	if (WARN_ON(priv->kbd_bl.initialized))
>  		return -EEXIST;
>  
> -	brightness = ideapad_kbd_bl_brightness_get(priv);
> -	if (brightness < 0)
> -		return brightness;
> -
> -	priv->kbd_bl.last_brightness = brightness;
> -
>  	if (ideapad_kbd_bl_check_tristate(priv->kbd_bl.type)) {
>  		priv->kbd_bl.led.max_brightness = 2;
>  	} else {
>  		priv->kbd_bl.led.max_brightness = 1;
>  	}
>  
> +	brightness = ideapad_kbd_bl_brightness_get(priv);
> +	if (brightness < 0)
> +		return brightness;
> +
> +	priv->kbd_bl.last_brightness = brightness;
>  	priv->kbd_bl.led.name                    = "platform::" LED_FUNCTION_KBD_BACKLIGHT;
>  	priv->kbd_bl.led.brightness_get          = ideapad_kbd_bl_led_cdev_brightness_get;
>  	priv->kbd_bl.led.brightness_set_blocking = ideapad_kbd_bl_led_cdev_brightness_set;
>