Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout

[Thierry Reding]

On Thu, Dec 13, 2018 at 09:52:13AM +0100, Uwe Kleine-KÃnig wrote:
> On Wed, Dec 12, 2018 at 11:54:32AM +0100, Thierry Reding wrote:
> > On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal VokÃÄ wrote:
> > > Implement the get_state() function and set the initial state to reflect
> > > real state of the hardware. This allows to keep the PWM running if it was
> > > enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
> > > set as output in bootloader keep the same setting in Linux unless it is
> > > reconfigured.
> > > 
> > > If we find the PWM block enabled we need to prepare and enable its source
> > > clock otherwise the clock will be disabled late in the boot as unused.
> > > That will leave the PWM in enabled state but with disabled clock. That has
> > > a side effect that the PWM output is left at its current level at which
> > > the clock was disabled. It is totally non-deterministic and it may be LOW
> > > or HIGH.
> > > 
> > > Signed-off-by: Michal VokÃÄ <michal.vokac@xxxxxxxxx>
> > > ---
> > >  drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 53 insertions(+)
> > 
> > Applied, thanks.
> 
> Did you miss my feedback for this patch or did you choose to ignore it?

Don't have anything in my inbox, but I see that it's there on patchwork,
so I suspect it was eaten by the spam filter. Let me copy-paste here and
go through it.

> > @@ -93,6 +96,55 @@ struct imx_chip {
> >
> >  #define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
> >
> > +static void imx_pwm_get_state(struct pwm_chip *chip,
> > +  struct pwm_device *pwm, struct pwm_state *state)
> 
> broken alignment.

I can fix that up, no need to resend for that.

> > +{
> > + struct imx_chip *imx = to_imx_chip(chip);
> > + u32 period, prescaler, pwm_clk, ret, val;
> > + u64 tmp;
> > +
> > + val = readl(imx->mmio_base + MX3_PWMCR);
> > +
> > + if (val & MX3_PWMCR_EN) {
> > +  state->enabled = true;
> > +  ret = clk_prepare_enable(imx->clk_per);
> > +  if (ret)
> > +   return;
> > + } else {
> > +  state->enabled = false;
> > + }
> > +
> > + switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
> > + case MX3_PWMCR_POUTC_NORMAL:
> > +  state->polarity = PWM_POLARITY_NORMAL;
> > +  break;
> > + case MX3_PWMCR_POUTC_INVERTED:
> > +  state->polarity = PWM_POLARITY_INVERSED;
> > +  break;
> > + default:
> > +  dev_warn(chip->dev, "can't set polarity, output disconnected");
> 
> Should we return an error here?

We can't return an error here because the function has a void return
type. I'm not sure what it means if the POUTC is neither "normal" nor
"inverted", but perhaps a good idea would be to default to either of
those two in that case, or perhaps forcibly disable the PWM so that
we get known-good values in the registers?

I'm tempted not to treat this as a blocker because it's not actually
a bug or anything. Prior to this patch we also ignore whatever this
field was set to.

> > + }
> > +
> > + prescaler = MX3_PWMCR_PRESCALER_GET(val);
> > + pwm_clk = clk_get_rate(imx->clk_per);
> > + pwm_clk = DIV_ROUND_CLOSEST_ULL(pwm_clk, prescaler);
> > + val = readl(imx->mmio_base + MX3_PWMPR);
> 
> It would be more cautionous to not rely on the reserved bits to read as
> zero. So I suggest to mask the value with 0xffff.

If that's what the documentation says I think it's okay to rely on it.
But I can add the mask if we want to play it extra safe.

> > + period = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
> > +
> > + /* PWMOUT (Hz) = PWMCLK / (PWMPR + 2) */
> > + tmp = NSEC_PER_SEC * (u64)(period + 2);
> > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
> 
> Would it make sense to introduce a policy about how to round in this
> case? (Similarily for .apply?) This is of course out of scope for this
> patch.

I'm not sure what you means by policy. The above already does round to
closest. Is that not an appropriate policy?

Thierry

Attachment: signature.asc
Description: PGP signature