Re: [PATCH v1 1/2] pwm: Add count to sysfs for Intel PWM driver

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

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

Hello,

On Mon, Jan 03, 2022 at 01:46:09PM +0530, vishakha.joshi@intel.com wrote:
> From: Vishakha Joshi <vishakha.joshi@intel.com>
> 
> The number of cycles in the PWM waveform is generated according to the
> count value configured in the sysfs interface.
> This helps to control the duration of the PWM waveform in the KeemBay
> SoC.
> In case of default count value which is zero, the PWM waveform repeats
> indefinitely.
> 
> Signed-off-by: Vishakha Joshi <vishakha.joshi@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-class-pwm |  8 ++++++
>  drivers/pwm/sysfs.c                       | 34 +++++++++++++++++++++++
>  include/linux/pwm.h                       |  2 ++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-pwm b/Documentation/ABI/testing/sysfs-class-pwm
> index 3d65285bcd5f..dde57b5a359f 100644
> --- a/Documentation/ABI/testing/sysfs-class-pwm
> +++ b/Documentation/ABI/testing/sysfs-class-pwm
> @@ -86,3 +86,11 @@ Description:
>  		Capture information about a PWM signal. The output format is a
>  		pair unsigned integers (period and duty cycle), separated by a
>  		single space.
> +
> +What:		/sys/class/pwm/pwmchip<N>/pwmX/count
> +Date:		December 2021
> +KernelVersion:  5.17
> +Contact:	Vishakha Joshi <vishakha.joshi@intel.com>
> +Description:
> +		The PWM repeat count of the number of cycles in the waveform.
> +		The default value for the count is zero with infinite repetition.
> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> index 9903c3a7eced..4d8fbd134f1d 100644
> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c
> @@ -215,11 +215,44 @@ static ssize_t capture_show(struct device *child,
>  	return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
>  }
>  
> +static ssize_t count_store(struct device *child, struct device_attribute *attr, const char *buf,
> +			   size_t size)
> +{
> +	struct pwm_export *export = child_to_pwm_export(child);
> +	struct pwm_device *pwm = export->pwm;
> +	struct pwm_state state;
> +	unsigned int count;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 0, &count);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&export->lock);
> +	pwm_get_state(pwm, &state);
> +	state.count = count;
> +	ret = pwm_apply_state(pwm, &state);
> +	mutex_unlock(&export->lock);
> +
> +	return ret ?: size;
> +}
> +
> +static ssize_t count_show(struct device *child, struct device_attribute *attr, char *buf)
> +{
> +	const struct pwm_device *pwm = child_to_pwm_device(child);
> +	struct pwm_state state;
> +
> +	pwm_get_state(pwm, &state);
> +
> +	return sysfs_emit(buf, "%d\n", state.count);
> +}
> +
>  static DEVICE_ATTR_RW(period);
>  static DEVICE_ATTR_RW(duty_cycle);
>  static DEVICE_ATTR_RW(enable);
>  static DEVICE_ATTR_RW(polarity);
>  static DEVICE_ATTR_RO(capture);
> +static DEVICE_ATTR_RW(count);
>  
>  static struct attribute *pwm_attrs[] = {
>  	&dev_attr_period.attr,
> @@ -227,6 +260,7 @@ static struct attribute *pwm_attrs[] = {
>  	&dev_attr_enable.attr,
>  	&dev_attr_polarity.attr,
>  	&dev_attr_capture.attr,
> +	&dev_attr_count.attr,
>  	NULL
>  };
>  ATTRIBUTE_GROUPS(pwm);
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index e6dac95e4960..dc0612867e65 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -52,6 +52,7 @@ enum {
>   * struct pwm_state - state of a PWM channel
>   * @period: PWM period (in nanoseconds)
>   * @duty_cycle: PWM duty cycle (in nanoseconds)
> + * @count: PWM repeat count
>   * @polarity: PWM polarity
>   * @enabled: PWM enabled status
>   * @usage_power: If set, the PWM driver is only required to maintain the power
> @@ -62,6 +63,7 @@ enum {
>  struct pwm_state {
>  	u64 period;
>  	u64 duty_cycle;
> +	u32 count;
>  	enum pwm_polarity polarity;
>  	bool enabled;
>  	bool usage_power;

This needs more documentation about what the semantic should be. E.g.
what should .get_state return?

Also I doubt this is a good idea given that most controllers cannot
implement it.

If we really want to support a count, I request that all drivers that
don't support it get updated to refuse a request with count != 0.

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 --]