Re: [PATCH 1/3] pwm: Change prototype of .get_state() callback to return an error

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

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

On Wed, Sep 28, 2022 at 02:49:00PM +0200, Thierry Reding wrote:
> On Fri, Sep 16, 2022 at 05:15:04PM +0200, Uwe Kleine-König wrote:
> [...]
> > diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
> > index 7b357d1cf642..811e6f424927 100644
> > --- a/drivers/pwm/pwm-crc.c
> > +++ b/drivers/pwm/pwm-crc.c
> > @@ -121,8 +121,8 @@ static int crc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	return 0;
> >  }
> >  
> > -static void crc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > -			      struct pwm_state *state)
> > +static int crc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			     struct pwm_state *state)
> >  {
> >  	struct crystalcove_pwm *crc_pwm = to_crc_pwm(chip);
> >  	struct device *dev = crc_pwm->chip.dev;
> > @@ -132,13 +132,13 @@ static void crc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	error = regmap_read(crc_pwm->regmap, PWM0_CLK_DIV, &clk_div_reg);
> >  	if (error) {
> >  		dev_err(dev, "Error reading PWM0_CLK_DIV %d\n", error);
> > -		return;
> > +		return -EIO;
> >  	}
> >  
> >  	error = regmap_read(crc_pwm->regmap, PWM0_DUTY_CYCLE, &duty_cycle_reg);
> >  	if (error) {
> >  		dev_err(dev, "Error reading PWM0_DUTY_CYCLE %d\n", error);
> > -		return;
> > +		return -EIO;
> >  	}
> 
> In other drivers you propagate errors from regmap_read(), why not here?

Oh, this is indeed wrong and should be "return error".

> 
> > diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
> > index 7004f55bbf11..aa06b3ce81a6 100644
> > --- a/drivers/pwm/pwm-sprd.c
> > +++ b/drivers/pwm/pwm-sprd.c
> > @@ -65,8 +65,8 @@ static void sprd_pwm_write(struct sprd_pwm_chip *spc, u32 hwid,
> >  	writel_relaxed(val, spc->base + offset);
> >  }
> >  
> > -static void sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > -			       struct pwm_state *state)
> > +static int sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			      struct pwm_state *state)
> >  {
> >  	struct sprd_pwm_chip *spc =
> >  		container_of(chip, struct sprd_pwm_chip, chip);
> > @@ -80,11 +80,8 @@ static void sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	 * reading to the registers.
> >  	 */
> >  	ret = clk_bulk_prepare_enable(SPRD_PWM_CHN_CLKS_NUM, chn->clks);
> > -	if (ret) {
> > -		dev_err(spc->dev, "failed to enable pwm%u clocks\n",
> > -			pwm->hwpwm);
> 
> This might be useful information, so perhaps leave it in?

Ok, I don't like .get_state emitting an error, but agreed, that's an
orthogonal issue that shouldn't be addressed en passant in this change.

> [...]
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index c8445b0a3339..ead909400e64 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -108,9 +108,9 @@ static inline void sun4i_pwm_writel(struct sun4i_pwm_chip *chip,
> >  	writel(val, chip->base + offset);
> >  }
> >  
> > -static void sun4i_pwm_get_state(struct pwm_chip *chip,
> > -				struct pwm_device *pwm,
> > -				struct pwm_state *state)
> > +static int sun4i_pwm_get_state(struct pwm_chip *chip,
> > +			       struct pwm_device *pwm,
> > +			       struct pwm_state *state)
> >  {
> >  	struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> >  	u64 clk_rate, tmp;
> > @@ -132,7 +132,7 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip,
> >  		state->duty_cycle = DIV_ROUND_UP_ULL(state->period, 2);
> >  		state->polarity = PWM_POLARITY_NORMAL;
> >  		state->enabled = true;
> > -		return;
> > +		return 0;
> >  	}
> >  
> >  	if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
> > @@ -142,7 +142,8 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip,
> >  		prescaler = prescaler_table[PWM_REG_PRESCAL(val, pwm->hwpwm)];
> >  
> >  	if (prescaler == 0)
> > -		return;
> > +		/* huh? is this an error? */
> > +		return 0;
> 
> Yeah, I think this would count as an error. The prescaler value returned
> from that table is 0 in what seems to be "invalid" configurations. If
> you look at how this is used in sun4i_pwm_calculate(), these entries are
> skipped for the computation of the duty cycle. So I would expect this to
> happen in either an invalidly configured or completely unconfigured PWM.
> 
> That raises the question about what to do in these cases. If we return
> an error, that could potentially throw off consumers. So perhaps the
> closest would be to return a disabled PWM? Or perhaps it'd be up to the
> consumer to provide some fallback configuration for invalidly configured
> or unconfigured PWMs.

This is something I'd address on the framework level. i.e. don't care in
the lowlevel driver about setting .enabled = false (or whatever we
choose to do) but care for that in drivers/pwm/core.c.

Note that the status quo is that if that error happens the consumer sees
whatever state the lowlevel driver stored in pwm->state, without an
error indication.

Will send a v2.

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