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

[Takashi Iwai <tiwai@suse.de>]

On Mon, 31 Jul 2023 16:05:18 +0200,
Marek Vasut wrote:
> 
> On 7/31/23 14:15, Takashi Iwai wrote:
> > 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 mean, I can imagine one could possibly use PCM DMA to cook samples
> to feed some of the PWM devices so they could possibly be used to
> generate low quality audio, as a weird limited DAC, but ... that's not
> really generic, and not what I want.

Oh I see how the misunderstanding came; I didn't mean the PCM
implementation like pcsp driver.  The pcsp driver is a real hack and
it's there just for fun, not for any real practical use.  
What I meant was rather that you can create a sound device containing
a mixer volume control that serves exactly like the sysfs or whatever
other interface, without any PCM stream or other interface.

> >> 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.
> 
> The whole discussion above has been exactly about this. Basically the
> thing is, we can either have:
> - SND_TONE (via some /dev/input/eventX) + sysfs volume control
>   -> This is simple, but sounds racy between input and sysfs accesses

Hmm, how can it be racy if you do proper locking?

> - SND_TONE + SND_TONE_SET_VOLUME
>   -> User needs to do two ioctls, hum
> - some new SND_TONE_WITH_VOLUME
>   -> Probably the best option, user sets both tone frequency and volume
>      in one go, and it also only extends the IOCTL interface, so older
>      userspace won't have issues

Those are "extensions" I have mentioned, and I'm not a big fan for
that, honestly speaking.

The fact that the beep *output* stuff is provided by the *input*
device is already confusing (it was so just because of historical
reason), and yet you start implementing more full-featured mixer
control.  I'd rather keep fingers away.

Again, if user-space requires the compatible behavior like the
existing desktop usages, it can be implemented in a similar way like
the existing ones; i.e. provide a mixer control with a proper sound
device.  The sound device doesn't need to provide a PCM interface but
just with a mixer interface.

Or, if the purpose of your target device is a special usage, you don't
need to consider too much about the existing interface, and try to
keep the change as minimal as possible without too intrusive API
changes.


Takashi