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

[Heiko Schocher]

Hello Dmitry,

Am 14.02.2017 um 05:27 schrieb Dmitry Torokhov:

On Wed, Feb 08, 2017 at 10:11:21AM +, Jonas Mark (ST-FIR/ENG1) wrote:

Hello Dmitry,


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


No, SND_BELL is literally SND_TONE @1000Hz. There should be no
customizing. If applications want to use different frequency then should
be using SND_TONE.


We are not aiming for an application shortcut here. Instead, changing
the bell frequency shall be a system setting. That is, every
application which wants to make a bell sound shall use the alternative
frequency.

The reason why we are deviating from the default 1000 Hz is that on
our hardware we are using a loudspeaker which is rated for 2.7 kHz.
That is, it will only sound at the specified volume and frequency if
you feed it with a 2.7 kHz square wave. If you deviate from it, e.g.
by using 1000 Hz, the output will be dim and squeaky. Worst case,
SND_BELL would be completely silent on our system. So the only bell
sound we can reliably generate on our system has 2.7 kHz.


OK, fair enough. Please address Rob's comments on binding and resend.


Done already, just waited for more comments before sending v2.


Also I am not sure why you needed to change the switch statement around,
you only need to replace 1000 with value from the attribute.


Hmm.. the resulting code looks cleaner to me ... First we check all
the reasons for returning an error code, after that, we can do
the functions work ... but I can revert this part ... should I ?


Oh, and please use device_property_*() API instead of of_*().


Thanks! Good tip, changed.

bye,
Heiko
--
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

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

[Heiko Schocher]

Hello Dmitry,

Am 14.02.2017 um 05:27 schrieb Dmitry Torokhov:

On Wed, Feb 08, 2017 at 10:11:21AM +, Jonas Mark (ST-FIR/ENG1) wrote:

Hello Dmitry,


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


No, SND_BELL is literally SND_TONE @1000Hz. There should be no
customizing. If applications want to use different frequency then should
be using SND_TONE.


We are not aiming for an application shortcut here. Instead, changing
the bell frequency shall be a system setting. That is, every
application which wants to make a bell sound shall use the alternative
frequency.

The reason why we are deviating from the default 1000 Hz is that on
our hardware we are using a loudspeaker which is rated for 2.7 kHz.
That is, it will only sound at the specified volume and frequency if
you feed it with a 2.7 kHz square wave. If you deviate from it, e.g.
by using 1000 Hz, the output will be dim and squeaky. Worst case,
SND_BELL would be completely silent on our system. So the only bell
sound we can reliably generate on our system has 2.7 kHz.


OK, fair enough. Please address Rob's comments on binding and resend.


Done already, just waited for more comments before sending v2.


Also I am not sure why you needed to change the switch statement around,
you only need to replace 1000 with value from the attribute.


Hmm.. the resulting code looks cleaner to me ... First we check all
the reasons for returning an error code, after that, we can do
the functions work ... but I can revert this part ... should I ?


Oh, and please use device_property_*() API instead of of_*().


Thanks! Good tip, changed.

bye,
Heiko
--
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

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

[Dmitry Torokhov]

On Wed, Feb 08, 2017 at 10:11:21AM +, Jonas Mark (ST-FIR/ENG1) wrote:
> Hello Dmitry,
> 
> > > extend the pwm-beeper driver to support customized frequency
> > > for SND_BELL from device tree.
> > 
> > No, SND_BELL is literally SND_TONE @1000Hz. There should be no
> > customizing. If applications want to use different frequency then should
> > be using SND_TONE.
> 
> We are not aiming for an application shortcut here. Instead, changing
> the bell frequency shall be a system setting. That is, every
> application which wants to make a bell sound shall use the alternative
> frequency.
> 
> The reason why we are deviating from the default 1000 Hz is that on
> our hardware we are using a loudspeaker which is rated for 2.7 kHz.
> That is, it will only sound at the specified volume and frequency if
> you feed it with a 2.7 kHz square wave. If you deviate from it, e.g.
> by using 1000 Hz, the output will be dim and squeaky. Worst case,
> SND_BELL would be completely silent on our system. So the only bell
> sound we can reliably generate on our system has 2.7 kHz.

OK, fair enough. Please address Rob's comments on binding and resend.

Also I am not sure why you needed to change the switch statement around,
you only need to replace 1000 with value from the attribute.

Oh, and please use device_property_*() API instead of of_*().

Thanks.

-- 
Dmitry

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

[Dmitry Torokhov]

On Wed, Feb 08, 2017 at 10:11:21AM +, Jonas Mark (ST-FIR/ENG1) wrote:
> Hello Dmitry,
> 
> > > extend the pwm-beeper driver to support customized frequency
> > > for SND_BELL from device tree.
> > 
> > No, SND_BELL is literally SND_TONE @1000Hz. There should be no
> > customizing. If applications want to use different frequency then should
> > be using SND_TONE.
> 
> We are not aiming for an application shortcut here. Instead, changing
> the bell frequency shall be a system setting. That is, every
> application which wants to make a bell sound shall use the alternative
> frequency.
> 
> The reason why we are deviating from the default 1000 Hz is that on
> our hardware we are using a loudspeaker which is rated for 2.7 kHz.
> That is, it will only sound at the specified volume and frequency if
> you feed it with a 2.7 kHz square wave. If you deviate from it, e.g.
> by using 1000 Hz, the output will be dim and squeaky. Worst case,
> SND_BELL would be completely silent on our system. So the only bell
> sound we can reliably generate on our system has 2.7 kHz.

OK, fair enough. Please address Rob's comments on binding and resend.

Also I am not sure why you needed to change the switch statement around,
you only need to replace 1000 with value from the attribute.

Oh, and please use device_property_*() API instead of of_*().

Thanks.

-- 
Dmitry

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

[Heiko Schocher]

Hello Rob,

Am 10.02.2017 um 16:48 schrieb Rob Herring:

On Tue, Feb 07, 2017 at 06:21:34AM +0100, Heiko Schocher wrote:

From: Guan Ben 

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

Signed-off-by: Guan Ben 
Signed-off-by: Mark Jonas 
[h...@denx.de: adapted to 4.10-rc7]
Signed-off-by: Heiko Schocher 

---

  .../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..438c6e0 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:
+- bell-frequency:  bell frequency in Hz


Needs a unit suffix:
bell-frequency-hz or just bell-hz as hz implies frequency.

Or maybe beeper-hz would be more consistant.


Ok, I change it to "beeper-hz". Are this all issues with this patch?

bye,
Heiko
--
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

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

[Heiko Schocher]

Hello Rob,

Am 10.02.2017 um 16:48 schrieb Rob Herring:

On Tue, Feb 07, 2017 at 06:21:34AM +0100, Heiko Schocher wrote:

From: Guan Ben 

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

Signed-off-by: Guan Ben 
Signed-off-by: Mark Jonas 
[h...@denx.de: adapted to 4.10-rc7]
Signed-off-by: Heiko Schocher 

---

  .../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..438c6e0 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:
+- bell-frequency:  bell frequency in Hz


Needs a unit suffix:
bell-frequency-hz or just bell-hz as hz implies frequency.

Or maybe beeper-hz would be more consistant.


Ok, I change it to "beeper-hz". Are this all issues with this patch?

bye,
Heiko
--
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

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

[Rob Herring]

On Tue, Feb 07, 2017 at 06:21:34AM +0100, Heiko Schocher wrote:
> From: Guan Ben 
> 
> extend the pwm-beeper driver to support customized frequency
> for SND_BELL from device tree.
> 
> Signed-off-by: Guan Ben 
> Signed-off-by: Mark Jonas 
> [h...@denx.de: adapted to 4.10-rc7]
> Signed-off-by: Heiko Schocher 
> 
> ---
> 
>  .../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..438c6e0 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:
> +- bell-frequency:  bell frequency in Hz

Needs a unit suffix:
bell-frequency-hz or just bell-hz as hz implies frequency.

Or maybe beeper-hz would be more consistant.

Rob

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

[Rob Herring]

On Tue, Feb 07, 2017 at 06:21:34AM +0100, Heiko Schocher wrote:
> From: Guan Ben 
> 
> extend the pwm-beeper driver to support customized frequency
> for SND_BELL from device tree.
> 
> Signed-off-by: Guan Ben 
> Signed-off-by: Mark Jonas 
> [h...@denx.de: adapted to 4.10-rc7]
> Signed-off-by: Heiko Schocher 
> 
> ---
> 
>  .../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..438c6e0 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:
> +- bell-frequency:  bell frequency in Hz

Needs a unit suffix:
bell-frequency-hz or just bell-hz as hz implies frequency.

Or maybe beeper-hz would be more consistant.

Rob

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

[Jonas Mark (ST-FIR/ENG1)]

Hello Dmitry,

> > extend the pwm-beeper driver to support customized frequency
> > for SND_BELL from device tree.
> 
> No, SND_BELL is literally SND_TONE @1000Hz. There should be no
> customizing. If applications want to use different frequency then should
> be using SND_TONE.

We are not aiming for an application shortcut here. Instead, changing the bell 
frequency shall be a system setting. That is, every application which wants to 
make a bell sound shall use the alternative frequency.

The reason why we are deviating from the default 1000 Hz is that on our 
hardware we are using a loudspeaker which is rated for 2.7 kHz. That is, it 
will only sound at the specified volume and frequency if you feed it with a 2.7 
kHz square wave. If you deviate from it, e.g. by using 1000 Hz, the output will 
be dim and squeaky. Worst case, SND_BELL would be completely silent on our 
system. So the only bell sound we can reliably generate on our system has 2.7 
kHz.

The good news for everybody else's systems is that the patch does not change 
the default behavior. If not specified in the DT, the frequency will still be 
1000 Hz. But other systems which are similarly challenged could now offer a 
reasonable bell sound, too.

Can you point me to a specification or standard which defines that SND_BELL has 
to be 1000 Hz without any exception?

I did some research on that topic and was not able to discover such a 
specification or standard. What I did find though was that the ASCII or Unicode 
BEL character is most likely very closely related to SND_BELL. And for BEL 
there is no frequency specified. Historically it once was a real bell which was 
rung. The intention was to notify the human on the other end of a teletype to 
have a look at it. And that is guaranteed with 1000 Hz as well as with 2.7 kHz 
as long as the sound is perceptible to the human ear.

https://en.wikipedia.org/wiki/Bell_character

Regards,
Mark

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

[Jonas Mark (ST-FIR/ENG1)]

Hello Dmitry,

> > extend the pwm-beeper driver to support customized frequency
> > for SND_BELL from device tree.
> 
> No, SND_BELL is literally SND_TONE @1000Hz. There should be no
> customizing. If applications want to use different frequency then should
> be using SND_TONE.

We are not aiming for an application shortcut here. Instead, changing the bell 
frequency shall be a system setting. That is, every application which wants to 
make a bell sound shall use the alternative frequency.

The reason why we are deviating from the default 1000 Hz is that on our 
hardware we are using a loudspeaker which is rated for 2.7 kHz. That is, it 
will only sound at the specified volume and frequency if you feed it with a 2.7 
kHz square wave. If you deviate from it, e.g. by using 1000 Hz, the output will 
be dim and squeaky. Worst case, SND_BELL would be completely silent on our 
system. So the only bell sound we can reliably generate on our system has 2.7 
kHz.

The good news for everybody else's systems is that the patch does not change 
the default behavior. If not specified in the DT, the frequency will still be 
1000 Hz. But other systems which are similarly challenged could now offer a 
reasonable bell sound, too.

Can you point me to a specification or standard which defines that SND_BELL has 
to be 1000 Hz without any exception?

I did some research on that topic and was not able to discover such a 
specification or standard. What I did find though was that the ASCII or Unicode 
BEL character is most likely very closely related to SND_BELL. And for BEL 
there is no frequency specified. Historically it once was a real bell which was 
rung. The intention was to notify the human on the other end of a teletype to 
have a look at it. And that is guaranteed with 1000 Hz as well as with 2.7 kHz 
as long as the sound is perceptible to the human ear.

https://en.wikipedia.org/wiki/Bell_character

Regards,
Mark

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

[Dmitry Torokhov]

On Tue, Feb 07, 2017 at 06:21:34AM +0100, Heiko Schocher wrote:
> From: Guan Ben 
> 
> extend the pwm-beeper driver to support customized frequency
> for SND_BELL from device tree.

No, SND_BELL is literally SND_TONE @1000Hz. There should be no
customizing. If applications want to use different frequency then should
be using SND_TONE.

Thanks.

-- 
Dmitry

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

[Dmitry Torokhov]

On Tue, Feb 07, 2017 at 06:21:34AM +0100, Heiko Schocher wrote:
> From: Guan Ben 
> 
> extend the pwm-beeper driver to support customized frequency
> for SND_BELL from device tree.

No, SND_BELL is literally SND_TONE @1000Hz. There should be no
customizing. If applications want to use different frequency then should
be using SND_TONE.

Thanks.

-- 
Dmitry

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

[Heiko Schocher]

From: Guan Ben 

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

Signed-off-by: Guan Ben 
Signed-off-by: Mark Jonas 
[h...@denx.de: adapted to 4.10-rc7]
Signed-off-by: Heiko Schocher 

---

 .../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..438c6e0 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:
+- bell-frequency:  bell frequency in Hz
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 5f9655d..99591d5 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) (10UL/(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(>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 error;
+
+   if (IS_ENABLED(CONFIG_OF)) {
+   node = dev->of_node;
+   error = of_property_read_u32(node, "bell-frequency",
+_frequency);
+   if (error < 0)
+   dev_dbg(dev, "Failed to read bell-frequency, 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(>dev);
@@ -122,6 +139,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
pwm_apply_args(beeper->pwm);
 
INIT_WORK(>work, pwm_beeper_work);
+   pwm_beeper_init_bell_frequency(>dev, beeper);
 
beeper->input = input_allocate_device();
if (!beeper->input) {
-- 
2.7.4

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

[Heiko Schocher]

From: Guan Ben 

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

Signed-off-by: Guan Ben 
Signed-off-by: Mark Jonas 
[h...@denx.de: adapted to 4.10-rc7]
Signed-off-by: Heiko Schocher 

---

 .../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..438c6e0 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:
+- bell-frequency:  bell frequency in Hz
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 5f9655d..99591d5 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) (10UL/(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(>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 error;
+
+   if (IS_ENABLED(CONFIG_OF)) {
+   node = dev->of_node;
+   error = of_property_read_u32(node, "bell-frequency",
+_frequency);
+   if (error < 0)
+   dev_dbg(dev, "Failed to read bell-frequency, 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(>dev);
@@ -122,6 +139,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
pwm_apply_args(beeper->pwm);
 
INIT_WORK(>work, pwm_beeper_work);
+   pwm_beeper_init_bell_frequency(>dev, beeper);
 
beeper->input = input_allocate_device();
if (!beeper->input) {
-- 
2.7.4