diff mbox series

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

Message ID 149cd2e217dfb90210cc6fff332ec776371b0387.1751995302.git.u.kleine-koenig@baylibre.com
State Accepted
Headers show
Series sysfs: Various fixes and an improvement | expand

Commit Message

Uwe Kleine-König July 8, 2025, 5:24 p.m. UTC 
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(-)

Comments

Uwe Kleine-König July 9, 2025, 6:14 a.m. UTC | #1 
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
diff mbox series

Patch

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) {