On Mon, 31 Jul 2023 16:05:18 +0200, Marek Vasut wrote:
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.
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. 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.
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?
- 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 (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 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.
Takashi