[PATCH v2 0/3] Input: add optional amplifier regulator to pwm-beeper​ (previously "Input: add optional enable gpio to pwm-beeper​")

[PATCH v2 0/3] Input: add optional amplifier regulator to pwm-beeper​ (previously "Input: add optional enable gpio to pwm-beeper​")

[David Lechner]

This series adds an optional regulator to the pwm-beeper driver and device tree
bindings. The regulator acts as an amplifier that powers the beeper.

v2 changes:
* Changed from using a gpio to a regulator
* Rebased on the pwm-beeper cleanup series "Input: pwm-beeper - remove calls to
  legacy pwm_request API"


David Lechner (3):
  Input: pwm-beeper: suppress error message on probe defer
  dt-bindings: Input: Add optional amp-supply property to pwm-beeper
  Input: pwm-beeper: add optional amplifier regulator

 .../devicetree/bindings/input/pwm-beeper.txt       | 16 ++++++++++
 drivers/input/misc/pwm-beeper.c                    | 36 +++++++++++++++++++---
 2 files changed, 48 insertions(+), 4 deletions(-)

-- 
2.7.4

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

[David Lechner]

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>
---
 drivers/input/misc/pwm-beeper.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index ce6eec4..30ac227 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -104,9 +104,10 @@ static int pwm_beeper_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	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);
+	error = PTR_ERR_OR_ZERO(beeper->pwm);
+	if (error) {
+		if (error != -EPROBE_DEFER)
+			dev_err(dev, "Failed to request pwm device\n");
 		return error;
 	}
 
-- 
2.7.4

[PATCH v2 2/3] dt-bindings: Input: Add optional amp-supply property to pwm-beeper

[David Lechner]

This adds an optional amp-supply property to pwm-beeper. This is a
regulator that acts as an amplifier for the beeper.

Signed-off-by: David Lechner <david@lechnology.com>
---
 Documentation/devicetree/bindings/input/pwm-beeper.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.txt b/Documentation/devicetree/bindings/input/pwm-beeper.txt
index be332ae..529408b 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>;
+};
-- 
2.7.4

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

[David Lechner]

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>
---
 drivers/input/misc/pwm-beeper.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 30ac227..708e88e 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,8 +26,10 @@
 struct pwm_beeper {
 	struct input_dev *input;
 	struct pwm_device *pwm;
+	struct regulator *reg;
 	struct work_struct work;
 	unsigned long period;
+	bool reg_enabled;
 };
 
 #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
@@ -38,8 +41,20 @@ static void __pwm_beeper_set(struct pwm_beeper *beeper)
 	if (period) {
 		pwm_config(beeper->pwm, period / 2, period);
 		pwm_enable(beeper->pwm);
-	} else
+		if (beeper->reg) {
+			int error;
+
+			error = regulator_enable(beeper->reg);
+			if (!error)
+				beeper->reg_enabled = true;
+		}
+	} else {
+		if (beeper->reg_enabled) {
+			regulator_disable(beeper->reg);
+			beeper->reg_enabled = false;
+		}
 		pwm_disable(beeper->pwm);
+	}
 }
 
 static void pwm_beeper_work(struct work_struct *work)
@@ -82,6 +97,10 @@ static void pwm_beeper_stop(struct pwm_beeper *beeper)
 {
 	cancel_work_sync(&beeper->work);
 
+	if (beeper->reg_enabled) {
+		regulator_disable(beeper->reg);
+		beeper->reg_enabled = false;
+	}
 	if (beeper->period)
 		pwm_disable(beeper->pwm);
 }
@@ -111,6 +130,14 @@ static int pwm_beeper_probe(struct platform_device *pdev)
 		return error;
 	}
 
+	beeper->reg = devm_regulator_get_optional(&pdev->dev, "amp");
+	error = PTR_ERR_OR_ZERO(beeper->reg);
+	if (error) {
+		if (error != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get amp regulator\n");
+		return error;
+	}
+
 	/*
 	 * FIXME: pwm_apply_args() should be removed when switching to
 	 * the atomic PWM API.
-- 
2.7.4

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

[Dmitry Torokhov]

On Wed, Jan 11, 2017 at 02:01:59PM -0600, David Lechner wrote:
> 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>
> ---
>  drivers/input/misc/pwm-beeper.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> index ce6eec4..30ac227 100644
> --- a/drivers/input/misc/pwm-beeper.c
> +++ b/drivers/input/misc/pwm-beeper.c
> @@ -104,9 +104,10 @@ static int pwm_beeper_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	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);
> +	error = PTR_ERR_OR_ZERO(beeper->pwm);
> +	if (error) {

I do not find it in any way clearer than

	if (IS_ERR()) {
		...

I prefer PTR_ERR_OR_ZERO be used when you need to return value without
acting on it.

I can adjust locally, no need to resubmit.

> +		if (error != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to request pwm device\n");
>  		return error;
>  	}
>  
> -- 
> 2.7.4
> 

-- 
Dmitry

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

[Dmitry Torokhov]

On Wed, Jan 11, 2017 at 02:02:01PM -0600, David Lechner wrote:
> 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>
> ---
>  drivers/input/misc/pwm-beeper.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> index 30ac227..708e88e 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,8 +26,10 @@
>  struct pwm_beeper {
>  	struct input_dev *input;
>  	struct pwm_device *pwm;
> +	struct regulator *reg;
>  	struct work_struct work;
>  	unsigned long period;
> +	bool reg_enabled;
>  };
>  
>  #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
> @@ -38,8 +41,20 @@ static void __pwm_beeper_set(struct pwm_beeper *beeper)
>  	if (period) {
>  		pwm_config(beeper->pwm, period / 2, period);
>  		pwm_enable(beeper->pwm);
> -	} else
> +		if (beeper->reg) {
> +			int error;
> +
> +			error = regulator_enable(beeper->reg);
> +			if (!error)
> +				beeper->reg_enabled = true;
> +		}
> +	} else {
> +		if (beeper->reg_enabled) {
> +			regulator_disable(beeper->reg);
> +			beeper->reg_enabled = false;
> +		}
>  		pwm_disable(beeper->pwm);
> +	}
>  }
>  
>  static void pwm_beeper_work(struct work_struct *work)
> @@ -82,6 +97,10 @@ static void pwm_beeper_stop(struct pwm_beeper *beeper)
>  {
>  	cancel_work_sync(&beeper->work);
>  
> +	if (beeper->reg_enabled) {
> +		regulator_disable(beeper->reg);
> +		beeper->reg_enabled = false;
> +	}
>  	if (beeper->period)
>  		pwm_disable(beeper->pwm);
>  }
> @@ -111,6 +130,14 @@ static int pwm_beeper_probe(struct platform_device *pdev)
>  		return error;
>  	}
>  
> +	beeper->reg = devm_regulator_get_optional(&pdev->dev, "amp");

If you do not use optional regulator then you will not have to check if
you have it or not everywhere: regulator core will give you a dummy that
you can toggle to your heart's content.

> +	error = PTR_ERR_OR_ZERO(beeper->reg);
> +	if (error) {
> +		if (error != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to get amp regulator\n");
> +		return error;
> +	}
> +
>  	/*
>  	 * FIXME: pwm_apply_args() should be removed when switching to
>  	 * the atomic PWM API.
> -- 
> 2.7.4
> 

Thanks.

-- 
Dmitry

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

[David Lechner]

On 01/14/2017 01:19 PM, Dmitry Torokhov wrote:
> On Wed, Jan 11, 2017 at 02:02:01PM -0600, David Lechner wrote:
>> 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>
>> ---
>>  drivers/input/misc/pwm-beeper.c | 29 ++++++++++++++++++++++++++++-
>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
>> index 30ac227..708e88e 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,8 +26,10 @@
>>  struct pwm_beeper {
>>  	struct input_dev *input;
>>  	struct pwm_device *pwm;
>> +	struct regulator *reg;
>>  	struct work_struct work;
>>  	unsigned long period;
>> +	bool reg_enabled;
>>  };
>>
>>  #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
>> @@ -38,8 +41,20 @@ static void __pwm_beeper_set(struct pwm_beeper *beeper)
>>  	if (period) {
>>  		pwm_config(beeper->pwm, period / 2, period);
>>  		pwm_enable(beeper->pwm);
>> -	} else
>> +		if (beeper->reg) {
>> +			int error;
>> +
>> +			error = regulator_enable(beeper->reg);
>> +			if (!error)
>> +				beeper->reg_enabled = true;
>> +		}
>> +	} else {
>> +		if (beeper->reg_enabled) {
>> +			regulator_disable(beeper->reg);
>> +			beeper->reg_enabled = false;
>> +		}
>>  		pwm_disable(beeper->pwm);
>> +	}
>>  }
>>
>>  static void pwm_beeper_work(struct work_struct *work)
>> @@ -82,6 +97,10 @@ static void pwm_beeper_stop(struct pwm_beeper *beeper)
>>  {
>>  	cancel_work_sync(&beeper->work);
>>
>> +	if (beeper->reg_enabled) {
>> +		regulator_disable(beeper->reg);
>> +		beeper->reg_enabled = false;
>> +	}
>>  	if (beeper->period)
>>  		pwm_disable(beeper->pwm);
>>  }
>> @@ -111,6 +130,14 @@ static int pwm_beeper_probe(struct platform_device *pdev)
>>  		return error;
>>  	}
>>
>> +	beeper->reg = devm_regulator_get_optional(&pdev->dev, "amp");
>
> If you do not use optional regulator then you will not have to check if
> you have it or not everywhere: regulator core will give you a dummy that
> you can toggle to your heart's content.

Some months ago, I learned that if you are not using device tree and you 
do not call regulator_has_full_constraints(), then you do not get a 
dummy regulator. And here, we are only checking if the regulator exists 
in one place. We will still need the checks for beeper->reg_enabled to 
keep calls to regulator_enable() and regulator_disable() balanced.

On the other hand, it is recommended that you always call 
regulator_has_full_constraints(), so I don't mind changing it if that is 
what you think we should do. But, I don't really see much of an 
advantage to changing it compared to the current implementation.

>
>> +	error = PTR_ERR_OR_ZERO(beeper->reg);
>> +	if (error) {
>> +		if (error != -EPROBE_DEFER)
>> +			dev_err(dev, "Failed to get amp regulator\n");
>> +		return error;
>> +	}
>> +
>>  	/*
>>  	 * FIXME: pwm_apply_args() should be removed when switching to
>>  	 * the atomic PWM API.
>> --
>> 2.7.4
>>
>
> Thanks.
>

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

[Dmitry Torokhov]

On Sun, Jan 15, 2017 at 06:12:29PM -0600, David Lechner wrote:
> On 01/14/2017 01:19 PM, Dmitry Torokhov wrote:
> >On Wed, Jan 11, 2017 at 02:02:01PM -0600, David Lechner wrote:
> >>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>
> >>---
> >> drivers/input/misc/pwm-beeper.c | 29 ++++++++++++++++++++++++++++-
> >> 1 file changed, 28 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> >>index 30ac227..708e88e 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,8 +26,10 @@
> >> struct pwm_beeper {
> >> 	struct input_dev *input;
> >> 	struct pwm_device *pwm;
> >>+	struct regulator *reg;
> >> 	struct work_struct work;
> >> 	unsigned long period;
> >>+	bool reg_enabled;
> >> };
> >>
> >> #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
> >>@@ -38,8 +41,20 @@ static void __pwm_beeper_set(struct pwm_beeper *beeper)
> >> 	if (period) {
> >> 		pwm_config(beeper->pwm, period / 2, period);
> >> 		pwm_enable(beeper->pwm);
> >>-	} else
> >>+		if (beeper->reg) {
> >>+			int error;
> >>+
> >>+			error = regulator_enable(beeper->reg);
> >>+			if (!error)
> >>+				beeper->reg_enabled = true;
> >>+		}
> >>+	} else {
> >>+		if (beeper->reg_enabled) {
> >>+			regulator_disable(beeper->reg);
> >>+			beeper->reg_enabled = false;
> >>+		}
> >> 		pwm_disable(beeper->pwm);
> >>+	}
> >> }
> >>
> >> static void pwm_beeper_work(struct work_struct *work)
> >>@@ -82,6 +97,10 @@ static void pwm_beeper_stop(struct pwm_beeper *beeper)
> >> {
> >> 	cancel_work_sync(&beeper->work);
> >>
> >>+	if (beeper->reg_enabled) {
> >>+		regulator_disable(beeper->reg);
> >>+		beeper->reg_enabled = false;
> >>+	}
> >> 	if (beeper->period)
> >> 		pwm_disable(beeper->pwm);
> >> }
> >>@@ -111,6 +130,14 @@ static int pwm_beeper_probe(struct platform_device *pdev)
> >> 		return error;
> >> 	}
> >>
> >>+	beeper->reg = devm_regulator_get_optional(&pdev->dev, "amp");
> >
> >If you do not use optional regulator then you will not have to check if
> >you have it or not everywhere: regulator core will give you a dummy that
> >you can toggle to your heart's content.
> 
> Some months ago, I learned that if you are not using device tree and
> you do not call regulator_has_full_constraints(), then you do not
> get a dummy regulator. And here, we are only checking if the
> regulator exists in one place. We will still need the checks for
> beeper->reg_enabled to keep calls to regulator_enable() and
> regulator_disable() balanced.

Why? You do not have checks for calls to pwm_enable() and pwm_disable(),
(or rather beeper->period is used as such flag) why regulator would be
any different?

> 
> On the other hand, it is recommended that you always call
> regulator_has_full_constraints(), so I don't mind changing it if
> that is what you think we should do. But, I don't really see much of
> an advantage to changing it compared to the current implementation.

It greatly simplifies control flow in the driver (since I believe you
can get rid of the flags you introduced).

As far as arch not having full constraints - I am not sure if this makes
sense anymore. I am not quite sure what the original intent here was, we
should probably ask Mark Brown. But a lot of drivers do expect the dummy
substitution to imply work.

Thanks.

-- 
Dmitry

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

[David Lechner]

On 01/15/2017 06:34 PM, Dmitry Torokhov wrote:
> On Sun, Jan 15, 2017 at 06:12:29PM -0600, David Lechner wrote:
>> On 01/14/2017 01:19 PM, Dmitry Torokhov wrote:
>>> On Wed, Jan 11, 2017 at 02:02:01PM -0600, David Lechner wrote:
>>>> 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>
>>>> ---
>>>> drivers/input/misc/pwm-beeper.c | 29 ++++++++++++++++++++++++++++-
>>>> 1 file changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
>>>> index 30ac227..708e88e 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,8 +26,10 @@
>>>> struct pwm_beeper {
>>>> 	struct input_dev *input;
>>>> 	struct pwm_device *pwm;
>>>> +	struct regulator *reg;
>>>> 	struct work_struct work;
>>>> 	unsigned long period;
>>>> +	bool reg_enabled;
>>>> };
>>>>
>>>> #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
>>>> @@ -38,8 +41,20 @@ static void __pwm_beeper_set(struct pwm_beeper *beeper)
>>>> 	if (period) {
>>>> 		pwm_config(beeper->pwm, period / 2, period);
>>>> 		pwm_enable(beeper->pwm);
>>>> -	} else
>>>> +		if (beeper->reg) {
>>>> +			int error;
>>>> +
>>>> +			error = regulator_enable(beeper->reg);
>>>> +			if (!error)
>>>> +				beeper->reg_enabled = true;
>>>> +		}
>>>> +	} else {
>>>> +		if (beeper->reg_enabled) {
>>>> +			regulator_disable(beeper->reg);
>>>> +			beeper->reg_enabled = false;
>>>> +		}
>>>> 		pwm_disable(beeper->pwm);
>>>> +	}
>>>> }
>>>>
>>>> static void pwm_beeper_work(struct work_struct *work)
>>>> @@ -82,6 +97,10 @@ static void pwm_beeper_stop(struct pwm_beeper *beeper)
>>>> {
>>>> 	cancel_work_sync(&beeper->work);
>>>>
>>>> +	if (beeper->reg_enabled) {
>>>> +		regulator_disable(beeper->reg);
>>>> +		beeper->reg_enabled = false;
>>>> +	}
>>>> 	if (beeper->period)
>>>> 		pwm_disable(beeper->pwm);
>>>> }
>>>> @@ -111,6 +130,14 @@ static int pwm_beeper_probe(struct platform_device *pdev)
>>>> 		return error;
>>>> 	}
>>>>
>>>> +	beeper->reg = devm_regulator_get_optional(&pdev->dev, "amp");
>>>
>>> If you do not use optional regulator then you will not have to check if
>>> you have it or not everywhere: regulator core will give you a dummy that
>>> you can toggle to your heart's content.
>>
>> Some months ago, I learned that if you are not using device tree and
>> you do not call regulator_has_full_constraints(), then you do not
>> get a dummy regulator. And here, we are only checking if the
>> regulator exists in one place. We will still need the checks for
>> beeper->reg_enabled to keep calls to regulator_enable() and
>> regulator_disable() balanced.
>
> Why? You do not have checks for calls to pwm_enable() and pwm_disable(),
> (or rather beeper->period is used as such flag) why regulator would be
> any different?

regulator_enable() has a __must_check attribute on it, so we get 
compiler warnings if we do not check the return value. Also, if enabling 
the regulator fails and returns an error, then calling 
regulator_disable() later would cause an imbalance.

pwm_enable() and pwm_disable() work differently because they don't count 
how many times they have been called. regulator_enable() and 
regulator_disable(), on the other hand, work like reference counting.

>
>>
>> On the other hand, it is recommended that you always call
>> regulator_has_full_constraints(), so I don't mind changing it if
>> that is what you think we should do. But, I don't really see much of
>> an advantage to changing it compared to the current implementation.
>
> It greatly simplifies control flow in the driver (since I believe you
> can get rid of the flags you introduced).
>
> As far as arch not having full constraints - I am not sure if this makes
> sense anymore. I am not quite sure what the original intent here was, we
> should probably ask Mark Brown. But a lot of drivers do expect the dummy
> substitution to imply work.

I am OK with using the dummy regulator, but I don't see how I can get 
rid of the beeper->reg_enabled flag.

Re: [PATCH v2 2/3] dt-bindings: Input: Add optional amp-supply property to pwm-beeper

[Rob Herring]

On Wed, Jan 11, 2017 at 02:02:00PM -0600, David Lechner wrote:
> This adds an optional amp-supply property to pwm-beeper. This is a
> regulator that acts as an amplifier for the beeper.
> 
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>  Documentation/devicetree/bindings/input/pwm-beeper.txt | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

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

[Dmitry Torokhov]

On Sun, Jan 15, 2017 at 07:04:09PM -0600, David Lechner wrote:
> On 01/15/2017 06:34 PM, Dmitry Torokhov wrote:
> >On Sun, Jan 15, 2017 at 06:12:29PM -0600, David Lechner wrote:
> >>On 01/14/2017 01:19 PM, Dmitry Torokhov wrote:
> >>>On Wed, Jan 11, 2017 at 02:02:01PM -0600, David Lechner wrote:
> >>>>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>
> >>>>---
> >>>>drivers/input/misc/pwm-beeper.c | 29 ++++++++++++++++++++++++++++-
> >>>>1 file changed, 28 insertions(+), 1 deletion(-)
> >>>>
> >>>>diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> >>>>index 30ac227..708e88e 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,8 +26,10 @@
> >>>>struct pwm_beeper {
> >>>>	struct input_dev *input;
> >>>>	struct pwm_device *pwm;
> >>>>+	struct regulator *reg;
> >>>>	struct work_struct work;
> >>>>	unsigned long period;
> >>>>+	bool reg_enabled;
> >>>>};
> >>>>
> >>>>#define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
> >>>>@@ -38,8 +41,20 @@ static void __pwm_beeper_set(struct pwm_beeper *beeper)
> >>>>	if (period) {
> >>>>		pwm_config(beeper->pwm, period / 2, period);
> >>>>		pwm_enable(beeper->pwm);
> >>>>-	} else
> >>>>+		if (beeper->reg) {
> >>>>+			int error;
> >>>>+
> >>>>+			error = regulator_enable(beeper->reg);
> >>>>+			if (!error)
> >>>>+				beeper->reg_enabled = true;
> >>>>+		}
> >>>>+	} else {
> >>>>+		if (beeper->reg_enabled) {
> >>>>+			regulator_disable(beeper->reg);
> >>>>+			beeper->reg_enabled = false;
> >>>>+		}
> >>>>		pwm_disable(beeper->pwm);
> >>>>+	}
> >>>>}
> >>>>
> >>>>static void pwm_beeper_work(struct work_struct *work)
> >>>>@@ -82,6 +97,10 @@ static void pwm_beeper_stop(struct pwm_beeper *beeper)
> >>>>{
> >>>>	cancel_work_sync(&beeper->work);
> >>>>
> >>>>+	if (beeper->reg_enabled) {
> >>>>+		regulator_disable(beeper->reg);
> >>>>+		beeper->reg_enabled = false;
> >>>>+	}
> >>>>	if (beeper->period)
> >>>>		pwm_disable(beeper->pwm);
> >>>>}
> >>>>@@ -111,6 +130,14 @@ static int pwm_beeper_probe(struct platform_device *pdev)
> >>>>		return error;
> >>>>	}
> >>>>
> >>>>+	beeper->reg = devm_regulator_get_optional(&pdev->dev, "amp");
> >>>
> >>>If you do not use optional regulator then you will not have to check if
> >>>you have it or not everywhere: regulator core will give you a dummy that
> >>>you can toggle to your heart's content.
> >>
> >>Some months ago, I learned that if you are not using device tree and
> >>you do not call regulator_has_full_constraints(), then you do not
> >>get a dummy regulator. And here, we are only checking if the
> >>regulator exists in one place. We will still need the checks for
> >>beeper->reg_enabled to keep calls to regulator_enable() and
> >>regulator_disable() balanced.
> >
> >Why? You do not have checks for calls to pwm_enable() and pwm_disable(),
> >(or rather beeper->period is used as such flag) why regulator would be
> >any different?
> 
> regulator_enable() has a __must_check attribute on it, so we get
> compiler warnings if we do not check the return value. Also, if
> enabling the regulator fails and returns an error, then calling
> regulator_disable() later would cause an imbalance.
> 
> pwm_enable() and pwm_disable() work differently because they don't
> count how many times they have been called. regulator_enable() and
> regulator_disable(), on the other hand, work like reference
> counting.

Ah, you are right, but it is more than that. It is possible to receive
multiple SND_BELL/SND_TONE events with non-0 value. You need to check if
regulator is already enabled before trying to enable it second time, or
your counting will be off.

Thanks.

-- 
Dmitry