Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Nicola, Uwe,

thanks for looping me in on this very interesting driver!

This can be really helpful, I already see that it would be possible to
replace the hopeless idiomatic driver for the NSLU2 ixp4xx
beeper speaker with this driver.

On Sun, Jan 17, 2021 at 2:04 PM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:

> > +static void pwm_gpio_work(struct work_struct *work)
> > +{
> > +     struct pwm_gpio *pwm_gpio = container_of(work, struct pwm_gpio, work);
> > +
> > +     gpiod_set_value_cansleep(pwm_gpio->desc, pwm_gpio->output);
>
> Usually you want to check the return value of gpiod_set_value. @Linus +
> Bartosz: What do you think, does it need checking for an error here?

We have traditionally been quite relaxed on that. But since it is
the cansleep variant, meaning this GPIO could be on a GPIO
expander on I2C or SPI, it would be more important.

However: in this specific case for PWM I wonder if we should
stick with gpio_set_value() without _cansleep, and then we can
definitely skip the error check.

That means GPIO PWM can happen on on-chip GPIOs that
are just a register write. Certainly no need to check the return
value of these. Also we know it will satisfy timing.

If we allow GPIO PWM on slow buses and expanders that can
sleep I do not think the PWM will be very good or reliable.

For example on an I2C expander, with the bus speed of
max 400 kHz, what kind of PWM can we create on that?
Certainly now above 200 kHz (Nyquist frequency) and
probably less. And we have to way to actually check the
frequency of the underlying bus, so I am a bit skeptic
here.

Would gpio_set_value() work for now or do we need to
support expanders and _cansleep?

Yours,
Linus Walleij