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

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 8907 bytes --]

Hello,

[added the gpio people to Cc:]

On Fri, Dec 11, 2020 at 06:04:31PM +0100, Nicola Di Lieto wrote:
> This new driver allows pulse width modulating any GPIOs using
> a high resolution timer. It is fully generic and can be useful
> in a variety of situations. As an example I used it to provide
> a pwm to the pwm-beeper driver so that my embedded system can
> produce tones through a piezo buzzer connected to a GPIO which
> unfortunately is not hardware PWM capable.
> 
> Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
> ---
>  MAINTAINERS            |   7 ++
>  drivers/pwm/Kconfig    |  10 +++
>  drivers/pwm/Makefile   |   1 +
>  drivers/pwm/pwm-gpio.c | 188 +++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 206 insertions(+)
>  create mode 100644 drivers/pwm/pwm-gpio.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 52086876ce40..486a8857b47b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14222,6 +14222,13 @@ F:	Documentation/devicetree/bindings/hwmon/pwm-fan.txt
>  F:	Documentation/hwmon/pwm-fan.rst
>  F:	drivers/hwmon/pwm-fan.c
>  
> +PWM GPIO DRIVER
> +M:	Nicola Di Lieto <nicola.dilieto@gmail.com>
> +L:	linux-pwm@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/pwm/pwm-gpio.yaml
> +F:	drivers/pwm/pwm-gpio.c

Maybe only add the line for the binding doc in the second patch?

> +
>  PWM IR Transmitter
>  M:	Sean Young <sean@mess.org>
>  L:	linux-media@vger.kernel.org
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 63be5362fd3a..5432084c6276 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -181,6 +181,16 @@ config PWM_FSL_FTM
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-fsl-ftm.
>  
> +config PWM_GPIO
> +	tristate "PWM GPIO support"
> +	depends on GPIOLIB
> +	depends on HIGH_RES_TIMERS
> +	help
> +	  Generic PWM for software modulation of GPIOs
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-gpio.
> +
>  config PWM_HIBVT
>  	tristate "HiSilicon BVT PWM support"
>  	depends on ARCH_HISI || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index cbdcd55d69ee..eea0216215a7 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
>  obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.o
>  obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
>  obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
> +obj-$(CONFIG_PWM_GPIO)		+= pwm-gpio.o
>  obj-$(CONFIG_PWM_HIBVT)		+= pwm-hibvt.o
>  obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
>  obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
> diff --git a/drivers/pwm/pwm-gpio.c b/drivers/pwm/pwm-gpio.c
> new file mode 100644
> index 000000000000..06b4ddee389d
> --- /dev/null
> +++ b/drivers/pwm/pwm-gpio.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Generic software PWM for modulating GPIOs
> + *
> + * Copyright 2020 Nicola Di Lieto
> + *
> + * Author: Nicola Di Lieto <nicola.dilieto@gmail.com>
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/hrtimer.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/spinlock.h>
> +#include <linux/workqueue.h>
> +
> +struct pwm_gpio {
> +	struct pwm_chip chip;
> +	struct gpio_desc *desc;
> +	struct work_struct work;
> +	struct hrtimer timer;
> +	atomic_t enabled;
> +	spinlock_t lock;
> +	struct {
> +		u64 ton_ns;
> +		u64 toff_ns;
> +		bool invert;
> +	} cur, new;
> +	bool state;
> +	bool output;
> +};

I would have called this struct pwm_gpio_ddata. Given that pwm_gpio is
the common prefix of all variables and functions in this driver,
pwm_gpio alone is a bit short.

> +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?

> +}
> +
> +enum hrtimer_restart pwm_gpio_do_timer(struct hrtimer *handle)
> +{
> +	struct pwm_gpio *pwm_gpio = container_of(handle, struct pwm_gpio, timer);
> +	u64 ns;
> +
> +	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.

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?

> +
> +	return HRTIMER_RESTART;
> +}
> +
> +static inline struct pwm_gpio *pwm_gpio_from_chip(struct pwm_chip *_chip)
> +{
> +	return container_of(_chip, struct pwm_gpio, chip);
> +}
> +
> +static void pwm_gpio_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct pwm_gpio *pwm_gpio = pwm_gpio_from_chip(chip);
> +
> +	cancel_work_sync(&pwm_gpio->work);
> +	gpiod_set_value_cansleep(pwm_gpio->desc, pwm_gpio->cur.invert);
> +}
> +
> +static int pwm_gpio_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			  const struct pwm_state *state)
> +{
> +	struct pwm_gpio *pwm_gpio = pwm_gpio_from_chip(chip);
> +
> +	spin_lock(&pwm_gpio->lock);
> +	pwm_gpio->new.ton_ns = state->duty_cycle;
> +	pwm_gpio->new.toff_ns = state->period - state->duty_cycle;
> +	pwm_gpio->new.invert = (state->polarity == PWM_POLARITY_INVERSED);
> +	spin_unlock(&pwm_gpio->lock);
> +
> +	if (state->enabled && !atomic_cmpxchg(&pwm_gpio->enabled, 0, 1)) {
> +		hrtimer_start(&pwm_gpio->timer, 0, HRTIMER_MODE_REL);
> +	} else if (!state->enabled && atomic_cmpxchg(&pwm_gpio->enabled, 1, 0)) {
> +		pwm_gpio->state = false;
> +		pwm_gpio->output = (state->polarity == PWM_POLARITY_INVERSED);
> +		schedule_work(&pwm_gpio->work);
> +	}
> +	return 0;
> +}
> +
> +static void pwm_gpio_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +			       struct pwm_state *state)
> +{
> +	struct pwm_gpio *pwm_gpio = pwm_gpio_from_chip(chip);
> +
> +	state->duty_cycle = pwm_gpio->new.ton_ns;
> +	state->period = pwm_gpio->new.ton_ns + pwm_gpio->new.toff_ns;
> +	state->polarity = pwm_gpio->new.invert ? PWM_POLARITY_INVERSED
> +					       : PWM_POLARITY_NORMAL;
> +	state->enabled = atomic_read(&pwm_gpio->enabled);
> +}
> +
> +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.

> +	.apply = pwm_gpio_apply,
> +	.get_state = pwm_gpio_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int pwm_gpio_probe(struct platform_device *pdev)
> +{
> +	struct pwm_gpio *pwm_gpio;
> +
> +	pwm_gpio = devm_kzalloc(&pdev->dev, sizeof(*pwm_gpio), GFP_KERNEL);
> +	if (!pwm_gpio)
> +		return -ENOMEM;
> +
> +	pwm_gpio->desc = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW);
> +	if (IS_ERR(pwm_gpio->desc))
> +		return PTR_ERR(pwm_gpio->desc);
> +
> +	INIT_WORK(&pwm_gpio->work, pwm_gpio_work);
> +
> +	hrtimer_init(&pwm_gpio->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	pwm_gpio->timer.function = pwm_gpio_do_timer;
> +	pwm_gpio->chip.dev = &pdev->dev;
> +	pwm_gpio->chip.ops = &pwm_gpio_ops;
> +	pwm_gpio->chip.npwm = 1;
> +	pwm_gpio->chip.base = -1;
> +
> +	platform_set_drvdata(pdev, pwm_gpio);
> +
> +	spin_lock_init(&pwm_gpio->lock);
> +
> +	return pwmchip_add(&pwm_gpio->chip);

Error message please if pwmchip_add fails

> +}
> +
> +static int pwm_gpio_remove(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct pwm_gpio *pwm_gpio = platform_get_drvdata(pdev);
> +
> +	ret = pwmchip_remove(&pwm_gpio->chip);
> +	if (ret)
> +		return ret;

This exit path is bad. The return value of the remove callback is
ignored and after pwm_gpio_remove() returns the gpio and *pwm_gpio are
freed.

> +
> +	hrtimer_cancel(&pwm_gpio->timer);
> +
> +	return 0;
> +}

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Nicola Di Lieto]

Hello,

thanks for reviewing this.

On Sun, Jan 17, 2021 at 02:04:34PM +0100, Uwe Kleine-König wrote:
>Maybe only add the line for the binding doc in the second patch?

>I would have called this struct pwm_gpio_ddata. Given that pwm_gpio is
>the common prefix of all variables and functions in this driver,
>pwm_gpio alone is a bit short.

I will change these as suggested.

>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?

I will wait until they reply.

>> +
>> +	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.

>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.

>> +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?

>Error message please if pwmchip_add fails

I will add this.

>> +}
>> +
>> +static int pwm_gpio_remove(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	struct pwm_gpio *pwm_gpio = platform_get_drvdata(pdev);
>> +
>> +	ret = pwmchip_remove(&pwm_gpio->chip);
>> +	if (ret)
>> +		return ret;
>
>This exit path is bad. The return value of the remove callback is
>ignored and after pwm_gpio_remove() returns the gpio and *pwm_gpio are
>freed.
>
>> +
>> +	hrtimer_cancel(&pwm_gpio->timer);
>> +
>> +	return 0;
>> +}

Would it be ok to cancel the timer first and then "return 
pwmchip_remove(...)"?

Best regards,
Nicola

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2664 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Nicola Di Lieto]

Hello Uwe,

On Sun, Jan 17, 2021 at 07:45:56PM +0100, Uwe Kleine-König wrote:
>> > > +	pwm_gpio->output = pwm_gpio->state ^ pwm_gpio->cur.invert;
>
>So far I understood also only comment. What wasn't obvious immediately
>is the state member.

Would it become clear enough by adding: "state is the logical PWM 
output; the actual PIN output level is inverted by XORing with 
cur.invert when the latter is true" ?

>> Would it be ok to cancel the timer first and then "return
>> pwmchip_remove(...)"?
>
>No. The PWM must stay functional until pwmchip_remove() returns.
>

Could you please clarify what I should do when pwmchip_remove returns 
non-zero? In my original implementation

- if pwmchip_remove returns a non-zero error code, I return it to the 
  caller and do not cancel the timer.
- if pwmchip_remove returns zero, I cancel the timer and return zero to 
  the caller

So under no circumstance I return a different code from what 
pwmchip_remove does...

Best regards,
Nicola

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2934 bytes --]

Hello Nicola,

On Sun, Jan 17, 2021 at 10:06:18PM +0100, Nicola Di Lieto wrote:
> On Sun, Jan 17, 2021 at 07:45:56PM +0100, Uwe Kleine-König wrote:
> > > > > +	pwm_gpio->output = pwm_gpio->state ^ pwm_gpio->cur.invert;
> > 
> > So far I understood also only comment. What wasn't obvious immediately
> > is the state member.
> 
> Would it become clear enough by adding: "state is the logical PWM output;
> the actual PIN output level is inverted by XORing with cur.invert when the
> latter is true" ?

This was at least good enough for me to understand it now.

So iff state is true, the PWM is in the active phase of the current
period. Maybe "currently_active" is a better name for this variable?

Then the code could (with some comments added and a few more variables
renamed) could look as follows:

	if (ddata->currently_active) {
		/* Enter the inactive part of the current period. */
		ddata->currently_active = false;
		next_transistion = ddata->cur.toff_ns;
	} else {
		/*
		 * Start a new period. First check if there is a new
		 * configuration setting pending in ddata->new.
		 */
		ddata->currently_active = true;

		if (spin_trylock(&ddata->lock)) {
			ddata->cur = ddata->new;
			spin_unlock(&ddata->lock);
		}
		next_transition = ddata->cur.ton_ns;
	}
	...

which IMHO is easier to understand.

I think there are still two problems with this approach:

 - The locking is hard to follow, .enabled is accessed using atomic
   accessors, .new is protected by the spinlock and the other members
   are not accessed concurrently, right?
   If pwm_apply(..., {.enabled = false}) and pwm_apply(.., {.enabled =
   true}) are called in quick sequence (e.g. faster than the first call
   triggers the work queue) there is trouble ahead, isn't there?

 - If .duty_cycle is equal to 0 (or .period) the output should be
   constant. I think this isn't what will happen.

> > > Would it be ok to cancel the timer first and then "return
> > > pwmchip_remove(...)"?
> > 
> > No. The PWM must stay functional until pwmchip_remove() returns.
> > 
> 
> Could you please clarify what I should do when pwmchip_remove returns
> non-zero? In my original implementation
> - if pwmchip_remove returns a non-zero error code, I return it to the
> caller and do not cancel the timer.
> - if pwmchip_remove returns zero, I cancel the timer and return zero to  the
> caller

IMHO it's a bug that pwmchip_remove() can return an error code. I think
the best you can do currently is:

	ret = pwmchip_remove(...)
	WARN_ON(ret);

	hrtimer_cancel(..);

	return 0;

because whatever you do is wrong. To sort this out needs some thought
and work in the framework and so is unrelated to this patch.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Linus Walleij]

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@pengutronix.de> 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

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

[Angelo Compagnucci]

Il giorno ven 22 gen 2021 alle ore 11:20 Linus Walleij
<linus.walleij@linaro.org> 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@pengutronix.de> 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

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

[andy.shevchenko]

Fri, Feb 10, 2023 at 10:54:49PM +0100, Angelo Compagnucci kirjoitti:
> Il giorno ven 22 gen 2021 alle ore 11:20 Linus Walleij
> <linus.walleij@linaro.org> ha scritto:

...

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

Seems not much interest neither from community nor from author. Maybe later
people will look into this?

P.S> FWIW, I have reviewed code more than a week ago.

-- 
With Best Regards,
Andy Shevchenko

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

[Nicola Di Lieto]

Hello Andy

On Wed, Feb 22, 2023 at 02:51:00PM +0200, andy.shevchenko@gmail.com wrote:
>
>Seems not much interest neither from community nor from author. Maybe later
>people will look into this?
>

It's not lack of interest, but rather lack of time. I should be able to 
have a look at this sometime the week after next.

Nicola

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

[Linus Walleij]

On Wed, Feb 22, 2023 at 4:00 PM Nicola Di Lieto
<nicola.dilieto@gmail.com> wrote:
> On Wed, Feb 22, 2023 at 02:51:00PM +0200, andy.shevchenko@gmail.com wrote:
> >
> >Seems not much interest neither from community nor from author. Maybe later
> >people will look into this?
> >
>
> It's not lack of interest, but rather lack of time. I should be able to
> have a look at this sometime the week after next.

Thanks Nicola, I am also interested in seeing this driver upstream.

Yours,
Linus Walleij

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

[Philip Howard]

On 22/02/2023 15:00, Nicola Di Lieto wrote:
> Hello Andy
> 
> On Wed, Feb 22, 2023 at 02:51:00PM +0200, andy.shevchenko@gmail.com wrote:
>>
>> Seems not much interest neither from community nor from author. Maybe 
>> later
>> people will look into this?
>>
> 
> It's not lack of interest, but rather lack of time. I should be able to 
> have a look at this sometime the week after next.

I'd love to know if you're still likely to look into this.

As part of the shift away from memory-mapped GPIO and sysfs on Raspberry 
Pi, I have some legacy devices which need PWM on arbitrary pins and no 
canonical solution. As such- I am interested!

> 
> Nicola

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

[Andy Shevchenko]

On Fri, Jan 12, 2024 at 3:38 PM Philip Howard <phil@gadgetoid.com> wrote:
> On 22/02/2023 15:00, Nicola Di Lieto wrote:
> > On Wed, Feb 22, 2023 at 02:51:00PM +0200, andy.shevchenko@gmail.com wrote:

...

> I'd love to know if you're still likely to look into this.

If you are asking me as well (since To refers to me), Cc for the new
version and I might look at it. Currently I am OoO till the end of
month and after that I probably will have more tasks to do, so no
guarantee for this cycle, but just in case Cc and we will see.

-- 
With Best Regards,
Andy Shevchenko

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

[Phil Howard]

On Fri, 12 Jan 2024 at 18:32, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Fri, Jan 12, 2024 at 3:38 PM Philip Howard <phil@gadgetoid.com> wrote:
> > On 22/02/2023 15:00, Nicola Di Lieto wrote:
> > > On Wed, Feb 22, 2023 at 02:51:00PM +0200, andy.shevchenko@gmail.com wrote:
>
> ...
>
> > I'd love to know if you're still likely to look into this.
>
> If you are asking me as well (since To refers to me), Cc for the new
> version and I might look at it. Currently I am OoO till the end of
> month and after that I probably will have more tasks to do, so no
> guarantee for this cycle, but just in case Cc and we will see.

Thanks for your reply!

I'm not sure I know how to "Cc for the new version," it took me about
an hour to generate a (hopefully) suitably formatted email response
to this patch. I've only just subbed to linux-pwm.

But yes- my reply was cast out into the ether in the hopes that logging
my interest might get things moving again, or at least let me know if I
should try tackling it myself.

I'm moving over to libgpiod on Raspberry Pi, losing software PWM (
from the library I was using) in the process. I don't want to use or
contrive another not-invented-here solution.

>
> --
> With Best Regards,
> Andy Shevchenko

--
Philip Howard

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

[Andy Shevchenko]

On Tue, Jan 16, 2024 at 4:15 PM Phil Howard <phil@gadgetoid.com> wrote:
> On Fri, 12 Jan 2024 at 18:32, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jan 12, 2024 at 3:38 PM Philip Howard <phil@gadgetoid.com> wrote:
> > > On 22/02/2023 15:00, Nicola Di Lieto wrote:
> > > > On Wed, Feb 22, 2023 at 02:51:00PM +0200, andy.shevchenko@gmail.com wrote:

...

> > > I'd love to know if you're still likely to look into this.
> >
> > If you are asking me as well (since To refers to me), Cc for the new
> > version and I might look at it. Currently I am OoO till the end of
> > month and after that I probably will have more tasks to do, so no
> > guarantee for this cycle, but just in case Cc and we will see.
>
> Thanks for your reply!
>
> I'm not sure I know how to "Cc for the new version," it took me about
> an hour to generate a (hopefully) suitably formatted email response
> to this patch. I've only just subbed to linux-pwm.

I use the script [1] that uses a heuristics for the Cc/To fields,
additionally you can add --cc "Andy Shevchenko ..." at the end.


> But yes- my reply was cast out into the ether in the hopes that logging
> my interest might get things moving again, or at least let me know if I
> should try tackling it myself.
>
> I'm moving over to libgpiod on Raspberry Pi, losing software PWM (
> from the library I was using) in the process. I don't want to use or
> contrive another not-invented-here solution.

Totally supporting this motivation!

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

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

[Stefan Wahren]

Hi,

i was working on this a little bit during the holiday. Personally i
prefer Vincent's approach [1], which is easier to read and more
consequent by rejecting sleeping GPIOs. So i prepared a WIP branch [2],
which was tested on a Raspberry Pi 3 B Plus + a cheap Logic analyzer. So
maybe you want to give it a try. I will try to send a proper series soon.

Changes:
- rebased Vincent's last patch series on top of Linux 6.7
- cherry picked some improvements from Nicola's series
- tried to address Uwe's, Linus' and Andy's comments
- tried to avoid glitches during probe

Best regards

[1] -
https://lore.kernel.org/all/20200915135445.al75xmjxudj2rgcp@axis.com/T/
[2] - https://github.com/lategoodbye/rpi-zero/commits/v6.7-pwm-gpio/

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

[Linus Walleij]

On Wed, Jan 24, 2024 at 8:37 PM Stefan Wahren <wahrenst@gmx.net> wrote:

> i was working on this a little bit during the holiday. Personally i
> prefer Vincent's approach [1], which is easier to read and more
> consequent by rejecting sleeping GPIOs. So i prepared a WIP branch [2],
> which was tested on a Raspberry Pi 3 B Plus + a cheap Logic analyzer. So
> maybe you want to give it a try. I will try to send a proper series soon.
>
> Changes:
> - rebased Vincent's last patch series on top of Linux 6.7
> - cherry picked some improvements from Nicola's series
> - tried to address Uwe's, Linus' and Andy's comments
> - tried to avoid glitches during probe

This looks good, I guess you will just squash and send it Co-developed-by?

Thanks for looking into this!

Yours,
Linus Walleij