On 5/15/23 16:25, Jeff LaBundy wrote:
Hi Marek and Traut,
Hi,
On Mon, May 15, 2023 at 03:36:02PM +0200, Marek Vasut wrote:
On 5/15/23 08:50, Traut Manuel LCPF-CH wrote:
Hi Marek,
Hi,
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 ?
I tend to not change existing user-space interfaces. I would prefer to have an additional event or using sysfs.
I am increasingly concerned about the race condition between change of
volume (via sysfs) and frequency (via SND_TONE) . So I would be banking
toward additional event, like SND_TONE_WITH_VOLUME or something along those
lines.
This is just my $.02, but I don't see anything wrong with proposing an
_additive_ change to the ABI such as this. My only concern is that this
kind of change seems useful to any effect (e.g. SND_BEEP) and not limited
to only tones. Unless volume adjustment is less useful for those?
I would say the volume should also apply to SND_BEEP, sure.
---
NOTE: This uses approach similar to [1], except it is much simpler.
[1] https://patchwork.kernel.org/project/linux-input/cover/20230201152128.614439-1-manuel.traut@xxxxxx/
This one is more complex, because the mapping between duty cycle and volume is not linear. Probably it depends also on the used beeper hardware which values are doing a significant change in volume. Therefore the patchset introduced a mapping between volume levels and duty cycle times in the device-tree to allow user-space applications to control the beeper volume hardware independently.
I wonder whether this mapping shouldn't be considered policy and left to
userspace to deal with, instead of swamping the kernel or DT with it ?
I agree that the kernel need not try and linearize the values; in fact LEDs
already have the same problem. I still feel however that imposing a unique
maximum value (e.g. 50,000) is inappropriate because the range should remain
the same regardless of the underlying HW implementation (PWM, class A/B, etc.).
We can easily just say 0..65536 if we agree the 16 MSbits are the volume
, that's really not a problem.
