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

[Marek Vasut <marex@denx.de>]

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.

> (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.