mbox series

[v7,0/2] pwm: Add GPIO PWM driver

Message ID 20240604-pwm-gpio-v7-0-6b67cf60db92@linaro.org
Headers show
Series pwm: Add GPIO PWM driver | expand

Message

Linus Walleij June 4, 2024, 9 p.m. UTC
Add a software PWM which toggles a GPIO from a high-resolution timer.

Recent discussions in the Raspberry Pi community revealt that a lot
of users still use MMIO userspace tools for GPIO access. One argument
for this approach is the lack of a GPIO PWM kernel driver. So this
series tries to fill this gap.

This continues the work of Vincent Whitchurch [1], which is easier
to read and more consequent by rejecting sleeping GPIOs than Nicola's
approach [2]. It further takes over where Stefan Wahren left off.

I have not looked into the interrupt storm problem mentioned in [3]
but instead focused on some real-life tests:

The IXP4xx NSLU2 has a speaker connected directly to a GPIO, and I
wanted to use this patch to provide a proper beeper for the machine
and not have to rely on custom hacks.

I added a DTS patch like this:

gpio_pwm: pwm {
        #pwm-cells = <3>;
        compatible = "pwm-gpio";
        gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
};

beeper {
        compatible = "pwm-beeper";
        pwms = <&gpio_pwm 0 1 0>;
        beeper-hz = <1000>;
};

Then activated the necessary drivers with input evdev and they
both probe fine. Then I use this test program:
https://gist.github.com/licheegh/3dbdc8918325e8af8d4d13805cd0c350
with a few different beep frequencies, such as "beep 400"
for an OK sounding 400 Hz signal.

Above ~10000 Hz the sound becomes bad and mostly sound like
different kinds of noise. But this is not so bad for the NSLU2
which is a 266 MHz ARM XScale machine, and we do not need any better
on this machine so mission accomplished. Pushing the playback
to higher and higher rates does not crash the machine, the
sample program always terminates.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes in v7:
- Drop platform_set_drvdata() as nothing use this anymore.
- Add a small blurb documenting the pwm-cells in the binding.
- Link to v6: https://lore.kernel.org/r/20240602-pwm-gpio-v6-0-e8f6ec9cc783@linaro.org

v6:
- Add a small blurb to drivers-on-gpio.rst
- Add missing headers to driver
- Use guarded spinlocks from <linux/cleanup.h>
- Drop minor surplus whitespace
- Use dev pointer everywhere in probe()
- Drop dev_info() chatter at end of probe()
- Use devm_add_action_or_reset() for a hook to disable the hrtimer and
  get rid of the .remove() callback altogether.

V5:
 - add Reviewed-by's for dt-binding patch
 - rebase on top of v6.10-rc1
 - print hr resolution at driver probe
 - fix grammar in Kconfig
 - fix return type of pwm_gpio_toggle
 - implement hrtimer resolution rounding as noted by Uwe
 - use firmware node path instead of GPIO numbers as suggested by Andy
 - adjust some header includes and style issues as noted by Andy

V4:
 - address DT bindings comments from Conor Dooley
 - drop unnecessary MODULE_ALIAS() as suggested by Krzysztof Kozlowski
 - add range checks to apply which consider the hrtimer resolution
   (idea comes from Sean Young)
 - mark driver as atomic as suggested by Sean Young

V3:
 - rebase on top of v6.8-pwm-next
 - cherry-pick improvements from Nicola's series
 - try to address Uwe's, Linus' and Andy's comments
 - try to avoid GPIO glitches during probe
 - fix pwm_gpio_remove()
 - some code clean up's and comments

V2:
 - Rename gpio to gpios in binding
 - Calculate next expiry from expected current expiry rather than "now"
 - Only change configuration after current period ends
 - Implement get_state()
 - Add error message for probe failures
 - Stop PWM before unregister

[1] - https://lore.kernel.org/all/20200915135445.al75xmjxudj2rgcp@axis.com/T/
[2] - https://lore.kernel.org/all/20201205214353.xapax46tt5snzd2v@einstein.dilieto.eu/
[3] - https://lore.kernel.org/linux-pwm/CACRpkdary+kDrTJ=u4VbSTv7wXGLQj9_fy7mv0w-Zg+eDvGXVQ@mail.gmail.com/T/#m513f255daa9f30c325d11999cacd086411591bf9

---
Nicola Di Lieto (1):
      dt-bindings: pwm: Add pwm-gpio

Vincent Whitchurch (1):
      pwm: Add GPIO PWM driver

 .../devicetree/bindings/pwm/pwm-gpio.yaml          |  46 ++++
 Documentation/driver-api/gpio/drivers-on-gpio.rst  |   7 +-
 drivers/pwm/Kconfig                                |  11 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-gpio.c                             | 241 +++++++++++++++++++++
 5 files changed, 305 insertions(+), 1 deletion(-)
---
base-commit: cc97b8f37b097a1598410154ca472964a1d9b255
change-id: 20240530-pwm-gpio-e9133a1806ec

Best regards,

Comments

Phil Howard June 18, 2024, 11:51 a.m. UTC | #1
On Tue, 4 Jun 2024 at 22:00, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Add a software PWM which toggles a GPIO from a high-resolution timer.
>
> Recent discussions in the Raspberry Pi community revealt that a lot
> of users still use MMIO userspace tools for GPIO access. One argument
> for this approach is the lack of a GPIO PWM kernel driver. So this
> series tries to fill this gap.
>
> This continues the work of Vincent Whitchurch [1], which is easier
> to read and more consequent by rejecting sleeping GPIOs than Nicola's
> approach [2]. It further takes over where Stefan Wahren left off.
>
> I have not looked into the interrupt storm problem mentioned in [3]
> but instead focused on some real-life tests:
>
> The IXP4xx NSLU2 has a speaker connected directly to a GPIO, and I
> wanted to use this patch to provide a proper beeper for the machine
> and not have to rely on custom hacks.
>
> I added a DTS patch like this:
>
> gpio_pwm: pwm {
>         #pwm-cells = <3>;
>         compatible = "pwm-gpio";
>         gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
> };
>
> beeper {
>         compatible = "pwm-beeper";
>         pwms = <&gpio_pwm 0 1 0>;
>         beeper-hz = <1000>;
> };
>
> Then activated the necessary drivers with input evdev and they
> both probe fine. Then I use this test program:
> https://gist.github.com/licheegh/3dbdc8918325e8af8d4d13805cd0c350
> with a few different beep frequencies, such as "beep 400"
> for an OK sounding 400 Hz signal.
>
> Above ~10000 Hz the sound becomes bad and mostly sound like
> different kinds of noise. But this is not so bad for the NSLU2
> which is a 266 MHz ARM XScale machine, and we do not need any better
> on this machine so mission accomplished. Pushing the playback
> to higher and higher rates does not crash the machine, the
> sample program always terminates.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Changes in v7:
> - Drop platform_set_drvdata() as nothing use this anymore.
> - Add a small blurb documenting the pwm-cells in the binding.
> - Link to v6: https://lore.kernel.org/r/20240602-pwm-gpio-v6-0-e8f6ec9cc783@linaro.org
>
> v6:
> - Add a small blurb to drivers-on-gpio.rst
> - Add missing headers to driver
> - Use guarded spinlocks from <linux/cleanup.h>
> - Drop minor surplus whitespace
> - Use dev pointer everywhere in probe()
> - Drop dev_info() chatter at end of probe()
> - Use devm_add_action_or_reset() for a hook to disable the hrtimer and
>   get rid of the .remove() callback altogether.
>
> V5:
>  - add Reviewed-by's for dt-binding patch
>  - rebase on top of v6.10-rc1
>  - print hr resolution at driver probe
>  - fix grammar in Kconfig
>  - fix return type of pwm_gpio_toggle
>  - implement hrtimer resolution rounding as noted by Uwe
>  - use firmware node path instead of GPIO numbers as suggested by Andy
>  - adjust some header includes and style issues as noted by Andy
>
> V4:
>  - address DT bindings comments from Conor Dooley
>  - drop unnecessary MODULE_ALIAS() as suggested by Krzysztof Kozlowski
>  - add range checks to apply which consider the hrtimer resolution
>    (idea comes from Sean Young)
>  - mark driver as atomic as suggested by Sean Young
>
> V3:
>  - rebase on top of v6.8-pwm-next
>  - cherry-pick improvements from Nicola's series
>  - try to address Uwe's, Linus' and Andy's comments
>  - try to avoid GPIO glitches during probe
>  - fix pwm_gpio_remove()
>  - some code clean up's and comments
>
> V2:
>  - Rename gpio to gpios in binding
>  - Calculate next expiry from expected current expiry rather than "now"
>  - Only change configuration after current period ends
>  - Implement get_state()
>  - Add error message for probe failures
>  - Stop PWM before unregister
>
> [1] - https://lore.kernel.org/all/20200915135445.al75xmjxudj2rgcp@axis.com/T/
> [2] - https://lore.kernel.org/all/20201205214353.xapax46tt5snzd2v@einstein.dilieto.eu/
> [3] - https://lore.kernel.org/linux-pwm/CACRpkdary+kDrTJ=u4VbSTv7wXGLQj9_fy7mv0w-Zg+eDvGXVQ@mail.gmail.com/T/#m513f255daa9f30c325d11999cacd086411591bf9
>
> ---
> Nicola Di Lieto (1):
>       dt-bindings: pwm: Add pwm-gpio
>
> Vincent Whitchurch (1):
>       pwm: Add GPIO PWM driver
>
>  .../devicetree/bindings/pwm/pwm-gpio.yaml          |  46 ++++
>  Documentation/driver-api/gpio/drivers-on-gpio.rst  |   7 +-
>  drivers/pwm/Kconfig                                |  11 +
>  drivers/pwm/Makefile                               |   1 +
>  drivers/pwm/pwm-gpio.c                             | 241 +++++++++++++++++++++
>  5 files changed, 305 insertions(+), 1 deletion(-)
> ---
> base-commit: cc97b8f37b097a1598410154ca472964a1d9b255
> change-id: 20240530-pwm-gpio-e9133a1806ec
>
> Best regards,
> --
> Linus Walleij <linus.walleij@linaro.org>
>

Thank you for picking this up, it's reassuring to see it making progress.

I will give it a shot today.

- Phil
Uwe Kleine-König June 27, 2024, 6:52 a.m. UTC | #2
Hello,

On Tue, Jun 04, 2024 at 11:00:39PM +0200, Linus Walleij wrote:
> Add a software PWM which toggles a GPIO from a high-resolution timer.
> 
> Recent discussions in the Raspberry Pi community revealt that a lot
> of users still use MMIO userspace tools for GPIO access. One argument
> for this approach is the lack of a GPIO PWM kernel driver. So this
> series tries to fill this gap.
> 
> This continues the work of Vincent Whitchurch [1], which is easier
> to read and more consequent by rejecting sleeping GPIOs than Nicola's
> approach [2]. It further takes over where Stefan Wahren left off.
> 
> I have not looked into the interrupt storm problem mentioned in [3]
> but instead focused on some real-life tests:
> 
> The IXP4xx NSLU2 has a speaker connected directly to a GPIO, and I
> wanted to use this patch to provide a proper beeper for the machine
> and not have to rely on custom hacks.
> 
> I added a DTS patch like this:
> 
> gpio_pwm: pwm {
>         #pwm-cells = <3>;
>         compatible = "pwm-gpio";
>         gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
> };
> 
> beeper {
>         compatible = "pwm-beeper";
>         pwms = <&gpio_pwm 0 1 0>;
>         beeper-hz = <1000>;
> };

Applied both patches to 
https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next

Thanks
Uwe
Stefan Wahren June 27, 2024, 9:54 a.m. UTC | #3
Am 27.06.24 um 08:52 schrieb Uwe Kleine-König:
> Hello,
>
> On Tue, Jun 04, 2024 at 11:00:39PM +0200, Linus Walleij wrote:
>> Add a software PWM which toggles a GPIO from a high-resolution timer.
>>
>> Recent discussions in the Raspberry Pi community revealt that a lot
>> of users still use MMIO userspace tools for GPIO access. One argument
>> for this approach is the lack of a GPIO PWM kernel driver. So this
>> series tries to fill this gap.
>>
>> This continues the work of Vincent Whitchurch [1], which is easier
>> to read and more consequent by rejecting sleeping GPIOs than Nicola's
>> approach [2]. It further takes over where Stefan Wahren left off.
>>
>> I have not looked into the interrupt storm problem mentioned in [3]
>> but instead focused on some real-life tests:
>>
>> The IXP4xx NSLU2 has a speaker connected directly to a GPIO, and I
>> wanted to use this patch to provide a proper beeper for the machine
>> and not have to rely on custom hacks.
>>
>> I added a DTS patch like this:
>>
>> gpio_pwm: pwm {
>>          #pwm-cells = <3>;
>>          compatible = "pwm-gpio";
>>          gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
>> };
>>
>> beeper {
>>          compatible = "pwm-beeper";
>>          pwms = <&gpio_pwm 0 1 0>;
>>          beeper-hz = <1000>;
>> };
> Applied both patches to
> https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next
\o/

Thanks
>
> Thanks
> Uwe