Hello,
On Thu, Mar 14, 2019 at 09:49:06AM +0000, Anson Huang wrote:
> > On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + static bool tpm_cnt_initialized;
> > > + unsigned int duty_cnt;
> > > + u32 val;
> > > + u64 tmp;
> > > +
> > > + /*
> > > + * TPM counter is shared by multi channels, let's make it to be
> > > + * ONLY first channel can config TPM counter's precale and period
> > > + * count.
> > > + */
> > > + if (!tpm_cnt_initialized) {
> > > + imx_tpm_pwm_config_counter(chip, state->period);
> > > + tpm_cnt_initialized = true;
> > > + }
> >
> > So the period can only be configured once. That is not as good as it could be.
> > You should allow a change whenever there is exactly one PWM in use.
>
> OK, maybe I can add check for other channels' statue here, and allow the period
> update if ONLY 1 channel is enabled.
See how the SiFive patch that I already pointed out solves this same
problem.
> > > + /* set duty counter */
> > > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > > + tmp *= state->duty_cycle;
> > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
> >
> > Uah, you use state->period here even though for the 2nd PWM the Divider
> > might not be setup appropriately.
>
> I think that is 1 limitation here, the dts should make sure the period used for
> different channels are same or at least they can share same divider, otherwise,
> what if multiple channels can NOT find a divider good for every channel? How to
> deal with this case?
You should return -ERANGE or -EINVAL for the calls that cannot be
satisfied.
> > > [...]
> > > +static int imx_tpm_pwm_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;
> > > + unsigned long flags;
> > > +
> > > + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > > +
> > > + spin_lock_irqsave(&tpm->lock, flags);
> > > +
> > > + if (state->period != curstate.period ||
> > > + state->duty_cycle != curstate.duty_cycle ||
> > > + state->polarity != curstate.polarity)
> > > + imx_tpm_pwm_config(chip, pwm, state);
> > > +
> > > + if (state->enabled != curstate.enabled)
> > > + imx_tpm_pwm_enable(chip, pwm, state->enabled);
> >
> > This is wrong. This sequence:
> >
> > pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> > true });
> > pwm_apply_state(pwm, { .duty_cycle = 10000, .period =
> > 10000, .enabled = false });
> >
> > must keep the output pin low which isn't guaranteed here.
>
> So you mean for every .apply operation, the channel MUST be disabled first, then config
> it, then enable it?
No. I only say that you should not configure the new period and duty
cycle if in the end the hardware should be disabled. Always disabling is
wrong, too.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
