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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Sat, May 13, 2023 at 11:02:30PM +0200, Marek Vasut wrote:
> On 5/13/23 03:51, Marek Vasut wrote:
> > 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.
> 
> Thinking about this further, we could try and add some
> 
> EV_SND SND_TONE_WITH_VOLUME
> 
> to avoid overloading EV_SND SND_TONE , and at the same time allow the user
> to set both frequency and volume for the tone without any race condition
> between the two.
> 
> The EV_SND SND_TONE_WITH_VOLUME would still take one 32bit parameter, except
> this time the parameter 16 LSbits would be the frequency and 16 MSbits would
> be the volume.
> 
> But again, here I would like input from the maintainers.

Beeper was supposed to be an extremely simple device with minimal
controls. I wonder if there is need for volume controls, etc, etc are we
not better moving it over to the sound subsystem. We already have:

	sound/drivers/pcsp/pcsp.c

and

	sound/pci/hda/hda_beep.c

there, can we have other "advanced" beepers there as well? Adding sound
maintainers to CC...

Thanks.

-- 
Dmitry