Messages in this thread | | | Date | Thu, 13 Dec 2018 18:00:00 +0100 | From | Thierry Reding <> | Subject | Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout |
| |
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@ysoft.com> > > > --- > > > 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 [unhandled content-type:application/pgp-signature] | |