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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx>
---
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@xxxxxx/
---
Cc: "Uwe Kleine-König" <u.kleine-koenig@xxxxxxxxxxxxxx>
Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
Cc: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
Cc: Manuel Traut <manuel.traut@xxxxxx>
Cc: Marek Vasut <marex@xxxxxxx>
Cc: Thierry Reding <thierry.reding@xxxxxxxxx>
Cc: linux-input@xxxxxxxxxxxxxxx
Cc: linux-pwm@xxxxxxxxxxxxxxx
---
  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.



[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux