On Fri, Aug 11, 2023 at 10:47:34AM +0000, Traut Manuel LCPF-CH wrote: > Hi > > > On Fri, 11 Aug 2023 06:19:50 +0200, > > Jeff LaBundy wrote: > > > > > > Hi Marek, Dmitry and Takashi, > > > > > > On Tue, Aug 01, 2023 at 01:51:50PM +0200, Marek Vasut wrote: > > > > On 8/1/23 09:28, Dmitry Torokhov wrote: > > > > > On Mon, Jul 31, 2023 at 09:56:09PM -0500, Jeff LaBundy wrote: > > > > > > Hi all, > > > > > > > > > > > > On Mon, Jul 31, 2023 at 07:49:50PM +0200, Marek Vasut wrote: > > > > > > > On 7/31/23 18:24, Dmitry Torokhov wrote: > > > > > > > > On Mon, Jul 31, 2023 at 04:36:01PM +0200, Marek Vasut wrote: > > > > > > > > > On 7/31/23 16:20, Takashi Iwai wrote: > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > Ah :) > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > Ahhh, hum, I still feel like this might be a bit overkill here. > > > > > > > > > > > > > > > > > > > > > > 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? > > > > > > > > > > > > > > > > > > I can imagine two applications can each grab one of the > > > > > > > > > controls and that makes the interface a bit not nice. That > > > > > > > > > would require extra synchronization in userspace and so on. > > > > > > > > > > > > > > > > > > > > - 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 > > > > > > > > > > > > > > > > > > I agree, this confused me as well. > > > > > > > > > > > > > > > > This comes from the times when keyboards themselves were > > > > > > > > capable of emitting bells (SUN, DEC, etc). In hindsight it > > > > > > > > was not the best way of structuring things, same with the > > > > > > > > keyboard LEDs (that are now plugged into the LED subsystem, but > > still allow be driven through input). > > > > > > > > > > > > > > > > And in the same vein I wonder if we should bite the bullet > > > > > > > > and pay with a bit of complexity but move sound-related things to > > sound subsystem. > > > > > > > > > > > > > > I am not sure that's the right approach here, since the device > > > > > > > cannot do PCM playback, just bleeps. > > > > > > > > > > > > > > > > > (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 does not. These pwm-beeper devices keep showing up in > > > > > > > > > various embedded devices these days. > > > > > > > > > > > > > > > > > > > , 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. > > > > > > > > > > > > > > > > > > My use case is almost perfectly matched by the current > > > > > > > > > input pwm-beeper driver, the only missing bit is the > > > > > > > > > ability to control the loudness at runtime. I think adding > > > > > > > > > the SND_TONE_WITH_VOLUME parameter would cover it, with > > least intrusive API changes. > > > > > > > > > > > > > > > > > > The SND_TONE already supports configuring tone frequency > > > > > > > > > in Hz as its parameter. Since anything above 64 kHz is > > > > > > > > > certainly not hearable by humans, I would say the > > > > > > > > > SND_TONE_WITH_VOLUME could use 16 LSbits for frequency (so > > up to 65535 Hz , 0 is OFF), and 16 MSbits for volume . > > > > > > > > > > > > > > > > > > I'm hesitant to overcomplicate something which can > > > > > > > > > currently be controlled via single ioctl by pulling in sound > > subsystem into the picture. > > > > > > > > > > > > > > > > Can you tell a bit more about your use case? What needs to > > > > > > > > control the volume of beeps? Is this the only source of sounds on > > the system? > > > > > > > > > > > > > > Custom user space application. The entire userspace is custom > > > > > > > built in this case. > > > > > > > > > > > > > > In this case, it is a single-use device (think e.g. the kind > > > > > > > of thermometer you stick in your ear when you're ill, to find out > > how warm you are). > > > > > > > > > > > > > > The beeper there is used to do just that, bleep (with > > > > > > > different frequencies to indicate different stuff), and that > > > > > > > works. What I need in addition to that is control the volume > > > > > > > of the bleeps from the application, so it isn't too noisy. And > > > > > > > that needs to be user-controllable at runtime, so not something that > > goes in DT. > > > > > > > > > > > > > > Right now there is just the bleeper , yes. > > > > > > > > > > > > It sounds like we essentially need an option within pcsp to > > > > > > drive PWM instead of PCM, but input already has pwm-beeper; it > > > > > > seems harmless to gently extend the latter for this use-case as > > > > > > opposed to reworking the former. > > > > > > > > > > > > I agree that we should not invest too heavily in a legacy ABI, > > > > > > however something like SND_BELL_VOL seems like a low-cost > > > > > > addition that doesn't work against extending pcsp in the future. > > > > > > In fact, input already has precedent for this exact same thing > > > > > > by way of FF rumble effects, which are often PWM-based themselves. > > > > > > > > > > > > If SND_BELL_VOL or similar is not acceptable, then the original > > > > > > sysfs approach seems like the next-best compromise. My only > > > > > > issue with it was that I felt the range was not abstracted enough. > > > > > > > > > > If we want to extend the API we will need to define exactly how it > > > > > will all work. I.e. what happens if userspace mixes the old > > > > > SND_TONE and SND_BELL with the new SND_BELL_VOL or whatever. > > Does > > > > > it play with previously set volume? The default one? > > > > > > > > Default one, to preserve current behavior, yes. > > > > > > This was my idea as well, but I appreciate that the devil is in the > > > details and each driver may have to duplicate some overhead. > > > > > > > > > > > > How to set the default one? > > > > > > > > We do not, we can call pwm_get_duty_cycle() to get the current duty > > > > cycle of the PWM to figure out the default. > > > > > > > > > How > > > > > to figure out what the current volume is if we decide to make > > > > > volume "sticky"? > > > > > > > > The patch stores the current volume configured via sysfs into > > > > beeper->duty_cycle . > > > > > > > > > As far as userspace I expect it is more common to have one program > > > > > (or component of a program) to set volume and then something else > > > > > requests sound, so having one-shot API is of dubious value to me. > > > > > > > > Currently the use case I have for this is a single user facing > > > > application which configures both. > > > > > > > > > I hope we can go with Takashi's proposal downthread, but if not I > > > > > wonder if the sysfs approach is not the simplest one. Do we expect > > > > > more beepers that can control volume besides pwm-beeper? > > > > > > > > It seems to me pulling in dependency on the entire sound subsystem > > > > only to set beeper volume is overkill. I currently don't even have > > > > sound subsystem compiled in. > > > > > > I like Takashi's patch; it seems like a more scalable solution. > > > However, I can appreciate the reluctance to bring in the entire sound > > > subsytem for what is probably a tiny piezoelectric buzzer. > > > > > > It seems like the sysfs solution is the best compromise in the > > > meantime. If more and more users need to shoe-horn these kind of > > > features in the future, we can make more informed decisions as to how to > > extend the API (if at all). > > > > That's my impression, too. The original sysfs usage would be the right fit at > > this moment. > > I am fine with both using the Sound API and sysfs. I would additionally like to > specify the pwm values in device-tree like done in pwm-backlight. It really depends > on the hardware which values actually make a difference in volume. OK, let's go with the sysfs API for now as I am not sure if we have more drivers being able to control volume of beeps. Marek, I think there were some minor comments on the patch, could you please address them and respin? Thanks. -- Dmitry
