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@xxxxxxx>
---
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.
