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

[Marek Vasut <marex@denx.de>]

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.

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

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

No, it doesn't, but see SND_TONE_WITH_VOLUME option above. I think that 
might be the best so far.