From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751718AbdB0TrV (ORCPT ); Mon, 27 Feb 2017 14:47:21 -0500 Received: from vern.gendns.com ([206.190.152.46]:59327 "EHLO vern.gendns.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751531AbdB0TrQ (ORCPT ); Mon, 27 Feb 2017 14:47:16 -0500 Subject: Re: [v2] Input: pwm-beeper: support customized freq for SND_BELL To: Heiko Schocher , linux-input@vger.kernel.org References: <1487579863-7256-1-git-send-email-hs@denx.de> Cc: Guan Ben , Mark Jonas , devicetree@vger.kernel.org, Thierry Reding , linux-kernel@vger.kernel.org, Boris Brezillon , Rob Herring , Dmitry Torokhov , Manfred Schlaegl , Mark Rutland From: David Lechner Message-ID: <06af466d-ac78-ea38-77d2-3bf32a6e157c@lechnology.com> Date: Mon, 27 Feb 2017 12:11:06 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <1487579863-7256-1-git-send-email-hs@denx.de> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - vern.gendns.com X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - lechnology.com X-Get-Message-Sender-Via: vern.gendns.com: authenticated_id: davidmain+lechnology.com/only user confirmed/virtual account not confirmed X-Authenticated-Sender: vern.gendns.com: davidmain@lechnology.com X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/20/2017 02:37 AM, 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 > [hs@denx.de: adapted to 4.10-rc7] > Signed-off-by: Heiko Schocher > --- > > 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) { >