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

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

[vishakha.joshi]

From: Vishakha Joshi <vishakha.joshi@intel.com>

Hi,

The patch 1 adds the count for PWM waveform to sysfs interface.
The patch 2 updates the count in the KeemBay PWM driver.

Please help to review these patches.

Thanks,
Vishakha Joshi

Vishakha Joshi (2):
  pwm: Add count to sysfs for Intel PWM driver
  pwm: Update the REPEAT_COUNT value

 Documentation/ABI/testing/sysfs-class-pwm |  8 ++++++
 drivers/pwm/pwm-keembay.c                 |  9 ++++--
 drivers/pwm/sysfs.c                       | 34 +++++++++++++++++++++++
 include/linux/pwm.h                       |  2 ++
 4 files changed, 50 insertions(+), 3 deletions(-)

-- 
2.17.1

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

[vishakha.joshi]

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;
-- 
2.17.1

[PATCH v1 2/2] pwm: Update the REPEAT_COUNT value

[vishakha.joshi]

From: Vishakha Joshi <vishakha.joshi@intel.com>

Update the count value in the PWM_LEADIN register.

Signed-off-by: Vishakha Joshi <vishakha.joshi@intel.com>
---
 drivers/pwm/pwm-keembay.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c
index 733811b05721..4494e54e23b8 100644
--- a/drivers/pwm/pwm-keembay.c
+++ b/drivers/pwm/pwm-keembay.c
@@ -128,11 +128,14 @@ static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (state->polarity != PWM_POLARITY_NORMAL)
 		return -EINVAL;
 
+	if (state->count > KMB_PWM_COUNT_MAX)
+		return -EINVAL;
+
 	/*
-	 * Configure the pwm repeat count as infinite at (15:0) and leadin
-	 * low time as 0 at (30:16), which is in terms of clock cycles.
+	 * Configure the PWM repeat count at (15:0) and LEADIN low time as 0 at
+	 * (30:16), which is in terms of clock cycles.
 	 */
-	keembay_pwm_update_bits(priv, KMB_PWM_LEADIN_MASK, 0,
+	keembay_pwm_update_bits(priv, KMB_PWM_LEADIN_MASK, state->count,
 				KMB_PWM_LEADIN_OFFSET(pwm->hwpwm));
 
 	keembay_pwm_get_state(chip, pwm, &current_state);
-- 
2.17.1

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

[Uwe Kleine-König]

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

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

[Andy Shevchenko]

On Mon, Jan 03, 2022 at 01:14:54PM +0100, Uwe Kleine-König wrote:
> On Mon, Jan 03, 2022 at 01:46:09PM +0530, vishakha.joshi@intel.com wrote:
> > From: Vishakha Joshi <vishakha.joshi@intel.com>

...

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

Good point. I guess ideally we should return the amount of the periods we still
need to produce, but I'm not sure if it can be done cleanly.

It would probably require a timer to be run in parallel, hence almost full
software implementation of the feature. (This answers the second concern)

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

Hmm... Not sure it worth it, perhaps taking into account above the -1
(in unsigned type) returned on ->get_state() can suffice as not supporting
feature?

-- 
With Best Regards,
Andy Shevchenko

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

[Uwe Kleine-König]

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

Hello,

On Tue, Jan 04, 2022 at 03:58:55PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 03, 2022 at 01:14:54PM +0100, Uwe Kleine-König wrote:
> > 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.
> 
> Hmm... Not sure it worth it, perhaps taking into account above the -1
> (in unsigned type) returned on ->get_state() can suffice as not supporting
> feature?

In my eyes that's a bad idea. You have to touch most drivers anyhow to
set the -1. So the outcome is the worst possible combination: Many
changes and still much implicit logic distributed between the drivers
and the core about what is supported and what not.

(Or you have to initialize .count = -1 before calling the get_state
callback. Then you get rid of "have to touch most drivers". But that's
still ugly IMO.)

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