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

[Thierry Reding <thierry.reding@gmail.com>]

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

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?

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

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

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]