[PATCH libpwm 0/4] sysfs: Various fixes and an improvement

[PATCH libpwm 0/4] sysfs: Various fixes and an improvement

[Uwe Kleine-König]

Hello,

while working on the pwm-mediatek driver I found a few corners in libpwm
that doesn't seem to have seen much testing before with a driver that
can only implement normal polarity.

Best regards
Uwe

Uwe Kleine-König (4):
  sysfs: Fix a wrong condition for duty_cycle writing
  sysfs: Fix polarity handling
  sysfs: Keep polarity for constant waveforms
  sysfs: Implement fine grained cache control

 sysfs.c | 134 +++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 104 insertions(+), 30 deletions(-)


base-commit: 3a9a9d36d95e8aa5ed563590d53c1715285a5ffb
-- 
2.49.0

[PATCH libpwm 1/4] sysfs: Fix a wrong condition for duty_cycle writing

[Uwe Kleine-König]

It's an invalid cache that must result in setting the duty_cycle
explicitly. In this else branch it's already known that cache_valid is
true. So the damage is small and the condition can just be dropped.

Fixes: 67f0b9f2a2aa ("First prototype for libpwm")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 sysfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sysfs.c b/sysfs.c
index fe4edea1e102..0f87f2b87028 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -212,8 +212,7 @@ static int pwm_chip_sysfs_set_waveform(struct pwm *pwm,
 			pwm_sysfs->wf.duty_length_ns = wf->duty_length_ns;
 		}
 	} else {
-		if (!!pwm_sysfs->cache_valid ||
-		    pwm_sysfs->wf.duty_length_ns != wf->duty_length_ns) {
+		if (pwm_sysfs->wf.duty_length_ns != wf->duty_length_ns) {
 			ret = pwm_chip_sysfs_write_prop(pwm_sysfs, "duty_cycle",
 							"%" PRIu64 "\n", wf->duty_length_ns);
 			if (ret)
-- 
2.49.0

[PATCH libpwm 2/4] sysfs: Fix polarity handling

[Uwe Kleine-König]

Depending on polarity the sysfs duty_cycle either defines the active or the
inactive time of the PWM output. This has three effects that both were not
considered before in the sysfs backend:

 - If polarity changes this affects the waveform's duty_length;
 - if duty_length_ns changes and polarity is inverted this affects
   duty_offset; and
 - for inverted polarity the written duty_cycle value must be
   period_length_ns - duty_length_ns.

To simplify handling the first two items, rework the cache representation
to use the parameters of the sysfs representation.

For the second introduce a helper variable.

Fixes: 67f0b9f2a2aa ("First prototype for libpwm")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 sysfs.c | 46 ++++++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/sysfs.c b/sysfs.c
index 0f87f2b87028..151b035f72d1 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -22,9 +22,11 @@ struct pwm_sysfs {
 	struct pwm pwm;
 	int dirfd;
 
-	/* .wf tracks the wf assuming the PWM is enabled. */
-	struct pwm_waveform wf;
+	/* cached settings */
 	bool enabled;
+	uint64_t period;
+	uint64_t duty_cycle;
+	bool inverted_polarity;
 	bool cache_valid;
 };
 
@@ -155,6 +157,7 @@ static int pwm_chip_sysfs_set_waveform(struct pwm *pwm,
 {
 	struct pwm_sysfs *pwm_sysfs = container_of(pwm, struct pwm_sysfs, pwm);
 	int ret;
+	uint64_t duty_cycle;
 
 	/* period_length_ns = 0 is interpreted as disabled */
 	if (wf->period_length_ns == 0) {
@@ -169,55 +172,58 @@ static int pwm_chip_sysfs_set_waveform(struct pwm *pwm,
 	}
 
 	if (!pwm_sysfs->cache_valid ||
-	    (wf->duty_offset_ns < wf->period_length_ns - wf->duty_length_ns) !=
-	     (pwm_sysfs->wf.duty_offset_ns < pwm_sysfs->wf.period_length_ns - pwm_sysfs->wf.duty_length_ns)) {
+	    (wf->duty_offset_ns >= wf->period_length_ns - wf->duty_length_ns) != pwm_sysfs->inverted_polarity) {
 		if (wf->duty_offset_ns < wf->period_length_ns - wf->duty_length_ns) {
 			ret = pwm_chip_sysfs_write_prop(pwm_sysfs, "polarity", "normal\n");
 			if (ret)
 				return ret;
 
-			pwm_sysfs->wf.duty_offset_ns = 0;
+			pwm_sysfs->inverted_polarity = false;
 		} else {
 			ret = pwm_chip_sysfs_write_prop(pwm_sysfs, "polarity", "inversed\n");
 			if (ret)
 				return ret;
 
-			pwm_sysfs->wf.duty_offset_ns = wf->period_length_ns - wf->duty_length_ns;
+			pwm_sysfs->inverted_polarity = true;
 		}
 	}
 
+	if (pwm_sysfs->inverted_polarity)
+		duty_cycle = wf->period_length_ns - wf->duty_length_ns;
+	else
+		duty_cycle = wf->duty_length_ns;
+
 	/*
-	 * Ensure that we never hit duty_length_ns > period_length_ns. As updating
-	 * period_length_ns and duty_length_ns cannot be done in a single step write
-	 * period_length_ns first if period_length_ns increases and write duty_length_ns first
-	 * if period_length_ns decreases.
+	 * Ensure that we never hit duty_cycle > period. As updating period and
+	 * duty_cycle cannot be done in a single step write period first if
+	 * period increases and write duty_cycle first if period decreases.
 	 */
 	if (!pwm_sysfs->cache_valid ||
-	    pwm_sysfs->wf.period_length_ns <= wf->period_length_ns) {
+	    pwm_sysfs->period <= wf->period_length_ns) {
 		if (!pwm_sysfs->cache_valid ||
-		    pwm_sysfs->wf.period_length_ns != wf->period_length_ns) {
+		    pwm_sysfs->period != wf->period_length_ns) {
 			ret = pwm_chip_sysfs_write_prop(pwm_sysfs, "period",
 							"%" PRIu64 "\n", wf->period_length_ns);
 			if (ret)
 				return ret;
-			pwm_sysfs->wf.period_length_ns = wf->period_length_ns;
+			pwm_sysfs->period = wf->period_length_ns;
 		}
 
 		if (!pwm_sysfs->cache_valid ||
-		    pwm_sysfs->wf.duty_length_ns != wf->duty_length_ns) {
+		    pwm_sysfs->duty_cycle != wf->duty_length_ns) {
 			ret = pwm_chip_sysfs_write_prop(pwm_sysfs, "duty_cycle",
-							"%" PRIu64 "\n", wf->duty_length_ns);
+							"%" PRIu64 "\n", duty_cycle);
 			if (ret)
 				return ret;
-			pwm_sysfs->wf.duty_length_ns = wf->duty_length_ns;
+			pwm_sysfs->duty_cycle = duty_cycle;
 		}
 	} else {
-		if (pwm_sysfs->wf.duty_length_ns != wf->duty_length_ns) {
+		if (pwm_sysfs->duty_cycle != wf->duty_length_ns) {
 			ret = pwm_chip_sysfs_write_prop(pwm_sysfs, "duty_cycle",
-							"%" PRIu64 "\n", wf->duty_length_ns);
+							"%" PRIu64 "\n", duty_cycle);
 			if (ret)
 				return ret;
-			pwm_sysfs->wf.duty_length_ns = wf->duty_length_ns;
+			pwm_sysfs->duty_cycle = duty_cycle;
 		}
 
 		/*
@@ -229,7 +235,7 @@ static int pwm_chip_sysfs_set_waveform(struct pwm *pwm,
 						"%" PRIu64 "\n", wf->period_length_ns);
 		if (ret)
 			return ret;
-		pwm_sysfs->wf.period_length_ns = wf->period_length_ns;
+		pwm_sysfs->period = wf->period_length_ns;
 	}
 
 	if (!pwm_sysfs->cache_valid || !pwm_sysfs->enabled) {
-- 
2.49.0

[PATCH libpwm 3/4] sysfs: Keep polarity for constant waveforms

[Uwe Kleine-König]

Some waveforms have equivalent representations in sysfs with different
polarities. As some PWMs only support a single polarity, minimize polarity
changes.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 sysfs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/sysfs.c b/sysfs.c
index 151b035f72d1..9eac066eb8bf 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -122,6 +122,43 @@ static struct pwm *pwm_chip_sysfs_get_pwm(struct pwm_chip *chip,
 	return pwm;
 }
 
+static ssize_t pwm_chip_sysfs_read_prop(const struct pwm_sysfs *pwm_sysfs,
+					char *propname,
+					char *buf, size_t count)
+{
+	int fd;
+	va_list ap;
+	int ret;
+	size_t cntread = 0;
+
+	fd = openat(pwm_sysfs->dirfd, propname, O_RDONLY | O_CLOEXEC);
+	if (fd < 0)
+		return -1;
+
+	while (cntread < count) {
+		ret = read(fd, buf + cntread, count - cntread);
+		if (ret < 0) {
+			int saved_errno = errno;
+
+			close(fd);
+
+			errno = saved_errno;
+
+			return ret;
+		} else if (ret == 0) {
+			break;
+		}
+
+		cntread += ret;
+	}
+
+	ret = close(fd);
+	if (ret)
+		return ret;
+
+	return cntread;
+}
+
 static int pwm_chip_sysfs_write_prop(const struct pwm_sysfs *pwm_sysfs,
 					 char *propname,
 					 const char *restrict format, ...)
@@ -173,7 +210,26 @@ static int pwm_chip_sysfs_set_waveform(struct pwm *pwm,
 
 	if (!pwm_sysfs->cache_valid ||
 	    (wf->duty_offset_ns >= wf->period_length_ns - wf->duty_length_ns) != pwm_sysfs->inverted_polarity) {
-		if (wf->duty_offset_ns < wf->period_length_ns - wf->duty_length_ns) {
+		if (wf->duty_length_ns == wf->period_length_ns || wf->duty_length_ns == 0) {
+			/*
+			 * Waveforms with constant inactive output have two
+			 * possible representations in sysfs. Either with normal
+			 * polarity and duty_cycle = 0, or with inverted
+			 * polarity and duty_cycle = period. The analogous
+			 * statement is true for constant active waveforms. As
+			 * many PWMs only support a single polarity, and also to
+			 * minimize sysfs access, keep the current polarity in
+			 * this case.
+			 */
+			if (!pwm_sysfs->cache_valid) {
+				char buf[20];
+
+				ret = pwm_chip_sysfs_read_prop(pwm_sysfs, "polarity", buf, sizeof(buf));
+				if (ret < 0)
+					return ret;
+				pwm_sysfs->inverted_polarity = buf[0] == 'i';
+			}
+		} else if (wf->duty_offset_ns < wf->period_length_ns - wf->duty_length_ns) {
 			ret = pwm_chip_sysfs_write_prop(pwm_sysfs, "polarity", "normal\n");
 			if (ret)
 				return ret;
-- 
2.49.0

[PATCH libpwm 4/4] sysfs: Implement fine grained cache control

[Uwe Kleine-König]

To save a few slow sysfs write accesses, track the validity of the four
sysfs properties separately.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 sysfs.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/sysfs.c b/sysfs.c
index 9eac066eb8bf..97e5b6e59ec0 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -27,7 +27,11 @@ struct pwm_sysfs {
 	uint64_t period;
 	uint64_t duty_cycle;
 	bool inverted_polarity;
-	bool cache_valid;
+
+	bool enabled_cache_valid;
+	bool period_cache_valid;
+	bool duty_cycle_cache_valid;
+	bool polarity_cache_valid;
 };
 
 struct pwm_chip_sysfs {
@@ -117,7 +121,10 @@ static struct pwm *pwm_chip_sysfs_get_pwm(struct pwm_chip *chip,
 
 	chip_sysfs->pwms[offset] = pwm_sysfs;
 
-	pwm_sysfs->cache_valid = false;
+	pwm_sysfs->enabled_cache_valid = false;
+	pwm_sysfs->period_cache_valid = false;
+	pwm_sysfs->duty_cycle_cache_valid = false;
+	pwm_sysfs->polarity_cache_valid = false;
 
 	return pwm;
 }
@@ -208,7 +215,7 @@ static int pwm_chip_sysfs_set_waveform(struct pwm *pwm,
 		return 0;
 	}
 
-	if (!pwm_sysfs->cache_valid ||
+	if (!pwm_sysfs->polarity_cache_valid ||
 	    (wf->duty_offset_ns >= wf->period_length_ns - wf->duty_length_ns) != pwm_sysfs->inverted_polarity) {
 		if (wf->duty_length_ns == wf->period_length_ns || wf->duty_length_ns == 0) {
 			/*
@@ -221,7 +228,7 @@ static int pwm_chip_sysfs_set_waveform(struct pwm *pwm,
 			 * minimize sysfs access, keep the current polarity in
 			 * this case.
 			 */
-			if (!pwm_sysfs->cache_valid) {
+			if (!pwm_sysfs->polarity_cache_valid) {
 				char buf[20];
 
 				ret = pwm_chip_sysfs_read_prop(pwm_sysfs, "polarity", buf, sizeof(buf));
@@ -242,7 +249,9 @@ static int pwm_chip_sysfs_set_waveform(struct pwm *pwm,
 
 			pwm_sysfs->inverted_polarity = true;
 		}
+
 	}
+	pwm_sysfs->polarity_cache_valid = true;
 
 	if (pwm_sysfs->inverted_polarity)
 		duty_cycle = wf->period_length_ns - wf->duty_length_ns;
@@ -254,9 +263,9 @@ static int pwm_chip_sysfs_set_waveform(struct pwm *pwm,
 	 * duty_cycle cannot be done in a single step write period first if
 	 * period increases and write duty_cycle first if period decreases.
 	 */
-	if (!pwm_sysfs->cache_valid ||
+	if (!pwm_sysfs->period_cache_valid || !pwm_sysfs->duty_cycle_cache_valid ||
 	    pwm_sysfs->period <= wf->period_length_ns) {
-		if (!pwm_sysfs->cache_valid ||
+		if (!pwm_sysfs->period_cache_valid ||
 		    pwm_sysfs->period != wf->period_length_ns) {
 			ret = pwm_chip_sysfs_write_prop(pwm_sysfs, "period",
 							"%" PRIu64 "\n", wf->period_length_ns);
@@ -264,8 +273,9 @@ static int pwm_chip_sysfs_set_waveform(struct pwm *pwm,
 				return ret;
 			pwm_sysfs->period = wf->period_length_ns;
 		}
+		pwm_sysfs->period_cache_valid = true;
 
-		if (!pwm_sysfs->cache_valid ||
+		if (!pwm_sysfs->duty_cycle_cache_valid ||
 		    pwm_sysfs->duty_cycle != wf->duty_length_ns) {
 			ret = pwm_chip_sysfs_write_prop(pwm_sysfs, "duty_cycle",
 							"%" PRIu64 "\n", duty_cycle);
@@ -273,6 +283,7 @@ static int pwm_chip_sysfs_set_waveform(struct pwm *pwm,
 				return ret;
 			pwm_sysfs->duty_cycle = duty_cycle;
 		}
+		pwm_sysfs->duty_cycle_cache_valid = true;
 	} else {
 		if (pwm_sysfs->duty_cycle != wf->duty_length_ns) {
 			ret = pwm_chip_sysfs_write_prop(pwm_sysfs, "duty_cycle",
@@ -281,6 +292,7 @@ static int pwm_chip_sysfs_set_waveform(struct pwm *pwm,
 				return ret;
 			pwm_sysfs->duty_cycle = duty_cycle;
 		}
+		pwm_sysfs->duty_cycle_cache_valid = true;
 
 		/*
 		 * It's already known that
@@ -292,15 +304,16 @@ static int pwm_chip_sysfs_set_waveform(struct pwm *pwm,
 		if (ret)
 			return ret;
 		pwm_sysfs->period = wf->period_length_ns;
+		pwm_sysfs->period_cache_valid = true;
 	}
 
-	if (!pwm_sysfs->cache_valid || !pwm_sysfs->enabled) {
+	if (!pwm_sysfs->enabled_cache_valid || !pwm_sysfs->enabled) {
 		ret = pwm_chip_sysfs_write_prop(pwm_sysfs, "enable", "1\n");
 		if (ret)
 			return ret;
 		pwm_sysfs->enabled = true;
 	}
-	pwm_sysfs->cache_valid = true;
+	pwm_sysfs->enabled_cache_valid = true;
 
 	return 0;
 }
-- 
2.49.0

Re: [PATCH libpwm 2/4] sysfs: Fix polarity handling

[Uwe Kleine-König]

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

On Tue, Jul 08, 2025 at 07:24:14PM +0200, Uwe Kleine-König wrote:
> Depending on polarity the sysfs duty_cycle either defines the active or the
> inactive time of the PWM output. This has three effects that both were not
> considered before in the sysfs backend:
> 
>  - If polarity changes this affects the waveform's duty_length;
>  - if duty_length_ns changes and polarity is inverted this affects
>    duty_offset; and
>  - for inverted polarity the written duty_cycle value must be
>    period_length_ns - duty_length_ns.
> 
> To simplify handling the first two items, rework the cache representation
> to use the parameters of the sysfs representation.
> 
> For the second introduce a helper variable.

Reminder to myself: The issue was initially about two effects and the
message still talks about "both". Also "second" should be "third".

Just for that I won't send a v2 and just fixup while applying if no
further review requires an iteration.

Best regards
Uwe

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

Re: [PATCH libpwm 0/4] sysfs: Various fixes and an improvement

[Uwe Kleine-König]

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

Hello,

On Tue, Jul 08, 2025 at 07:24:12PM +0200, Uwe Kleine-König wrote:
> while working on the pwm-mediatek driver I found a few corners in libpwm
> that doesn't seem to have seen much testing before with a driver that
> can only implement normal polarity.

applied with the announced fixup to patch #2 to
https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/libpwm.git main
.

Best regards
Uwe

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