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

[Marek Vasut <marex@denx.de>]

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