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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3909 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]