| Message ID | 1486444894-18321-1-git-send-email-hs@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 07, 2017 at 06:21:34AM +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. 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.
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 -- 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
On Tue, Feb 07, 2017 at 06:21:34AM +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> > > --- > > .../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 -- 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
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 <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> >> >> --- >> >> .../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
On Wed, Feb 08, 2017 at 10:11:21AM +0000, 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.
Hello Dmitry, Am 14.02.2017 um 05:27 schrieb Dmitry Torokhov: > On Wed, Feb 08, 2017 at 10:11:21AM +0000, 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
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) (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 error; + + if (IS_ENABLED(CONFIG_OF)) { + node = dev->of_node; + error = of_property_read_u32(node, "bell-frequency", + &bell_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(&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) {