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;
}
