Re: [PATCH v2 2/2] pwm: add T-HEAD PWM driver

[Jisheng Zhang]

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