Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs

[Takashi Iwai <tiwai@suse.de>]

On Tue, 01 Aug 2023 04:56:09 +0200,
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.

Nah, please forget pcsp driver.  As mentioned earlier, it's a driver
that is present just for fun.

I believe what we need is a simple sound card instance providing a
mixer control for the beep volume, something like a patch like below
(totally untested!)


Takashi

--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -595,6 +595,13 @@ config INPUT_PWM_BEEPER
 	  To compile this driver as a module, choose M here: the module will be
 	  called pwm-beeper.
 
+config INPUT_PWM_BEEPER_MIXER
+	bool "Mixer volume support for PWM beeper"
+	depends on INPUT_PWM_BEEPER
+	depends on SND=y || INPUT_PWM_BEEPER=SND
+	help
+	  Say Y here to enable sound mixer for PWM beeper volume.
+
 config INPUT_PWM_VIBRA
 	tristate "PWM vibrator support"
 	depends on PWM
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -14,6 +14,8 @@
 #include <linux/pwm.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
+#include <sound/core.h>
+#include <sound/control.h>
 
 struct pwm_beeper {
 	struct input_dev *input;
@@ -21,6 +23,7 @@ struct pwm_beeper {
 	struct regulator *amplifier;
 	struct work_struct work;
 	unsigned long period;
+	unsigned long duty_cycle;
 	unsigned int bell_frequency;
 	bool suspended;
 	bool amplifier_on;
@@ -37,7 +40,7 @@ static int pwm_beeper_on(struct pwm_beeper *beeper, unsigned long period)
 
 	state.enabled = true;
 	state.period = period;
-	pwm_set_relative_duty_cycle(&state, 50, 100);
+	pwm_set_relative_duty_cycle(&state, beeper->duty_cycle, 100000);
 
 	error = pwm_apply_state(beeper->pwm, &state);
 	if (error)
@@ -112,6 +115,66 @@ static void pwm_beeper_stop(struct pwm_beeper *beeper)
 	pwm_beeper_off(beeper);
 }
 
+#ifdef CONFIG_INPUT_PWM_BEEPER_MIXER
+static int pwm_beeper_mixer_info(struct snd_kcontrol *kcontrol,
+				 struct snd_ctl_elem_info *uinfo)
+{
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = 1;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = 50000;
+	return 0;
+}
+
+static int pwm_beeper_mixer_get(struct snd_kcontrol *kcontrol,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	struct pwm_beeper *beeper = snd_kcontrol_chip(kcontrol);
+
+	ucontrol->value.integer.value[0] = beeper->duty_cycle;
+	return 0;
+}
+
+static int pwm_beeper_mixer_put(struct snd_kcontrol *kcontrol,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	struct pwm_beeper *beeper = snd_kcontrol_chip(kcontrol);
+	unsigned long val = ucontrol->value.integer.value[0];
+
+	val = min(val, 50000UL);
+	if (beeper->duty_cycle == val)
+		return 0;
+	beeper->duty_cycle = val;
+	if (!beeper->suspended)
+		schedule_work(&beeper->work);
+	return 1;
+}
+
+static const struct snd_kcontrol_new pwm_beeper_mixer_ctl = {
+	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+	.name =	"Beep Playback Switch",
+	.info = pwm_beeper_mixer_info,
+	.get = pwm_beeper_mixer_get,
+	.put = pwm_beeper_mixer_put,
+};
+
+static int pwm_beeper_mixer_attach(struct device *dev, struct pwm_beeper *beeper)
+{
+	struct snd_card *card;
+	int err;
+
+	err = snd_devm_card_new(dev, 0, NULL, THIS_MODULE, 0, &card);
+	if (err)
+		return err;
+
+	err = snd_ctl_add(card, snd_ctl_new1(&pwm_beeper_mixer_ctl, beeper));
+	if (err)
+		return err;
+
+	return snd_card_register(card);
+}
+#endif /* CONFIG_INPUT_PWM_BEEPER_MIXER */
+
 static void pwm_beeper_close(struct input_dev *input)
 {
 	struct pwm_beeper *beeper = input_get_drvdata(input);
@@ -189,6 +252,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
 
 	beeper->input->event = pwm_beeper_event;
 	beeper->input->close = pwm_beeper_close;
+	beeper->duty_cycle = 50000;
 
 	input_set_drvdata(beeper->input, beeper);
 
@@ -200,6 +264,11 @@ static int pwm_beeper_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, beeper);
 
+#ifdef CONFIG_INPUT_PWM_BEEPER_MIXER
+	error = pwm_beeper_mixer_attach(dev, beeper);
+	if (error)
+		return error;
+#endif
 	return 0;
 }