From: Takashi Iwai <tiwai@suse.de>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: "Marek Vasut" <marex@denx.de>, "Jeff LaBundy" <jeff@labundy.com>,
linux-input@vger.kernel.org,
"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
"Frieder Schrempf" <frieder.schrempf@kontron.de>,
"Manuel Traut" <manuel.traut@mt.com>,
"Thierry Reding" <thierry.reding@gmail.com>,
linux-pwm@vger.kernel.org, alsa-devel@alsa-project.org,
"Jaroslav Kysela" <perex@perex.cz>,
"Takashi Iwai" <tiwai@suse.com>
Subject: Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
Date: Mon, 31 Jul 2023 08:21:09 +0200 [thread overview]
Message-ID: <873514d2ju.wl-tiwai@suse.de> (raw)
In-Reply-To: <ZMdIZiC453onyeHh@google.com>
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.
thanks,
Takashi
next prev parent reply other threads:[~2023-07-31 6:21 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-12 18:55 [PATCH] Input: pwm-beeper - Support volume setting via sysfs Marek Vasut
2023-05-13 1:12 ` Jeff LaBundy
2023-05-13 1:51 ` Marek Vasut
2023-05-13 21:02 ` Marek Vasut
2023-07-31 5:36 ` Dmitry Torokhov
2023-07-31 6:21 ` Takashi Iwai [this message]
2023-07-31 11:49 ` Marek Vasut
2023-07-31 12:15 ` Takashi Iwai
2023-07-31 14:05 ` Marek Vasut
2023-07-31 14:20 ` Takashi Iwai
2023-07-31 14:36 ` Marek Vasut
2023-07-31 16:24 ` Dmitry Torokhov
2023-07-31 17:49 ` Marek Vasut
2023-08-01 2:56 ` Jeff LaBundy
2023-08-01 6:11 ` Takashi Iwai
2023-08-01 11:38 ` Marek Vasut
2023-08-01 12:25 ` Takashi Iwai
2023-08-01 7:28 ` Dmitry Torokhov
2023-08-01 11:51 ` Marek Vasut
2023-08-11 4:19 ` Jeff LaBundy
2023-08-11 7:52 ` Takashi Iwai
2023-08-11 10:47 ` Traut Manuel LCPF-CH
2023-08-15 21:33 ` Dmitry Torokhov
2023-08-17 10:50 ` Marek Vasut
2023-08-11 12:39 ` John Watts
2023-08-14 2:26 ` Marek Vasut
2023-05-15 6:50 ` AW: EXTERNAL - " Traut Manuel LCPF-CH
2023-05-15 13:36 ` Marek Vasut
2023-05-15 14:25 ` Jeff LaBundy
2023-05-15 17:27 ` Marek Vasut
2023-05-15 15:24 ` AW: AW: " Traut Manuel LCPF-CH
2023-05-15 17:28 ` Marek Vasut
2023-06-14 6:45 ` Uwe Kleine-König
2023-06-14 9:30 ` Marek Vasut
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=873514d2ju.wl-tiwai@suse.de \
--to=tiwai@suse.de \
--cc=alsa-devel@alsa-project.org \
--cc=dmitry.torokhov@gmail.com \
--cc=frieder.schrempf@kontron.de \
--cc=jeff@labundy.com \
--cc=linux-input@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=manuel.traut@mt.com \
--cc=marex@denx.de \
--cc=perex@perex.cz \
--cc=thierry.reding@gmail.com \
--cc=tiwai@suse.com \
--cc=u.kleine-koenig@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).