Hello Nicola,
On Sun, Jan 17, 2021 at 02:58:03PM +0100, Nicola Di Lieto wrote:
> On Sun, Jan 17, 2021 at 02:04:34PM +0100, Uwe Kleine-König wrote:
> > > + if (!atomic_read(&pwm_gpio->enabled))
> > > + return HRTIMER_NORESTART;
> > > +
> > > + if (pwm_gpio->state) {
> > > + ns = pwm_gpio->cur.toff_ns;
> > > + pwm_gpio->state = false;
> > > + } else {
> > > + if (spin_trylock(&pwm_gpio->lock)) {
> > > + pwm_gpio->cur = pwm_gpio->new;
> > > + spin_unlock(&pwm_gpio->lock);
> > > + }
> > > + ns = pwm_gpio->cur.ton_ns;
> > > + pwm_gpio->state = true;
> > > + }
> > > + pwm_gpio->output = pwm_gpio->state ^ pwm_gpio->cur.invert;
> > > +
> > > + schedule_work(&pwm_gpio->work);
> > > + hrtimer_forward(handle, hrtimer_get_expires(handle), ns_to_ktime(ns));
> >
> > This is hard to understand. I think we need more comments explaining
> > (e.g.) the semantic of the members of struct pwm_gpio.
>
> Would it be OK if I added the following comment in the code?
>
> pwm_gpio_apply writes the new settings to pwm_gpio->new, synchronized by the
> spinlock. At the beginning of every PWM cycle pwm_gpio->new is copied into
> pwm_gpio->cur, but only if the spinlock can be locked immediately (meaning
> the settings aren't being applied concurrently to the beginning of the
> period). By not waiting for the spinlock, no extra jitter in the PWM period
> is introduced.
So far I understood also only comment. What wasn't obvious immediately
is the state member.
> > Does it make sense to use the workqueue only if the GPIO is a sleeping
> > one and for fast GPIOs call gpiod_set_value directly?
>
> I thought about this but didn't implement it as it seemed overkill to me.
> The workqueue is needed anyway to cope with sleeping GPIOs, and it can deal
> with fast GPIOs with insignificant degradation compared to a direct
> implementation.
OK. If later the need arises this can be added then.
> > > +static const struct pwm_ops pwm_gpio_ops = {
> > > + .free = pwm_gpio_free,
> >
> > A free without request seems wrong. The callback stops the PWM, this is
> > wrong, the PWM should continue to toggle.
> >
>
> Would it be OK to remove the pwm_gpio_free callback altogether and move the
> cancel_work_sync() call to pwm_gpio_remove?
Yes, that sounds right.
> Would it be ok to cancel the timer first and then "return
> pwmchip_remove(...)"?
No. The PWM must stay functional until pwmchip_remove() returns.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
