| Message ID | 20210618085844.231751-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 1181f2164135 |
| Headers | show |
| Series | backlight: lm3630a: convert to atomic PWM API and check for errors | expand |
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 >
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
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 */
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(-)