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:
signature.asc
Description: PGP signature
