| Message ID | 20230512185551.183049-1-marex@denx.de |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Input: pwm-beeper - Support volume setting via sysfs | expand |
Hi Marek, 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. 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? > NOTE: This uses approach similar to [1], except it is much simpler. > [1] https://patchwork.kernel.org/project/linux-input/cover/20230201152128.614439-1-manuel.traut@mt.com/ > --- > Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Frieder Schrempf <frieder.schrempf@kontron.de> > Cc: Manuel Traut <manuel.traut@mt.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Thierry Reding <thierry.reding@gmail.com> > Cc: linux-input@vger.kernel.org > Cc: linux-pwm@vger.kernel.org > --- > drivers/input/misc/pwm-beeper.c | 58 ++++++++++++++++++++++++++++++++- > 1 file changed, 57 insertions(+), 1 deletion(-) > > diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c > index 3cf1812384e6a..f63d0ebbaf573 100644 > --- a/drivers/input/misc/pwm-beeper.c > +++ b/drivers/input/misc/pwm-beeper.c > @@ -21,6 +21,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 +38,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) > @@ -119,6 +120,53 @@ static void pwm_beeper_close(struct input_dev *input) > pwm_beeper_stop(beeper); > } > > +static ssize_t volume_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct pwm_beeper *beeper = dev_get_drvdata(dev); > + > + return sysfs_emit(buf, "%ld\n", beeper->duty_cycle); > +} > + > +static ssize_t volume_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct pwm_beeper *beeper = dev_get_drvdata(dev); > + unsigned long val; > + > + if (kstrtoul(buf, 0, &val) < 0) > + return -EINVAL; > + > + /* > + * Volume is really PWM duty cycle in pcm (per cent mille, 1/1000th > + * of percent). This value therefore ranges from 0 to 50000 . Duty > + * cycle of 50% = 50000pcm is the maximum volume . > + */ > + val = clamp(val, 0UL, 50000UL); > + /* No change in current settings, do nothing. */ > + if (val == beeper->duty_cycle) > + return count; > + > + /* Update current settings and apply to PWM chip. */ > + beeper->duty_cycle = val; > + if (!beeper->suspended) > + schedule_work(&beeper->work); > + > + return count; > +} > +static DEVICE_ATTR_RW(volume); > + > +static struct attribute *pwm_beeper_dev_attrs[] = { > + &dev_attr_volume.attr, > + NULL > +}; > + > +static const struct attribute_group pwm_beeper_attr_group = { > + .attrs = pwm_beeper_dev_attrs, > +}; > + > static int pwm_beeper_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -192,6 +240,14 @@ static int pwm_beeper_probe(struct platform_device *pdev) > > input_set_drvdata(beeper->input, beeper); > > + beeper->duty_cycle = 50000; > + error = devm_device_add_group(dev, &pwm_beeper_attr_group); > + if (error) { > + dev_err(dev, "Unable to create sysfs attributes: %d\n", > + error); > + return error; > + } > + > error = input_register_device(beeper->input); > if (error) { > dev_err(dev, "Failed to register input device: %d\n", error); > -- > 2.39.2 > Kind regards, Jeff LaBundy
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.
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.
Hi Marek, > 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 ? I tend to not change existing user-space interfaces. I would prefer to have an additional event or using sysfs. >--- >NOTE: This uses approach similar to [1], except it is much simpler. > [1] https://patchwork.kernel.org/project/linux-input/cover/20230201152128.614439-1-manuel.traut@mt.com/ This one is more complex, because the mapping between duty cycle and volume is not linear. Probably it depends also on the used beeper hardware which values are doing a significant change in volume. Therefore the patchset introduced a mapping between volume levels and duty cycle times in the device-tree to allow user-space applications to control the beeper volume hardware independently. Regards Manuel
On 5/15/23 08:50, Traut Manuel LCPF-CH wrote: > Hi Marek, Hi, >> 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 ? > > I tend to not change existing user-space interfaces. I would prefer to have an additional event or using sysfs. I am increasingly concerned about the race condition between change of volume (via sysfs) and frequency (via SND_TONE) . So I would be banking toward additional event, like SND_TONE_WITH_VOLUME or something along those lines. >> --- >> NOTE: This uses approach similar to [1], except it is much simpler. >> [1] https://patchwork.kernel.org/project/linux-input/cover/20230201152128.614439-1-manuel.traut@mt.com/ > > This one is more complex, because the mapping between duty cycle and volume is not linear. Probably it depends also on the used beeper hardware which values are doing a significant change in volume. Therefore the patchset introduced a mapping between volume levels and duty cycle times in the device-tree to allow user-space applications to control the beeper volume hardware independently. I wonder whether this mapping shouldn't be considered policy and left to userspace to deal with, instead of swamping the kernel or DT with it ?
Hi Marek and Traut, On Mon, May 15, 2023 at 03:36:02PM +0200, Marek Vasut wrote: > On 5/15/23 08:50, Traut Manuel LCPF-CH wrote: > > Hi Marek, > > Hi, > > > > 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 ? > > > > I tend to not change existing user-space interfaces. I would prefer to have an additional event or using sysfs. > > I am increasingly concerned about the race condition between change of > volume (via sysfs) and frequency (via SND_TONE) . So I would be banking > toward additional event, like SND_TONE_WITH_VOLUME or something along those > lines. This is just my $.02, but I don't see anything wrong with proposing an _additive_ change to the ABI such as this. My only concern is that this kind of change seems useful to any effect (e.g. SND_BEEP) and not limited to only tones. Unless volume adjustment is less useful for those? > > > > --- > > > NOTE: This uses approach similar to [1], except it is much simpler. > > > [1] https://patchwork.kernel.org/project/linux-input/cover/20230201152128.614439-1-manuel.traut@mt.com/ > > > > This one is more complex, because the mapping between duty cycle and volume is not linear. Probably it depends also on the used beeper hardware which values are doing a significant change in volume. Therefore the patchset introduced a mapping between volume levels and duty cycle times in the device-tree to allow user-space applications to control the beeper volume hardware independently. > > I wonder whether this mapping shouldn't be considered policy and left to > userspace to deal with, instead of swamping the kernel or DT with it ? I agree that the kernel need not try and linearize the values; in fact LEDs already have the same problem. I still feel however that imposing a unique maximum value (e.g. 50,000) is inappropriate because the range should remain the same regardless of the underlying HW implementation (PWM, class A/B, etc.). > > -- > Best regards, > Marek Vasut Kind regards, Jeff LaBundy
Hi Marek, >> I tend to not change existing user-space interfaces. I would prefer to have an additional event or using sysfs. > I am increasingly concerned about the race condition between change of volume (via sysfs) and frequency (via SND_TONE) . So I would be banking toward additional event, like SND_TONE_WITH_VOLUME or something along those lines. SND_TONE_WITH_VOLUME is also ok from my side. But implementing some locking shall also be possible. >>> NOTE: This uses approach similar to [1], except it is much simpler. >>> [1] >>> https://patchwork.kernel.org/project/linux-input/cover/20230201152128 >>> .614439-1-manuel.traut@mt.com/ >> >> This one is more complex, because the mapping between duty cycle and volume is not linear. Probably it depends also on the used beeper hardware which values are doing a significant change in volume. Therefore the patchset introduced a mapping between volume levels and duty cycle times in the device-tree to allow user-space applications to control the beeper volume hardware independently. > I wonder whether this mapping shouldn't be considered policy and left to userspace to deal with, instead of swamping the kernel or DT with it ? How could a Linux distribution detect which mapping is required to be installed? For me it seems to be easier to have the device-specific information in the device-tree. Regards Manuel
On 5/15/23 16:25, Jeff LaBundy wrote: > Hi Marek and Traut, Hi, > On Mon, May 15, 2023 at 03:36:02PM +0200, Marek Vasut wrote: >> On 5/15/23 08:50, Traut Manuel LCPF-CH wrote: >>> Hi Marek, >> >> Hi, >> >>>> 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 ? >>> >>> I tend to not change existing user-space interfaces. I would prefer to have an additional event or using sysfs. >> >> I am increasingly concerned about the race condition between change of >> volume (via sysfs) and frequency (via SND_TONE) . So I would be banking >> toward additional event, like SND_TONE_WITH_VOLUME or something along those >> lines. > > This is just my $.02, but I don't see anything wrong with proposing an > _additive_ change to the ABI such as this. My only concern is that this > kind of change seems useful to any effect (e.g. SND_BEEP) and not limited > to only tones. Unless volume adjustment is less useful for those? I would say the volume should also apply to SND_BEEP, sure. >>>> --- >>>> NOTE: This uses approach similar to [1], except it is much simpler. >>>> [1] https://patchwork.kernel.org/project/linux-input/cover/20230201152128.614439-1-manuel.traut@mt.com/ >>> >>> This one is more complex, because the mapping between duty cycle and volume is not linear. Probably it depends also on the used beeper hardware which values are doing a significant change in volume. Therefore the patchset introduced a mapping between volume levels and duty cycle times in the device-tree to allow user-space applications to control the beeper volume hardware independently. >> >> I wonder whether this mapping shouldn't be considered policy and left to >> userspace to deal with, instead of swamping the kernel or DT with it ? > > I agree that the kernel need not try and linearize the values; in fact LEDs > already have the same problem. I still feel however that imposing a unique > maximum value (e.g. 50,000) is inappropriate because the range should remain > the same regardless of the underlying HW implementation (PWM, class A/B, etc.). We can easily just say 0..65536 if we agree the 16 MSbits are the volume , that's really not a problem.
On 5/15/23 17:24, Traut Manuel LCPF-CH wrote: > Hi Marek, Hi, >>> I tend to not change existing user-space interfaces. I would prefer to have an additional event or using sysfs. >> I am increasingly concerned about the race condition between change of volume (via sysfs) and frequency (via SND_TONE) . So I would be banking toward additional event, like SND_TONE_WITH_VOLUME or something along those lines. > > SND_TONE_WITH_VOLUME is also ok from my side. But implementing some locking shall also be possible. > >>>> NOTE: This uses approach similar to [1], except it is much simpler. >>>> [1] >>>> https://patchwork.kernel.org/project/linux-input/cover/20230201152128 >>>> .614439-1-manuel.traut@mt.com/ >>> >>> This one is more complex, because the mapping between duty cycle and volume is not linear. Probably it depends also on the used beeper hardware which values are doing a significant change in volume. Therefore the patchset introduced a mapping between volume levels and duty cycle times in the device-tree to allow user-space applications to control the beeper volume hardware independently. > >> I wonder whether this mapping shouldn't be considered policy and left to userspace to deal with, instead of swamping the kernel or DT with it ? > How could a Linux distribution detect which mapping is required to be installed? > For me it seems to be easier to have the device-specific information in the device-tree. The alternative might be to have volume in 0..65535 range (i.e. the top 16 MSbits) and be done with it, then the PWM subsystem is responsible for mapping this to 0..50% of PWM duty cycle .
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 ? > --- > NOTE: This uses approach similar to [1], except it is much simpler. > [1] https://patchwork.kernel.org/project/linux-input/cover/20230201152128.614439-1-manuel.traut@mt.com/ > --- > Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Frieder Schrempf <frieder.schrempf@kontron.de> > Cc: Manuel Traut <manuel.traut@mt.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Thierry Reding <thierry.reding@gmail.com> > Cc: linux-input@vger.kernel.org > Cc: linux-pwm@vger.kernel.org > --- > drivers/input/misc/pwm-beeper.c | 58 ++++++++++++++++++++++++++++++++- > 1 file changed, 57 insertions(+), 1 deletion(-) > > diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c > index 3cf1812384e6a..f63d0ebbaf573 100644 > --- a/drivers/input/misc/pwm-beeper.c > +++ b/drivers/input/misc/pwm-beeper.c > @@ -21,6 +21,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 +38,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) > @@ -119,6 +120,53 @@ static void pwm_beeper_close(struct input_dev *input) > pwm_beeper_stop(beeper); > } > > +static ssize_t volume_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct pwm_beeper *beeper = dev_get_drvdata(dev); > + > + return sysfs_emit(buf, "%ld\n", beeper->duty_cycle); > +} > + > +static ssize_t volume_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct pwm_beeper *beeper = dev_get_drvdata(dev); > + unsigned long val; > + > + if (kstrtoul(buf, 0, &val) < 0) > + return -EINVAL; > + > + /* > + * Volume is really PWM duty cycle in pcm (per cent mille, 1/1000th > + * of percent). This value therefore ranges from 0 to 50000 . Duty > + * cycle of 50% = 50000pcm is the maximum volume . > + */ > + val = clamp(val, 0UL, 50000UL); I wonder if you want to refuse values here that are not in the specified range, that is, something like: if (val != clamp(val, 0UL, 50000UL)) return -EINVAL; I think this is more in line who other sysfs properties work?! Best regards Uwe
On 6/14/23 08:45, Uwe Kleine-König wrote: > 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 ? >> --- >> NOTE: This uses approach similar to [1], except it is much simpler. >> [1] https://patchwork.kernel.org/project/linux-input/cover/20230201152128.614439-1-manuel.traut@mt.com/ >> --- >> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> Cc: Frieder Schrempf <frieder.schrempf@kontron.de> >> Cc: Manuel Traut <manuel.traut@mt.com> >> Cc: Marek Vasut <marex@denx.de> >> Cc: Thierry Reding <thierry.reding@gmail.com> >> Cc: linux-input@vger.kernel.org >> Cc: linux-pwm@vger.kernel.org >> --- >> drivers/input/misc/pwm-beeper.c | 58 ++++++++++++++++++++++++++++++++- >> 1 file changed, 57 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c >> index 3cf1812384e6a..f63d0ebbaf573 100644 >> --- a/drivers/input/misc/pwm-beeper.c >> +++ b/drivers/input/misc/pwm-beeper.c >> @@ -21,6 +21,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 +38,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) >> @@ -119,6 +120,53 @@ static void pwm_beeper_close(struct input_dev *input) >> pwm_beeper_stop(beeper); >> } >> >> +static ssize_t volume_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct pwm_beeper *beeper = dev_get_drvdata(dev); >> + >> + return sysfs_emit(buf, "%ld\n", beeper->duty_cycle); >> +} >> + >> +static ssize_t volume_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct pwm_beeper *beeper = dev_get_drvdata(dev); >> + unsigned long val; >> + >> + if (kstrtoul(buf, 0, &val) < 0) >> + return -EINVAL; >> + >> + /* >> + * Volume is really PWM duty cycle in pcm (per cent mille, 1/1000th >> + * of percent). This value therefore ranges from 0 to 50000 . Duty >> + * cycle of 50% = 50000pcm is the maximum volume . >> + */ >> + val = clamp(val, 0UL, 50000UL); > > I wonder if you want to refuse values here that are not in the specified > range, that is, something like: > > if (val != clamp(val, 0UL, 50000UL)) > return -EINVAL; > > I think this is more in line who other sysfs properties work?! I am still waiting for the more general API design decision here from input maintainer, i.e. what was designed with Jeff above. Yes, we can clamp the value, but I won't work on this unless there is clear answer how to go on with the API first.
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... Thanks.
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
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@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. Uh, I don't need a full sound device to emit beeps, that's not even possible with this hardware. 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).
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@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. > > 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 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. And, IMO, extending the SND_BEEP with a volume value doesn't sound like a good idea. thanks, Takashi
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@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. >> >> 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. >> 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 - 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 > And, IMO, extending the SND_BEEP with a volume value doesn't sound > like a good idea. No, it doesn't, but see SND_TONE_WITH_VOLUME option above. I think that might be the best so far.
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@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. > >> > >> 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
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. > (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.
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. > > > (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? Thanks.
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.
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. A fourth option would be to use leds-pwm with a oneshot trigger; this would render the exact same function. This is obviously a hack, however downstream vendor kernels implement this kind of blasphemy all the time. Kind regards, Jeff LaBundy
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; }
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? How to set the default one? How to figure out what the current volume is if we decide to make volume "sticky"? 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. 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? Thanks.
On 8/1/23 08:11, Takashi Iwai wrote: > 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!) Do we really want to add dependency on the entire sound subsystem (which is currently not needed on the device I care about) only to configure one single tunable of the PWM beeper ? It seems to add too much bloat to me.
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. > 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.
On Tue, 01 Aug 2023 13:38:54 +0200, Marek Vasut wrote: > > On 8/1/23 08:11, Takashi Iwai wrote: > > 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!) > > Do we really want to add dependency on the entire sound subsystem > (which is currently not needed on the device I care about) only to > configure one single tunable of the PWM beeper ? It seems to add too > much bloat to me. That really depends on the use case. If the driver is supposed to be used generically as seen in the desktop scenes, it's worth to have a support of the standard interface like others. OTOH, if the driver is for limited situations and better to be as slim as possible, a tailored interface like sysfs would be the way to go. My proposal was under assumption of the former -- a generic usage. If the latter scene is expected, a sysfs implementation can be the right way, IMO. OTOH, we really need to be careful about the blind extension of API. Although adding a new input event type sounds easy, the influence could be much more than seen there... Takashi
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). Kind regards, Jeff LaBundy
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. thanks, Takashi
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. Regards Manuel
On Tue, Aug 01, 2023 at 12:28:29AM -0700, Dmitry Torokhov wrote: > 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? How to set the default one? How > to figure out what the current volume is if we decide to make volume > "sticky"? > > 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. > > 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? > > Thanks. > > -- > Dmitry (Just to duck in as someone that has written a little program to play beeps and tones using the EV_TONE API) It might be worth distinguishing between the goals of having some beeps with different volumes compared to all beeps with different volumes. Sound card mixers generally control some sort of global volume while I would imagine the tone API would control per-tone volume. I don't know too much about safety guarantees but writing an input then sysfs or mixer then input again seems like it could get jumbled up. In that speicfic case I think it would make more sense to send volume and tone from whatever beep API is being used, with the volume being a multiplier of the loudest volume. This is similar to how audio works with PCM output. Existing beeps would have the volume set to 100%. John.
On 8/11/23 14:39, John Watts wrote: > On Tue, Aug 01, 2023 at 12:28:29AM -0700, Dmitry Torokhov wrote: >> 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? How to set the default one? How >> to figure out what the current volume is if we decide to make volume >> "sticky"? >> >> 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. >> >> 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? >> >> Thanks. >> >> -- >> Dmitry > > (Just to duck in as someone that has written a little program to play beeps and > tones using the EV_TONE API) > > It might be worth distinguishing between the goals of having some beeps with > different volumes compared to all beeps with different volumes. > > Sound card mixers generally control some sort of global volume while I would > imagine the tone API would control per-tone volume. I don't know too much about > safety guarantees but writing an input then sysfs or mixer then input again > seems like it could get jumbled up. > > In that speicfic case I think it would make more sense to send volume and tone > from whatever beep API is being used, with the volume being a multiplier of the > loudest volume. This is similar to how audio works with PCM output. Existing > beeps would have the volume set to 100%. I agree binding tone frequency and volume together would be better. The API would be nicer and easier to use in my opinion too.
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.
On 8/15/23 23:33, Dmitry Torokhov wrote: > 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? I mean, yeah, but I think the input from John makes sense -- can we go with new TONE_WITH_VOLUME parameter instead of the sysfs interface ? That would be much nicer to work with .
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c index 3cf1812384e6a..f63d0ebbaf573 100644 --- a/drivers/input/misc/pwm-beeper.c +++ b/drivers/input/misc/pwm-beeper.c @@ -21,6 +21,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 +38,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) @@ -119,6 +120,53 @@ static void pwm_beeper_close(struct input_dev *input) pwm_beeper_stop(beeper); } +static ssize_t volume_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct pwm_beeper *beeper = dev_get_drvdata(dev); + + return sysfs_emit(buf, "%ld\n", beeper->duty_cycle); +} + +static ssize_t volume_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct pwm_beeper *beeper = dev_get_drvdata(dev); + unsigned long val; + + if (kstrtoul(buf, 0, &val) < 0) + return -EINVAL; + + /* + * Volume is really PWM duty cycle in pcm (per cent mille, 1/1000th + * of percent). This value therefore ranges from 0 to 50000 . Duty + * cycle of 50% = 50000pcm is the maximum volume . + */ + val = clamp(val, 0UL, 50000UL); + /* No change in current settings, do nothing. */ + if (val == beeper->duty_cycle) + return count; + + /* Update current settings and apply to PWM chip. */ + beeper->duty_cycle = val; + if (!beeper->suspended) + schedule_work(&beeper->work); + + return count; +} +static DEVICE_ATTR_RW(volume); + +static struct attribute *pwm_beeper_dev_attrs[] = { + &dev_attr_volume.attr, + NULL +}; + +static const struct attribute_group pwm_beeper_attr_group = { + .attrs = pwm_beeper_dev_attrs, +}; + static int pwm_beeper_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -192,6 +240,14 @@ static int pwm_beeper_probe(struct platform_device *pdev) input_set_drvdata(beeper->input, beeper); + beeper->duty_cycle = 50000; + error = devm_device_add_group(dev, &pwm_beeper_attr_group); + if (error) { + dev_err(dev, "Unable to create sysfs attributes: %d\n", + error); + return error; + } + error = input_register_device(beeper->input); if (error) { dev_err(dev, "Failed to register input device: %d\n", error);
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 ? --- NOTE: This uses approach similar to [1], except it is much simpler. [1] https://patchwork.kernel.org/project/linux-input/cover/20230201152128.614439-1-manuel.traut@mt.com/ --- Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Frieder Schrempf <frieder.schrempf@kontron.de> Cc: Manuel Traut <manuel.traut@mt.com> Cc: Marek Vasut <marex@denx.de> Cc: Thierry Reding <thierry.reding@gmail.com> Cc: linux-input@vger.kernel.org Cc: linux-pwm@vger.kernel.org --- drivers/input/misc/pwm-beeper.c | 58 ++++++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-)