Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs

From: Marek Vasut <marex@denx.de>
To: Jeff LaBundy <jeff@labundy.com>
Cc: linux-input@vger.kernel.org,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Dmitry Torokhov" <dmitry.torokhov@gmail.com>,
	"Frieder Schrempf" <frieder.schrempf@kontron.de>,
	"Manuel Traut" <manuel.traut@mt.com>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	linux-pwm@vger.kernel.org
Subject: Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
Date: Sat, 13 May 2023 03:51:19 +0200	[thread overview]
Message-ID: <0ef98ec1-6191-c72e-2362-310db7f09b84@denx.de> (raw)
In-Reply-To: <ZF7kCjKGVjsxK8ec@nixie71>

On 5/13/23 03:12, Jeff LaBundy wrote:
> Hi Marek,

Hi,

> On Fri, May 12, 2023 at 08:55:51PM +0200, Marek Vasut wrote:
>> The PWM beeper volume can be controlled by adjusting the PWM duty cycle,
>> expose volume setting via sysfs, so users can make the beeper quieter.
>> This patch adds sysfs attribute 'volume' in range 0..50000, i.e. from 0
>> to 50% in 1/1000th of percent steps, this resolution should be sufficient.
>>
>> The reason for 50000 cap on volume or PWM duty cycle is because duty cycle
>> above 50% again reduces the loudness, the PWM wave form is inverted wave
>> form of the one for duty cycle below 50% and the beeper gets quieter the
>> closer the setting is to 100% . Hence, 50% cap where the wave form yields
>> the loudest result.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> An alternative option would be to extend the userspace input ABI, e.g. by
>> using SND_TONE top 16bits to encode the duty cycle in 0..50000 range, and
>> bottom 16bit to encode the existing frequency in Hz . Since frequency in
>> Hz is likely to be below some 25 kHz for audible bell, this fits in 16bits
>> just fine. Thoughts ?
>> ---
> 
> Thanks for the patch; this seems like a useful feature.
> 
> My first thought is that 50000 seems like an oddly specific limit to impose
> upon user space. Ideally, user space need not even care that the beeper is
> implemented via PWM and why 50000 is significant.
> 
> Instead, what about accepting 0..255 as the LED subsystem does for brightness,
> then map these values to 0..50000 internally? In fact, the leds-pwm driver
> does something similar.

The pwm_set_relative_duty_cycle() function can map whatever range to 
whatever other range of the PWM already, so that's not an issues here. 
It seems to me the 0..127 or 0..255 range is a bit too limiting . I 
think even for the LEDs the reason for that limit is legacy design, but 
here I might be wrong.

> I'm also curious as to whether this function should be a rogue sysfs control
> limited to this driver, or a generic operation in input. For example, input
> already allows user space to specify the magnitude of an FF effect; perhaps
> something similar is warranted here?

See the "An alternative ..." part above, I was wondering about this too, 
whether this can be added into the input ABI, but I am somewhat 
reluctant to fiddle with the ABI.