Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1556878 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/3] Input: add optional amplifier regulator to pwm-beeper​ (previously "Input: add optional enable gpio to pwm-beeper​") David Lechner <david@lechnology.com> - 2017-01-11 21:10 +0100
    [PATCH v2 1/3] Input: pwm-beeper: suppress error message on probe defer David Lechner <david@lechnology.com> - 2017-01-11 21:10 +0100
      Re: [PATCH v2 1/3] Input: pwm-beeper: suppress error message on  probe defer Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-14 20:20 +0100
    [PATCH v2 2/3] dt-bindings: Input: Add optional amp-supply property to pwm-beeper David Lechner <david@lechnology.com> - 2017-01-11 21:10 +0100
    [PATCH v2 3/3] Input: pwm-beeper: add optional amplifier regulator David Lechner <david@lechnology.com> - 2017-01-11 21:10 +0100
      Re: [PATCH v2 3/3] Input: pwm-beeper: add optional amplifier  regulator Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-14 20:20 +0100
        Re: [PATCH v2 3/3] Input: pwm-beeper: add optional amplifier  regulator David Lechner <david@lechnology.com> - 2017-01-16 01:20 +0100
          Re: [PATCH v2 3/3] Input: pwm-beeper: add optional amplifier  regulator Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-16 01:40 +0100
            Re: [PATCH v2 3/3] Input: pwm-beeper: add optional amplifier  regulator David Lechner <david@lechnology.com> - 2017-01-16 02:10 +0100

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web