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

[Takashi Iwai <tiwai@suse.de>]

On Mon, 31 Jul 2023 07:36:38 +0200,
Dmitry Torokhov wrote:
> 
> 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...

I don't mind it put to sound/*.  But, note that pcsp.c you pointed in
the above is a PCM tone generator driver with a PC beep device, and it
provides the normal SND_BEEP input only for compatibility.

Indeed there have been already many sound drivers providing the beep
capability, and they bind with the input device using SND_BEEP.  And,
for the beep volume, "Beep Playback Volume" mixer control is provided,
too.


thanks,

Takashi