* Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver
2024-02-04 22:08 ` [PATCH V4 2/2] pwm: Add GPIO PWM driver Stefan Wahren
@ 2024-02-04 22:50 ` Linus Walleij
2024-02-05 6:11 ` Dhruva Gole
` (3 subsequent siblings)
4 siblings, 0 replies; 28+ messages in thread
From: Linus Walleij @ 2024-02-04 22:50 UTC (permalink / raw)
To: Stefan Wahren
Cc: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, andy.shevchenko, Angelo Compagnucci, Philip Howard,
Sean Young, linux-pwm, devicetree, linux-gpio, Vincent Whitchurch
On Sun, Feb 4, 2024 at 11:09 PM Stefan Wahren <wahrenst@gmx.net> wrote:
> From: Vincent Whitchurch <vincent.whitchurch@axis.com>
>
> Add a software PWM which toggles a GPIO from a high-resolution timer.
>
> This will naturally not be as accurate or as efficient as a hardware
> PWM, but it is useful in some cases. I have for example used it for
> evaluating LED brightness handling (via leds-pwm) on a board where the
> LED was just hooked up to a GPIO, and for a simple verification of the
> timer frequency on another platform.
>
> Since high-resolution timers are used, sleeping gpio chips are not
> supported and are rejected in the probe function.
>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> Co-developed-by: Stefan Wahren <wahrenst@gmx.net>
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
This sure looks good to me!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver
2024-02-04 22:08 ` [PATCH V4 2/2] pwm: Add GPIO PWM driver Stefan Wahren
2024-02-04 22:50 ` Linus Walleij
@ 2024-02-05 6:11 ` Dhruva Gole
2024-02-05 7:16 ` Stefan Wahren
2024-02-05 10:09 ` Uwe Kleine-König
` (2 subsequent siblings)
4 siblings, 1 reply; 28+ messages in thread
From: Dhruva Gole @ 2024-02-05 6:11 UTC (permalink / raw)
To: Stefan Wahren
Cc: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, andy.shevchenko, Angelo Compagnucci, Philip Howard,
Sean Young, Linus Walleij, linux-pwm, devicetree, linux-gpio,
Vincent Whitchurch
On Feb 04, 2024 at 23:08:51 +0100, Stefan Wahren wrote:
> From: Vincent Whitchurch <vincent.whitchurch@axis.com>
>
> Add a software PWM which toggles a GPIO from a high-resolution timer.
>
> This will naturally not be as accurate or as efficient as a hardware
> PWM, but it is useful in some cases. I have for example used it for
> evaluating LED brightness handling (via leds-pwm) on a board where the
> LED was just hooked up to a GPIO, and for a simple verification of the
> timer frequency on another platform.
>
> Since high-resolution timers are used, sleeping gpio chips are not
> supported and are rejected in the probe function.
>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> Co-developed-by: Stefan Wahren <wahrenst@gmx.net>
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-gpio.c | 228 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 240 insertions(+)
> create mode 100644 drivers/pwm/pwm-gpio.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 4b956d661755..7cfda2cde130 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -227,6 +227,17 @@ 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 "GPIO PWM support"
> + depends on GPIOLIB
> + depends on HIGH_RES_TIMERS
> + help
> + Generic PWM framework driver for a software PWM toggling a GPIO pin
>> a software PWM
Nit: remove the "a", as it's not required.
> + from kernel high-resolution timers.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-gpio.
> +
[..snip..]
> +
> +static int pwm_gpio_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&gpwm->lock, flags);
> +
> + if (gpwm->changing)
> + *state = gpwm->next_state;
> + else
> + *state = gpwm->state;
> +
> + spin_unlock_irqrestore(&gpwm->lock, flags);
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops pwm_gpio_ops = {
> + .apply = pwm_gpio_apply,
Kinda looks like this is setting state? Can we be consistent with the
naming then, like pwm_gpio_set_state?
This will help those contributing to the driver or referring to it to
understand better what each function is doing exactly.
> + .get_state = pwm_gpio_get_state,
> +};
Otherwise, driver looks good to me,
Reviewed-by: Dhruva Gole <d-gole@ti.com>
--
Best regards,
Dhruva Gole <d-gole@ti.com>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver
2024-02-05 6:11 ` Dhruva Gole
@ 2024-02-05 7:16 ` Stefan Wahren
0 siblings, 0 replies; 28+ messages in thread
From: Stefan Wahren @ 2024-02-05 7:16 UTC (permalink / raw)
To: Dhruva Gole
Cc: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, andy.shevchenko, Angelo Compagnucci, Philip Howard,
Sean Young, Linus Walleij, linux-pwm, devicetree, linux-gpio
Hi Dhruva,
[drop Vincent's address because it bounces]
Am 05.02.24 um 07:11 schrieb Dhruva Gole:
> On Feb 04, 2024 at 23:08:51 +0100, Stefan Wahren wrote:
>> From: Vincent Whitchurch <vincent.whitchurch@axis.com>
>>
>> Add a software PWM which toggles a GPIO from a high-resolution timer.
>>
>> This will naturally not be as accurate or as efficient as a hardware
>> PWM, but it is useful in some cases. I have for example used it for
>> evaluating LED brightness handling (via leds-pwm) on a board where the
>> LED was just hooked up to a GPIO, and for a simple verification of the
>> timer frequency on another platform.
>>
>> Since high-resolution timers are used, sleeping gpio chips are not
>> supported and are rejected in the probe function.
>>
>> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
>> Co-developed-by: Stefan Wahren <wahrenst@gmx.net>
>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>> ---
>> drivers/pwm/Kconfig | 11 ++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm-gpio.c | 228 +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 240 insertions(+)
>> create mode 100644 drivers/pwm/pwm-gpio.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 4b956d661755..7cfda2cde130 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -227,6 +227,17 @@ 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 "GPIO PWM support"
>> + depends on GPIOLIB
>> + depends on HIGH_RES_TIMERS
>> + help
>> + Generic PWM framework driver for a software PWM toggling a GPIO pin
>>> a software PWM
> Nit: remove the "a", as it's not required.
thanks will fix this.
>
>> + from kernel high-resolution timers.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called pwm-gpio.
>> +
> [..snip..]
>> +
>> +static int pwm_gpio_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>> + struct pwm_state *state)
>> +{
>> + struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&gpwm->lock, flags);
>> +
>> + if (gpwm->changing)
>> + *state = gpwm->next_state;
>> + else
>> + *state = gpwm->state;
>> +
>> + spin_unlock_irqrestore(&gpwm->lock, flags);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct pwm_ops pwm_gpio_ops = {
>> + .apply = pwm_gpio_apply,
> Kinda looks like this is setting state? Can we be consistent with the
> naming then, like pwm_gpio_set_state?
The pwm_op is called apply, so the expected suffix would be _apply like
all the other PWM driver does.
grep ".apply =" drivers/pwm/*
drivers/pwm/pwm-ab8500.c: .apply = ab8500_pwm_apply,
drivers/pwm/pwm-apple.c: .apply = apple_pwm_apply,
drivers/pwm/pwm-atmel.c: .apply = atmel_pwm_apply,
...
> This will help those contributing to the driver or referring to it to
> understand better what each function is doing exactly.
Using ...set_state here would confuse all the experienced kernel developer.
>
>> + .get_state = pwm_gpio_get_state,
>> +};
> Otherwise, driver looks good to me,
> Reviewed-by: Dhruva Gole <d-gole@ti.com>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver
2024-02-04 22:08 ` [PATCH V4 2/2] pwm: Add GPIO PWM driver Stefan Wahren
2024-02-04 22:50 ` Linus Walleij
2024-02-05 6:11 ` Dhruva Gole
@ 2024-02-05 10:09 ` Uwe Kleine-König
2024-02-05 12:21 ` Stefan Wahren
2024-02-27 15:32 ` Chris Morgan
2024-02-27 16:41 ` Andy Shevchenko
4 siblings, 1 reply; 28+ messages in thread
From: Uwe Kleine-König @ 2024-02-05 10:09 UTC (permalink / raw)
To: Stefan Wahren
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, andy.shevchenko,
Angelo Compagnucci, Philip Howard, Sean Young, Linus Walleij,
linux-pwm, devicetree, linux-gpio, Vincent Whitchurch
[-- Attachment #1: Type: text/plain, Size: 5469 bytes --]
Hello,
On Sun, Feb 04, 2024 at 11:08:51PM +0100, Stefan Wahren wrote:
> +static enum hrtimer_restart pwm_gpio_timer(struct hrtimer *gpio_timer)
> +{
> + struct pwm_gpio *gpwm = container_of(gpio_timer, struct pwm_gpio,
> + gpio_timer);
> + unsigned long next_toggle;
> + unsigned long flags;
> + bool new_level;
> +
> + spin_lock_irqsave(&gpwm->lock, flags);
> +
> + /* Apply new state at end of current period */
> + if (!gpwm->level && gpwm->changing) {
> + gpwm->changing = false;
> + gpwm->state = gpwm->next_state;
> + new_level = !!gpwm->state.duty_cycle;
> + } else {
> + new_level = !gpwm->level;
> + }
> +
> + next_toggle = pwm_gpio_toggle(gpwm, new_level);
> + if (next_toggle) {
> + hrtimer_forward(gpio_timer, hrtimer_get_expires(gpio_timer),
> + ns_to_ktime(next_toggle));
How does this work in relation to hrtimer_resolution? If the resolution
is (say) 300 and next_toggle is 2000. Does the trigger trigger at
hrtimer_get_expires(...) + 1800, or at ... + 2100?
If you assume we have period = 6000 and duty_cycle = 2000, the delta to
forward the driver alternates between 1800 and 3900 (if rounding down)
or between 2100 and 4200 if rounding up. Both is not optimal.
Ideally you'd round down the active phase (i.e. pick 1800) and for the
inactive phase you'd use rounddown(period) - rounddown(duty_cycle) which
gives 4200 here. Does this make sense?
> + }
> +
> + spin_unlock_irqrestore(&gpwm->lock, flags);
> +
> + return next_toggle ? HRTIMER_RESTART : HRTIMER_NORESTART;
> +}
> +
> +static int pwm_gpio_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
> + bool invert = state->polarity == PWM_POLARITY_INVERSED;
> + unsigned long flags;
> +
> + if (state->duty_cycle && state->duty_cycle < hrtimer_resolution)
> + return -EINVAL;
> +
> + if (state->duty_cycle != state->period &&
> + (state->period - state->duty_cycle < hrtimer_resolution))
> + return -EINVAL;
If you assume that hrtimer_resolution = 300 again, you don't want to
refuse
.duty_cycle = 200
.period = 200
do you? I think you want:
mininterval = min(state->duty_cycle, state->period - state->duty_cycle);
if (mininterval && mininterval < hrtimer_resolution)
return -EINVAL;
to catch both cases in a single check.
> + if (!state->enabled) {
> + hrtimer_cancel(&gpwm->gpio_timer);
> + } else if (!gpwm->running) {
> + /*
> + * This just enables the output, but pwm_gpio_toggle()
> + * really starts the duty cycle.
> + */
> + int ret = gpiod_direction_output(gpwm->gpio, invert);
> +
> + if (ret)
> + return ret;
> + }
> +
> + spin_lock_irqsave(&gpwm->lock, flags);
> +
> + if (!state->enabled) {
> + gpwm->state = *state;
> + gpwm->running = false;
> + gpwm->changing = false;
> +
> + gpiod_set_value(gpwm->gpio, invert);
> + } else if (gpwm->running) {
> + gpwm->next_state = *state;
> + gpwm->changing = true;
> + } else {
> + unsigned long next_toggle;
> +
> + gpwm->state = *state;
> + gpwm->changing = false;
> +
> + next_toggle = pwm_gpio_toggle(gpwm, !!state->duty_cycle);
> + if (next_toggle) {
> + hrtimer_start(&gpwm->gpio_timer, next_toggle,
> + HRTIMER_MODE_REL);
> + }
The curly braces can be dropped here and in a few more locations.
> + }
> +
> + spin_unlock_irqrestore(&gpwm->lock, flags);
> +
> + return 0;
> +}
> +
> +static int pwm_gpio_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&gpwm->lock, flags);
> +
> + if (gpwm->changing)
> + *state = gpwm->next_state;
> + else
> + *state = gpwm->state;
> +
> + spin_unlock_irqrestore(&gpwm->lock, flags);
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops pwm_gpio_ops = {
> + .apply = pwm_gpio_apply,
> + .get_state = pwm_gpio_get_state,
> +};
> +
> +static int pwm_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct pwm_gpio *gpwm;
> + int ret;
> +
> + gpwm = devm_kzalloc(dev, sizeof(*gpwm), GFP_KERNEL);
> + if (!gpwm)
> + return -ENOMEM;
> +
> + spin_lock_init(&gpwm->lock);
> +
> + gpwm->gpio = devm_gpiod_get(dev, NULL, GPIOD_ASIS);
> + if (IS_ERR(gpwm->gpio)) {
> + return dev_err_probe(dev, PTR_ERR(gpwm->gpio),
> + "could not get gpio\n");
> + }
> +
> + if (gpiod_cansleep(gpwm->gpio)) {
> + return dev_err_probe(dev, -EINVAL,
> + "sleeping GPIO %d not supported\n",
> + desc_to_gpio(gpwm->gpio));
> + }
Is it still state of the art to add gpio numbers to error messages?
> + gpwm->chip.dev = dev;
> + gpwm->chip.ops = &pwm_gpio_ops;
> + gpwm->chip.npwm = 1;
> + gpwm->chip.atomic = true;
> +
> + hrtimer_init(&gpwm->gpio_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + gpwm->gpio_timer.function = pwm_gpio_timer;
> +
> + ret = pwmchip_add(&gpwm->chip);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "could not add pwmchip\n");
> +
> + platform_set_drvdata(pdev, gpwm);
> +
> + 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver
2024-02-05 10:09 ` Uwe Kleine-König
@ 2024-02-05 12:21 ` Stefan Wahren
0 siblings, 0 replies; 28+ messages in thread
From: Stefan Wahren @ 2024-02-05 12:21 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, andy.shevchenko,
Angelo Compagnucci, Philip Howard, Sean Young, Linus Walleij,
linux-pwm, devicetree, linux-gpio, Vincent Whitchurch
Hi Uwe,
Am 05.02.24 um 11:09 schrieb Uwe Kleine-König:
> Hello,
>
> On Sun, Feb 04, 2024 at 11:08:51PM +0100, Stefan Wahren wrote:
>> +static enum hrtimer_restart pwm_gpio_timer(struct hrtimer *gpio_timer)
>> +{
>> + struct pwm_gpio *gpwm = container_of(gpio_timer, struct pwm_gpio,
>> + gpio_timer);
>> + unsigned long next_toggle;
>> + unsigned long flags;
>> + bool new_level;
>> +
>> + spin_lock_irqsave(&gpwm->lock, flags);
>> +
>> + /* Apply new state at end of current period */
>> + if (!gpwm->level && gpwm->changing) {
>> + gpwm->changing = false;
>> + gpwm->state = gpwm->next_state;
>> + new_level = !!gpwm->state.duty_cycle;
>> + } else {
>> + new_level = !gpwm->level;
>> + }
>> +
>> + next_toggle = pwm_gpio_toggle(gpwm, new_level);
>> + if (next_toggle) {
>> + hrtimer_forward(gpio_timer, hrtimer_get_expires(gpio_timer),
>> + ns_to_ktime(next_toggle));
> How does this work in relation to hrtimer_resolution? If the resolution
> is (say) 300 and next_toggle is 2000. Does the trigger trigger at
> hrtimer_get_expires(...) + 1800, or at ... + 2100?
>
> If you assume we have period = 6000 and duty_cycle = 2000, the delta to
> forward the driver alternates between 1800 and 3900 (if rounding down)
> or between 2100 and 4200 if rounding up. Both is not optimal.
>
> Ideally you'd round down the active phase (i.e. pick 1800) and for the
> inactive phase you'd use rounddown(period) - rounddown(duty_cycle) which
> gives 4200 here. Does this make sense?
oh dear, looks like a can of worms. I will look into it. Btw according
to multi_v7_defconfig the hrtimer_resolution on the Pi is 1 ns.
Does it make sense to log the hrtimer_resolution e.g. at probe?
>
>> + }
>> +
>> + spin_unlock_irqrestore(&gpwm->lock, flags);
>> +
>> + return next_toggle ? HRTIMER_RESTART : HRTIMER_NORESTART;
>> +}
>> +
>> +static int pwm_gpio_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> + const struct pwm_state *state)
>> +{
>> + struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
>> + bool invert = state->polarity == PWM_POLARITY_INVERSED;
>> + unsigned long flags;
>> +
>> + if (state->duty_cycle && state->duty_cycle < hrtimer_resolution)
>> + return -EINVAL;
>> +
>> + if (state->duty_cycle != state->period &&
>> + (state->period - state->duty_cycle < hrtimer_resolution))
>> + return -EINVAL;
> If you assume that hrtimer_resolution = 300 again, you don't want to
> refuse
>
> .duty_cycle = 200
> .period = 200
>
> do you?
Actually i had rejected it. Yes, this specific corner case does work
with such a resolution. But if the user want a steady level, the user
would use a plain GPIO not a PWM. As soon the duty cycle get lower this
would be rejected and as a user i would be confused.
Another issue here, is that we don't have a good interface to tell the
limitations.
> I think you want:
>
> mininterval = min(state->duty_cycle, state->period - state->duty_cycle);
> if (mininterval && mininterval < hrtimer_resolution)
> return -EINVAL;
>
> to catch both cases in a single check.
>
>> + if (!state->enabled) {
>> + hrtimer_cancel(&gpwm->gpio_timer);
>> + } else if (!gpwm->running) {
>> + /*
>> + * This just enables the output, but pwm_gpio_toggle()
>> + * really starts the duty cycle.
>> + */
>> + int ret = gpiod_direction_output(gpwm->gpio, invert);
>> +
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + spin_lock_irqsave(&gpwm->lock, flags);
>> +
>> + if (!state->enabled) {
>> + gpwm->state = *state;
>> + gpwm->running = false;
>> + gpwm->changing = false;
>> +
>> + gpiod_set_value(gpwm->gpio, invert);
>> + } else if (gpwm->running) {
>> + gpwm->next_state = *state;
>> + gpwm->changing = true;
>> + } else {
>> + unsigned long next_toggle;
>> +
>> + gpwm->state = *state;
>> + gpwm->changing = false;
>> +
>> + next_toggle = pwm_gpio_toggle(gpwm, !!state->duty_cycle);
>> + if (next_toggle) {
>> + hrtimer_start(&gpwm->gpio_timer, next_toggle,
>> + HRTIMER_MODE_REL);
>> + }
> The curly braces can be dropped here and in a few more locations.
>
>> + }
>> +
>> + spin_unlock_irqrestore(&gpwm->lock, flags);
>> +
>> + return 0;
>> +}
>> +
>> +static int pwm_gpio_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>> + struct pwm_state *state)
>> +{
>> + struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&gpwm->lock, flags);
>> +
>> + if (gpwm->changing)
>> + *state = gpwm->next_state;
>> + else
>> + *state = gpwm->state;
>> +
>> + spin_unlock_irqrestore(&gpwm->lock, flags);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct pwm_ops pwm_gpio_ops = {
>> + .apply = pwm_gpio_apply,
>> + .get_state = pwm_gpio_get_state,
>> +};
>> +
>> +static int pwm_gpio_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct pwm_gpio *gpwm;
>> + int ret;
>> +
>> + gpwm = devm_kzalloc(dev, sizeof(*gpwm), GFP_KERNEL);
>> + if (!gpwm)
>> + return -ENOMEM;
>> +
>> + spin_lock_init(&gpwm->lock);
>> +
>> + gpwm->gpio = devm_gpiod_get(dev, NULL, GPIOD_ASIS);
>> + if (IS_ERR(gpwm->gpio)) {
>> + return dev_err_probe(dev, PTR_ERR(gpwm->gpio),
>> + "could not get gpio\n");
>> + }
>> +
>> + if (gpiod_cansleep(gpwm->gpio)) {
>> + return dev_err_probe(dev, -EINVAL,
>> + "sleeping GPIO %d not supported\n",
>> + desc_to_gpio(gpwm->gpio));
>> + }
> Is it still state of the art to add gpio numbers to error messages?
I was unsure how to handle this user-friendly. Just simply logging
"sleeping GPIO not supported" doesn't provide a reference on the
affected GPIO. GPIO names are optional, so maybe empty.
I'm open to suggestions :-)
Best regards
>
>> + gpwm->chip.dev = dev;
>> + gpwm->chip.ops = &pwm_gpio_ops;
>> + gpwm->chip.npwm = 1;
>> + gpwm->chip.atomic = true;
>> +
>> + hrtimer_init(&gpwm->gpio_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>> + gpwm->gpio_timer.function = pwm_gpio_timer;
>> +
>> + ret = pwmchip_add(&gpwm->chip);
>> + if (ret < 0)
>> + return dev_err_probe(dev, ret, "could not add pwmchip\n");
>> +
>> + platform_set_drvdata(pdev, gpwm);
>> +
>> + return 0;
>> +}
> Best regards
> Uwe
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver
2024-02-04 22:08 ` [PATCH V4 2/2] pwm: Add GPIO PWM driver Stefan Wahren
` (2 preceding siblings ...)
2024-02-05 10:09 ` Uwe Kleine-König
@ 2024-02-27 15:32 ` Chris Morgan
2024-02-27 16:59 ` Stefan Wahren
2024-02-27 16:41 ` Andy Shevchenko
4 siblings, 1 reply; 28+ messages in thread
From: Chris Morgan @ 2024-02-27 15:32 UTC (permalink / raw)
To: Stefan Wahren
Cc: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, andy.shevchenko, Angelo Compagnucci, Philip Howard,
Sean Young, Linus Walleij, linux-pwm, devicetree, linux-gpio,
Vincent Whitchurch
On Sun, Feb 04, 2024 at 11:08:51PM +0100, Stefan Wahren wrote:
> From: Vincent Whitchurch <vincent.whitchurch@axis.com>
>
> Add a software PWM which toggles a GPIO from a high-resolution timer.
>
> This will naturally not be as accurate or as efficient as a hardware
> PWM, but it is useful in some cases. I have for example used it for
> evaluating LED brightness handling (via leds-pwm) on a board where the
> LED was just hooked up to a GPIO, and for a simple verification of the
> timer frequency on another platform.
>
> Since high-resolution timers are used, sleeping gpio chips are not
> supported and are rejected in the probe function.
>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> Co-developed-by: Stefan Wahren <wahrenst@gmx.net>
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-gpio.c | 228 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 240 insertions(+)
> create mode 100644 drivers/pwm/pwm-gpio.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 4b956d661755..7cfda2cde130 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -227,6 +227,17 @@ 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 "GPIO PWM support"
> + depends on GPIOLIB
> + depends on HIGH_RES_TIMERS
> + help
> + Generic PWM framework driver for a software PWM toggling a GPIO pin
> + from kernel high-resolution timers.
> +
> + 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 c5ec9e168ee7..59d1a46bb1af 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_DWC_CORE) += pwm-dwc-core.o
> obj-$(CONFIG_PWM_DWC) += pwm-dwc.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..4435f275f509
> --- /dev/null
> +++ b/drivers/pwm/pwm-gpio.c
> @@ -0,0 +1,228 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Generic software PWM for modulating GPIOs
> + *
> + * Copyright (C) 2020 Axis Communications AB
> + * Copyright (C) 2020 Nicola Di Lieto
> + * Copyright (C) 2024 Stefan Wahren
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/hrtimer.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/spinlock.h>
> +
> +struct pwm_gpio {
> + struct pwm_chip chip;
> + struct hrtimer gpio_timer;
> + struct gpio_desc *gpio;
> + struct pwm_state state;
> + struct pwm_state next_state;
> +
> + /* Protect internal state between pwm_ops and hrtimer */
> + spinlock_t lock;
> +
> + bool changing;
> + bool running;
> + bool level;
> +};
> +
> +static unsigned long pwm_gpio_toggle(struct pwm_gpio *gpwm, bool level)
> +{
> + const struct pwm_state *state = &gpwm->state;
> + bool invert = state->polarity == PWM_POLARITY_INVERSED;
> +
> + gpwm->level = level;
> + gpiod_set_value(gpwm->gpio, gpwm->level ^ invert);
> +
> + if (!state->duty_cycle || state->duty_cycle == state->period) {
> + gpwm->running = false;
> + return 0;
> + }
> +
> + gpwm->running = true;
> + return level ? state->duty_cycle : state->period - state->duty_cycle;
> +}
> +
> +static enum hrtimer_restart pwm_gpio_timer(struct hrtimer *gpio_timer)
> +{
> + struct pwm_gpio *gpwm = container_of(gpio_timer, struct pwm_gpio,
> + gpio_timer);
> + unsigned long next_toggle;
> + unsigned long flags;
> + bool new_level;
> +
> + spin_lock_irqsave(&gpwm->lock, flags);
> +
> + /* Apply new state at end of current period */
> + if (!gpwm->level && gpwm->changing) {
> + gpwm->changing = false;
> + gpwm->state = gpwm->next_state;
> + new_level = !!gpwm->state.duty_cycle;
> + } else {
> + new_level = !gpwm->level;
> + }
> +
> + next_toggle = pwm_gpio_toggle(gpwm, new_level);
> + if (next_toggle) {
> + hrtimer_forward(gpio_timer, hrtimer_get_expires(gpio_timer),
> + ns_to_ktime(next_toggle));
> + }
> +
> + spin_unlock_irqrestore(&gpwm->lock, flags);
> +
> + return next_toggle ? HRTIMER_RESTART : HRTIMER_NORESTART;
> +}
> +
> +static int pwm_gpio_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
> + bool invert = state->polarity == PWM_POLARITY_INVERSED;
> + unsigned long flags;
> +
> + if (state->duty_cycle && state->duty_cycle < hrtimer_resolution)
> + return -EINVAL;
> +
> + if (state->duty_cycle != state->period &&
> + (state->period - state->duty_cycle < hrtimer_resolution))
> + return -EINVAL;
> +
> + if (!state->enabled) {
> + hrtimer_cancel(&gpwm->gpio_timer);
> + } else if (!gpwm->running) {
> + /*
> + * This just enables the output, but pwm_gpio_toggle()
> + * really starts the duty cycle.
> + */
> + int ret = gpiod_direction_output(gpwm->gpio, invert);
> +
> + if (ret)
> + return ret;
> + }
> +
> + spin_lock_irqsave(&gpwm->lock, flags);
> +
> + if (!state->enabled) {
> + gpwm->state = *state;
> + gpwm->running = false;
> + gpwm->changing = false;
> +
> + gpiod_set_value(gpwm->gpio, invert);
> + } else if (gpwm->running) {
> + gpwm->next_state = *state;
> + gpwm->changing = true;
> + } else {
> + unsigned long next_toggle;
> +
> + gpwm->state = *state;
> + gpwm->changing = false;
> +
> + next_toggle = pwm_gpio_toggle(gpwm, !!state->duty_cycle);
> + if (next_toggle) {
> + hrtimer_start(&gpwm->gpio_timer, next_toggle,
> + HRTIMER_MODE_REL);
> + }
> + }
> +
> + spin_unlock_irqrestore(&gpwm->lock, flags);
> +
> + return 0;
> +}
> +
> +static int pwm_gpio_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&gpwm->lock, flags);
> +
> + if (gpwm->changing)
> + *state = gpwm->next_state;
> + else
> + *state = gpwm->state;
> +
> + spin_unlock_irqrestore(&gpwm->lock, flags);
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops pwm_gpio_ops = {
> + .apply = pwm_gpio_apply,
> + .get_state = pwm_gpio_get_state,
> +};
> +
> +static int pwm_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct pwm_gpio *gpwm;
> + int ret;
> +
> + gpwm = devm_kzalloc(dev, sizeof(*gpwm), GFP_KERNEL);
> + if (!gpwm)
> + return -ENOMEM;
> +
> + spin_lock_init(&gpwm->lock);
> +
> + gpwm->gpio = devm_gpiod_get(dev, NULL, GPIOD_ASIS);
> + if (IS_ERR(gpwm->gpio)) {
> + return dev_err_probe(dev, PTR_ERR(gpwm->gpio),
> + "could not get gpio\n");
> + }
> +
> + if (gpiod_cansleep(gpwm->gpio)) {
> + return dev_err_probe(dev, -EINVAL,
> + "sleeping GPIO %d not supported\n",
> + desc_to_gpio(gpwm->gpio));
> + }
> +
> + gpwm->chip.dev = dev;
> + gpwm->chip.ops = &pwm_gpio_ops;
> + gpwm->chip.npwm = 1;
> + gpwm->chip.atomic = true;
> +
> + hrtimer_init(&gpwm->gpio_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + gpwm->gpio_timer.function = pwm_gpio_timer;
> +
> + ret = pwmchip_add(&gpwm->chip);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "could not add pwmchip\n");
> +
> + platform_set_drvdata(pdev, gpwm);
> +
> + return 0;
> +}
> +
> +static void pwm_gpio_remove(struct platform_device *pdev)
> +{
> + struct pwm_gpio *gpwm = platform_get_drvdata(pdev);
> +
> + pwmchip_remove(&gpwm->chip);
> + hrtimer_cancel(&gpwm->gpio_timer);
> +}
> +
> +static const struct of_device_id pwm_gpio_dt_ids[] = {
> + { .compatible = "pwm-gpio" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, pwm_gpio_dt_ids);
> +
> +static struct platform_driver pwm_gpio_driver = {
> + .driver = {
> + .name = "pwm-gpio",
> + .of_match_table = pwm_gpio_dt_ids,
> + },
> + .probe = pwm_gpio_probe,
> + .remove_new = pwm_gpio_remove,
> +};
> +module_platform_driver(pwm_gpio_driver);
> +
> +MODULE_DESCRIPTION("PWM GPIO driver");
> +MODULE_AUTHOR("Vincent Whitchurch");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
I have a series of devices with GPIO controlled force feedback that
this driver helps us control better. So I'm looking forward to this,
thank you.
Note that when I set the resolution too low (I got confused and set
the period to 255) my device locked up hard and only a forced
power cycle could bring it back.
Tested-by: Chris Morgan <macromorgan@hotmail.com>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver
2024-02-27 15:32 ` Chris Morgan
@ 2024-02-27 16:59 ` Stefan Wahren
2024-02-28 12:43 ` Phil Howard
2024-02-29 8:45 ` Sean Young
0 siblings, 2 replies; 28+ messages in thread
From: Stefan Wahren @ 2024-02-27 16:59 UTC (permalink / raw)
To: Chris Morgan
Cc: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, andy.shevchenko, Angelo Compagnucci, Philip Howard,
Sean Young, Linus Walleij, linux-pwm, devicetree, linux-gpio,
Vincent Whitchurch
Hi Chris,
Am 27.02.24 um 16:32 schrieb Chris Morgan:
> I have a series of devices with GPIO controlled force feedback that
> this driver helps us control better. So I'm looking forward to this,
> thank you.
Thanks for testing. I didn't had much time recently and i was fighting
with hr timer resolution stuff. But will try to send the next version in
March.
> Note that when I set the resolution too low (I got confused and set
> the period to 255) my device locked up hard and only a forced
> power cycle could bring it back.
Unfortunately this is a general design issue by driving the GPIO by a
kernel driver and "expected" behavior. I didn't have a good solution for
it yet.
What period is too low without limiting other users?
The only idea which comes to my mind is to introduce a kernel parameter
for this driver to set a lower period limit. This can be provided by
some administrator or system designer with enough experience. So a
general user doesn't need to care about it.
Best regards
> Tested-by: Chris Morgan <macromorgan@hotmail.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver
2024-02-27 16:59 ` Stefan Wahren
@ 2024-02-28 12:43 ` Phil Howard
2024-02-29 8:45 ` Sean Young
1 sibling, 0 replies; 28+ messages in thread
From: Phil Howard @ 2024-02-28 12:43 UTC (permalink / raw)
To: Stefan Wahren
Cc: Chris Morgan, Uwe Kleine-König, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, andy.shevchenko,
Angelo Compagnucci, Sean Young, Linus Walleij, linux-pwm,
devicetree, linux-gpio, Vincent Whitchurch
On Tue, 27 Feb 2024 at 16:59, Stefan Wahren <wahrenst@gmx.net> wrote:
>
> Hi Chris,
>
> Am 27.02.24 um 16:32 schrieb Chris Morgan:
> > I have a series of devices with GPIO controlled force feedback that
> > this driver helps us control better. So I'm looking forward to this,
> > thank you.
> Thanks for testing. I didn't had much time recently and i was fighting
> with hr timer resolution stuff. But will try to send the next version in
> March.
> > Note that when I set the resolution too low (I got confused and set
> > the period to 255) my device locked up hard and only a forced
> > power cycle could bring it back.
> Unfortunately this is a general design issue by driving the GPIO by a
> kernel driver and "expected" behavior. I didn't have a good solution for
> it yet.
>
> What period is too low without limiting other users?
>
> The only idea which comes to my mind is to introduce a kernel parameter
> for this driver to set a lower period limit. This can be provided by
> some administrator or system designer with enough experience. So a
> general user doesn't need to care about it.
This works for me. I also mucked up the period to see what appears to be
a signal in the MHz range, but got a dropped SSH connection for my troubles.
255ns would be ~3.9MHz which is quite spectacularly far outside of the
range I've come to expect from software PWM, but any conservative hard
limit would be trivialised by a faster CPU.
>
> Best regards
>
> > Tested-by: Chris Morgan <macromorgan@hotmail.com>
>
--
Philip Howard
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver
2024-02-27 16:59 ` Stefan Wahren
2024-02-28 12:43 ` Phil Howard
@ 2024-02-29 8:45 ` Sean Young
2024-02-29 8:57 ` Sean Young
1 sibling, 1 reply; 28+ messages in thread
From: Sean Young @ 2024-02-29 8:45 UTC (permalink / raw)
To: Stefan Wahren
Cc: Chris Morgan, Uwe Kleine-König, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, andy.shevchenko,
Angelo Compagnucci, Philip Howard, Linus Walleij, linux-pwm,
devicetree, linux-gpio, Vincent Whitchurch
On Tue, Feb 27, 2024 at 05:59:40PM +0100, Stefan Wahren wrote:
> Hi Chris,
>
> Am 27.02.24 um 16:32 schrieb Chris Morgan:
> > I have a series of devices with GPIO controlled force feedback that
> > this driver helps us control better. So I'm looking forward to this,
> > thank you.
> Thanks for testing. I didn't had much time recently and i was fighting
> with hr timer resolution stuff. But will try to send the next version in
> March.
> > Note that when I set the resolution too low (I got confused and set
> > the period to 255) my device locked up hard and only a forced
> > power cycle could bring it back.
> Unfortunately this is a general design issue by driving the GPIO by a
> kernel driver and "expected" behavior. I didn't have a good solution for
> it yet.
When we reprogram the timer with hrtimer_forward(), we could check whether
we have overrun - i.e. we are already beyond the expires time. This could
be a hint that a) we cannot generate the pwm signal and b) this might
be what causes the hang, because we are returning HRTIMER_RESTART yet
no new expires has been programmed.
Crashing the machine if the period is too short is not really good enough
for mainline, I think. There is talk of pwm chardevs in the future.
Sean
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver
2024-02-29 8:45 ` Sean Young
@ 2024-02-29 8:57 ` Sean Young
0 siblings, 0 replies; 28+ messages in thread
From: Sean Young @ 2024-02-29 8:57 UTC (permalink / raw)
To: Stefan Wahren
Cc: Chris Morgan, Uwe Kleine-König, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, andy.shevchenko,
Angelo Compagnucci, Philip Howard, Linus Walleij, linux-pwm,
devicetree, linux-gpio, Vincent Whitchurch
On Thu, Feb 29, 2024 at 08:45:54AM +0000, Sean Young wrote:
> On Tue, Feb 27, 2024 at 05:59:40PM +0100, Stefan Wahren wrote:
> > Hi Chris,
> >
> > Am 27.02.24 um 16:32 schrieb Chris Morgan:
> > > I have a series of devices with GPIO controlled force feedback that
> > > this driver helps us control better. So I'm looking forward to this,
> > > thank you.
> > Thanks for testing. I didn't had much time recently and i was fighting
> > with hr timer resolution stuff. But will try to send the next version in
> > March.
> > > Note that when I set the resolution too low (I got confused and set
> > > the period to 255) my device locked up hard and only a forced
> > > power cycle could bring it back.
> > Unfortunately this is a general design issue by driving the GPIO by a
> > kernel driver and "expected" behavior. I didn't have a good solution for
> > it yet.
>
> When we reprogram the timer with hrtimer_forward(), we could check whether
> we have overrun - i.e. we are already beyond the expires time. This could
> be a hint that a) we cannot generate the pwm signal and b) this might
> be what causes the hang, because we are returning HRTIMER_RESTART yet
> no new expires has been programmed.
I mean something like:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/pwm-ir-tx.c#n144
Sean
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver
2024-02-04 22:08 ` [PATCH V4 2/2] pwm: Add GPIO PWM driver Stefan Wahren
` (3 preceding siblings ...)
2024-02-27 15:32 ` Chris Morgan
@ 2024-02-27 16:41 ` Andy Shevchenko
2024-02-27 20:24 ` Stefan Wahren
4 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2024-02-27 16:41 UTC (permalink / raw)
To: Stefan Wahren
Cc: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Angelo Compagnucci, Philip Howard, Sean Young,
Linus Walleij, linux-pwm, devicetree, linux-gpio,
Vincent Whitchurch
On Sun, Feb 04, 2024 at 11:08:51PM +0100, Stefan Wahren wrote:
> From: Vincent Whitchurch <vincent.whitchurch@axis.com>
>
> Add a software PWM which toggles a GPIO from a high-resolution timer.
>
> This will naturally not be as accurate or as efficient as a hardware
> PWM, but it is useful in some cases. I have for example used it for
> evaluating LED brightness handling (via leds-pwm) on a board where the
> LED was just hooked up to a GPIO, and for a simple verification of the
> timer frequency on another platform.
>
> Since high-resolution timers are used, sleeping gpio chips are not
GPIO
> supported and are rejected in the probe function.
Overall LGTM, but I have a few comments below.
...
+ container_of.h
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/hrtimer.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/spinlock.h>
+ time.h
+ types.h
...
> +static enum hrtimer_restart pwm_gpio_timer(struct hrtimer *gpio_timer)
> +{
> + struct pwm_gpio *gpwm = container_of(gpio_timer, struct pwm_gpio,
> + gpio_timer);
> + unsigned long next_toggle;
> + unsigned long flags;
> + bool new_level;
> + spin_lock_irqsave(&gpwm->lock, flags);
Can we use cleanup.h from day 1?
> + /* Apply new state at end of current period */
> + if (!gpwm->level && gpwm->changing) {
> + gpwm->changing = false;
> + gpwm->state = gpwm->next_state;
> + new_level = !!gpwm->state.duty_cycle;
> + } else {
> + new_level = !gpwm->level;
> + }
> +
> + next_toggle = pwm_gpio_toggle(gpwm, new_level);
> + if (next_toggle) {
> + hrtimer_forward(gpio_timer, hrtimer_get_expires(gpio_timer),
> + ns_to_ktime(next_toggle));
> + }
> +
> + spin_unlock_irqrestore(&gpwm->lock, flags);
> +
> + return next_toggle ? HRTIMER_RESTART : HRTIMER_NORESTART;
> +}
...
> + /*
> + * This just enables the output, but pwm_gpio_toggle()
> + * really starts the duty cycle.
> + */
> + int ret = gpiod_direction_output(gpwm->gpio, invert);
> +
> + if (ret)
> + return ret;
Better to have it written as
int ret;
/*
* This just enables the output, but pwm_gpio_toggle()
* really starts the duty cycle.
*/
ret = gpiod_direction_output(gpwm->gpio, invert);
if (ret)
return ret;
...
> + gpwm->gpio = devm_gpiod_get(dev, NULL, GPIOD_ASIS);
> + if (IS_ERR(gpwm->gpio)) {
> + return dev_err_probe(dev, PTR_ERR(gpwm->gpio),
> + "could not get gpio\n");
> + }
{} are not needed.
...
> + if (gpiod_cansleep(gpwm->gpio)) {
> + return dev_err_probe(dev, -EINVAL,
> + "sleeping GPIO %d not supported\n",
> + desc_to_gpio(gpwm->gpio));
Do not use plain GPIO numbers.
> + }
{} are not needed.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver
2024-02-27 16:41 ` Andy Shevchenko
@ 2024-02-27 20:24 ` Stefan Wahren
2024-02-27 20:47 ` Andy Shevchenko
0 siblings, 1 reply; 28+ messages in thread
From: Stefan Wahren @ 2024-02-27 20:24 UTC (permalink / raw)
To: Andy Shevchenko, Uwe Kleine-König
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Angelo Compagnucci, Philip Howard, Sean Young, Linus Walleij,
linux-pwm, devicetree, linux-gpio, Vincent Whitchurch
Hi,
Am 27.02.24 um 17:41 schrieb Andy Shevchenko:
> On Sun, Feb 04, 2024 at 11:08:51PM +0100, Stefan Wahren wrote:
> ...
>
>> + if (gpiod_cansleep(gpwm->gpio)) {
>> + return dev_err_probe(dev, -EINVAL,
>> + "sleeping GPIO %d not supported\n",
>> + desc_to_gpio(gpwm->gpio));
> Do not use plain GPIO numbers.
Uwe already complained this, but i didn't receive a reply on the
question how do we provide a useful log message (reference to the
affected GPIO) here? AFAIK the GPIO names are optional.
Regards
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver
2024-02-27 20:24 ` Stefan Wahren
@ 2024-02-27 20:47 ` Andy Shevchenko
2024-02-28 18:06 ` Stefan Wahren
0 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2024-02-27 20:47 UTC (permalink / raw)
To: Stefan Wahren
Cc: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Angelo Compagnucci, Philip Howard, Sean Young,
Linus Walleij, linux-pwm, devicetree, linux-gpio,
Vincent Whitchurch
On Tue, Feb 27, 2024 at 10:25 PM Stefan Wahren <wahrenst@gmx.net> wrote:
>
> Hi,
>
> Am 27.02.24 um 17:41 schrieb Andy Shevchenko:
> > On Sun, Feb 04, 2024 at 11:08:51PM +0100, Stefan Wahren wrote:
> > ...
> >
> >> + if (gpiod_cansleep(gpwm->gpio)) {
> >> + return dev_err_probe(dev, -EINVAL,
> >> + "sleeping GPIO %d not supported\n",
> >> + desc_to_gpio(gpwm->gpio));
> > Do not use plain GPIO numbers.
> Uwe already complained this, but i didn't receive a reply on the
> question how do we provide a useful log message (reference to the
> affected GPIO) here? AFAIK the GPIO names are optional.
You have a firmware node path, also you may add a label to GPIO, but
it's unrelated to the message (as it's constant).
%pfw
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver
2024-02-27 20:47 ` Andy Shevchenko
@ 2024-02-28 18:06 ` Stefan Wahren
0 siblings, 0 replies; 28+ messages in thread
From: Stefan Wahren @ 2024-02-28 18:06 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Angelo Compagnucci, Philip Howard, Sean Young,
Linus Walleij, linux-pwm, devicetree, linux-gpio
Am 27.02.24 um 21:47 schrieb Andy Shevchenko:
> On Tue, Feb 27, 2024 at 10:25 PM Stefan Wahren <wahrenst@gmx.net> wrote:
>> Hi,
>>
>> Am 27.02.24 um 17:41 schrieb Andy Shevchenko:
>>> On Sun, Feb 04, 2024 at 11:08:51PM +0100, Stefan Wahren wrote:
>>> ...
>>>
>>>> + if (gpiod_cansleep(gpwm->gpio)) {
>>>> + return dev_err_probe(dev, -EINVAL,
>>>> + "sleeping GPIO %d not supported\n",
>>>> + desc_to_gpio(gpwm->gpio));
>>> Do not use plain GPIO numbers.
>> Uwe already complained this, but i didn't receive a reply on the
>> question how do we provide a useful log message (reference to the
>> affected GPIO) here? AFAIK the GPIO names are optional.
> You have a firmware node path, also you may add a label to GPIO, but
> it's unrelated to the message (as it's constant).
> %pfw
Thanks
^ permalink raw reply [flat|nested] 28+ messages in thread