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