[PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Matthias Schiffer]

duty_cycle was only set, never read.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/pwm/pwm-pca9685.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index b07bdca3d510..19ac97108a64 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -69,7 +69,6 @@
 struct pca9685 {
 	struct pwm_chip chip;
 	struct regmap *regmap;
-	int duty_ns;
 	int period_ns;
 #if IS_ENABLED(CONFIG_GPIOLIB)
 	struct mutex lock;
@@ -272,8 +271,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		}
 	}
 
-	pca->duty_ns = duty_ns;
-
 	if (duty_ns < 1) {
 		if (pwm->hwpwm >= PCA9685_MAXCHAN)
 			reg = PCA9685_ALL_LED_OFF_H;
@@ -449,7 +446,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 			ret);
 		return ret;
 	}
-	pca->duty_ns = 0;
 	pca->period_ns = PCA9685_DEFAULT_PERIOD;
 
 	i2c_set_clientdata(client, pca);
-- 
2.17.1

[PATCH 2/4] pwm: pca9685: remove ALL_LED PWM channel

[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 <matthias.schiffer@ew.tq-group.com>
---
 drivers/pwm/pwm-pca9685.c | 115 ++++++--------------------------------
 1 file changed, 17 insertions(+), 98 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 19ac97108a64..393ab92aa945 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -105,27 +105,12 @@ static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset)
 
 static bool pca9685_pwm_is_gpio(struct pca9685 *pca, struct pwm_device *pwm)
 {
-	bool is_gpio = false;
+	bool is_gpio;
 
 	mutex_lock(&pca->lock);
-
-	if (pwm->hwpwm >= PCA9685_MAXCHAN) {
-		unsigned int i;
-
-		/*
-		 * Check if any of the GPIOs are requested and in that case
-		 * prevent using the "all LEDs" channel.
-		 */
-		for (i = 0; i < pca->gpio.ngpio; i++)
-			if (gpiochip_is_requested(&pca->gpio, i)) {
-				is_gpio = true;
-				break;
-			}
-	} else if (pwm_get_chip_data(pwm)) {
-		is_gpio = true;
-	}
-
+	is_gpio = !!pwm_get_chip_data(pwm);
 	mutex_unlock(&pca->lock);
+
 	return is_gpio;
 }
 
@@ -239,7 +224,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct pca9685 *pca = to_pca(chip);
 	unsigned long long duty;
-	unsigned int reg;
 	int prescale;
 
 	if (period_ns != pca->period_ns) {
@@ -272,39 +256,18 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 	if (duty_ns < 1) {
-		if (pwm->hwpwm >= PCA9685_MAXCHAN)
-			reg = PCA9685_ALL_LED_OFF_H;
-		else
-			reg = LED_N_OFF_H(pwm->hwpwm);
-
-		regmap_write(pca->regmap, reg, LED_FULL);
+		regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), LED_FULL);
 
 		return 0;
 	}
 
 	if (duty_ns == period_ns) {
 		/* Clear both OFF registers */
-		if (pwm->hwpwm >= PCA9685_MAXCHAN)
-			reg = PCA9685_ALL_LED_OFF_L;
-		else
-			reg = LED_N_OFF_L(pwm->hwpwm);
-
-		regmap_write(pca->regmap, reg, 0x0);
-
-		if (pwm->hwpwm >= PCA9685_MAXCHAN)
-			reg = PCA9685_ALL_LED_OFF_H;
-		else
-			reg = LED_N_OFF_H(pwm->hwpwm);
-
-		regmap_write(pca->regmap, reg, 0x0);
+		regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0x0);
+		regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0x0);
 
 		/* Set the full ON bit */
-		if (pwm->hwpwm >= PCA9685_MAXCHAN)
-			reg = PCA9685_ALL_LED_ON_H;
-		else
-			reg = LED_N_ON_H(pwm->hwpwm);
-
-		regmap_write(pca->regmap, reg, LED_FULL);
+		regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), LED_FULL);
 
 		return 0;
 	}
@@ -312,27 +275,12 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
 	duty = DIV_ROUND_UP_ULL(duty, period_ns);
 
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_OFF_L;
-	else
-		reg = LED_N_OFF_L(pwm->hwpwm);
-
-	regmap_write(pca->regmap, reg, (int)duty & 0xff);
-
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_OFF_H;
-	else
-		reg = LED_N_OFF_H(pwm->hwpwm);
-
-	regmap_write(pca->regmap, reg, ((int)duty >> 8) & 0xf);
+	regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), (int)duty & 0xff);
+	regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm),
+		     ((int)duty >> 8) & 0xf);
 
 	/* Clear the full ON bit, otherwise the set OFF time has no effect */
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_ON_H;
-	else
-		reg = LED_N_ON_H(pwm->hwpwm);
-
-	regmap_write(pca->regmap, reg, 0);
+	regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), 0);
 
 	return 0;
 }
@@ -340,36 +288,19 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct pca9685 *pca = to_pca(chip);
-	unsigned int reg;
 
 	/*
 	 * The PWM subsystem does not support a pre-delay.
 	 * So, set the ON-timeout to 0
 	 */
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_ON_L;
-	else
-		reg = LED_N_ON_L(pwm->hwpwm);
-
-	regmap_write(pca->regmap, reg, 0);
-
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_ON_H;
-	else
-		reg = LED_N_ON_H(pwm->hwpwm);
-
-	regmap_write(pca->regmap, reg, 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.
 	 */
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_OFF_H;
-	else
-		reg = LED_N_OFF_H(pwm->hwpwm);
-
-	regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
+	regmap_update_bits(pca->regmap, LED_N_OFF_H(pwm->hwpwm), LED_FULL, 0x0);
 
 	return 0;
 }
@@ -377,22 +308,11 @@ static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct pca9685 *pca = to_pca(chip);
-	unsigned int reg;
-
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_OFF_H;
-	else
-		reg = LED_N_OFF_H(pwm->hwpwm);
 
-	regmap_write(pca->regmap, reg, LED_FULL);
+	regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), LED_FULL);
 
 	/* Clear the LED_OFF counter. */
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_OFF_L;
-	else
-		reg = LED_N_OFF_L(pwm->hwpwm);
-
-	regmap_write(pca->regmap, reg, 0x0);
+	regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0x0);
 }
 
 static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -469,8 +389,7 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
 
 	pca->chip.ops = &pca9685_pwm_ops;
-	/* add an extra channel for ALL_LED */
-	pca->chip.npwm = PCA9685_MAXCHAN + 1;
+	pca->chip.npwm = PCA9685_MAXCHAN;
 
 	pca->chip.dev = &client->dev;
 	pca->chip.base = -1;
-- 
2.17.1

Re: [PATCH 2/4] pwm: pca9685: remove ALL_LED PWM channel

[Thierry Reding]

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

On Wed, Feb 26, 2020 at 02:52:27PM +0100, Matthias Schiffer wrote:
> 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 <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/pwm/pwm-pca9685.c | 115 ++++++--------------------------------
>  1 file changed, 17 insertions(+), 98 deletions(-)

Applied, thanks.

Thierry

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

Re: [PATCH 2/4] pwm: pca9685: remove ALL_LED PWM channel

[Thierry Reding]

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

On Mon, Mar 30, 2020 at 03:07:57PM +0200, Thierry Reding wrote:
> On Wed, Feb 26, 2020 at 02:52:27PM +0100, Matthias Schiffer wrote:
> > 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 <matthias.schiffer@ew.tq-group.com>
> > ---
> >  drivers/pwm/pwm-pca9685.c | 115 ++++++--------------------------------
> >  1 file changed, 17 insertions(+), 98 deletions(-)
> 
> Applied, thanks.

By the way, shouldn't we add something like this:

--- >8 ---
diff --git a/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt b/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt
index f21b55c95738..49fff008af09 100644
--- a/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt
@@ -5,8 +5,6 @@ Required properties:
   - compatible: "nxp,pca9685-pwm"
   - #pwm-cells: Should be 2. See pwm.yaml in this directory for a description of
     the cells format.
-    The index 16 is the ALLCALL channel, that sets all PWM channels at the same
-    time.

 Optional properties:
   - invert (bool): boolean to enable inverted logic
--- >8 ---

To make sure we reflect this in the device tree bindings? It doesn't
seem like anyone uses that channel (in fact, it doesn't seem like any
device trees actually exist in-tree that use one of these chips), so it
should be fine to drop that.

Thierry

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

Re: [PATCH 2/4] pwm: pca9685: remove ALL_LED PWM channel

[Andy Shevchenko]

On Mon, Mar 30, 2020 at 4:10 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Wed, Feb 26, 2020 at 02:52:27PM +0100, Matthias Schiffer wrote:
> > 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 <matthias.schiffer@ew.tq-group.com>
> > ---
> >  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?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] pwm: pca9685: remove ALL_LED PWM channel

[Thierry Reding]

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

On Mon, Mar 30, 2020 at 04:19:55PM +0300, Andy Shevchenko wrote:
> On Mon, Mar 30, 2020 at 4:10 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Wed, Feb 26, 2020 at 02:52:27PM +0100, Matthias Schiffer wrote:
> > > 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 <matthias.schiffer@ew.tq-group.com>
> > > ---
> > >  drivers/pwm/pwm-pca9685.c | 115 ++++++--------------------------------
> > >  1 file changed, 17 insertions(+), 98 deletions(-)
> >
> > Applied, thanks.
> 
> This seems to be ABI breakage.

Nothing seemed to be using that feature. As a matter of fact, I don't
really see anyone using this chip in an upstream kernel, so it's
difficult to understand what the dependencies are.

> Thierry, do you have hardware to test?

No, I don't. Do you?

Does anybody have a good understanding of where the users of this are
and where the code is? There seems to be a lot of confusion around this
driver/chip and how it's being used and it keeps causing a lot of this
back and forth, so can we please gather some basic information and then
I'll add that to the driver along with perhaps a list of contacts that
want to be involved.

As it is, it seems like applying patches as I see fit is the only way to
get people to object, which is a suboptimal way to do things. =)

Thierry

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

Re: [PATCH 2/4] pwm: pca9685: remove ALL_LED PWM channel

[Clemens Gruber]

Hi,

On Mon, Mar 30, 2020 at 03:07:57PM +0200, Thierry Reding wrote:
> On Wed, Feb 26, 2020 at 02:52:27PM +0100, Matthias Schiffer wrote:
> > 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 <matthias.schiffer@ew.tq-group.com>
> > ---
> >  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.

Best regards,
Clemens

Re: [PATCH 2/4] pwm: pca9685: remove ALL_LED PWM channel

[Thierry Reding]

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

On Mon, Mar 30, 2020 at 03:34:50PM +0200, Clemens Gruber wrote:
> Hi,
> 
> On Mon, Mar 30, 2020 at 03:07:57PM +0200, Thierry Reding wrote:
> > On Wed, Feb 26, 2020 at 02:52:27PM +0100, Matthias Schiffer wrote:
> > > 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 <matthias.schiffer@ew.tq-group.com>
> > > ---
> > >  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.

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?

Thierry

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

Re: [PATCH 2/4] pwm: pca9685: remove ALL_LED PWM channel

[Thierry Reding]

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

On Mon, Mar 30, 2020 at 05:40:36PM +0200, Thierry Reding wrote:
> On Mon, Mar 30, 2020 at 03:34:50PM +0200, Clemens Gruber wrote:
> > Hi,
> > 
> > On Mon, Mar 30, 2020 at 03:07:57PM +0200, Thierry Reding wrote:
> > > On Wed, Feb 26, 2020 at 02:52:27PM +0100, Matthias Schiffer wrote:
> > > > 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 <matthias.schiffer@ew.tq-group.com>
> > > > ---
> > > >  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.
> 
> 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?

Nevermind, mixed up the series. I ended up applying only patches 1 and 2
from this because Uwe had some concerns about patches 3 and 4. So no
need to test those until Matthias has fixed them up.

Matthias, I'll keep patch 1 of this applied, but as you noticed, this
ALL_LED features is indeed used, so you can drop then when you resend
the series.

Thierry

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

Re: [PATCH 2/4] pwm: pca9685: remove ALL_LED PWM channel

[Clemens Gruber]

On Mon, Mar 30, 2020 at 05:40:36PM +0200, Thierry Reding wrote:
> On Mon, Mar 30, 2020 at 03:34:50PM +0200, Clemens Gruber wrote:
> > Hi,
> > 
> > On Mon, Mar 30, 2020 at 03:07:57PM +0200, Thierry Reding wrote:
> > > On Wed, Feb 26, 2020 at 02:52:27PM +0100, Matthias Schiffer wrote:
> > > > 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 <matthias.schiffer@ew.tq-group.com>
> > > > ---
> > > >  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.
> 
> 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.

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

Re: (EXT) Re: [PATCH 2/4] pwm: pca9685: remove ALL_LED PWM channel

[Matthias Schiffer]

On Mon, 2020-03-30 at 18:07 +0200, Clemens Gruber wrote:
> On Mon, Mar 30, 2020 at 05:40:36PM +0200, Thierry Reding wrote:
> > On Mon, Mar 30, 2020 at 03:34:50PM +0200, Clemens Gruber wrote:
> > > Hi,
> > > 
> > > On Mon, Mar 30, 2020 at 03:07:57PM +0200, Thierry Reding wrote:
> > > > On Wed, Feb 26, 2020 at 02:52:27PM +0100, Matthias Schiffer
> > > > wrote:
> > > > > 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 <
> > > > > matthias.schiffer@ew.tq-group.com>
> > > > > ---
> > > > >  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.
> > 
> > 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.
> 
> > 
> > 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

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.

Matthias

Re: (EXT) Re: [PATCH 2/4] pwm: pca9685: remove ALL_LED PWM channel

[Clemens Gruber]

On Tue, Mar 31, 2020 at 02:09:37PM +0200, Matthias Schiffer wrote:
> On Mon, 2020-03-30 at 18:07 +0200, Clemens Gruber wrote:
> > On Mon, Mar 30, 2020 at 05:40:36PM +0200, Thierry Reding wrote:
> > > On Mon, Mar 30, 2020 at 03:34:50PM +0200, Clemens Gruber wrote:
> > > > Hi,
> > > > 
> > > > On Mon, Mar 30, 2020 at 03:07:57PM +0200, Thierry Reding wrote:
> > > > > On Wed, Feb 26, 2020 at 02:52:27PM +0100, Matthias Schiffer
> > > > > wrote:
> > > > > > 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 <
> > > > > > matthias.schiffer@ew.tq-group.com>
> > > > > > ---
> > > > > >  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.
> > > 
> > > 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.
> > 
> > > 
> > > 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
> 
> 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...

I agree that it would be a good idea to make this exclusive. This change
would at least not break our application, because we unexport the
ALL_LED channel before requesting an individual channel.
Not sure about other users, but using both individual and ALL channels
at the same time is probably not a reasonable/sane usecase..

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

According to the datashet (7.3.4) the individual registers are filled if
the ALL_LED channel is used. However, in .disable, the OFF registers are
reset to 0. (And the ON registers are not used, except for the FULL_ON
bit)
So there should not be any side effects, as long as the access to the
ALL_LED channel is made exclusive and the user has to free it before he
can request individual channels.

Another quirk is the same prescaler/period for all channels. But I am
not sure what we can do about that. Applications might already depend on
the fact that the last set period wins.

Clemens

[PATCH 3/4] pwm: pca9685: initialize all LED registers during probe

[Matthias Schiffer]

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

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/pwm/pwm-pca9685.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 393ab92aa945..370691b21107 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -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.
@@ -388,6 +381,13 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 	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);
+
 	pca->chip.ops = &pca9685_pwm_ops;
 	pca->chip.npwm = PCA9685_MAXCHAN;
 
-- 
2.17.1

Re: [PATCH 3/4] pwm: pca9685: initialize all LED registers during probe

[Uwe Kleine-König]

On Wed, Feb 26, 2020 at 02:52:28PM +0100, Matthias Schiffer wrote:
> Initialize all ON delays to 0 during probe, rather than doing it in
> pca9685_pwm_enable.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/pwm/pwm-pca9685.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 393ab92aa945..370691b21107 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -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.
> @@ -388,6 +381,13 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>  	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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: (EXT) Re: [PATCH 3/4] pwm: pca9685: initialize all LED registers during probe

[Matthias Schiffer]

On Wed, 2020-02-26 at 16:00 +0100, Uwe Kleine-König wrote:
> On Wed, Feb 26, 2020 at 02:52:28PM +0100, Matthias Schiffer wrote:
> > Initialize all ON delays to 0 during probe, rather than doing it in
> > pca9685_pwm_enable.
> > 
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com
> > >
> > ---
> >  drivers/pwm/pwm-pca9685.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 393ab92aa945..370691b21107 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -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.
> > @@ -388,6 +381,13 @@ static int pca9685_pwm_probe(struct i2c_client
> > *client,
> >  	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.
> 

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.

Matthias


> Best regards
> Uwe
> 

Re: (EXT) Re: [PATCH 3/4] pwm: pca9685: initialize all LED registers during probe

[Thierry Reding]

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

On Wed, Feb 26, 2020 at 05:13:51PM +0100, Matthias Schiffer wrote:
> On Wed, 2020-02-26 at 16:00 +0100, Uwe Kleine-König wrote:
> > On Wed, Feb 26, 2020 at 02:52:28PM +0100, Matthias Schiffer wrote:
> > > Initialize all ON delays to 0 during probe, rather than doing it in
> > > pca9685_pwm_enable.
> > > 
> > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com
> > > >
> > > ---
> > >  drivers/pwm/pwm-pca9685.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > index 393ab92aa945..370691b21107 100644
> > > --- a/drivers/pwm/pwm-pca9685.c
> > > +++ b/drivers/pwm/pwm-pca9685.c
> > > @@ -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.
> > > @@ -388,6 +381,13 @@ static int pca9685_pwm_probe(struct i2c_client
> > > *client,
> > >  	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.
> > 
> 
> 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.

I think what Uwe was suggesting was to delay setting these registers
until the PWM is actually used. We typically do that in order not to
mess with whatever the bootloader may have set up.

Consider for example the case where the PWM is used to drive a
backlight, which might be showing a splash screen. If you reinitialize
the PWM at probe time, the backlight might go dark some time before the
kernel gets around to initializing the backlight.

For now I think we should just leave this where it is because likely a
change in PWM will be followed by a pwm_enable() call with the legacy
API. Ideally this driver would be converted to the new atomic PWM API,
in which case it'd be best to write the entirety of the hardware
registers during ->apply().

Thierry

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

[PATCH 4/4] pwm: pca9685: migrate config/enable/disable to apply

[Matthias Schiffer]

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 <matthias.schiffer@ew.tq-group.com>
---
 drivers/pwm/pwm-pca9685.c | 53 +++++++++++----------------------------
 1 file changed, 14 insertions(+), 39 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 370691b21107..e266cbbd39bf 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -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),
@@ -285,29 +286,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
-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,
@@ -377,9 +352,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 
 	regmap_write(pca->regmap, PCA9685_MODE2, mode2);
 
-	/* clear all "full off" bits */
+	/* Set all LEDs full off */
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
-	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
+	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
 
 	/*
 	 * The PWM subsystem does not support a pre-delay.
-- 
2.17.1

Re: [PATCH 4/4] pwm: pca9685: migrate config/enable/disable to apply

[Uwe Kleine-König]

Hello,

On Wed, Feb 26, 2020 at 02:52:29PM +0100, Matthias Schiffer wrote:
> 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 <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/pwm/pwm-pca9685.c | 53 +++++++++++----------------------------
>  1 file changed, 14 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 370691b21107..e266cbbd39bf 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -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),
> @@ -285,29 +286,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return 0;

You seem to ignore state->polarity which is wrong.

>  }
>  
> -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,
> @@ -377,9 +352,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>  
>  	regmap_write(pca->regmap, PCA9685_MODE2, mode2);
>  
> -	/* clear all "full off" bits */
> +	/* Set all LEDs full off */
>  	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
> -	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
> +	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);

This looks wrong or at least unrelated?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Matthias Schiffer]

On Wed, 2020-02-26 at 16:10 +0100, Uwe Kleine-König wrote:
> 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:

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


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


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

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

Regards,
Matthias



> 
> Best regards
> Uwe
> 

Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Uwe Kleine-König]

On Wed, Feb 26, 2020 at 06:03:02PM +0100, Matthias Schiffer wrote:
> On Wed, 2020-02-26 at 16:10 +0100, Uwe Kleine-König wrote:
> > 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:
> 
> > 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.

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

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

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

This is optimal.

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

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: (EXT) Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Matthias Schiffer]

On Wed, 2020-02-26 at 20:21 +0100, Uwe Kleine-König wrote:
> On Wed, Feb 26, 2020 at 06:03:02PM +0100, Matthias Schiffer wrote:
> > On Wed, 2020-02-26 at 16:10 +0100, Uwe Kleine-König wrote:
> > > 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:
> > 
> > > 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.
> 
> 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).
>  
> > >  - 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).
> 
> I can offer a second pair of eyeballs to interpret the datasheet.
> Will
> take a look tomorrow.
> 
> > >  - 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.
> > 
> > >  - 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.
> 
> This is optimal.
> 
> > 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?
> 
> 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.

This raises the question what to do about the GPIO mode supported by
the driver: While the period does not affect GPIO usage of PWMs,
changing the period would put the chip in sleep mode and thus briefly
disable active GPIOs. I assume that this should preclude changing the
period when there are any PWMs requsted as GPIOs, but now the order in
which things are initialized becomes crucial:

- All references to PWMs of the same PCA9685 must specify the same
period
- When requesting a PWM as GPIO, no period can be specified
=> When a PWM referenced as GPIO is requested before the first actual
PWM, setting the correct period becomes impossible.

I can't think of a nice solution that doesn't require serious rework -
maybe we need at least an optional period property in DTS to support
this case? This could either be implemented as a default period or a
fixed period.

A more elaborate solution could be to remove the GPIO code from PCA9685
and implement a generic GPIO-PWM driver instead that could be
configured in DTS (again, I have no idea how to support non-DTS
platforms). Unfortunately, I assume I won't have time to realize such a
solution myself.

Matthias


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

Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Clemens Gruber]

Hi,

On Wed, Feb 26, 2020 at 06:03:02PM +0100, Matthias Schiffer wrote:
> 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.

This is expected, because after the SLEEP bit is set, we wait for
500usecs and then write to the LED ON/OFF registers.
This leaves the channel enabled with the new period (but with old
duty_ns value => different ratio)

A few years ago, I played around with the idea of remembering the
duty_ns to period_ns ratio and setting it accordingly after a period
change, possibly also with a shortcut of setting the RESTART bit if the
ratio did not change. Maybe after the switch to the atomic API, this
would be a nice improvement.

Best regards,
Clemens

Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Sven Van Asbroeck]

On Mon, Mar 30, 2020 at 11:12 AM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> On Wed, Feb 26, 2020 at 06:03:02PM +0100, Matthias Schiffer wrote:
> > 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.

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?

Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Clemens Gruber]

Hi,

On Fri, Apr 03, 2020 at 07:50:07PM -0400, Sven Van Asbroeck wrote:
> On Mon, Mar 30, 2020 at 11:12 AM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > On Wed, Feb 26, 2020 at 06:03:02PM +0100, Matthias Schiffer wrote:
> > > 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.
> 
> 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?

Yes. Both channels remain on.

Let's say I configure a period of 5ms for both channels 0 and 1, as well
as a duty cycle of 4ms, meaning a relative duty cycle of 80%.
If I then increase the period of channel 0 to 10ms, there will be a
relative duty cycle of 40% on channel 0, but channel 1 will remain at a
relative duty cycle of 80%.
This is due to the relative nature of the internal ON/OFF times. For
the channel with the period change however, we recalculate the duty_ns
to period_ns ratio and reprogram the ON/OFF registers, because the user
might have already given us a different duty cycle in .config / .apply.

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.

Clemens

Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Sven Van Asbroeck]

On Sat, Apr 4, 2020 at 1:35 PM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> 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) ?

Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Thierry Reding]

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

On Sat, Apr 04, 2020 at 04:17:00PM -0400, Sven Van Asbroeck wrote:
> On Sat, Apr 4, 2020 at 1:35 PM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > 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.

The .get_state() callback is actually called when the PWM is requested,
which could be much later than when the chip is registered. That doesn't
change the fact that it would be useless for this case, though.

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

It seems like PCA9685 is somewhere halfway between in that it will
actually keep the same duty-cycle/period ratio, but that doesn't mean it
is automatically okay to do this. The problem is that the duty-cycle to
period ratio is only relevant in some cases. If all you care about is
the power output of the PWM signal, which admittedly seems to be about
95% of all cases, then yes, this behaviour would be okay. But what if we
have a consumer that relies on a particular width of the PWM pulse in
absolute terms rather than relative to the period?

Thierry

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

Re: (EXT) Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Matthias Schiffer]

On Mon, 2020-04-06 at 11:51 +0200, Thierry Reding wrote:
> 
> On Sat, Apr 04, 2020 at 04:17:00PM -0400, Sven Van Asbroeck wrote:
> > 
> > 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.

These implementations don't deal with the issue in a consistent way
either though:

1) stm32_pwm_config() only checks for channels that are actually
enabled, regardless if they're requested

2) lpc18xx_pwm_config() checks requested channels instead

"2)" seems more correct to me, as the parameters of a requested channel
can't be changed by another user, but it seems to prevent certain
sequences. I don't have a good grasp of the the usual PWM request
control flow - is it correct that the PWM state is not updated with the
PWM args from OF when the PWM is requested? I only see
pwm_adjust_config() doing something like that, which is not used in
many places (and nothing preventing races) - but I might be overlooking
something.

Meaning, is it possible that two drivers request PWMs from the same
pwmchip of type "2)" (or even a single driver using two PWMs) without
configuring them right away may get into a situation where neither can
set up a period at all even when all users want to use the same period?

Matthias

Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Sven Van Asbroeck]

On Mon, Apr 6, 2020 at 5:51 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> 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.

Thank you for the clarification Thierry. Do you think this method would be
appropriate for this chip?

Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Sven Van Asbroeck]

On Wed, Feb 26, 2020 at 12:04 PM Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:
>
> >  - 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).

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:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-pca9685.c?h=v5.6#n25

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

Re: (EXT) Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Matthias Schiffer]

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

On Fri, 2020-04-03 at 19:47 -0400, Sven Van Asbroeck wrote:
> On Wed, Feb 26, 2020 at 12:04 PM Matthias Schiffer
> <matthias.schiffer@ew.tq-group.com> wrote:
> > 
> > >  - 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).
> 
> 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:
> 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-pca9685.c?h=v5.6#n25
> 
> 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

[-- Attachment #2: osc.jpg --]
[-- Type: image/jpeg, Size: 51521 bytes --]

Re: (EXT) Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Matthias Schiffer]

On Tue, 2020-04-07 at 16:46 +0200, Matthias Schiffer wrote:
> On Fri, 2020-04-03 at 19:47 -0400, Sven Van Asbroeck wrote:
> > On Wed, Feb 26, 2020 at 12:04 PM Matthias Schiffer
> > <matthias.schiffer@ew.tq-group.com> wrote:
> > > 
> > > >  - 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).
> > 
> > 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:
> > 
> 
> 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-pca9685.c?h=v5.6#n25
> > 
> > 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

And another kind of race condition that should be possible, although I
haven't seen it in practice:

High and low bits of the OFF counter currently aren't programmed
atomically, so with unlucky timing we get a cycle using new lower 8
bits with old upper 4 bits of the duty cycle.

Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Thierry Reding]

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

On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer wrote:
> duty_cycle was only set, never read.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/pwm/pwm-pca9685.c | 4 ----
>  1 file changed, 4 deletions(-)

Applied, thanks.

Thierry

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

Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Andy Shevchenko]

On Mon, Mar 30, 2020 at 4:09 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer wrote:
> > duty_cycle was only set, never read.
> >
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> >  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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Thierry Reding]

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

On Mon, Mar 30, 2020 at 04:18:22PM +0300, Andy Shevchenko wrote:
> On Mon, Mar 30, 2020 at 4:09 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer wrote:
> > > duty_cycle was only set, never read.
> > >
> > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > ---
> > >  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?

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

Thierry

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

Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Clemens Gruber]

On Mon, Mar 30, 2020 at 06:02:38PM +0200, Thierry Reding wrote:
> On Mon, Mar 30, 2020 at 04:18:22PM +0300, Andy Shevchenko wrote:
> > On Mon, Mar 30, 2020 at 4:09 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer wrote:
> > > > duty_cycle was only set, never read.
> > > >
> > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > > ---
> > > >  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?
> 
> > 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.

Yes. I'll have a look and report back in a few days.

Clemens

Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Clemens Gruber]

On Mon, Mar 30, 2020 at 06:02:38PM +0200, Thierry Reding wrote:
> On Mon, Mar 30, 2020 at 04:18:22PM +0300, Andy Shevchenko wrote:
> > On Mon, Mar 30, 2020 at 4:09 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer wrote:
> > > > duty_cycle was only set, never read.
> > > >
> > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > > ---
> > > >  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?
> 
> > 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.

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

Clemens

Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Thierry Reding]

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

On Wed, Apr 01, 2020 at 06:36:40PM +0200, Clemens Gruber wrote:
> On Mon, Mar 30, 2020 at 06:02:38PM +0200, Thierry Reding wrote:
> > On Mon, Mar 30, 2020 at 04:18:22PM +0300, Andy Shevchenko wrote:
> > > On Mon, Mar 30, 2020 at 4:09 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > >
> > > > On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer wrote:
> > > > > duty_cycle was only set, never read.
> > > > >
> > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > > > ---
> > > > >  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?
> > 
> > > 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.
> 
> 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!

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

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

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

Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

[Matthias Schiffer]

On Wed, 2020-04-01 at 19:45 +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Apr 01, 2020 at 06:36:40PM +0200, Clemens Gruber wrote:
> > On Mon, Mar 30, 2020 at 06:02:38PM +0200, Thierry Reding wrote:
> > > On Mon, Mar 30, 2020 at 04:18:22PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Mar 30, 2020 at 4:09 PM Thierry Reding <
> > > > thierry.reding@gmail.com> wrote:
> > > > > 
> > > > > On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer
> > > > > wrote:
> > > > > > duty_cycle was only set, never read.
> > > > > > 
> > > > > > Signed-off-by: Matthias Schiffer <
> > > > > > matthias.schiffer@ew.tq-group.com>
> > > > > > ---
> > > > > >  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?
> > > 
> > > > 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.
> > 
> > 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!
> 
> > 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..?)
> 
> 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.

I think what you're seeing is the same bug that prompted me to send my
patchset - patch 4/4 moves to the atomic API and changes the logic to
fix the interaction between config() and enable()/disable(). As it
seems like 2/4 will have to be reverted though and my 4/4 depends on
the cleanup of 2/4 and 3/4 (and 3/4 and 4/4 still need some work as
well), feel free to have a jab at the issue yourself, as I don't know
when I'll be able to get back to it.

Thanks,
Matthias


> 
> Thierry
> 
> * Unknown Key
> * 0x7F3EB3A1