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

[Uwe Kleine-König]

On Mon, Mar 18, 2019 at 07:41:02AM +0000, Anson Huang wrote:
> Hi,Uwe
> > > +	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.

> > > +	/* 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?!

> > I wonder why MSA and MSB are two bits instead of making this a field of
> > width 2 with 2b10 meaning PWM mode. But maybe it's just me not
> > understanding the independent semantic of these two bits?
> 
> I think making them a field makes more sense, but anyway we just follow
> the RM.

If it makes the driver easier to understand (which I think it does) feel
free to derivate from the RM. Just add a comment to the definition, then
it's fine.

> > Reading the reference manual I'd say in PWM mode the semantic of ELSA
> > and ELSB is:
> > 
> > 	On counter reload set the output to ELSB
> > 	On counter match set the output to ELSA
> > 
> > Noting that in a comment would ease understanding the code here.
> 
> I added below comment for PWM modes:
> 
> 137         /*
> 138          * set polarity (for edge-aligned PWM modes)
> 139          *
> 140          * CPWMS  MSB:MSA  ELSB:ELSA  Mode  Configuration
> 141          * 0      10       10         PWM   High-true pulse
> 142          * 0      10       00         PWM   Reserved
> 143          * 0      10       01         PWM   Low-true pulse
> 144          * 0      10       11         PWM   Reserved
> 145          *
> 146          * High-true pulse: clear output on counter match, set output on
> 147          * counter reload, set output when counter first enabled or paused.
> 148          *
> 149          * Low-true pulse: set output on counter match, clear output on
> 150          * counter reload, clear output when counter first enabled or paused.
> 151          */

I stumbled over "high-true" and "low-true" in the RM, too. In my bubble
this is an uncommon wording. I'd write instead:

	/*
	 * set polarity
	 *
	 * ELSB:ELSA = 2b10 yields normal polarity behaviour, ELSB:ELSA
	 * = 2b01 yields inversed polarity. The other values are
	 * reserved.
	 */

And don't write about CPWM, MSA and MSB which are always used with fixed
values anyhow in the driver. 
 
> > > +		/* 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.

> > > +		 */
> > > +		if (tpm->user_count != 1)
> > > +			return -EBUSY;
> > 
> > Ideally if say period = 37 is requested but currently we have period =
> > 36 and configuring 37 would result in 36 anyhow, don't return EBUSY.
> 
> I think here the protection is just for making sure that is there are multiple
> users, period can NOT be changed, since all channels will be impacted.

I think you misunderstood what I intended to say here.

Consider that in the case that there is only a single PWM in use
configuring for a period of 37 ns actually yields 36 ns because the
hardware cannot provide 37 ns and 36 ns is the best match.

Then if a second user comes and requests 37 ns, the result here is, that
the second user gets the -EBUSY. This is ridiculous however because the
request is denied even though the period is already configured for the
length that would be configured if the second user were the only one.

> > > +	tpm->chip.dev = &pdev->dev;
> > > +	tpm->chip.ops = &imx_tpm_pwm_ops;
> > > +	tpm->chip.base = -1;
> > > +	tpm->chip.npwm = PWM_IMX_TPM_MAX_CHANNEL_NUM;
> > 
> > This is wrong, as some only have 2 channels?
> 
> I saw we can get channel number from register, will read register to determine
> the channel number, but for the channel config and status saved in struct, I will
> still use the MAX channel number to define the array.

Yeah, that is sensible.

Best regards
Uwe

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