diff mbox

[v2] Input: pwm-beeper: support customized freq for SND_BELL

Message ID 1487579863-7256-1-git-send-email-hs@denx.de (mailing list archive)
State New, archived
Headers show

Commit Message

Heiko Schocher Feb. 20, 2017, 8:37 a.m. UTC
From: Guan Ben <ben.guan@cn.bosch.com>

extend the pwm-beeper driver to support customized frequency
for SND_BELL from device tree.

Signed-off-by: Guan Ben <ben.guan@cn.bosch.com>
Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
[hs@denx.de: adapted to 4.10-rc7]
Signed-off-by: Heiko Schocher <hs@denx.de>

---

Changes in v2:
- add comment from Rob Herring:
  rename property name "bell-frequency" to "beeper-hz"
- add comment from Dmitry Torokhov:
  use device_property_read_u32() instead of of_property_read_u32()
- rebased against c470abd4fde40ea6a0846a2beab642a578c0b8cd
  Linux 4.10

 .../devicetree/bindings/input/pwm-beeper.txt       |  3 ++
 drivers/input/misc/pwm-beeper.c                    | 36 ++++++++++++++++------
 2 files changed, 30 insertions(+), 9 deletions(-)

Comments

David Lechner Feb. 27, 2017, 6:11 p.m. UTC | #1
On 02/20/2017 02:37 AM, Heiko Schocher wrote:
> From: Guan Ben <ben.guan@cn.bosch.com>
>
> extend the pwm-beeper driver to support customized frequency
> for SND_BELL from device tree.
>
> Signed-off-by: Guan Ben <ben.guan@cn.bosch.com>
> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
> [hs@denx.de: adapted to 4.10-rc7]
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
>
> Changes in v2:
> - add comment from Rob Herring:
>   rename property name "bell-frequency" to "beeper-hz"

Is there a separate patch for the devicetree bindings documentation?

> - add comment from Dmitry Torokhov:
>   use device_property_read_u32() instead of of_property_read_u32()
> - rebased against c470abd4fde40ea6a0846a2beab642a578c0b8cd
>   Linux 4.10
>
>  .../devicetree/bindings/input/pwm-beeper.txt       |  3 ++
>  drivers/input/misc/pwm-beeper.c                    | 36 ++++++++++++++++------
>  2 files changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.txt b/Documentation/devicetree/bindings/input/pwm-beeper.txt
> index be332ae..4e4e128 100644
> --- a/Documentation/devicetree/bindings/input/pwm-beeper.txt
> +++ b/Documentation/devicetree/bindings/input/pwm-beeper.txt
> @@ -5,3 +5,6 @@ Registers a PWM device as beeper.
>  Required properties:
>  - compatible: should be "pwm-beeper"
>  - pwms: phandle to the physical PWM device
> +
> +optional properties:
> +- beeper-hz:  bell frequency in Hz
> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> index 5f9655d..5ea6fda 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;
> +	unsigned int bell_frequency;
>  };
>
>  #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
> @@ -58,20 +59,17 @@ static int pwm_beeper_event(struct input_dev *input,
>  	if (type != EV_SND || value < 0)
>  		return -EINVAL;
>
> -	switch (code) {
> -	case SND_BELL:
> -		value = value ? 1000 : 0;

This would be much simpler if you just changed the single line above:

		value = value ? beeper->bell_frequency : 0;

> -		break;
> -	case SND_TONE:
> -		break;
> -	default:
> +	if (code != SND_BELL && code != SND_TONE)
>  		return -EINVAL;
> -	}
>
>  	if (value == 0)
>  		beeper->period = 0;
> -	else
> +	else {
> +		if (code == SND_BELL)
> +			value = beeper->bell_frequency;
> +
>  		beeper->period = HZ_TO_NANOSECONDS(value);
> +	}
>
>  	schedule_work(&beeper->work);
>
> @@ -93,6 +91,25 @@ static void pwm_beeper_close(struct input_dev *input)
>  	pwm_beeper_stop(beeper);
>  }
>
> +static void pwm_beeper_init_bell_frequency(struct device *dev,
> +					   struct pwm_beeper *beeper)
> +{
> +	struct device_node *node;
> +	unsigned int bell_frequency = 1000;
> +	int err;
> +
> +	if (IS_ENABLED(CONFIG_OF)) {

I don't think the check for CONFIG_OF is needed when using 
device_property_read_u32().

> +		node = dev->of_node;

node variable is never used

> +		err = device_property_read_u32(dev, "beeper-hz",
> +					       &bell_frequency);

Does the device_property_read_u32() function guarantee that 
bell_frequency is not modified when err < 0 ?

> +		if (err < 0)
> +			dev_dbg(dev, "Failed to read beeper-hz, using default: %u Hz\n",

"Failed" sounds like an error, but this is a perfectly normal thing to 
happen. Maybe better to say "'beeper-hz' not specified, using default: 
%u Hz\n".

> +				bell_frequency);
> +	}
> +
> +	beeper->bell_frequency = bell_frequency;
> +}
> +
>  static int pwm_beeper_probe(struct platform_device *pdev)
>  {
>  	unsigned long pwm_id = (unsigned long)dev_get_platdata(&pdev->dev);
> @@ -122,6 +139,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
>  	pwm_apply_args(beeper->pwm);
>
>  	INIT_WORK(&beeper->work, pwm_beeper_work);
> +	pwm_beeper_init_bell_frequency(&pdev->dev, beeper);

This function is so simple, it could just be done inline here.

>
>  	beeper->input = input_allocate_device();
>  	if (!beeper->input) {
>

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Feb. 27, 2017, 7:10 p.m. UTC | #2
On Mon, Feb 20, 2017 at 09:37:43AM +0100, Heiko Schocher wrote:
> From: Guan Ben <ben.guan@cn.bosch.com>
> 
> extend the pwm-beeper driver to support customized frequency
> for SND_BELL from device tree.
> 
> Signed-off-by: Guan Ben <ben.guan@cn.bosch.com>
> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
> [hs@denx.de: adapted to 4.10-rc7]
> Signed-off-by: Heiko Schocher <hs@denx.de>
> 
> ---
> 
> Changes in v2:
> - add comment from Rob Herring:
>   rename property name "bell-frequency" to "beeper-hz"
> - add comment from Dmitry Torokhov:
>   use device_property_read_u32() instead of of_property_read_u32()
> - rebased against c470abd4fde40ea6a0846a2beab642a578c0b8cd
>   Linux 4.10
> 
>  .../devicetree/bindings/input/pwm-beeper.txt       |  3 ++

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

A few comments on the driver though.

>  drivers/input/misc/pwm-beeper.c                    | 36 ++++++++++++++++------
>  2 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.txt b/Documentation/devicetree/bindings/input/pwm-beeper.txt
> index be332ae..4e4e128 100644
> --- a/Documentation/devicetree/bindings/input/pwm-beeper.txt
> +++ b/Documentation/devicetree/bindings/input/pwm-beeper.txt
> @@ -5,3 +5,6 @@ Registers a PWM device as beeper.
>  Required properties:
>  - compatible: should be "pwm-beeper"
>  - pwms: phandle to the physical PWM device
> +
> +optional properties:
> +- beeper-hz:  bell frequency in Hz
> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> index 5f9655d..5ea6fda 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;
> +	unsigned int bell_frequency;
>  };
>  
>  #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
> @@ -58,20 +59,17 @@ static int pwm_beeper_event(struct input_dev *input,
>  	if (type != EV_SND || value < 0)
>  		return -EINVAL;
>  
> -	switch (code) {
> -	case SND_BELL:
> -		value = value ? 1000 : 0;
> -		break;
> -	case SND_TONE:
> -		break;
> -	default:
> +	if (code != SND_BELL && code != SND_TONE)
>  		return -EINVAL;
> -	}
>  
>  	if (value == 0)
>  		beeper->period = 0;
> -	else
> +	else {
> +		if (code == SND_BELL)
> +			value = beeper->bell_frequency;
> +
>  		beeper->period = HZ_TO_NANOSECONDS(value);
> +	}
>  
>  	schedule_work(&beeper->work);
>  
> @@ -93,6 +91,25 @@ static void pwm_beeper_close(struct input_dev *input)
>  	pwm_beeper_stop(beeper);
>  }
>  
> +static void pwm_beeper_init_bell_frequency(struct device *dev,
> +					   struct pwm_beeper *beeper)
> +{
> +	struct device_node *node;
> +	unsigned int bell_frequency = 1000;
> +	int err;
> +
> +	if (IS_ENABLED(CONFIG_OF)) {

You don't really need this. There should be an empty version of the 
function.

> +		node = dev->of_node;

You don't need this line.

> +		err = device_property_read_u32(dev, "beeper-hz",
> +					       &bell_frequency);
> +		if (err < 0)
> +			dev_dbg(dev, "Failed to read beeper-hz, using default: %u Hz\n",
> +				bell_frequency);
> +	}
> +
> +	beeper->bell_frequency = bell_frequency;
> +}
> +
>  static int pwm_beeper_probe(struct platform_device *pdev)
>  {
>  	unsigned long pwm_id = (unsigned long)dev_get_platdata(&pdev->dev);
> @@ -122,6 +139,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
>  	pwm_apply_args(beeper->pwm);
>  
>  	INIT_WORK(&beeper->work, pwm_beeper_work);
> +	pwm_beeper_init_bell_frequency(&pdev->dev, beeper);
>  
>  	beeper->input = input_allocate_device();
>  	if (!beeper->input) {
> -- 
> 2.7.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Heiko Schocher Feb. 28, 2017, 5:19 a.m. UTC | #3
Hello David,

Am 27.02.2017 um 19:11 schrieb David Lechner:
> On 02/20/2017 02:37 AM, Heiko Schocher wrote:
>> From: Guan Ben <ben.guan@cn.bosch.com>
>>
>> extend the pwm-beeper driver to support customized frequency
>> for SND_BELL from device tree.
>>
>> Signed-off-by: Guan Ben <ben.guan@cn.bosch.com>
>> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
>> [hs@denx.de: adapted to 4.10-rc7]
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
>>
>> Changes in v2:
>> - add comment from Rob Herring:
>>   rename property name "bell-frequency" to "beeper-hz"
>
> Is there a separate patch for the devicetree bindings documentation?

No, it is in this patch ... In the meantime I got an
Acked-by from Rob Herring ...

>> - add comment from Dmitry Torokhov:
>>   use device_property_read_u32() instead of of_property_read_u32()
>> - rebased against c470abd4fde40ea6a0846a2beab642a578c0b8cd
>>   Linux 4.10
>>
>>  .../devicetree/bindings/input/pwm-beeper.txt       |  3 ++
>>  drivers/input/misc/pwm-beeper.c                    | 36 ++++++++++++++++------
>>  2 files changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.txt
>> b/Documentation/devicetree/bindings/input/pwm-beeper.txt
>> index be332ae..4e4e128 100644
>> --- a/Documentation/devicetree/bindings/input/pwm-beeper.txt
>> +++ b/Documentation/devicetree/bindings/input/pwm-beeper.txt
>> @@ -5,3 +5,6 @@ Registers a PWM device as beeper.
>>  Required properties:
>>  - compatible: should be "pwm-beeper"
>>  - pwms: phandle to the physical PWM device
>> +
>> +optional properties:
>> +- beeper-hz:  bell frequency in Hz
>> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
>> index 5f9655d..5ea6fda 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;
>> +    unsigned int bell_frequency;
>>  };
>>
>>  #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
>> @@ -58,20 +59,17 @@ static int pwm_beeper_event(struct input_dev *input,
>>      if (type != EV_SND || value < 0)
>>          return -EINVAL;
>>
>> -    switch (code) {
>> -    case SND_BELL:
>> -        value = value ? 1000 : 0;
>
> This would be much simpler if you just changed the single line above:
>
>          value = value ? beeper->bell_frequency : 0;

Ok, I readded the switch statement, and changed this line.

>> -        break;
>> -    case SND_TONE:
>> -        break;
>> -    default:
>> +    if (code != SND_BELL && code != SND_TONE)
>>          return -EINVAL;
>> -    }
>>
>>      if (value == 0)
>>          beeper->period = 0;
>> -    else
>> +    else {
>> +        if (code == SND_BELL)
>> +            value = beeper->bell_frequency;
>> +
>>          beeper->period = HZ_TO_NANOSECONDS(value);
>> +    }
>>
>>      schedule_work(&beeper->work);
>>
>> @@ -93,6 +91,25 @@ static void pwm_beeper_close(struct input_dev *input)
>>      pwm_beeper_stop(beeper);
>>  }
>>
>> +static void pwm_beeper_init_bell_frequency(struct device *dev,
>> +                       struct pwm_beeper *beeper)
>> +{
>> +    struct device_node *node;
>> +    unsigned int bell_frequency = 1000;
>> +    int err;
>> +
>> +    if (IS_ENABLED(CONFIG_OF)) {
>
> I don't think the check for CONFIG_OF is needed when using device_property_read_u32().

Yes, removed.

>
>> +        node = dev->of_node;
>
> node variable is never used

Removed.

>
>> +        err = device_property_read_u32(dev, "beeper-hz",
>> +                           &bell_frequency);
>
> Does the device_property_read_u32() function guarantee that bell_frequency is not modified when err
> < 0 ?

Yes. This function "ends" in of_property_read_variable_u32_array()
which first searches the property (If not found returns) and if all
checks are fine, fills the "*out_values" with the values from the
property...

>> +        if (err < 0)
>> +            dev_dbg(dev, "Failed to read beeper-hz, using default: %u Hz\n",
>
> "Failed" sounds like an error, but this is a perfectly normal thing to happen. Maybe better to say
> "'beeper-hz' not specified, using default: %u Hz\n".

Changed.

>
>> +                bell_frequency);
>> +    }
>> +
>> +    beeper->bell_frequency = bell_frequency;
>> +}
>> +
>>  static int pwm_beeper_probe(struct platform_device *pdev)
>>  {
>>      unsigned long pwm_id = (unsigned long)dev_get_platdata(&pdev->dev);
>> @@ -122,6 +139,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
>>      pwm_apply_args(beeper->pwm);
>>
>>      INIT_WORK(&beeper->work, pwm_beeper_work);
>> +    pwm_beeper_init_bell_frequency(&pdev->dev, beeper);
>
> This function is so simple, it could just be done inline here.

Indeed ... changed.

Thanks!

bye,
Heiko
>
>>
>>      beeper->input = input_allocate_device();
>>      if (!beeper->input) {
>>
>
Heiko Schocher Feb. 28, 2017, 5:20 a.m. UTC | #4
Hello Rob,

Am 27.02.2017 um 20:10 schrieb Rob Herring:
> On Mon, Feb 20, 2017 at 09:37:43AM +0100, Heiko Schocher wrote:
>> From: Guan Ben <ben.guan@cn.bosch.com>
>>
>> extend the pwm-beeper driver to support customized frequency
>> for SND_BELL from device tree.
>>
>> Signed-off-by: Guan Ben <ben.guan@cn.bosch.com>
>> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
>> [hs@denx.de: adapted to 4.10-rc7]
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>
>> ---
>>
>> Changes in v2:
>> - add comment from Rob Herring:
>>    rename property name "bell-frequency" to "beeper-hz"
>> - add comment from Dmitry Torokhov:
>>    use device_property_read_u32() instead of of_property_read_u32()
>> - rebased against c470abd4fde40ea6a0846a2beab642a578c0b8cd
>>    Linux 4.10
>>
>>   .../devicetree/bindings/input/pwm-beeper.txt       |  3 ++
>
> Acked-by: Rob Herring <robh@kernel.org>

Thanks!

> A few comments on the driver though.
>
>>   drivers/input/misc/pwm-beeper.c                    | 36 ++++++++++++++++------
>>   2 files changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.txt b/Documentation/devicetree/bindings/input/pwm-beeper.txt
>> index be332ae..4e4e128 100644
>> --- a/Documentation/devicetree/bindings/input/pwm-beeper.txt
>> +++ b/Documentation/devicetree/bindings/input/pwm-beeper.txt
>> @@ -5,3 +5,6 @@ Registers a PWM device as beeper.
>>   Required properties:
>>   - compatible: should be "pwm-beeper"
>>   - pwms: phandle to the physical PWM device
>> +
>> +optional properties:
>> +- beeper-hz:  bell frequency in Hz
>> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
>> index 5f9655d..5ea6fda 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;
>> +	unsigned int bell_frequency;
>>   };
>>
>>   #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
>> @@ -58,20 +59,17 @@ static int pwm_beeper_event(struct input_dev *input,
>>   	if (type != EV_SND || value < 0)
>>   		return -EINVAL;
>>
>> -	switch (code) {
>> -	case SND_BELL:
>> -		value = value ? 1000 : 0;
>> -		break;
>> -	case SND_TONE:
>> -		break;
>> -	default:
>> +	if (code != SND_BELL && code != SND_TONE)
>>   		return -EINVAL;
>> -	}
>>
>>   	if (value == 0)
>>   		beeper->period = 0;
>> -	else
>> +	else {
>> +		if (code == SND_BELL)
>> +			value = beeper->bell_frequency;
>> +
>>   		beeper->period = HZ_TO_NANOSECONDS(value);
>> +	}
>>
>>   	schedule_work(&beeper->work);
>>
>> @@ -93,6 +91,25 @@ static void pwm_beeper_close(struct input_dev *input)
>>   	pwm_beeper_stop(beeper);
>>   }
>>
>> +static void pwm_beeper_init_bell_frequency(struct device *dev,
>> +					   struct pwm_beeper *beeper)
>> +{
>> +	struct device_node *node;
>> +	unsigned int bell_frequency = 1000;
>> +	int err;
>> +
>> +	if (IS_ENABLED(CONFIG_OF)) {
>
> You don't really need this. There should be an empty version of the
> function.

removed, as also David mentioned this.

>
>> +		node = dev->of_node;
>
> You don't need this line.

Yep, removed.

>
>> +		err = device_property_read_u32(dev, "beeper-hz",
>> +					       &bell_frequency);
>> +		if (err < 0)
>> +			dev_dbg(dev, "Failed to read beeper-hz, using default: %u Hz\n",
>> +				bell_frequency);
>> +	}
>> +
>> +	beeper->bell_frequency = bell_frequency;
>> +}
>> +
>>   static int pwm_beeper_probe(struct platform_device *pdev)
>>   {
>>   	unsigned long pwm_id = (unsigned long)dev_get_platdata(&pdev->dev);
>> @@ -122,6 +139,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
>>   	pwm_apply_args(beeper->pwm);
>>
>>   	INIT_WORK(&beeper->work, pwm_beeper_work);
>> +	pwm_beeper_init_bell_frequency(&pdev->dev, beeper);
>>
>>   	beeper->input = input_allocate_device();
>>   	if (!beeper->input) {
>> --
>> 2.7.4
>>
>

Thanks!

bye,
Heiko
David Lechner Feb. 28, 2017, 5:30 a.m. UTC | #5
On 02/27/2017 11:19 PM, Heiko Schocher wrote:
> Hello David,
>
> Am 27.02.2017 um 19:11 schrieb David Lechner:
>> On 02/20/2017 02:37 AM, Heiko Schocher wrote:
>>> From: Guan Ben <ben.guan@cn.bosch.com>
>>>
>>> extend the pwm-beeper driver to support customized frequency
>>> for SND_BELL from device tree.
>>>
>>> Signed-off-by: Guan Ben <ben.guan@cn.bosch.com>
>>> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
>>> [hs@denx.de: adapted to 4.10-rc7]
>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>> ---
>>>
>>> Changes in v2:
>>> - add comment from Rob Herring:
>>>   rename property name "bell-frequency" to "beeper-hz"
>>
>> Is there a separate patch for the devicetree bindings documentation?
>
> No, it is in this patch ... In the meantime I got an
> Acked-by from Rob Herring ...
>

What I should have said is that, in general, I think it is preferred to 
have the device tree bindings as a separate patch from the code changes.

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Heiko Schocher Feb. 28, 2017, 6:13 a.m. UTC | #6
Hello David,

Am 28.02.2017 um 06:30 schrieb David Lechner:
> On 02/27/2017 11:19 PM, Heiko Schocher wrote:
>> Hello David,
>>
>> Am 27.02.2017 um 19:11 schrieb David Lechner:
>>> On 02/20/2017 02:37 AM, Heiko Schocher wrote:
>>>> From: Guan Ben <ben.guan@cn.bosch.com>
>>>>
>>>> extend the pwm-beeper driver to support customized frequency
>>>> for SND_BELL from device tree.
>>>>
>>>> Signed-off-by: Guan Ben <ben.guan@cn.bosch.com>
>>>> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
>>>> [hs@denx.de: adapted to 4.10-rc7]
>>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - add comment from Rob Herring:
>>>>   rename property name "bell-frequency" to "beeper-hz"
>>>
>>> Is there a separate patch for the devicetree bindings documentation?
>>
>> No, it is in this patch ... In the meantime I got an
>> Acked-by from Rob Herring ...
>>
>
> What I should have said is that, in general, I think it is preferred to have the device tree
> bindings as a separate patch from the code changes.

I can split this into 2 patches ...

bye,
Heiko
Dmitry Torokhov Feb. 28, 2017, 6:20 a.m. UTC | #7
On February 27, 2017 10:13:43 PM PST, Heiko Schocher <hs@denx.de> wrote:
>Hello David,
>
>Am 28.02.2017 um 06:30 schrieb David Lechner:
>> On 02/27/2017 11:19 PM, Heiko Schocher wrote:
>>> Hello David,
>>>
>>> Am 27.02.2017 um 19:11 schrieb David Lechner:
>>>> On 02/20/2017 02:37 AM, Heiko Schocher wrote:
>>>>> From: Guan Ben <ben.guan@cn.bosch.com>
>>>>>
>>>>> extend the pwm-beeper driver to support customized frequency
>>>>> for SND_BELL from device tree.
>>>>>
>>>>> Signed-off-by: Guan Ben <ben.guan@cn.bosch.com>
>>>>> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
>>>>> [hs@denx.de: adapted to 4.10-rc7]
>>>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - add comment from Rob Herring:
>>>>>   rename property name "bell-frequency" to "beeper-hz"
>>>>
>>>> Is there a separate patch for the devicetree bindings
>documentation?
>>>
>>> No, it is in this patch ... In the meantime I got an
>>> Acked-by from Rob Herring ...
>>>
>>
>> What I should have said is that, in general, I think it is preferred
>to have the device tree
>> bindings as a separate patch from the code changes.
>
>I can split this into 2 patches ...

Don't - since Rob already asked DT change you don't need to post to device tree list again and I'll merge them back together since they do not make sense separately.


Thanks.
Heiko Schocher March 1, 2017, 5:05 a.m. UTC | #8
Hello Dimitry,

Am 28.02.2017 um 07:20 schrieb Dmitry Torokhov:
> On February 27, 2017 10:13:43 PM PST, Heiko Schocher <hs@denx.de> wrote:
>> Hello David,
>>
>> Am 28.02.2017 um 06:30 schrieb David Lechner:
>>> On 02/27/2017 11:19 PM, Heiko Schocher wrote:
>>>> Hello David,
>>>>
>>>> Am 27.02.2017 um 19:11 schrieb David Lechner:
>>>>> On 02/20/2017 02:37 AM, Heiko Schocher wrote:
>>>>>> From: Guan Ben <ben.guan@cn.bosch.com>
>>>>>>
>>>>>> extend the pwm-beeper driver to support customized frequency
>>>>>> for SND_BELL from device tree.
>>>>>>
>>>>>> Signed-off-by: Guan Ben <ben.guan@cn.bosch.com>
>>>>>> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
>>>>>> [hs@denx.de: adapted to 4.10-rc7]
>>>>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>> - add comment from Rob Herring:
>>>>>>    rename property name "bell-frequency" to "beeper-hz"
>>>>>
>>>>> Is there a separate patch for the devicetree bindings
>> documentation?
>>>>
>>>> No, it is in this patch ... In the meantime I got an
>>>> Acked-by from Rob Herring ...
>>>>
>>>
>>> What I should have said is that, in general, I think it is preferred
>> to have the device tree
>>> bindings as a separate patch from the code changes.
>>
>> I can split this into 2 patches ...
>
> Don't - since Rob already asked DT change you don't need to post to device tree list again and I'll merge them back together since they do not make sense separately.

Ok, I post a v3 with the changes suggested from David and Rob soon,
without splitting.

Thanks!

bye,
Heiko
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.txt b/Documentation/devicetree/bindings/input/pwm-beeper.txt
index be332ae..4e4e128 100644
--- a/Documentation/devicetree/bindings/input/pwm-beeper.txt
+++ b/Documentation/devicetree/bindings/input/pwm-beeper.txt
@@ -5,3 +5,6 @@  Registers a PWM device as beeper.
 Required properties:
 - compatible: should be "pwm-beeper"
 - pwms: phandle to the physical PWM device
+
+optional properties:
+- beeper-hz:  bell frequency in Hz
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 5f9655d..5ea6fda 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;
+	unsigned int bell_frequency;
 };
 
 #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
@@ -58,20 +59,17 @@  static int pwm_beeper_event(struct input_dev *input,
 	if (type != EV_SND || value < 0)
 		return -EINVAL;
 
-	switch (code) {
-	case SND_BELL:
-		value = value ? 1000 : 0;
-		break;
-	case SND_TONE:
-		break;
-	default:
+	if (code != SND_BELL && code != SND_TONE)
 		return -EINVAL;
-	}
 
 	if (value == 0)
 		beeper->period = 0;
-	else
+	else {
+		if (code == SND_BELL)
+			value = beeper->bell_frequency;
+
 		beeper->period = HZ_TO_NANOSECONDS(value);
+	}
 
 	schedule_work(&beeper->work);
 
@@ -93,6 +91,25 @@  static void pwm_beeper_close(struct input_dev *input)
 	pwm_beeper_stop(beeper);
 }
 
+static void pwm_beeper_init_bell_frequency(struct device *dev,
+					   struct pwm_beeper *beeper)
+{
+	struct device_node *node;
+	unsigned int bell_frequency = 1000;
+	int err;
+
+	if (IS_ENABLED(CONFIG_OF)) {
+		node = dev->of_node;
+		err = device_property_read_u32(dev, "beeper-hz",
+					       &bell_frequency);
+		if (err < 0)
+			dev_dbg(dev, "Failed to read beeper-hz, using default: %u Hz\n",
+				bell_frequency);
+	}
+
+	beeper->bell_frequency = bell_frequency;
+}
+
 static int pwm_beeper_probe(struct platform_device *pdev)
 {
 	unsigned long pwm_id = (unsigned long)dev_get_platdata(&pdev->dev);
@@ -122,6 +139,7 @@  static int pwm_beeper_probe(struct platform_device *pdev)
 	pwm_apply_args(beeper->pwm);
 
 	INIT_WORK(&beeper->work, pwm_beeper_work);
+	pwm_beeper_init_bell_frequency(&pdev->dev, beeper);
 
 	beeper->input = input_allocate_device();
 	if (!beeper->input) {