On Wed, Oct 04, 2023 at 04:01:30PM +0200, Uwe Kleine-König wrote:
> On Wed, Oct 04, 2023 at 05:27:31PM +0800, Jisheng Zhang wrote:
> > T-HEAD SoCs such as the TH1520 contain a PWM controller used
> > to control the LCD backlight, fan and so on. Add driver for it.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@xxxxxxxxxx>
> > ---
...
Hi Uwe,
Thanks a lot for your review and nice suggestions. v3 has been sent out.
And I want to add more comments to your questions here.
> > +
> > +static int thead_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > +{
> > + struct thead_pwm_chip *priv = thead_pwm_from_chip(chip);
> > + u64 period_cycle, duty_cycle, rate;
> > + u32 val;
> > +
> > + /* if ever started, can't change the polarity */
> > + if (priv->ever_started && state->polarity != pwm->state.polarity)
> > + return -EINVAL;
This is the polority check[1] for ever started channel.
> > +
> > + if (!state->enabled) {
> > + if (pwm->state.enabled) {
> > + val = readl(priv->mmio_base + THEAD_PWM_CTRL(pwm->hwpwm));
> > + val &= ~THEAD_PWM_CFG_UPDATE;
> > + writel(val, priv->mmio_base + THEAD_PWM_CTRL(pwm->hwpwm));
> > +
> > + writel(0, priv->mmio_base + THEAD_PWM_FP(pwm->hwpwm));
> > +
> > + val |= THEAD_PWM_CFG_UPDATE;
> > + writel(val, priv->mmio_base + THEAD_PWM_CTRL(pwm->hwpwm));
> > + }
> > + return 0;
> > + }
> > +
> > + if (!pwm->state.enabled)
> > + pm_runtime_get_sync(chip->dev);
>
> pm_runtime_get_sync() returns an int that you shouldn't ignore.
In v3 I switch to pm_runtime_resume_and_get() because it can simplify
the error handling code.
>
> > + val = readl(priv->mmio_base + THEAD_PWM_CTRL(pwm->hwpwm));
> > + val &= ~THEAD_PWM_CFG_UPDATE;
> > +
> > + if (state->polarity == PWM_POLARITY_INVERSED)
> > + val &= ~THEAD_PWM_FPOUT;
> > + else
> > + val |= THEAD_PWM_FPOUT;
>
> What happens here if the bootloader already touched that flag? Or the
> driver is reloaded/rebound?
Only polarity can't be changed once started, so if bootloader already
configured polarity and started the pwm channel, and we want to change
to a different polarity, the check[1] in the beginning of this function
will fail so return -EINVAL.
>
> > + writel(val, priv->mmio_base + THEAD_PWM_CTRL(pwm->hwpwm));
> > +
> > + rate = clk_get_rate(priv->clk);
> > + /*
> > + * The following calculations might overflow if clk is bigger
> > + * than 1 GHz. In practise it's 24MHz, so this limitation
> > + * is only theoretic.
> > + */
> > + if (rate > (u64)NSEC_PER_SEC)
>
> this cast isn't needed.
>
> > + return -EINVAL;
> > +
> > + period_cycle = mul_u64_u64_div_u64(rate, state->period, NSEC_PER_SEC);
> > + if (period_cycle > THEAD_PWM_MAX_PERIOD)
> > + period_cycle = THEAD_PWM_MAX_PERIOD;
> > + /*
> > + * With limitation above we have period_cycle <= THEAD_PWM_MAX_PERIOD,
> > + * so this cannot overflow.
> > + */
> > + writel((u32)period_cycle, priv->mmio_base + THEAD_PWM_PER(pwm->hwpwm));
>
> This cast can also be dropped.
>
> > +
> > + duty_cycle = mul_u64_u64_div_u64(rate, state->duty_cycle, NSEC_PER_SEC);
> > + if (duty_cycle > THEAD_PWM_MAX_DUTY)
> > + duty_cycle = THEAD_PWM_MAX_DUTY;
> > + /*
> > + * With limitation above we have duty_cycle <= THEAD_PWM_MAX_PERIOD,
> > + * so this cannot overflow.
> > + */
> > + writel((u32)duty_cycle, priv->mmio_base + THEAD_PWM_FP(pwm->hwpwm));
>
> ...
>
> > +
> > + val |= THEAD_PWM_CFG_UPDATE;
> > + writel(val, priv->mmio_base + THEAD_PWM_CTRL(pwm->hwpwm));
> > +
> > + if (!pwm->state.enabled) {
> > + val |= THEAD_PWM_START;
> > + writel(val, priv->mmio_base + THEAD_PWM_CTRL(pwm->hwpwm));
> > + priv->ever_started = true;
> > + }
>
> Further above you conditionally call pm_runtime_get_sync(), there should
> be a matching pm_runtime_put().
In v3, I call pm_runtime_put_sync() when pwm channel is disabled.
Thanks
