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

[Marek Vasut <marex@denx.de>]

On 8/15/23 23:33, Dmitry Torokhov wrote:
> 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?

I mean, yeah, but I think the input from John makes sense -- can we go 
with new TONE_WITH_VOLUME parameter instead of the sysfs interface ? 
That would be much nicer to work with .