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

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

On Fri, Aug 11, 2023 at 10:47:34AM +0000, Traut Manuel LCPF-CH wrote:
> Hi
> 
> > On Fri, 11 Aug 2023 06:19:50 +0200,
> > Jeff LaBundy wrote:
> > >
> > > Hi Marek, Dmitry and Takashi,
> > >
> > > On Tue, Aug 01, 2023 at 01:51:50PM +0200, Marek Vasut wrote:
> > > > On 8/1/23 09:28, Dmitry Torokhov wrote:
> > > > > On Mon, Jul 31, 2023 at 09:56:09PM -0500, Jeff LaBundy wrote:
> > > > > > Hi all,
> > > > > >
> > > > > > On Mon, Jul 31, 2023 at 07:49:50PM +0200, Marek Vasut wrote:
> > > > > > > On 7/31/23 18:24, Dmitry Torokhov wrote:
> > > > > > > > On Mon, Jul 31, 2023 at 04:36:01PM +0200, Marek Vasut wrote:
> > > > > > > > > On 7/31/23 16:20, Takashi Iwai wrote:
> > > > > > > > >
> > > > > > > > > [...]
> > > > > > > > >
> > > > > > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > Ah :)
> > > > > > > > >
> > > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > Ahhh, hum, I still feel like this might be a bit overkill here.
> > > > > > > > >
> > > > > > > > > > > > > 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?
> > > > > > > > >
> > > > > > > > > I can imagine two applications can each grab one of the
> > > > > > > > > controls and that makes the interface a bit not nice. That
> > > > > > > > > would require extra synchronization in userspace and so on.
> > > > > > > > >
> > > > > > > > > > > - 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
> > > > > > > > >
> > > > > > > > > I agree, this confused me as well.
> > > > > > > >
> > > > > > > > This comes from the times when keyboards themselves were
> > > > > > > > capable of emitting bells (SUN, DEC, etc). In hindsight it
> > > > > > > > was not the best way of structuring things, same with the
> > > > > > > > keyboard LEDs (that are now plugged into the LED subsystem, but
> > still allow be driven through input).
> > > > > > > >
> > > > > > > > And in the same vein I wonder if we should bite the bullet
> > > > > > > > and pay with a bit of complexity but move sound-related things to
> > sound subsystem.
> > > > > > >
> > > > > > > I am not sure that's the right approach here, since the device
> > > > > > > cannot do PCM playback, just bleeps.
> > > > > > >
> > > > > > > > > > (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 does not. These pwm-beeper devices keep showing up in
> > > > > > > > > various embedded devices these days.
> > > > > > > > >
> > > > > > > > > > , 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.
> > > > > > > > >
> > > > > > > > > My use case is almost perfectly matched by the current
> > > > > > > > > input pwm-beeper driver, the only missing bit is the
> > > > > > > > > ability to control the loudness at runtime. I think adding
> > > > > > > > > the SND_TONE_WITH_VOLUME parameter would cover it, with
> > least intrusive API changes.
> > > > > > > > >
> > > > > > > > > The SND_TONE already supports configuring tone frequency
> > > > > > > > > in Hz as its parameter. Since anything above 64 kHz is
> > > > > > > > > certainly not hearable by humans, I would say the
> > > > > > > > > SND_TONE_WITH_VOLUME could use 16 LSbits for frequency (so
> > up to 65535 Hz , 0 is OFF), and 16 MSbits for volume .
> > > > > > > > >
> > > > > > > > > I'm hesitant to overcomplicate something which can
> > > > > > > > > currently be controlled via single ioctl by pulling in sound
> > subsystem into the picture.
> > > > > > > >
> > > > > > > > Can you tell a bit more about your use case? What needs to
> > > > > > > > control the volume of beeps? Is this the only source of sounds on
> > the system?
> > > > > > >
> > > > > > > Custom user space application. The entire userspace is custom
> > > > > > > built in this case.
> > > > > > >
> > > > > > > In this case, it is a single-use device (think e.g. the kind
> > > > > > > of thermometer you stick in your ear when you're ill, to find out
> > how warm you are).
> > > > > > >
> > > > > > > The beeper there is used to do just that, bleep (with
> > > > > > > different frequencies to indicate different stuff), and that
> > > > > > > works. What I need in addition to that is control the volume
> > > > > > > of the bleeps from the application, so it isn't too noisy. And
> > > > > > > that needs to be user-controllable at runtime, so not something that
> > goes in DT.
> > > > > > >
> > > > > > > Right now there is just the bleeper , yes.
> > > > > >
> > > > > > It sounds like we essentially need an option within pcsp to
> > > > > > drive PWM instead of PCM, but input already has pwm-beeper; it
> > > > > > seems harmless to gently extend the latter for this use-case as
> > > > > > opposed to reworking the former.
> > > > > >
> > > > > > I agree that we should not invest too heavily in a legacy ABI,
> > > > > > however something like SND_BELL_VOL seems like a low-cost
> > > > > > addition that doesn't work against extending pcsp in the future.
> > > > > > In fact, input already has precedent for this exact same thing
> > > > > > by way of FF rumble effects, which are often PWM-based themselves.
> > > > > >
> > > > > > If SND_BELL_VOL or similar is not acceptable, then the original
> > > > > > sysfs approach seems like the next-best compromise. My only
> > > > > > issue with it was that I felt the range was not abstracted enough.
> > > > >
> > > > > If we want to extend the API we will need to define exactly how it
> > > > > will all work. I.e. what happens if userspace mixes the old
> > > > > SND_TONE and SND_BELL with the new SND_BELL_VOL or whatever.
> > Does
> > > > > it play with previously set volume? The default one?
> > > >
> > > > Default one, to preserve current behavior, yes.
> > >
> > > This was my idea as well, but I appreciate that the devil is in the
> > > details and each driver may have to duplicate some overhead.
> > >
> > > >
> > > > > How to set the default one?
> > > >
> > > > We do not, we can call pwm_get_duty_cycle() to get the current duty
> > > > cycle of the PWM to figure out the default.
> > > >
> > > > > How
> > > > > to figure out what the current volume is if we decide to make
> > > > > volume "sticky"?
> > > >
> > > > The patch stores the current volume configured via sysfs into
> > > > beeper->duty_cycle .
> > > >
> > > > > As far as userspace I expect it is more common to have one program
> > > > > (or component of a program) to set volume and then something else
> > > > > requests sound, so having one-shot API is of dubious value to me.
> > > >
> > > > Currently the use case I have for this is a single user facing
> > > > application which configures both.
> > > >
> > > > > I hope we can go with Takashi's proposal downthread, but if not I
> > > > > wonder if the sysfs approach is not the simplest one. Do we expect
> > > > > more beepers that can control volume besides pwm-beeper?
> > > >
> > > > It seems to me pulling in dependency on the entire sound subsystem
> > > > only to set beeper volume is overkill. I currently don't even have
> > > > sound subsystem compiled in.
> > >
> > > I like Takashi's patch; it seems like a more scalable solution.
> > > However, I can appreciate the reluctance to bring in the entire sound
> > > subsytem for what is probably a tiny piezoelectric buzzer.
> > >
> > > It seems like the sysfs solution is the best compromise in the
> > > meantime. If more and more users need to shoe-horn these kind of
> > > features in the future, we can make more informed decisions as to how to
> > extend the API (if at all).
> > 
> > That's my impression, too.  The original sysfs usage would be the right fit at
> > this moment.
> 
> I am fine with both using the Sound API and sysfs. I would additionally like to
> specify the pwm values in device-tree like done in pwm-backlight. It really depends
> on the hardware which values actually make a difference in volume.

OK, let's go with the sysfs API for now as I am not sure if we have more
drivers being able to control volume of beeps.

Marek, I think there were some minor comments on the patch, could you
please address them and respin?

Thanks.

-- 
Dmitry