Input: pwm-beeper - remove calls to legacy pwm_request API
Subsystems (3)
Dmitry Torokhov
maintainerPATCH v2 1/7 Input: pwm-beeper - remove calls to legacy pwm_request API
There are no more users of pwm-beeper driver in mainline relying on this legacy API, so let's remove it and simplify the driver code.
--- drivers/input/misc/pwm-beeper.c | 6 ------ 1 file changed, 6 deletions(-)
drivers/input/misc/pwm-beeper.c +0 -6
@@ -95,7 +95,6 @@ static void pwm_beeper_close(struct input_dev *input)static int pwm_beeper_probe (struct platform_device *pdev) { -unsigned long pwm_id = (unsigned long )dev_get_platdata (&pdev->dev);struct pwm_beeper *beeper;int error;
@@ -105,11 +104,6 @@ static int pwm_beeper_probe(struct platform_device *pdev) beeper->pwm =pwm_get (&pdev->dev,NULL );if (IS_ERR (beeper->pwm)) { -dev_dbg (&pdev->dev,"unable to request PWM, trying legacy API\n" ); - beeper->pwm =pwm_request (pwm_id,"pwm beeper" ); - } - -if (IS_ERR (beeper->pwm)) { error =PTR_ERR (beeper->pwm);dev_err (&pdev->dev,"Failed to request pwm device: %d\n" , error); goto err_free;
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Dmitry Torokhov
maintainerPATCH v2 2/7 Input: pwm-beeper - switch to using managed resources
Use of managed resources (devm) simplifies error handling and tear down of the driver.
--- drivers/input/misc/pwm-beeper.c | 44 ++++++++++------------------------------- 1 file changed, 10 insertions(+), 34 deletions(-)
drivers/input/misc/pwm-beeper.c +10 -34
@@ -95,18 +95,19 @@ static void pwm_beeper_close(struct input_dev *input)static int pwm_beeper_probe (struct platform_device *pdev) { +struct device *dev = &pdev->dev;struct pwm_beeper *beeper;int error; - beeper =kzalloc (sizeof (*beeper),GFP_KERNEL ); + beeper =devm_kzalloc (dev,sizeof (*beeper),GFP_KERNEL );if (!beeper)return -ENOMEM ; - beeper->pwm =pwm_get (&pdev->dev,NULL ); + beeper->pwm =devm_pwm_get (dev,NULL );if (IS_ERR (beeper->pwm)) { error =PTR_ERR (beeper->pwm); -dev_err (&pdev->dev,"Failed to request pwm device: %d\n" , error); - goto err_free; +dev_err (dev,"Failed to request pwm device: %d\n" , error); +return error; }/*
@@ -117,13 +118,11 @@ static int pwm_beeper_probe(struct platform_device *pdev)INIT_WORK (&beeper->work, pwm_beeper_work); - beeper->input =input_allocate_device (); + beeper->input =devm_input_allocate_device (dev);if (!beeper->input) { -dev_err (&pdev->dev,"Failed to allocate input device\n" ); - error = -ENOMEM ; - goto err_pwm_free; +dev_err (dev,"Failed to allocate input device\n" ); +return -ENOMEM ; } - beeper->input->dev.parent = &pdev->dev; beeper->input->name ="pwm-beeper" ; beeper->input->phys ="pwm/input0" ;
@@ -142,35 +141,13 @@ static int pwm_beeper_probe(struct platform_device *pdev) error =input_register_device (beeper->input);if (error) { -dev_err (&pdev->dev,"Failed to register input device: %d\n" , error); - goto err_input_free; +dev_err (dev,"Failed to register input device: %d\n" , error); +return error; }platform_set_drvdata (pdev, beeper);return 0 ; - -err_input_free: -input_free_device (beeper->input); -err_pwm_free: -pwm_free (beeper->pwm); -err_free: -kfree (beeper); - -return error; -} - -static int pwm_beeper_remove (struct platform_device *pdev) -{ -struct pwm_beeper *beeper =platform_get_drvdata (pdev); - -input_unregister_device (beeper->input); - -pwm_free (beeper->pwm); - -kfree (beeper); - -return 0 ; }static int __maybe_unused pwm_beeper_suspend (struct device *dev)
@@ -205,7 +182,6 @@ MODULE_DEVICE_TABLE(of, pwm_beeper_match);static struct platform_driver pwm_beeper_driver = { .probe = pwm_beeper_probe, - .remove = pwm_beeper_remove, .driver = { .name ="pwm-beeper" , .pm = &pwm_beeper_pm_ops,
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Re: [PATCH v2 2/7] Input: pwm-beeper - switch to using managed resources
Quoted 22 lines · Dmitry Torokhov
Use of managed resources (devm) simplifies error handling and tear down of the driver.
Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/input/misc/pwm-beeper.c | 44 ++++++++++------------------------------- 1 file changed, 10 insertions(+), 34 deletions(-)drivers/input/misc/pwm-beeper.c +2
-1@@ -95,18 +95,19 @@ static void pwm_beeper_close(struct input_dev *input)static int pwm_beeper_probe (struct platform_device *pdev) { +struct device *dev = &pdev->dev;struct pwm_beeper *beeper;int error; - beeper =kzalloc (sizeof (*beeper),GFP_KERNEL ); + beeper =devm_kzalloc (dev,sizeof (*beeper),GFP_KERNEL );
drivers/input/misc/pwm-beeper.c static void pwm_beeper_close(struct input_dev *input) · line 109
Quoted 10 lines
drivers/input/misc/pwm-beeper.c +2
-3if (!beeper)return -ENOMEM ; - beeper->pwm =pwm_get (&pdev->dev,NULL ); + beeper->pwm =devm_pwm_get (dev,NULL );if (IS_ERR (beeper->pwm)) { error =PTR_ERR (beeper->pwm); -dev_err (&pdev->dev,"Failed to request pwm device: %d\n" , error); - goto err_free; +dev_err (dev,"Failed to request pwm device: %d\n" , error);
While at it, could you do a "pwm" -> "PWM" in the above. It's an abbreviation.
Otherwise this looks like great cleanup:
Dmitry Torokhov
maintainerPATCH v2 3/7 Input: pwm-beeper - use input_set_capability()
In reply to Dmitry TorokhovInstead of manipulating capability bits directly, let's use input_set_capability() API.
--- drivers/input/misc/pwm-beeper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/input/misc/pwm-beeper.c +2 -2
@@ -131,8 +131,8 @@ static int pwm_beeper_probe(struct platform_device *pdev) beeper->input->id.product =0x0001 ; beeper->input->id.version =0x0100 ; - beeper->input->evbit[0 ] =BIT (EV_SND ); - beeper->input->sndbit[0 ] =BIT (SND_TONE ) |BIT (SND_BELL ); +input_set_capability (beeper->input,EV_SND ,SND_TONE ); +input_set_capability (beeper->input,EV_SND ,SND_BELL ); beeper->input->event = pwm_beeper_event; beeper->input->close = pwm_beeper_close;
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Re: [PATCH v2 3/7] Input: pwm-beeper - use input_set_capability()
Quoted 7 lines · Dmitry Torokhov
Instead of manipulating capability bits directly, let's use
input_set_capability()API.Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/input/misc/pwm-beeper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Dmitry Torokhov
maintainerPATCH v2 4/7 Input: pwm-beeper - fix race when suspending
In reply to Dmitry TorokhovUsually userspace sends SND_BELL and SND_TONE events, and by the time pwm_beeper_suspend() runs userpsace is already frozen, but theoretically in-kernel users may send these events too, and that may cause pwm_beeper_event() scheduling another work after we canceled it.
Let's introduce a "suspended" flag and check it in pwm_beeper_event() to avoid this race.
--- drivers/input/misc/pwm-beeper.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
drivers/input/misc/pwm-beeper.c +18 -3
@@ -27,6 +27,7 @@ struct pwm_beeper {struct pwm_device *pwm;struct work_struct work;unsigned long period; +bool suspended; };#define HZ_TO_NANOSECONDS (x) (1000000000UL/(x))
@@ -73,7 +74,8 @@ static int pwm_beeper_event(struct input_dev *input,else beeper->period =HZ_TO_NANOSECONDS (value); -schedule_work (&beeper->work); +if (!beeper->suspended) +schedule_work (&beeper->work);return 0 ; }
@@ -154,6 +156,15 @@ static int __maybe_unused pwm_beeper_suspend(struct device *dev) {struct pwm_beeper *beeper =dev_get_drvdata (dev); +/* +* Spinlock is taken here is not to protect write to +* beeper->suspended, but to ensure that pwm_beeper_event +* does not re-submit work once flag is set. +*/ +spin_lock_irq (&beeper->input->event_lock); + beeper->suspended = true; +spin_unlock_irq (&beeper->input->event_lock); +pwm_beeper_stop (beeper);return 0 ;
@@ -163,8 +174,12 @@ static int __maybe_unused pwm_beeper_resume(struct device *dev) {struct pwm_beeper *beeper =dev_get_drvdata (dev); -if (beeper->period) -__pwm_beeper_set (beeper); +spin_lock_irq (&beeper->input->event_lock); + beeper->suspended = false; +spin_unlock_irq (&beeper->input->event_lock); + +/* Let worker figure out if we should resume beeping */ +schedule_work (&beeper->work);return 0 ; }
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Re: [PATCH v2 4/7] Input: pwm-beeper - fix race when suspending
Quoted 5 lines · Dmitry Torokhov
Usually userspace sends
SND_BELLandSND_TONEevents, and by the timepwm_beeper_suspend()runs userpsace is already frozen, but theoretically in-kernel users may send these events too, and that may causepwm_beeper_event()scheduling another work after we canceled it.
Quoted 7 lines
Let's introduce a "suspended" flag and check it in
pwm_beeper_event()to avoid this race.Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/input/misc/pwm-beeper.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
Dmitry Torokhov
maintainerPATCH v2 5/7 Input: pwm-beeper - suppress error message on probe defer
In reply to Dmitry TorokhovFrom: David Lechner <[email protected]>
This suppress printing an error message when pwm_get returns -EPROBE_DEFER. Otherwise you get a bunch of noise in the kernel log.
Patchwork-Id: 9499915
--- drivers/input/misc/pwm-beeper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/input/misc/pwm-beeper.c +2 -1
@@ -108,7 +108,8 @@ static int pwm_beeper_probe(struct platform_device *pdev) beeper->pwm =devm_pwm_get (dev,NULL );if (IS_ERR (beeper->pwm)) { error =PTR_ERR (beeper->pwm); -dev_err (dev,"Failed to request pwm device: %d\n" , error); +if (error != -EPROBE_DEFER ) +dev_err (dev,"Failed to request pwm device\n" );return error; }
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Re: [PATCH v2 5/7] Input: pwm-beeper - suppress error message on probe defer
Quoted 17 lines · Dmitry Torokhov
From: David Lechner <[email protected]>
This suppress printing an error message when
pwm_getreturns -EPROBE_DEFER. Otherwise you get a bunch of noise in the kernel log.Signed-off-by David Lechner <[email protected]>Patchwork-Id: 9499915
Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/input/misc/pwm-beeper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/input/misc/pwm-beeper.c +0
-0
drivers/input/misc/pwm-beeper.c static int pwm_beeper_probe(struct platform_device *pdev) · line 112
Quoted 6 lines
drivers/input/misc/pwm-beeper.c +2
-1@@ -108,7 +108,8 @@ static int pwm_beeper_probe(struct platform_device *pdev) beeper->pwm =devm_pwm_get (dev,NULL );if (IS_ERR (beeper->pwm)) { error =PTR_ERR (beeper->pwm); -dev_err (dev,"Failed to request pwm device: %d\n" , error); +if (error != -EPROBE_DEFER ) +dev_err (dev,"Failed to request pwm device\n" );
This also drops the error code from the message. I suspect that this was intentional because failure to probe will print out the error code anyway. Might be worth mentioning that in the commit message?
Either way:
Re: [PATCH v2 5/7] Input: pwm-beeper - suppress error message on probe defer
Quoted 13 lines · Thierry Reding
Quoted 12 lines · Dmitry Torokhov
From: David Lechner <[email protected]>
This suppress printing an error message when
pwm_getreturns -EPROBE_DEFER. Otherwise you get a bunch of noise in the kernel log.Signed-off-by David Lechner <[email protected]>Patchwork-Id: 9499915
Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/input/misc/pwm-beeper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Quoted 15 lines
drivers/input/misc/pwm-beeper.c static int pwm_beeper_probe(struct platform_device *pdev) · line 112
Quoted 11 lines
drivers/input/misc/pwm-beeper.c +2
-1@@ -108,7 +108,8 @@ static int pwm_beeper_probe(struct platform_device *pdev) beeper->pwm =devm_pwm_get (dev,NULL );if (IS_ERR (beeper->pwm)) { error =PTR_ERR (beeper->pwm); -dev_err (dev,"Failed to request pwm device: %d\n" , error); +if (error != -EPROBE_DEFER ) +dev_err (dev,"Failed to request pwm device\n" );This also drops the error code from the message. I suspect that this was intentional because failure to probe will print out the error code anyway. Might be worth mentioning that in the commit message?
Yes, it was intentional for that reason. And in fact we could do the same thing to the error messages that are touched in patch 2/7 of this series.
Quoted 5 lines
Either way:
Reviewed-by Thierry Reding <[email protected]>
Dmitry Torokhov
maintainerPATCH v2 6/7 Input: pwm-beeper - add optional amplifier regulator
In reply to Dmitry TorokhovFrom: David Lechner <[email protected]>
This adds an optional regulator to the pwm-beeper device. This regulator acts as an amplifier. The amplifier is only enabled while beeping in order to reduce power consumption.
Tested on LEGO MINDSTORMS EV3, which has a speaker connected to PWM through an amplifier.
Patchwork-Id: 9511151
--- .../devicetree/bindings/input/pwm-beeper.txt | 16 ++++++ drivers/input/misc/pwm-beeper.c | 62 +++++++++++++++++----- 2 files changed, 65 insertions(+), 13 deletions(-)
Documentation/devicetree/bindings/input/pwm-beeper.txt +16 -0
@@ -5,3 +5,19 @@ Registers a PWM device as beeper. Required properties: - compatible: should be "pwm-beeper" - pwms: phandle to the physical PWM device + +Optional properties: +- amp-supply: phandle to a regulator that acts as an amplifier for the beeper + +Example: + +beeper_amp: amplifier { + compatible = "fixed-regulator"; + gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>; +}; + +beeper { + compatible = "pwm-beeper"; + pwms = <&pwm0>; + amp-supply = <&beeper_amp>; +};
drivers/input/misc/pwm-beeper.c +49 -13
100 lines not on this page · raw message
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Re: [PATCH v2 6/7] Input: pwm-beeper - add optional amplifier regulator
On Thu, Jan 19, 2017 at 02:40:56PM -0800, Dmitry Torokhov wrote:
[...]
Quoted 13 lines
drivers/input/misc/pwm-beeper.c +1
-0@@ -14,6 +14,7 @@*/ #include <linux/input.h> +#include <linux/regulator/consumer.h> #include <linux/module.h> #include <linux/kernel.h> #include <linux/of.h>
drivers/input/misc/pwm-beeper.c line 33
Quoted 8 lines
drivers/input/misc/pwm-beeper.c +2
-0@@ -25,30 +26,59 @@struct pwm_beeper {struct input_dev *input;struct pwm_device *pwm; +struct regulator *amplifier;struct work_struct work;unsigned long period;bool suspended; +bool amplifier_on;
Why do you need to track this? I thought regulator_enable() and regulator_disable() were already reference counted?
Thierry
Dmitry Torokhov
maintainerRe: [PATCH v2 6/7] Input: pwm-beeper - add optional amplifier regulator
Quoted 18 lines · Thierry Reding
On Thu, Jan 19, 2017 at 02:40:56PM -0800, Dmitry Torokhov wrote:
[...]Quoted 16 lines
drivers/input/misc/pwm-beeper.c +1
-0@@ -14,6 +14,7 @@*/ #include <linux/input.h> +#include <linux/regulator/consumer.h> #include <linux/module.h> #include <linux/kernel.h> #include <linux/of.h> @@ -25,30 +26,59 @@struct pwm_beeper {struct input_dev *input;struct pwm_device *pwm;
Quoted 8 lines
drivers/input/misc/pwm-beeper.c line 33
Quoted 5 lines
drivers/input/misc/pwm-beeper.c +2
-0+struct regulator *amplifier;struct work_struct work;unsigned long period;bool suspended; +bool amplifier_on;Why do you need to track this? I thought
regulator_enable()andregulator_disable()were already reference counted?
That us exactly the issue: without the flag userspace application sending:
EV_SND/SND_TONE/100
EV_SND/SND_TONE/200
EV_SND/SND_TONE/300
EV_SND/SND_TONE/200
EV_SND/SND_TONE/100
EV_SND/SND_TONE/0
will result in enable count of 4 (and not 0) and regulator will stay on forever.
Thanks.
Signature · 1 line
Dmitry
Dmitry Torokhov
maintainerPATCH v2 7/7 Input: pwm-beeper - switch to using "atomic" PWM API
In reply to Dmitry TorokhovThe "atomic" API allows us to configure PWM period and duty cycle and enable it in one call.
--- drivers/input/misc/pwm-beeper.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)
drivers/input/misc/pwm-beeper.c +16 -9
@@ -37,13 +37,16 @@ struct pwm_beeper {static int pwm_beeper_on (struct pwm_beeper *beeper,unsigned long period) { +struct pwm_state state;int error; - error =pwm_config (beeper->pwm, period /2 , period); -if (error) -return error; +pwm_get_state (beeper->pwm, &state); - error =pwm_enable (beeper->pwm); + state.enabled = true; + state.period = period; +pwm_set_relative_duty_cycle (&state,50 ,100 ); + + error =pwm_apply_state (beeper->pwm, &state);if (error)return error;
@@ -127,6 +130,7 @@ static int pwm_beeper_probe(struct platform_device *pdev) {struct device *dev = &pdev->dev;struct pwm_beeper *beeper; +struct pwm_state state;int error; beeper =devm_kzalloc (dev,sizeof (*beeper),GFP_KERNEL );
@@ -141,11 +145,14 @@ static int pwm_beeper_probe(struct platform_device *pdev)return error; } -/* -* FIXME: pwm_apply_args() should be removed when switching to -* the atomic PWM API. -*/ -pwm_apply_args (beeper->pwm); +/* Sync up PWM state and ensure it is off. */ +pwm_init_state (beeper->pwm, &state); + state.enabled = false; + error =pwm_apply_state (beeper->pwm, &state); +if (error) { +dev_err (dev,"failed to apply initial pwm state\n" ); +return error; + } beeper->amplifier =devm_regulator_get (dev,"amp" );if (IS_ERR (beeper->amplifier)) {
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Re: [PATCH v2 7/7] Input: pwm-beeper - switch to using "atomic" PWM API
Quoted 23 lines · Dmitry Torokhov
The "atomic" API allows us to configure PWM period and duty cycle and enable it in one call.
Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/input/misc/pwm-beeper.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)drivers/input/misc/pwm-beeper.c +2
-3@@ -37,13 +37,16 @@ struct pwm_beeper {static int pwm_beeper_on (struct pwm_beeper *beeper,unsigned long period) { +struct pwm_state state;int error; - error =pwm_config (beeper->pwm, period /2 , period); -if (error) -return error; +pwm_get_state (beeper->pwm, &state);
drivers/input/misc/pwm-beeper.c struct pwm_beeper { · line 52
Quoted 10 lines
drivers/input/misc/pwm-beeper.c +5
-1- error =pwm_enable (beeper->pwm); + state.enabled = true; + state.period = period; +pwm_set_relative_duty_cycle (&state,50 ,100 ); + + error =pwm_apply_state (beeper->pwm, &state);if (error)return error;
I was going to say that you need to update pwm_beeper_off() to use the atomic API as well, but I think I might keep pwm_disable() around for cases such as this.
Quoted 7 lines · Dmitry Torokhov
There are no more users of pwm-beeper driver in mainline relying on this legacy API, so let's remove it and simplify the driver code.
Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/input/misc/pwm-beeper.c | 6 ------ 1 file changed, 6 deletions(-)
Very neat. Thanks for that!
Quoted 5 lines · Dmitry Torokhov
There are no more users of pwm-beeper driver in mainline relying on this legacy API, so let's remove it and simplify the driver code.
Signed-off-by Dmitry Torokhov <[email protected]>---
I've tested this patch series on LEGO MINDSTORMS EV3 and it looks good to me. I am not able to test the suspend race condition, but other than that (for the whole series)...