Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755730AbcJGJHl convert rfc822-to-8bit (ORCPT ); Fri, 7 Oct 2016 05:07:41 -0400 Received: from smtp.exceet.ch ([77.245.33.226]:10313 "EHLO smtp.exceet.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753038AbcJGJHW (ORCPT ); Fri, 7 Oct 2016 05:07:22 -0400 X-Greylist: delayed 380 seconds by postgrey-1.27 at vger.kernel.org; Fri, 07 Oct 2016 05:07:21 EDT From: Schrempf Frieder To: Rob Herring CC: "dmitry.torokhov@gmail.com" , "pawel.moll@arm.com" , "ijc+devicetree@hellion.org.uk" , "galak@codeaurora.org" , "luis@debethencourt.com" , "linux-input@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH 1/1] input: pwm-beeper: add feature to set volume via sysfs Thread-Topic: [PATCH 1/1] input: pwm-beeper: add feature to set volume via sysfs Thread-Index: AQHRkOZayuEgEiXG20aBzc3kE7zP3p+EyG4AgRjm2gA= Date: Fri, 7 Oct 2016 09:00:53 +0000 Message-ID: References: <1460044644-12419-1-git-send-email-frieder.schrempf@exceet.de> <20160411152154.GA9736@rob-hp-laptop> In-Reply-To: <20160411152154.GA9736@rob-hp-laptop> Accept-Language: de-DE, en-US Content-Language: de-DE X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [178.2.219.242] x-tm-as-product-ver: SMEX-11.0.0.4255-8.000.1202-22622.006 x-tm-as-result: No--47.015900-5.000000-31 x-tm-as-user-approved-sender: No x-tm-as-user-blocked-sender: No Content-Type: text/plain; charset="Windows-1252" Content-ID: Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3288 Lines: 84 Thank you Rob for looking at my first kernel contribution. I come back to it now, as I had some time to process your comments. On 11.04.2016 17:21, Rob Herring wrote: > On Thu, Apr 07, 2016 at 03:58:40PM +0000, Schrempf Frieder wrote: >> Make the driver accept different volume levels via sysfs. >> This can be helpful if the beep/bell sound intensity needs >> to be adapted to the environment of the device. > > It is preferred that bindings be a separate patch. Also, this is 2 > independent features: the sysfs interface and DT properties. So that is > probably 3 patches. > > Also, sysfs interfaces are supposed to be documented. I have splitted this into 3 patches and will send them soon as v2. I also added documentation for the sysfs interface. > >> >> The number of volume levels available and their values can >> be specified via device tree (similar to pwm-backlight). >> >> This patch was tested with linux-imx 3.10.17 and was >> applied to current mainline without any changes. > > You need to test with something not 3 years old... I tested the v2 patches with Linux 4.7.4 now. > >> >> Signed-off-by: Frieder Schrempf >> --- >> .../devicetree/bindings/input/pwm-beeper.txt | 20 ++++ >> drivers/input/misc/pwm-beeper.c | 109 ++++++++++++++++++- >> 2 files changed, 126 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.txt b/Documentation/devicetree/bindings/input/pwm-beeper.txt >> index be332ae..153cd3f 100644 >> --- a/Documentation/devicetree/bindings/input/pwm-beeper.txt >> +++ b/Documentation/devicetree/bindings/input/pwm-beeper.txt >> @@ -5,3 +5,23 @@ Registers a PWM device as beeper. >> Required properties: >> - compatible: should be "pwm-beeper" >> - pwms: phandle to the physical PWM device >> +- volume-levels: Array of distinct volume levels. These need to be in the >> + range of 0 to 500, while 0 means 0% duty cycle (mute) and 500 means >> + 50% duty cycle (max volume). > > Where does the scale of 0-500 come from? It is tenth of a percent? I used tenth of a percent because this is needed to define low volume levels with pwm duty cycles in the range of <1%. This is necessary because of the highly non-linear relation between duty cycle and actual perceived volume. The upper limit of 500 therefore results from 50% being the duty cycle with maximum volume. > > How many array entries? The array size and therefore the number of volume levels is dynamic and only defined by the number of entries in the volume-levels property. > >> + Please note that the actual volume of most beepers is highly >> + non-linear, which means that low volume levels are probably somewhere >> + in the range of 1 to 30 (0.1-3% duty cycle). >> +- default-volume-level: the default volume level (index into the >> + array defined by the "volume-levels" property) >> + >> +The volume level can be set via sysfs under /sys/class/input/inputX/volume. >> +The maximum volume level index can be read from /sys/class/input/inputX/max_volume_level. >> + >> +Example: >> + >> + pwm-beeper { >> + compatible = "pwm-beeper"; >> + pwms = <&pwm4 0 5000>; >> + volume-levels = <0 8 20 40 500>; >> + default-volume-level = <4>; >> + };