[PATCH v2 1/7] Input: pwm-beeper - remove calls to legacy pwm_request API

[PATCH v2 1/7] Input: pwm-beeper - remove calls to legacy pwm_request API

[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 <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/pwm-beeper.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 5f9655d49a65..cb87e475bd23 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -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;
-- 
2.11.0.483.g087da7b7c-goog

[PATCH v2 2/7] Input: pwm-beeper - switch to using managed resources

[Dmitry Torokhov]

Use of managed resources (devm) simplifies error handling and tear down
of the driver.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/pwm-beeper.c | 44 ++++++++++-------------------------------
 1 file changed, 10 insertions(+), 34 deletions(-)

diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index cb87e475bd23..14c52054f5b7 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -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,
-- 
2.11.0.483.g087da7b7c-goog

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

[Dmitry Torokhov]

Instead of manipulating capability bits directly, let's use
input_set_capability() API.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/pwm-beeper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 14c52054f5b7..ce6eec48ec5f 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -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;
-- 
2.11.0.483.g087da7b7c-goog

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

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

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/pwm-beeper.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index ce6eec48ec5f..04c8ad3827d9 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -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;
 }
-- 
2.11.0.483.g087da7b7c-goog

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

[Dmitry Torokhov]

From: David Lechner <david@lechnology.com>

This suppress printing an error message when pwm_get returns -EPROBE_DEFER.
Otherwise you get a bunch of noise in the kernel log.

Signed-off-by: David Lechner <david@lechnology.com>
Patchwork-Id: 9499915
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/pwm-beeper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 04c8ad3827d9..9964c46468d3 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -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;
 	}
 
-- 
2.11.0.483.g087da7b7c-goog

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

[Dmitry Torokhov]

From: David Lechner <david@lechnology.com>

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.

Signed-off-by: David Lechner <david@lechnology.com>
Acked-by: Rob Herring <robh@kernel.org>
Patchwork-Id: 9511151
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 .../devicetree/bindings/input/pwm-beeper.txt       | 16 ++++++
 drivers/input/misc/pwm-beeper.c                    | 62 +++++++++++++++++-----
 2 files changed, 65 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.txt b/Documentation/devicetree/bindings/input/pwm-beeper.txt
index be332ae4f2d6..529408b4431a 100644
--- a/Documentation/devicetree/bindings/input/pwm-beeper.txt
+++ b/Documentation/devicetree/bindings/input/pwm-beeper.txt
@@ -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>;
+};
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 9964c46468d3..7b213e0ab06c 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -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;
+	struct regulator *amplifier;
 	struct work_struct work;
 	unsigned long period;
 	bool suspended;
+	bool amplifier_on;
 };
 
 #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
 
-static void __pwm_beeper_set(struct pwm_beeper *beeper)
+static int pwm_beeper_on(struct pwm_beeper *beeper, unsigned long period)
 {
-	unsigned long period = beeper->period;
+	int error;
+
+	error = pwm_config(beeper->pwm, period / 2, period);
+	if (error)
+		return error;
+
+	error = pwm_enable(beeper->pwm);
+	if (error)
+		return error;
+
+	if (!beeper->amplifier_on) {
+		error = regulator_enable(beeper->amplifier);
+		if (error) {
+			pwm_disable(beeper->pwm);
+			return error;
+		}
+
+		beeper->amplifier_on = true;
+	}
+
+	return 0;
+}
 
-	if (period) {
-		pwm_config(beeper->pwm, period / 2, period);
-		pwm_enable(beeper->pwm);
-	} else
-		pwm_disable(beeper->pwm);
+static void pwm_beeper_off(struct pwm_beeper *beeper)
+{
+	if (beeper->amplifier_on) {
+		regulator_disable(beeper->amplifier);
+		beeper->amplifier_on = false;
+	}
+
+	pwm_disable(beeper->pwm);
 }
 
 static void pwm_beeper_work(struct work_struct *work)
 {
-	struct pwm_beeper *beeper =
-		container_of(work, struct pwm_beeper, work);
+	struct pwm_beeper *beeper = container_of(work, struct pwm_beeper, work);
+	unsigned long period = READ_ONCE(beeper->period);
 
-	__pwm_beeper_set(beeper);
+	if (period)
+		pwm_beeper_on(beeper, period);
+	else
+		pwm_beeper_off(beeper);
 }
 
 static int pwm_beeper_event(struct input_dev *input,
@@ -83,9 +113,7 @@ static int pwm_beeper_event(struct input_dev *input,
 static void pwm_beeper_stop(struct pwm_beeper *beeper)
 {
 	cancel_work_sync(&beeper->work);
-
-	if (beeper->period)
-		pwm_disable(beeper->pwm);
+	pwm_beeper_off(beeper);
 }
 
 static void pwm_beeper_close(struct input_dev *input)
@@ -119,6 +147,14 @@ static int pwm_beeper_probe(struct platform_device *pdev)
 	 */
 	pwm_apply_args(beeper->pwm);
 
+	beeper->amplifier = devm_regulator_get(dev, "amp");
+	if (IS_ERR(beeper->amplifier)) {
+		error = PTR_ERR(beeper->amplifier);
+		if (error != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get amp regulator\n");
+		return error;
+	}
+
 	INIT_WORK(&beeper->work, pwm_beeper_work);
 
 	beeper->input = devm_input_allocate_device(dev);
-- 
2.11.0.483.g087da7b7c-goog

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

[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 <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/pwm-beeper.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 7b213e0ab06c..3408dc666afa 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -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)) {
-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH v2 1/7] Input: pwm-beeper - remove calls to legacy pwm_request API

[Thierry Reding]

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

On Thu, Jan 19, 2017 at 02:40:51PM -0800, Dmitry Torokhov wrote:
> 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 <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/pwm-beeper.c | 6 ------
>  1 file changed, 6 deletions(-)

Very neat. Thanks for that!

Acked-by: Thierry Reding <thierry.reding@gmail.com>

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

Re: [PATCH v2 2/7] Input: pwm-beeper - switch to using managed resources

[Thierry Reding]

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

On Thu, Jan 19, 2017 at 02:40:52PM -0800, Dmitry Torokhov wrote:
> Use of managed resources (devm) simplifies error handling and tear down
> of the driver.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/pwm-beeper.c | 44 ++++++++++-------------------------------
>  1 file changed, 10 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> index cb87e475bd23..14c52054f5b7 100644
> --- a/drivers/input/misc/pwm-beeper.c
> +++ b/drivers/input/misc/pwm-beeper.c
> @@ -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);

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

Otherwise this looks like great cleanup:

Reviewed-by: Thierry Reding <thierry.reding@gmail.com>

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

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

[Thierry Reding]

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

On Thu, Jan 19, 2017 at 02:40:53PM -0800, Dmitry Torokhov wrote:
> Instead of manipulating capability bits directly, let's use
> input_set_capability() API.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/pwm-beeper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Thierry Reding <thierry.reding@gmail.com>

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

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

[Thierry Reding]

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

On Thu, Jan 19, 2017 at 02:40:54PM -0800, Dmitry Torokhov wrote:
> 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.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/pwm-beeper.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)

Reviewed-by: Thierry Reding <thierry.reding@gmail.com>

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

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

[Thierry Reding]

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

On Thu, Jan 19, 2017 at 02:40:55PM -0800, Dmitry Torokhov wrote:
> From: David Lechner <david@lechnology.com>
> 
> This suppress printing an error message when pwm_get returns -EPROBE_DEFER.
> Otherwise you get a bunch of noise in the kernel log.
> 
> Signed-off-by: David Lechner <david@lechnology.com>
> Patchwork-Id: 9499915
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/pwm-beeper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> index 04c8ad3827d9..9964c46468d3 100644
> --- a/drivers/input/misc/pwm-beeper.c
> +++ b/drivers/input/misc/pwm-beeper.c
> @@ -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:

Reviewed-by: Thierry Reding <thierry.reding@gmail.com>

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

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

[Thierry Reding]

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

On Thu, Jan 19, 2017 at 02:40:56PM -0800, Dmitry Torokhov wrote:
[...]
> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> index 9964c46468d3..7b213e0ab06c 100644
> --- a/drivers/input/misc/pwm-beeper.c
> +++ b/drivers/input/misc/pwm-beeper.c
> @@ -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;
> +	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

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

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

[Thierry Reding]

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

On Thu, Jan 19, 2017 at 02:40:57PM -0800, Dmitry Torokhov wrote:
> The "atomic" API allows us to configure PWM period and duty cycle and
> enable it in one call.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/pwm-beeper.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> index 7b213e0ab06c..3408dc666afa 100644
> --- a/drivers/input/misc/pwm-beeper.c
> +++ b/drivers/input/misc/pwm-beeper.c
> @@ -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;
>  

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.

Reviewed-by: Thierry Reding <thierry.reding@gmail.com>

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

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

[Dmitry Torokhov]

On Fri, Jan 20, 2017 at 11:19:16AM +0100, Thierry Reding wrote:
> On Thu, Jan 19, 2017 at 02:40:56PM -0800, Dmitry Torokhov wrote:
> [...]
> > diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> > index 9964c46468d3..7b213e0ab06c 100644
> > --- a/drivers/input/misc/pwm-beeper.c
> > +++ b/drivers/input/misc/pwm-beeper.c
> > @@ -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;
> > +	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.

-- 
Dmitry

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

[David Lechner]

On 01/20/2017 04:16 AM, Thierry Reding wrote:
> On Thu, Jan 19, 2017 at 02:40:55PM -0800, Dmitry Torokhov wrote:
>> From: David Lechner <david@lechnology.com>
>>
>> This suppress printing an error message when pwm_get returns -EPROBE_DEFER.
>> Otherwise you get a bunch of noise in the kernel log.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> Patchwork-Id: 9499915
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>>  drivers/input/misc/pwm-beeper.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
>> index 04c8ad3827d9..9964c46468d3 100644
>> --- a/drivers/input/misc/pwm-beeper.c
>> +++ b/drivers/input/misc/pwm-beeper.c
>> @@ -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.

>
> Either way:
>
> Reviewed-by: Thierry Reding <thierry.reding@gmail.com>
>

Re: [PATCH v2 1/7] Input: pwm-beeper - remove calls to legacy pwm_request API

[David Lechner]

On 01/19/2017 04:40 PM, Dmitry Torokhov wrote:
> 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 <dmitry.torokhov@gmail.com>
> ---


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

Tested-By: David Lechner <david@lechnology.com>