pwm: pca9685: remove unused duty_cycle struct element

The interaction of the ALL_LED PWM channel with the other channels was not well-defined. As the ALL_LED feature does not seem very useful and it was making the code significantly more complex, simply remove it.

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 115 ++++++--------------------------------
 1 file changed, 17 insertions(+), 98 deletions(-)

Quoted 8 lines · Matthias Schiffer

The interaction of the ALL_LED PWM channel with the other channels was not well-defined. As the ALL_LED feature does not seem very useful and it was making the code significantly more complex, simply remove it.

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 115 ++++++--------------------------------
 1 file changed, 17 insertions(+), 98 deletions(-)

Applied, thanks.

Quoted 4 lines · Matthias Schiffer

The interaction of the ALL_LED PWM channel with the other channels was not well-defined. As the ALL_LED feature does not seem very useful and it was making the code significantly more complex, simply remove it.

Quoted 4 lines

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 115 ++++++--------------------------------
 1 file changed, 17 insertions(+), 98 deletions(-)

Applied, thanks.

Quoted 6 lines · Thierry Reding

Quoted 4 lines · Matthias Schiffer

The interaction of the ALL_LED PWM channel with the other channels was not well-defined. As the ALL_LED feature does not seem very useful and it was making the code significantly more complex, simply remove it.

Quoted 6 lines

Quoted 4 lines

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 115 ++++++--------------------------------
 1 file changed, 17 insertions(+), 98 deletions(-)

Applied, thanks.

This seems to be ABI breakage.

Thierry, do you have hardware to test?

Quoted 4 lines · Matthias Schiffer

The interaction of the ALL_LED PWM channel with the other channels was not well-defined. As the ALL_LED feature does not seem very useful and it was making the code significantly more complex, simply remove it.

Quoted 4 lines

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 115 ++++++--------------------------------
 1 file changed, 17 insertions(+), 98 deletions(-)

Applied, thanks.

Thierry

Hi,

Quoted 5 lines · Thierry Reding

Quoted 4 lines · Matthias Schiffer

The interaction of the ALL_LED PWM channel with the other channels was not well-defined. As the ALL_LED feature does not seem very useful and it was making the code significantly more complex, simply remove it.

Quoted 8 lines

Quoted 4 lines

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 115 ++++++--------------------------------
 1 file changed, 17 insertions(+), 98 deletions(-)

Applied, thanks.

Thierry

I was not reading the mailing list in the last weeks, so I only saw the patch today.

We are using the ALL_LED channel in production to reduce the delay when all 16 PWM outputs need to be set to the same duty cycle.

I am not sure it is a good idea to remove this feature.

Quoted 17 lines · Clemens Gruber

Hi,

Quoted 5 lines · Thierry Reding

Quoted 4 lines · Matthias Schiffer

The interaction of the ALL_LED PWM channel with the other channels was not well-defined. As the ALL_LED feature does not seem very useful and it was making the code significantly more complex, simply remove it.

Quoted 8 lines

Quoted 4 lines

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 115 ++++++--------------------------------
 1 file changed, 17 insertions(+), 98 deletions(-)

Applied, thanks.

Thierry

Quoted 7 lines

I was not reading the mailing list in the last weeks, so I only saw the patch today.

We are using the ALL_LED channel in production to reduce the delay when all 16 PWM outputs need to be set to the same duty cycle.

I am not sure it is a good idea to remove this feature.

Can you specify what platform this is and where the code is that does this. I can't really find any device tree users of this and I don't know if there's a good way to find out what other users there are, but this isn't the first time this driver has created confusion, so please help collect some more information about it's use so we can avoid this in the future.

I'll back out this particular patch since you're using it. Can you give the other three patches a try to see if they work for you?

Quoted 23 lines · Clemens Gruber

Hi,

Quoted 5 lines · Thierry Reding

Quoted 4 lines · Matthias Schiffer

The interaction of the ALL_LED PWM channel with the other channels was not well-defined. As the ALL_LED feature does not seem very useful and it was making the code significantly more complex, simply remove it.

Quoted 8 lines

Quoted 4 lines

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 115 ++++++--------------------------------
 1 file changed, 17 insertions(+), 98 deletions(-)

Applied, thanks.

Thierry

I was not reading the mailing list in the last weeks, so I only saw the patch today.

We are using the ALL_LED channel in production to reduce the delay when all 16 PWM outputs need to be set to the same duty cycle.

Quoted 1 line

I am not sure it is a good idea to remove this feature.

Can you specify what platform this is and where the code is that does this. I can't really find any device tree users of this and I don't know if there's a good way to find out what other users there are, but this isn't the first time this driver has created confusion, so please help collect some more information about it's use so we can avoid this in the future.

I'll back out this particular patch since you're using it. Can you give the other three patches a try to see if they work for you?

Quoted 34 lines · Thierry Reding

Quoted 27 lines · Clemens Gruber

Hi,

Quoted 16 lines · Thierry Reding

Quoted 7 lines · Matthias Schiffer

The interaction of the ALL_LED PWM channel with the other channels was
not well-defined. As the ALL_LED feature does not seem very useful and
it was making the code significantly more complex, simply remove it.

Quoted 6 lines

Signed-off-by: Matthias Schiffer <
[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 115 ++++++--------------------
------------
 1 file changed, 17 insertions(+), 98 deletions(-)

Quoted 3 lines

Applied, thanks.

Thierry

I was not reading the mailing list in the last weeks, so I only saw the
patch today.

Quoted 5 lines

We are using the ALL_LED channel in production to reduce the delay when
all 16 PWM outputs need to be set to the same duty cycle.

I am not sure it is a good idea to remove this feature.

Quoted 11 lines

Can you specify what platform this is and where the code is that does
this. I can't really find any device tree users of this and I don't know
if there's a good way to find out what other users there are, but this
isn't the first time this driver has created confusion, so please help
collect some more information about it's use so we can avoid this in the
future.

The platform is ARM, it's a custom board with an NXP i.MX6. The device
tree is not upstreamed. As there are multiple companies involved in changes to this driver, I assume that it is in use, even though there
are no in-tree users.
Also: As you can set the ALL channel from userspace, it will be very difficult to find out how often the ALL feature is being used somewhere.

Quoted 4 lines

I'll back out this particular patch since you're using it. Can you give
the other three patches a try to see if they work for you?

Thanks! I saw your other mail. Patch 1 looks good to me. I will look at
the new version of patches 3 and 4 and test them when they appear on the
list.

Clemens

Quoted 62 lines · Clemens Gruber

Quoted 34 lines · Thierry Reding

Quoted 27 lines · Clemens Gruber

Hi,

Quoted 16 lines · Thierry Reding

Quoted 7 lines · Matthias Schiffer

The interaction of the ALL_LED PWM channel with the other channels was
not well-defined. As the ALL_LED feature does not seem very useful and
it was making the code significantly more complex, simply remove it.

Quoted 6 lines

Signed-off-by: Matthias Schiffer <
[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 115 ++++++--------------------
------------
 1 file changed, 17 insertions(+), 98 deletions(-)

Quoted 3 lines

Applied, thanks.

Thierry

I was not reading the mailing list in the last weeks, so I only saw the
patch today.

Quoted 5 lines

We are using the ALL_LED channel in production to reduce the delay when
all 16 PWM outputs need to be set to the same duty cycle.

I am not sure it is a good idea to remove this feature.

Quoted 11 lines

Can you specify what platform this is and where the code is that does
this. I can't really find any device tree users of this and I don't know
if there's a good way to find out what other users there are, but this
isn't the first time this driver has created confusion, so please help
collect some more information about it's use so we can avoid this in the
future.

The platform is ARM, it's a custom board with an NXP i.MX6. The device
tree is not upstreamed. As there are multiple companies involved in changes to this driver, I assume that it is in use, even though there
are no in-tree users.
Also: As you can set the ALL channel from userspace, it will be very difficult to find out how often the ALL feature is being used somewhere.

Quoted 4 lines

I'll back out this particular patch since you're using it. Can you give
the other three patches a try to see if they work for you?

Quoted 7 lines

Thanks! I saw your other mail. Patch 1 looks good to me. I will look at
the new version of patches 3 and 4 and test them when they appear on the
list.

Clemens

Thanks for the feedback, I'll have to respin my cleanup patches without removing this feature.

I wonder if we can come up with a sane semantics of how ALL_LED is supposed to interact with the individual channels? Optimally, changes made via ALL_LED should be reflected in the state of the other channels including their sysfs files, but I'm not sure if current APIs can support this cleanly. It might make sense to make requesting/exporting individual channels and ALL_LED mutually exclusive, so the state of a requested PWM can't change when it's supposed to be under exclusive control of one user. Of course, such a change can break existing users as well...

And what about state propagation in the other direction - how should the ALL_LED state reflect changes made to the other channels' settings? On the hardware side, the ALL_LED registers are write-only, as there aren't any sane values that could be returned.

Initialize all ON delays to 0 during probe, rather than doing it in pca9685_pwm_enable.

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/pwm/pwm-pca9685.c +0 -7

index 393ab92aa945..370691b21107 100644
@@ -289,13 +289,6 @@ static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct pca9685 *pca = to_pca(chip);
 
-	/*
-	 * The PWM subsystem does not support a pre-delay.
-	 * So, set the ON-timeout to 0
-	 */
-	regmap_write(pca->regmap, LED_N_ON_L(pwm->hwpwm), 0);
-	regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), 0);
-
 	/*
 	 * Clear the full-off bit.
 	 * It has precedence over the others and must be off.

Quoted 30 lines · Matthias Schiffer

Initialize all ON delays to 0 during probe, rather than doing it in pca9685_pwm_enable.

Signed-off-by Matthias Schiffer <[email protected]

Quoted 1 line

---
 drivers/pwm/pwm-pca9685.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/pwm/pwm-pca9685.c +0 -7

index 393ab92aa945..370691b21107 100644
@@ -289,13 +289,6 @@ static int pca9685_pwm_enable(struct pwm_chip
 *chip, struct pwm_device *pwm)
 {
 	struct pca9685 *pca = to_pca(chip);
 
-	/*
-	 * The PWM subsystem does not support a pre-delay.
-	 * So, set the ON-timeout to 0
-	 */
-	regmap_write(pca->regmap, LED_N_ON_L(pwm->hwpwm), 0);
-	regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), 0);
-
 	/*
 	 * Clear the full-off bit.

drivers/pwm/pwm-pca9685.c +0 -0

 	 * It has precedence over the others and must be off.

@@ -388,6 +381,13 @@ static int pca9685_pwm_probe(struct i2c_client
 *client,

drivers/pwm/pwm-pca9685.c static int pca9685_pwm_probe(struct i2c_client · line 391

Quoted 10 lines

drivers/pwm/pwm-pca9685.c +7 -0

 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
 
+	/*
+	 * The PWM subsystem does not support a pre-delay.
+	 * So, set the ON-timeout to 0
+	 */
+	regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
+	regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
+

What is a pre-delay: Something like:

          _________                   ______
    _____/         \_________________/
    ^                           ^

Where ^ marks the period start and then the time between period start and the rising signal is the pre-delay?

If so, the IMHO more right approach is to keep the pre-delay until a new
setting is applied and in .get_state ignore the pre-delay. This way you
don't modify the output in .probe() which sounds right.

Best regards
Uwe

Quoted 47 lines · Uwe Kleine-König

Quoted 30 lines · Matthias Schiffer

Initialize all ON delays to 0 during probe, rather than doing it in pca9685_pwm_enable.

Signed-off-by Matthias Schiffer <[email protected]

Quoted 1 line

---
 drivers/pwm/pwm-pca9685.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/pwm/pwm-pca9685.c +0 -7

index 393ab92aa945..370691b21107 100644
@@ -289,13 +289,6 @@ static int pca9685_pwm_enable(struct pwm_chip
 *chip, struct pwm_device *pwm)
 {
 	struct pca9685 *pca = to_pca(chip);
 
-	/*
-	 * The PWM subsystem does not support a pre-delay.
-	 * So, set the ON-timeout to 0
-	 */
-	regmap_write(pca->regmap, LED_N_ON_L(pwm->hwpwm), 0);
-	regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), 0);
-
 	/*
 	 * Clear the full-off bit.

drivers/pwm/pwm-pca9685.c +0 -0

 	 * It has precedence over the others and must be off.

@@ -388,6 +381,13 @@ static int pca9685_pwm_probe(struct i2c_client
 *client,

drivers/pwm/pwm-pca9685.c static int pca9685_pwm_probe(struct i2c_client · line 391

Quoted 10 lines

drivers/pwm/pwm-pca9685.c +7 -0

 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
 
+	/*
+	 * The PWM subsystem does not support a pre-delay.
+	 * So, set the ON-timeout to 0
+	 */
+	regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
+	regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
+

What is a pre-delay: Something like:

          _________                   ______
    _____/         \_________________/
    ^                           ^

Quoted 9 lines

Where ^ marks the period start and then the time between period start and the rising signal is the pre-delay?

If so, the IMHO more right approach is to keep the pre-delay until a new
setting is applied and in .get_state ignore the pre-delay. This way you
don't modify the output in .probe() which sounds right.

My approach was to get the hardware into a known state by resetting most registers (which is what the driver attempted to do so far). If getting the hardware state via get_state is preferable, I can implement that instead.

For consistency with disabled state, initialize all LEDs in FULL_OFF state during probe.

This also fixes a broken interaction between config with 100% duty cycle (which would set the LED to FULL_ON) and enable (which would unset FULL_ON), effectively disabling the LED again when enable was called after config. This behaviour was observed with the leds-pwm driver when directly switching from 0 to maximum brightness.

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 53 +++++++++++----------------------------
 1 file changed, 14 insertions(+), 39 deletions(-)

drivers/pwm/pwm-pca9685.c +11 -10

index 370691b21107..e266cbbd39bf 100644
@@ -219,15 +219,16 @@ static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
 	}
 }
 
-static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			      int duty_ns, int period_ns)
+static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			     const struct pwm_state *state)
 {
 	struct pca9685 *pca = to_pca(chip);
 	unsigned long long duty;
 	int prescale;
 
-	if (period_ns != pca->period_ns) {
-		prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
+	if (state->period != pca->period_ns) {
+		prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ *
+					     state->period,
 					     PCA9685_COUNTER_RANGE * 1000) - 1;
 
 		if (prescale >= PCA9685_PRESCALE_MIN &&

@@ -247,7 +248,7 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			/* Wake the chip up */
 			pca9685_set_sleep_mode(pca, false);
 
-			pca->period_ns = period_ns;
+			pca->period_ns = state->period;
 		} else {
 			dev_err(chip->dev,
 				"prescaler not set: period out of bounds!\n");

@@ -255,13 +256,13 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		}
 	}
 
-	if (duty_ns < 1) {
+	if (!state->enabled || state->duty_cycle < 1) {
+		/* Set the full OFF bit */
 		regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), LED_FULL);
-
 		return 0;
 	}
 
-	if (duty_ns == period_ns) {
+	if (state->duty_cycle == state->period) {
 		/* Clear both OFF registers */
 		regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0x0);
 		regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0x0);

@@ -272,8 +273,8 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		return 0;
 	}
 
-	duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
-	duty = DIV_ROUND_UP_ULL(duty, period_ns);
+	duty = PCA9685_COUNTER_RANGE * (unsigned long long)state->duty_cycle;
+	duty = DIV_ROUND_UP_ULL(duty, state->period);
 
 	regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), (int)duty & 0xff);
 	regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm),

drivers/pwm/pwm-pca9685.c +1 -27

 }
 
-static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct pca9685 *pca = to_pca(chip);
-
-	/*
-	 * Clear the full-off bit.
-	 * It has precedence over the others and must be off.
-	 */
-	regmap_update_bits(pca->regmap, LED_N_OFF_H(pwm->hwpwm), LED_FULL, 0x0);
-
-	return 0;
-}
-
-static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct pca9685 *pca = to_pca(chip);
-
-	regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), LED_FULL);
-
-	/* Clear the LED_OFF counter. */
-	regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0x0);
-}
-
 static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct pca9685 *pca = to_pca(chip);

@@ -321,14 +299,11 @@ static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 
 static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	pca9685_pwm_disable(chip, pwm);
 	pm_runtime_put(chip->dev);
 }
 
 static const struct pwm_ops pca9685_pwm_ops = {
-	.enable = pca9685_pwm_enable,
-	.disable = pca9685_pwm_disable,
-	.config = pca9685_pwm_config,
+	.apply = pca9685_pwm_apply,
 	.request = pca9685_pwm_request,
 	.free = pca9685_pwm_free,
 	.owner = THIS_MODULE,

Hello Matthias,

as you seem to have this hardware on your desk, it would be great if you
could answer the following questions:

- Does the hardware complete the currently running period before applying a new setting?

Because the loading of the LEDn_ON and LEDn_OFF registers is via the I 2 C-bus, and
asynchronous to the internal oscillator, we want to ensure that we do not see any visual
artifacts of changing the ON and OFF values. This is achieved by updating the changes at
the end of the LOW cycle.

- Is this racy somehow (i.e. can it happen that when going from duty_cycle/period = 1000/5000 to duty_cycle/period = 4000/10000
the
output is 1000/10000 (or 4000/5000) for one cycle)?

- Does the hardware complete the currently running period before .enabled = false is configured?

- How does the output pin behave on a disabled PWM. (Usual
candidates
are: freeze where is just happens to be, constant inactive and High-Z).

Best regards
Uwe

Quoted 8 lines · Uwe Kleine-König

Hello Matthias,

as you seem to have this hardware on your desk, it would be great if you
could answer the following questions:

- Does the hardware complete the currently running period before applying a new setting?

The datasheet claims:

Quoted 7 lines

Because the loading of the LEDn_ON and LEDn_OFF registers is via the I 2 C-bus, and
asynchronous to the internal oscillator, we want to ensure that we do not see any visual
artifacts of changing the ON and OFF values. This is achieved by updating the changes at
the end of the LOW cycle.

My interpretation is that the hardware will complete its period before applying the new settings. I might check with a scope tomorrow-ish.

Quoted 3 lines

- Is this racy somehow (i.e. can it happen that when going from duty_cycle/period = 1000/5000 to duty_cycle/period = 4000/10000 the output is 1000/10000 (or 4000/5000) for one cycle)?

It currently is racy. It should be possible to fix that either by updating all 4 registers in a single I2C write, or by using the "update on ACK" mode which requires all 4 registers to be updated before the new setting is applied (I'm not sure if this mode would require using a single I2C write as well though).

Quoted 2 lines

- Does the hardware complete the currently running period before .enabled = false is configured?

As my interpretation is that new settings are applied asynchronously, I assume that the final running period is completed after .enabled is set to false.

Quoted 3 lines

- How does the output pin behave on a disabled PWM. (Usual candidates
are: freeze where is just happens to be, constant inactive and High-Z).

Constant inactive. This is also the case when the chip is put into sleep mode. Note that the interpretation of "inactive" depends in the invert flag in the MODE2 register.

As it turns out, this driver is broken in yet another way I didn't find
before: For changing the global prescaler the chip needs to be put into sleep mode, but the driver doesn't follow the restart sequence described in the datasheet when waking it back up. In consequence, changing the period of one PWM does not only modify the period of all PWMs (which is bad enough, but can't be avoided with this hardware), but it also leaves all PWMs disabled...

As this hardware only has a single prescaler for all PWMs, should changing the period for individual PWMs even be allowed at all? Maybe only when all other PWMs are inactive?

I could imagine setting it in DTS instead (but I'm not sure what to do about non-OF users of this driver, for example when configured via ACPI).

Quoted 23 lines · Matthias Schiffer

Quoted 9 lines · Uwe Kleine-König

Hello Matthias,

as you seem to have this hardware on your desk, it would be great if
you
could answer the following questions:

- Does the hardware complete the currently running period before applying a new setting?

The datasheet claims:

Quoted 9 lines

Because the loading of the LEDn_ON and LEDn_OFF registers is via the
I 2 C-bus, and
asynchronous to the internal oscillator, we want to ensure that we do
not see any visual
artifacts of changing the ON and OFF values. This is achieved by updating the changes at
the end of the LOW cycle.

Quoted 3 lines

My interpretation is that the hardware will complete its period before
applying the new settings. I might check with a scope tomorrow-ish.

I agree given that you can update duty_cycle and period in a single write as you considered below. Maybe it is worth playing with small periods and a slow i2c bus speed (or hijack the bus by simulating a clock stretch).

Quoted 5 lines

Quoted 4 lines

- Is this racy somehow (i.e. can it happen that when going from duty_cycle/period = 1000/5000 to duty_cycle/period =
4000/10000 the
output is 1000/10000 (or 4000/5000) for one cycle)?

Quoted 8 lines

It currently is racy. It should be possible to fix that either by updating all 4 registers in a single I2C write, or by using the "update
on ACK" mode which requires all 4 registers to be updated before the
new setting is applied (I'm not sure if this mode would require using a
single I2C write as well though).

I can offer a second pair of eyeballs to interpret the datasheet. Will
take a look tomorrow.

Quoted 14 lines

Quoted 2 lines

- Does the hardware complete the currently running period before .enabled = false is configured?

As my interpretation is that new settings are applied asynchronously, I
assume that the final running period is completed after .enabled is set
to false.

Quoted 4 lines

- How does the output pin behave on a disabled PWM. (Usual
candidates
are: freeze where is just happens to be, constant inactive and High-Z).

Quoted 4 lines

Constant inactive. This is also the case when the chip is put into sleep mode. Note that the interpretation of "inactive" depends in the
invert flag in the MODE2 register.

This is optimal.

Quoted 12 lines

As it turns out, this driver is broken in yet another way I didn't find
before: For changing the global prescaler the chip needs to be put into
sleep mode, but the driver doesn't follow the restart sequence described in the datasheet when waking it back up. In consequence, changing the period of one PWM does not only modify the period of all
PWMs (which is bad enough, but can't be avoided with this hardware),
but it also leaves all PWMs disabled...

Quoted 4 lines

As this hardware only has a single prescaler for all PWMs, should changing the period for individual PWMs even be allowed at all? Maybe
only when all other PWMs are inactive?

yes, that is the general approach. Please document this in a
Limitiations: paragraph. See drivers/pwm/pwm-imx-tpm.c which has a similar problem.

Quoted 5 lines

I could imagine setting it in DTS instead (but I'm not sure what to do
about non-OF users of this driver, for example when configured via ACPI).

I don't like fixing the period in the device tree. This isn't a hardware
property and it is less flexible than possible.

Best regards
Uwe

As it turns out, this driver is broken in yet another way I didn't find
before: For changing the global prescaler the chip needs to be put into sleep mode, but the driver doesn't follow the restart sequence described in the datasheet when waking it back up. In consequence, changing the period of one PWM does not only modify the period of all PWMs (which is bad enough, but can't be avoided with this hardware), but it also leaves all PWMs disabled...

Quoted 7 lines · Matthias Schiffer

As it turns out, this driver is broken in yet another way I didn't find
before: For changing the global prescaler the chip needs to be put into sleep mode, but the driver doesn't follow the restart sequence described in the datasheet when waking it back up. In consequence, changing the period of one PWM does not only modify the period of all PWMs (which is bad enough, but can't be avoided with this hardware), but it also leaves all PWMs disabled...

I am unable to reproduce this: If I set a specific duty cycle on a channel and then change the period, the channel stays active. I can see the brightness of an LED decrease if I increase the period.

Quoted 10 lines · Clemens Gruber

Quoted 7 lines · Matthias Schiffer

As it turns out, this driver is broken in yet another way I didn't find
before: For changing the global prescaler the chip needs to be put into sleep mode, but the driver doesn't follow the restart sequence described in the datasheet when waking it back up. In consequence, changing the period of one PWM does not only modify the period of all PWMs (which is bad enough, but can't be avoided with this hardware), but it also leaves all PWMs disabled...

Quoted 3 lines

I am unable to reproduce this: If I set a specific duty cycle on a channel and then change the period, the channel stays active. I can see the brightness of an LED decrease if I increase the period.

What happens when pwm channels 0 and 1 are both enabled, and you change the pwm period of channel 0. Does channel 1 remain on?

As the user is setting the duty cycle in nanoseconds, it makes sense that the relative duty cycle decreases in an absolute period increase. As for the behavior that the other channels remain at the same relative duty cycle: Not sure how we can avoid this, other than reprogramming all 15 other channels if one of them is changed and that's not really acceptable, I think.

Quoted 7 lines · Clemens Gruber

As the user is setting the duty cycle in nanoseconds, it makes sense that the relative duty cycle decreases in an absolute period increase. As for the behavior that the other channels remain at the same relative duty cycle: Not sure how we can avoid this, other than reprogramming all 15 other channels if one of them is changed and that's not really acceptable, I think.

Thank you for the explanation, Clemens.

Yes, it does make sense that the relative duty cycle changes when we change the period. A relative duty cycle of duty_cycle / period is what the user would expect to see.

It also kind-of makes sense that the relative duty cycles of the other pwm channels
do not change: after all, the user is not touching them, so would not expect them to change.

However, the following does not make sense to me. Imagine pwm0 and pwm1 are both active and at 50%: period=5000000, duty_cycle=2500000. Then, change the period on pwm0:

$ echo 10000000 > pwm0/period

Then pwm0 gets dimmer (makes sense) and pwm1 keeps the same relative duty cycle (makes sense). However, if we now read out sysfs for pwm1, we get:

$ echo pwm1/period
5000000 (wrong!)
$ echo pwm1/duty_cycle
2500000 (wrong! although relative duty cycle is correct)

Although the pwm1 period has changed, the API calls do not reflect this. This makes it next to impossible for users to know what the current period is set to.

Moving to the atomic API won't help, because .get_state is called only once, when the chip is registered.

It does look like we have a square peg (this chip) in a round hole (the standard assumptions the pwm core makes) ?

Quoted 4 lines · Sven Van Asbroeck

It does look like we have a square peg (this chip) in a round hole (the
standard assumptions the pwm core makes) ?

There are other chips where a single period is shared across multiple PWM channels. Typically what we do there is once a period is configured
for a given channel, all subsequent PWM channel configurations must use
the same period, or otherwise the driver will return an error code.

See for example:

- stm32_pwm_config() in drivers/pwm/pwm-stm32.c
- lpc18xx_pwm_config() in drivers/pwm/pwm-lpc18xx-sct.c
- pwm_imx_tpm_apply_hw() in drivers/pwm/pwm-imx-tpm.c
- fsl_pwm_apply_config() in drivers/pwm/pwm-fsl-ftm.c

The rationale behind that is that we must not change a PWM configuration
without a consumer having explicitly requested it.

There are other chips where a single period is shared across multiple PWM channels. Typically what we do there is once a period is configured for a given channel, all subsequent PWM channel configurations must use the same period, or otherwise the driver will return an error code.

Quoted 5 lines

- Is this racy somehow (i.e. can it happen that when going from duty_cycle/period = 1000/5000 to duty_cycle/period = 4000/10000
the
output is 1000/10000 (or 4000/5000) for one cycle)?

It currently is racy. It should be possible to fix that either by updating all 4 registers in a single I2C write, or by using the "update on ACK" mode which requires all 4 registers to be updated before the new setting is applied (I'm not sure if this mode would require using a single I2C write as well though).

Quoted 7 lines · Matthias Schiffer

Quoted 6 lines

- Is this racy somehow (i.e. can it happen that when going from duty_cycle/period = 1000/5000 to duty_cycle/period =
4000/10000
the
output is 1000/10000 (or 4000/5000) for one cycle)?

Quoted 8 lines

It currently is racy. It should be possible to fix that either by updating all 4 registers in a single I2C write, or by using the "update
on ACK" mode which requires all 4 registers to be updated before the
new setting is applied (I'm not sure if this mode would require using a
single I2C write as well though).

Matthias, did you verify experimentally that changing the period is racy?

Looking at the datasheet and driver code, it shouldn't be. This is because
the OFF time is set as a proportion of the counter range. When the period
changes from 5000 to 10000, then 5000*20%/5000 and 10000*20%/10000 will result in the same 20% ratio (disregarding rounding errors).

This is documented at the beginning of the driver:

Should we move that comment to pwm_config(), so future versions of ourselves won't overlook it?

Quoted 29 lines · Sven Van Asbroeck

Quoted 8 lines · Matthias Schiffer

Quoted 7 lines

- Is this racy somehow (i.e. can it happen that when going
from
duty_cycle/period = 1000/5000 to duty_cycle/period =
4000/10000
the
output is 1000/10000 (or 4000/5000) for one cycle)?

Quoted 8 lines

It currently is racy. It should be possible to fix that either by updating all 4 registers in a single I2C write, or by using the "update
on ACK" mode which requires all 4 registers to be updated before the
new setting is applied (I'm not sure if this mode would require using a
single I2C write as well though).

Matthias, did you verify experimentally that changing the period is racy?

Looking at the datasheet and driver code, it shouldn't be. This is because
the OFF time is set as a proportion of the counter range. When the period
changes from 5000 to 10000, then 5000*20%/5000 and 10000*20%/10000 will result in the same 20% ratio (disregarding rounding errors).

Quoted 2 lines

This is documented at the beginning of the driver:

Quoted 3 lines

Should we move that comment to pwm_config(), so future versions of ourselves won't overlook it?

You are right, this results in the same ratio - the absolute on/off times may be wrong for a moment though when the period is changed.

In the attached image, I have changed the period, but kept the absolute
duty cycle fixed (note: this is in inverted mode, so the duty cycle controls the low time). It can be seen that after a too long high time
(chip is in sleep mode) one period with too long low time follows (new
period, old relative duty cycle), before the counter is reprogrammed to
match the previous absolute duty cycle.

I don't care too much about the details of the behaviour, as I only control LEDs using this chip and don't need to change the period after
initial setup, but we should accurately document the shortcomings of the hardware and the driver (when we have decided how to fix some of the driver issues).

Matthias

duty_cycle was only set, never read.

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 4 ----
 1 file changed, 4 deletions(-)

Quoted 6 lines · Matthias Schiffer

duty_cycle was only set, never read.

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 4 ----
 1 file changed, 4 deletions(-)

Applied, thanks.

Quoted 4 lines · Thierry Reding

Quoted 2 lines · Matthias Schiffer

duty_cycle was only set, never read.

Quoted 6 lines

Quoted 4 lines

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 4 ----
 1 file changed, 4 deletions(-)

Applied, thanks.

I'm not sure this patch is correct.

We already have broken GPIO in this driver. Do we need more breakage?

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Thierry Reding

Quoted 2 lines · Matthias Schiffer

duty_cycle was only set, never read.

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 4 ----
 1 file changed, 4 deletions(-)

Applied, thanks.

I'm not sure this patch is correct.

What makes you say that? If you look at the code, the driver sets this field to either 0 or some duty cycle value but ends up never using it. Why would it be wrong to remove that code?

Quoted 1 line

We already have broken GPIO in this driver. Do we need more breakage?

My understanding is that nobody was able to pinpoint exactly when this regressed, or if this only worked by accident to begin with. It sounds like Clemens has a way of testing this driver, so perhaps we can solve that GPIO issue while we're at it.

The last discussion on this seems to have been around the time when you posted a fix for it:

    https://patchwork.ozlabs.org/patch/1156012/

But then Sven had concerns that that also wasn't guaranteed to work:

    https://lkml.org/lkml/2019/6/2/73

So I think we could either apply your patch to restore the old behaviour which I assume you tested, so at least it seems to work in practice, even if there's still a potential race that Sven pointed out in the above link.

I'd prefer something alternative because it's obviously confusing and completely undocumented. Mika had already proposed something that's a little bit better, though still somewhat confusing.

Oh... actually reading further through those threads there seems to be a patch from Sven that was reviewed by Mika but then nothing happened:

	https://lkml.org/lkml/2019/6/4/1039

with the corresponding patchwork URL:

	https://patchwork.ozlabs.org/patch/1110083/

Andy, Clemens, do you have a way of testing the GPIO functionality of this driver? If so, it'd be great if you could check the above patch from Sven to fix PWM/GPIO interop.

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Thierry Reding

Quoted 2 lines · Matthias Schiffer

duty_cycle was only set, never read.

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 4 ----
 1 file changed, 4 deletions(-)

Applied, thanks.

I'm not sure this patch is correct.

What makes you say that? If you look at the code, the driver sets this field to either 0 or some duty cycle value but ends up never using it. Why would it be wrong to remove that code?

Quoted 1 line

We already have broken GPIO in this driver. Do we need more breakage?

My understanding is that nobody was able to pinpoint exactly when this regressed, or if this only worked by accident to begin with. It sounds like Clemens has a way of testing this driver, so perhaps we can solve that GPIO issue while we're at it.

The last discussion on this seems to have been around the time when you posted a fix for it:

    https://patchwork.ozlabs.org/patch/1156012/

But then Sven had concerns that that also wasn't guaranteed to work:

    https://lkml.org/lkml/2019/6/2/73

So I think we could either apply your patch to restore the old behaviour which I assume you tested, so at least it seems to work in practice, even if there's still a potential race that Sven pointed out in the above link.

I'd prefer something alternative because it's obviously confusing and completely undocumented. Mika had already proposed something that's a little bit better, though still somewhat confusing.

Oh... actually reading further through those threads there seems to be a patch from Sven that was reviewed by Mika but then nothing happened:

	https://lkml.org/lkml/2019/6/4/1039

with the corresponding patchwork URL:

	https://patchwork.ozlabs.org/patch/1110083/

Andy, Clemens, do you have a way of testing the GPIO functionality of this driver? If so, it'd be great if you could check the above patch from Sven to fix PWM/GPIO interop.

Quoted 53 lines · Thierry Reding

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Thierry Reding

Quoted 2 lines · Matthias Schiffer

duty_cycle was only set, never read.

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Signed-off-by Matthias Schiffer <[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 4 ----
 1 file changed, 4 deletions(-)

Applied, thanks.

I'm not sure this patch is correct.

What makes you say that? If you look at the code, the driver sets this field to either 0 or some duty cycle value but ends up never using it. Why would it be wrong to remove that code?

Quoted 1 line

We already have broken GPIO in this driver. Do we need more breakage?

My understanding is that nobody was able to pinpoint exactly when this regressed, or if this only worked by accident to begin with. It sounds like Clemens has a way of testing this driver, so perhaps we can solve that GPIO issue while we're at it.

The last discussion on this seems to have been around the time when you posted a fix for it:

    https://patchwork.ozlabs.org/patch/1156012/

But then Sven had concerns that that also wasn't guaranteed to work:

    https://lkml.org/lkml/2019/6/2/73

So I think we could either apply your patch to restore the old behaviour which I assume you tested, so at least it seems to work in practice, even if there's still a potential race that Sven pointed out in the above link.

I'd prefer something alternative because it's obviously confusing and completely undocumented. Mika had already proposed something that's a little bit better, though still somewhat confusing.

Oh... actually reading further through those threads there seems to be a patch from Sven that was reviewed by Mika but then nothing happened:

	https://lkml.org/lkml/2019/6/4/1039

with the corresponding patchwork URL:

	https://patchwork.ozlabs.org/patch/1110083/

Quoted 3 lines

Andy, Clemens, do you have a way of testing the GPIO functionality of this driver? If so, it'd be great if you could check the above patch from Sven to fix PWM/GPIO interop.

Looks good. Tested it today and I can no longer reproduce the issues when switching between PWM and GPIO modes.
It did not apply cleanly on the current mainline or for-next branch, so I'll send a fixed up version of the patch with my Tested-by tag shortly.

I noticed an unrelated issue when disabling and enabling the channel though, for which I will either send a patch or maybe try to convert the driver to the atomic API first and then look if it is still a problem. (Issue is that if you disable the channel, the LED_OFF counter is cleared, which means you have to reconfigure the duty cycle after reenabling. It's probably better if only the FULL_OFF bit is toggled in enable/disable as it has precedence over the others anyway and then the previous state would not be changed..?)

* PGP Signed by an unknown key

Quoted 78 lines · Clemens Gruber

Quoted 69 lines · Thierry Reding

Quoted 13 lines · Andy Shevchenko

Quoted 11 lines · Thierry Reding

Quoted 7 lines · Matthias Schiffer

duty_cycle was only set, never read.

Signed-off-by: Matthias Schiffer <
[email protected]>

---
 drivers/pwm/pwm-pca9685.c | 4 ----
 1 file changed, 4 deletions(-)

Quoted 3 lines

Quoted 1 line

Applied, thanks.

I'm not sure this patch is correct.

What makes you say that? If you look at the code, the driver sets this
field to either 0 or some duty cycle value but ends up never using it.
Why would it be wrong to remove that code?

Quoted 2 lines

We already have broken GPIO in this driver. Do we need more breakage?

My understanding is that nobody was able to pinpoint exactly when this
regressed, or if this only worked by accident to begin with. It sounds
like Clemens has a way of testing this driver, so perhaps we can solve
that GPIO issue while we're at it.

The last discussion on this seems to have been around the time when you
posted a fix for it:

    https://patchwork.ozlabs.org/patch/1156012/

But then Sven had concerns that that also wasn't guaranteed to work:

    https://lkml.org/lkml/2019/6/2/73

So I think we could either apply your patch to restore the old behaviour
which I assume you tested, so at least it seems to work in practice,
even if there's still a potential race that Sven pointed out in the
above link.

I'd prefer something alternative because it's obviously confusing and
completely undocumented. Mika had already proposed something that's a
little bit better, though still somewhat confusing.

Oh... actually reading further through those threads there seems to be a
patch from Sven that was reviewed by Mika but then nothing happened:

	https://lkml.org/lkml/2019/6/4/1039

with the corresponding patchwork URL:

Quoted 7 lines

	https://patchwork.ozlabs.org/patch/1110083/

Andy, Clemens, do you have a way of testing the GPIO functionality of
this driver? If so, it'd be great if you could check the above patch
from Sven to fix PWM/GPIO interop.

Quoted 7 lines

Looks good. Tested it today and I can no longer reproduce the issues
when switching between PWM and GPIO modes.
It did not apply cleanly on the current mainline or for-next branch, so
I'll send a fixed up version of the patch with my Tested-by tag shortly.

Awesome, thank you!

Quoted 5 lines

I noticed an unrelated issue when disabling and enabling the channel
though, for which I will either send a patch or maybe try to convert the
driver to the atomic API first and then look if it is still a

Quoted 8 lines

problem.
(Issue is that if you disable the channel, the LED_OFF counter is cleared, which means you have to reconfigure the duty cycle after reenabling. It's probably better if only the FULL_OFF bit is toggled in
enable/disable as it has precedence over the others anyway and then the
previous state would not be changed..?)

Converting to the atomic API would certainly be beneficial because it gives you much more control over what you want to program to hardware.

Other than that, yes, if ->enable() called after ->disable() doesn't put
the PWM into the same state that it was before ->disable(), then that'd
be a bug. From what you're saying this will make the driver only work if
there's a ->config() call after ->disable(). That's wrong.

So yes, setting only that LED_FULL bit in ->disable() and clearing it in
->enable() sounds like a better option than the current implementation.

Thierry

* Unknown Key
* 0x7F3EB3A1