All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] backlight: lm3630a: convert to atomic PWM API and check for errors
@ 2021-06-18  8:58 ` Uwe Kleine-König
  0 siblings, 0 replies; 5+ messages in thread
From: Uwe Kleine-König @ 2021-06-18  8:58 UTC (permalink / raw)
  To: Lee Jones, Daniel Thompson, Jingoo Han
  Cc: Thierry Reding, dri-devel, linux-fbdev, kernel

The practical upside here is that this only needs a single API call to
program the hardware which (depending on the underlaying hardware) can
be more effective and prevents glitches.

Up to now the return value of the pwm functions was ignored. Fix this
and propagate the error to the caller.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/video/backlight/lm3630a_bl.c | 33 +++++++++++++++-------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index e88a2b0e5904..24fa7e6938fa 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -52,6 +52,7 @@ struct lm3630a_chip {
 	struct gpio_desc *enable_gpio;
 	struct regmap *regmap;
 	struct pwm_device *pwmd;
+	struct pwm_state pwmd_state;
 };
 
 /* i2c access */
@@ -167,16 +168,19 @@ static int lm3630a_intr_config(struct lm3630a_chip *pchip)
 	return rval;
 }
 
-static void lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
+static int lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
 {
-	unsigned int period = pchip->pdata->pwm_period;
-	unsigned int duty = br * period / br_max;
+	int err;
 
-	pwm_config(pchip->pwmd, duty, period);
-	if (duty)
-		pwm_enable(pchip->pwmd);
-	else
-		pwm_disable(pchip->pwmd);
+	pchip->pwmd_state.period = pchip->pdata->pwm_period;
+
+	err = pwm_set_relative_duty_cycle(&pchip->pwmd_state, br, br_max);
+	if (err)
+		return err;
+
+	pchip->pwmd_state.enabled = pchip->pwmd_state.duty_cycle ? true : false;
+
+	return pwm_apply_state(pchip->pwmd, &pchip->pwmd_state);
 }
 
 /* update and get brightness */
@@ -188,8 +192,11 @@ static int lm3630a_bank_a_update_status(struct backlight_device *bl)
 
 	/* pwm control */
 	if ((pwm_ctrl & LM3630A_PWM_BANK_A) != 0) {
-		lm3630a_pwm_ctrl(pchip, bl->props.brightness,
-				 bl->props.max_brightness);
+		ret = lm3630a_pwm_ctrl(pchip, bl->props.brightness,
+				       bl->props.max_brightness);
+		if (ret)
+			return ret;
+
 		return bl->props.brightness;
 	}
 
@@ -563,11 +570,7 @@ static int lm3630a_probe(struct i2c_client *client,
 			return PTR_ERR(pchip->pwmd);
 		}
 
-		/*
-		 * FIXME: pwm_apply_args() should be removed when switching to
-		 * the atomic PWM API.
-		 */
-		pwm_apply_args(pchip->pwmd);
+		pwm_init_state(pchip->pwmd, &pchip->pwmd_state);
 	}
 
 	/* interrupt enable  : irq 0 is not allowed */
-- 
2.30.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH] backlight: lm3630a: convert to atomic PWM API and check for errors
@ 2021-06-18  8:58 ` Uwe Kleine-König
  0 siblings, 0 replies; 5+ messages in thread
From: Uwe Kleine-König @ 2021-06-18  8:58 UTC (permalink / raw)
  To: Lee Jones, Daniel Thompson, Jingoo Han
  Cc: linux-fbdev, Thierry Reding, kernel, dri-devel

The practical upside here is that this only needs a single API call to
program the hardware which (depending on the underlaying hardware) can
be more effective and prevents glitches.

Up to now the return value of the pwm functions was ignored. Fix this
and propagate the error to the caller.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/video/backlight/lm3630a_bl.c | 33 +++++++++++++++-------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index e88a2b0e5904..24fa7e6938fa 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -52,6 +52,7 @@ struct lm3630a_chip {
 	struct gpio_desc *enable_gpio;
 	struct regmap *regmap;
 	struct pwm_device *pwmd;
+	struct pwm_state pwmd_state;
 };
 
 /* i2c access */
@@ -167,16 +168,19 @@ static int lm3630a_intr_config(struct lm3630a_chip *pchip)
 	return rval;
 }
 
-static void lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
+static int lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
 {
-	unsigned int period = pchip->pdata->pwm_period;
-	unsigned int duty = br * period / br_max;
+	int err;
 
-	pwm_config(pchip->pwmd, duty, period);
-	if (duty)
-		pwm_enable(pchip->pwmd);
-	else
-		pwm_disable(pchip->pwmd);
+	pchip->pwmd_state.period = pchip->pdata->pwm_period;
+
+	err = pwm_set_relative_duty_cycle(&pchip->pwmd_state, br, br_max);
+	if (err)
+		return err;
+
+	pchip->pwmd_state.enabled = pchip->pwmd_state.duty_cycle ? true : false;
+
+	return pwm_apply_state(pchip->pwmd, &pchip->pwmd_state);
 }
 
 /* update and get brightness */
@@ -188,8 +192,11 @@ static int lm3630a_bank_a_update_status(struct backlight_device *bl)
 
 	/* pwm control */
 	if ((pwm_ctrl & LM3630A_PWM_BANK_A) != 0) {
-		lm3630a_pwm_ctrl(pchip, bl->props.brightness,
-				 bl->props.max_brightness);
+		ret = lm3630a_pwm_ctrl(pchip, bl->props.brightness,
+				       bl->props.max_brightness);
+		if (ret)
+			return ret;
+
 		return bl->props.brightness;
 	}
 
@@ -563,11 +570,7 @@ static int lm3630a_probe(struct i2c_client *client,
 			return PTR_ERR(pchip->pwmd);
 		}
 
-		/*
-		 * FIXME: pwm_apply_args() should be removed when switching to
-		 * the atomic PWM API.
-		 */
-		pwm_apply_args(pchip->pwmd);
+		pwm_init_state(pchip->pwmd, &pchip->pwmd_state);
 	}
 
 	/* interrupt enable  : irq 0 is not allowed */
-- 
2.30.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] backlight: lm3630a: convert to atomic PWM API and check for errors
  2021-06-18  8:58 ` Uwe Kleine-König
@ 2021-06-18 10:31   ` Daniel Thompson
  -1 siblings, 0 replies; 5+ messages in thread
From: Daniel Thompson @ 2021-06-18 10:31 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Lee Jones, Jingoo Han, Thierry Reding, dri-devel, linux-fbdev, kernel

On Fri, Jun 18, 2021 at 10:58:44AM +0200, Uwe Kleine-König wrote:
> The practical upside here is that this only needs a single API call to
> program the hardware which (depending on the underlaying hardware) can
> be more effective and prevents glitches.
> 
> Up to now the return value of the pwm functions was ignored. Fix this
> and propagate the error to the caller.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

New code looks OK but these changes reveal just how crazy the return codes
from this driver's update_status() methods are since now the new (PWM) error path is
structured completely differently to the existing (I2C) error path.

update_status() should return 0 on success and a -ve errno otherwise.

Thus the new code to return -errno is correct and so is the I2C success
code path. However the PWM success path and the I2C error path are both
broken.

Are you OK to add a patch *before* this one to fix the existing code
paths before making the PWM changes?


Daniel.

> ---
>  drivers/video/backlight/lm3630a_bl.c | 33 +++++++++++++++-------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
> index e88a2b0e5904..24fa7e6938fa 100644
> --- a/drivers/video/backlight/lm3630a_bl.c
> +++ b/drivers/video/backlight/lm3630a_bl.c
> @@ -52,6 +52,7 @@ struct lm3630a_chip {
>  	struct gpio_desc *enable_gpio;
>  	struct regmap *regmap;
>  	struct pwm_device *pwmd;
> +	struct pwm_state pwmd_state;
>  };
>  
>  /* i2c access */
> @@ -167,16 +168,19 @@ static int lm3630a_intr_config(struct lm3630a_chip *pchip)
>  	return rval;
>  }
>  
> -static void lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
> +static int lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
>  {
> -	unsigned int period = pchip->pdata->pwm_period;
> -	unsigned int duty = br * period / br_max;
> +	int err;
>  
> -	pwm_config(pchip->pwmd, duty, period);
> -	if (duty)
> -		pwm_enable(pchip->pwmd);
> -	else
> -		pwm_disable(pchip->pwmd);
> +	pchip->pwmd_state.period = pchip->pdata->pwm_period;
> +
> +	err = pwm_set_relative_duty_cycle(&pchip->pwmd_state, br, br_max);
> +	if (err)
> +		return err;
> +
> +	pchip->pwmd_state.enabled = pchip->pwmd_state.duty_cycle ? true : false;
> +
> +	return pwm_apply_state(pchip->pwmd, &pchip->pwmd_state);
>  }
>  
>  /* update and get brightness */
> @@ -188,8 +192,11 @@ static int lm3630a_bank_a_update_status(struct backlight_device *bl)
>  
>  	/* pwm control */
>  	if ((pwm_ctrl & LM3630A_PWM_BANK_A) != 0) {
> -		lm3630a_pwm_ctrl(pchip, bl->props.brightness,
> -				 bl->props.max_brightness);
> +		ret = lm3630a_pwm_ctrl(pchip, bl->props.brightness,
> +				       bl->props.max_brightness);
> +		if (ret)
> +			return ret;
> +
>  		return bl->props.brightness;
>  	}
>  
> @@ -563,11 +570,7 @@ static int lm3630a_probe(struct i2c_client *client,
>  			return PTR_ERR(pchip->pwmd);
>  		}
>  
> -		/*
> -		 * FIXME: pwm_apply_args() should be removed when switching to
> -		 * the atomic PWM API.
> -		 */
> -		pwm_apply_args(pchip->pwmd);
> +		pwm_init_state(pchip->pwmd, &pchip->pwmd_state);
>  	}
>  
>  	/* interrupt enable  : irq 0 is not allowed */
> -- 
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] backlight: lm3630a: convert to atomic PWM API and check for errors
@ 2021-06-18 10:31   ` Daniel Thompson
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Thompson @ 2021-06-18 10:31 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: linux-fbdev, Jingoo Han, dri-devel, Thierry Reding, kernel, Lee Jones

On Fri, Jun 18, 2021 at 10:58:44AM +0200, Uwe Kleine-König wrote:
> The practical upside here is that this only needs a single API call to
> program the hardware which (depending on the underlaying hardware) can
> be more effective and prevents glitches.
> 
> Up to now the return value of the pwm functions was ignored. Fix this
> and propagate the error to the caller.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

New code looks OK but these changes reveal just how crazy the return codes
from this driver's update_status() methods are since now the new (PWM) error path is
structured completely differently to the existing (I2C) error path.

update_status() should return 0 on success and a -ve errno otherwise.

Thus the new code to return -errno is correct and so is the I2C success
code path. However the PWM success path and the I2C error path are both
broken.

Are you OK to add a patch *before* this one to fix the existing code
paths before making the PWM changes?


Daniel.

> ---
>  drivers/video/backlight/lm3630a_bl.c | 33 +++++++++++++++-------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
> index e88a2b0e5904..24fa7e6938fa 100644
> --- a/drivers/video/backlight/lm3630a_bl.c
> +++ b/drivers/video/backlight/lm3630a_bl.c
> @@ -52,6 +52,7 @@ struct lm3630a_chip {
>  	struct gpio_desc *enable_gpio;
>  	struct regmap *regmap;
>  	struct pwm_device *pwmd;
> +	struct pwm_state pwmd_state;
>  };
>  
>  /* i2c access */
> @@ -167,16 +168,19 @@ static int lm3630a_intr_config(struct lm3630a_chip *pchip)
>  	return rval;
>  }
>  
> -static void lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
> +static int lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
>  {
> -	unsigned int period = pchip->pdata->pwm_period;
> -	unsigned int duty = br * period / br_max;
> +	int err;
>  
> -	pwm_config(pchip->pwmd, duty, period);
> -	if (duty)
> -		pwm_enable(pchip->pwmd);
> -	else
> -		pwm_disable(pchip->pwmd);
> +	pchip->pwmd_state.period = pchip->pdata->pwm_period;
> +
> +	err = pwm_set_relative_duty_cycle(&pchip->pwmd_state, br, br_max);
> +	if (err)
> +		return err;
> +
> +	pchip->pwmd_state.enabled = pchip->pwmd_state.duty_cycle ? true : false;
> +
> +	return pwm_apply_state(pchip->pwmd, &pchip->pwmd_state);
>  }
>  
>  /* update and get brightness */
> @@ -188,8 +192,11 @@ static int lm3630a_bank_a_update_status(struct backlight_device *bl)
>  
>  	/* pwm control */
>  	if ((pwm_ctrl & LM3630A_PWM_BANK_A) != 0) {
> -		lm3630a_pwm_ctrl(pchip, bl->props.brightness,
> -				 bl->props.max_brightness);
> +		ret = lm3630a_pwm_ctrl(pchip, bl->props.brightness,
> +				       bl->props.max_brightness);
> +		if (ret)
> +			return ret;
> +
>  		return bl->props.brightness;
>  	}
>  
> @@ -563,11 +570,7 @@ static int lm3630a_probe(struct i2c_client *client,
>  			return PTR_ERR(pchip->pwmd);
>  		}
>  
> -		/*
> -		 * FIXME: pwm_apply_args() should be removed when switching to
> -		 * the atomic PWM API.
> -		 */
> -		pwm_apply_args(pchip->pwmd);
> +		pwm_init_state(pchip->pwmd, &pchip->pwmd_state);
>  	}
>  
>  	/* interrupt enable  : irq 0 is not allowed */
> -- 
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] backlight: lm3630a: convert to atomic PWM API and check for errors
  2021-06-18 10:31   ` Daniel Thompson
  (?)
@ 2021-06-18 21:17   ` Uwe Kleine-König
  -1 siblings, 0 replies; 5+ messages in thread
From: Uwe Kleine-König @ 2021-06-18 21:17 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: linux-fbdev, Jingoo Han, dri-devel, Thierry Reding, kernel, Lee Jones

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

On Fri, Jun 18, 2021 at 11:31:09AM +0100, Daniel Thompson wrote:
> On Fri, Jun 18, 2021 at 10:58:44AM +0200, Uwe Kleine-König wrote:
> > The practical upside here is that this only needs a single API call to
> > program the hardware which (depending on the underlaying hardware) can
> > be more effective and prevents glitches.
> > 
> > Up to now the return value of the pwm functions was ignored. Fix this
> > and propagate the error to the caller.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> New code looks OK but these changes reveal just how crazy the return codes
> from this driver's update_status() methods are since now the new (PWM) error path is
> structured completely differently to the existing (I2C) error path.

Indeed, while working on the patch I noticed that sometimes a positive
value is returned but failed to note that when I sent out the patch.

> Are you OK to add a patch *before* this one to fix the existing code
> paths before making the PWM changes?

I didn't do that because I was unsure what is the right thing to do. Now
that you confirmed the documentation I can add such a patch. Will add
this to my todo list.

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 --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-06-18 21:17 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-18  8:58 [PATCH] backlight: lm3630a: convert to atomic PWM API and check for errors Uwe Kleine-König
2021-06-18  8:58 ` Uwe Kleine-König
2021-06-18 10:31 ` Daniel Thompson
2021-06-18 10:31   ` Daniel Thompson
2021-06-18 21:17   ` Uwe Kleine-König

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.