Skip to contentSkip to conversation

Input: pwm-beeper - remove calls to legacy pwm_request API

PATCH In review Dmitry Torokhov 3 participants 17 messages Jan 19, 2017 – Jan 20, 2017

Subsystems (3)
lore
Share & tools
lore

Dmitry Torokhov

author maintainer
#0 lore raw

PATCH 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
index 5f9655d49a65..cb87e475bd23 100644
@@ -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
author maintainer
#1 lore raw

PATCH 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
index cb87e475bd23..14c52054f5b7 100644
@@ -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
Reviewed-by on v2 2/7
#2 lore raw

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.

---
 drivers/input/misc/pwm-beeper.c | 44 ++++++++++-------------------------------
 1 file changed, 10 insertions(+), 34 deletions(-)
drivers/input/misc/pwm-beeper.c +2 -1
index cb87e475bd23..14c52054f5b7 100644
@@ -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 -3
 	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);

While at it, could you do a "pwm" -> "PWM" in the above. It's an abbreviation.

Otherwise this looks like great cleanup:

author maintainer
#3 lore raw

PATCH v2 3/7 Input: pwm-beeper - use input_set_capability()

In reply to Dmitry Torokhov

Instead 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
index 14c52054f5b7..ce6eec48ec5f 100644
@@ -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
author maintainer
#5 lore raw

PATCH v2 4/7 Input: pwm-beeper - fix race when suspending

In reply to Dmitry Torokhov

Usually 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
index ce6eec48ec5f..04c8ad3827d9 100644
@@ -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
Reviewed-by on v2 4/7
#6 lore raw

Re: [PATCH v2 4/7] Input: pwm-beeper - fix race when suspending

Quoted 5 lines · Dmitry Torokhov

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

Quoted 7 lines

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(-)
author maintainer
#7 lore raw

PATCH v2 5/7 Input: pwm-beeper - suppress error message on probe defer

In reply to Dmitry Torokhov

From: 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
index 04c8ad3827d9..9964c46468d3 100644
@@ -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
Reviewed-by on v2 5/7
#8 lore raw

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_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 +0 -0
index 04c8ad3827d9..9964c46468d3 100644

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:

#9 lore raw

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_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(-)
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
index 04c8ad3827d9..9964c46468d3 100644
@@ -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:

author maintainer
#10 lore raw

PATCH v2 6/7 Input: pwm-beeper - add optional amplifier regulator

In reply to Dmitry Torokhov

From: 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
index be332ae4f2d6..529408b4431a 100644
@@ -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
#11 lore raw

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
index 9964c46468d3..7b213e0ab06c 100644
@@ -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

author maintainer
#12 lore raw

Re: [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
index 9964c46468d3..7b213e0ab06c 100644
@@ -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() and regulator_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
author maintainer
#13 lore raw

PATCH v2 7/7 Input: pwm-beeper - switch to using "atomic" PWM API

In reply to Dmitry Torokhov

The "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
index 7b213e0ab06c..3408dc666afa 100644
@@ -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
Reviewed-by on v2 7/7
#14 lore raw

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.

---
 drivers/input/misc/pwm-beeper.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)
drivers/input/misc/pwm-beeper.c +2 -3
index 7b213e0ab06c..3408dc666afa 100644
@@ -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.

Tested-by on v2 1/7
#16 lore raw
In reply to Dmitry Torokhov
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.

---

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