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

[Angelo Compagnucci <angelo.compagnucci@xxxxxxxxx>]

Il giorno ven 22 gen 2021 alle ore 11:20 Linus Walleij
<linus.walleij@xxxxxxxxxx> ha scritto:
>
> 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?

More than a year passed from the last message, could we reopen the
discussion? I'd like to have this upstream!

Thanks!

>
> Yours,
> Linus Walleij



-- 
Profile: http://it.linkedin.com/in/compagnucciangelo