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

[Takashi Iwai <tiwai@suse.de>]

On Mon, 31 Jul 2023 13:49:46 +0200,
Marek Vasut wrote:
> 
> On 7/31/23 08:21, Takashi Iwai wrote:
> > 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.
> 
> Uh, I don't need a full sound device to emit beeps, that's not even
> possible with this hardware.

Heh, I also don't recommend that route, either :)
(Though, it must be possible to create a sound device with that beep
control in theory)

> I only need to control loudness of the
> beeper that is controlled by PWM output. That's why I am trying to
> extend the pwm-beeper driver, which seems the best fit for such a
> device, it is only missing this one feature (loudness control).

So the question is what's expected from user-space POV.  If a more
generic control of beep volume is required, e.g. for desktop-like
usages, an implementation of sound driver wouldn't be too bad.
OTOH, for other specific use-cases, it doesn't matter much in which
interface it's implemented, and sysfs could be an easy choice.

And, IMO, extending the SND_BEEP with a volume value doesn't sound
like a good idea.


thanks,

Takashi