Re: [PATCH V4 2/5] pwm: Add i.MX TPM PWM driver support

[Uwe Kleine-König]

Hello,

On Mon, Mar 18, 2019 at 02:04:00PM +0000, Anson Huang wrote:
> > > On Mon, Mar 18, 2019 at 07:41:02AM +0000, Anson Huang wrote:
> > > > > > +	val &= ~PWM_IMX_TPM_SC_CMOD;
> > > > > > +	writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > > >
> > > > > As this interrupts the output, please only do it if necessary.
> > > >
> > > > OK, will do it ONLY when it is enabled previously.
> > >
> > > I think you only need to do that when the value actually changes.
> > 
> > OK, I will save the period/div count and ONLY do it when the value actually
> > changes.
> 
> After further think, I added below tpm->period to save the current period settings,
> ONLY when the new period to be set is different from the current period, then the 
> pwm_imx_tpm_config_counter() is called, so I think no need to care about the value
> changes, the value is always changed when pwm_imx_tpm_config_counter() is called.
> 
>                  if (tpm->user_count != 1 && state->period != tpm->period)
>                          return -EBUSY;
>                  ret = pwm_imx_tpm_config_counter(chip, state->period);
>                  if (ret)
>                          return ret;

This is still not the optimal thing to do. What you really want is:

	p = round_period_according_to_hw_caps(state->period);
	if (p != actually_configured_period && tpm->user_count != 1)
		return -EBUSY;


> > > > > > +	/* set duty counter */
> > > > > > +	tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> > > > > > +PWM_IMX_TPM_MOD_MOD_MASK;
> > > > >
> > > > > I recommend storing this value in driver data.
> > > >
> > > > NOT quite understand, as we did NOT use it in other places except
> > > > the get_state, just reading the register once should be OK there.
> > >
> > > I had the impression it is used more than once. Will look again in the
> > > next iteration. But also note that shadowing the value might already
> > > be beneficial for a single call site as driver data might occupy more
> > > RAM than necessary anyhow and reading from RAM is faster than from the
> > > hardware's register.
> > > Probably this is not a fast path, so not worth the optimisation?!
> > 
> > OK, will save it in driver data and avoid accessing register again.

please apply some thought before following my advices. If this is not a
fast path and it hurts readability, don't shadow the register in driver
data.

> > > > > > +		/* disable channel */
> > > > > > +		writel(PWM_IMX_TPM_CnSC_CHF,
> > > > > > +			tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > > >
> > > > > Clearing CHF doens't disable the channel as I read the manual.
> > > >
> > > > This write clears CHF as well as writing other bits 0, to disable
> > > > the output. Maybe I can explicitly clear MSA/MSB/ELSA/ELSB to avoid
> > > > confusion.
> > >
> > > Ah, I misinterpreted the value written.
> > >
> > > > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > > +			     struct pwm_state *state) {
> > > > > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > > > +	struct pwm_state curstate;
> > > > > > +	u32 duty_cycle = state->duty_cycle;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	pwm_imx_tpm_get_state(chip, pwm, &curstate);
> > > > > > +
> > > > > > +	mutex_lock(&tpm->lock);
> > > > >
> > > > > What should this lock protect? Does it hurt if the state changes
> > > > > between pwm_imx_tpm_get_state releasing the lock and
> > > > > pwm_imx_tpm_apply taking it?
> > > >
> > > > The idea is to protect the share resourced by multiple channels, but
> > > > I think I can make the mutex_lock includes get_state and remove the
> > > > lock in get_state function.
> > >
> > > You might need it in .get_state to return a consistent state to the
> > > caller. In this case just introduce an unlocked variant of .get_state
> > > to share code between the two functions.
> > >
> > > And BTW the question was honest. I'm not entirely sure that you need
> > > to hold the lock.
> > 
> > Agreed, if the different channel configuration ONLY access its own register,
> > NOT any shared registers, then I think this lock is unnecessary.
> 
> Since all the functions in .apply function will need to access registers and these
> registers are shared by different channels, so I think the lock is necessary. 

I think so, too, but didn't thought it to the end. I can invest some
time here in the next review round.

BTW, one thing that I think is missing is, that .apply must only return
when the newly configured setting is already active.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |