[PATCH v2 0/3] pwm: pwm-pca9685 Disable unused alternative addresses

[PATCH v2 0/3] pwm: pwm-pca9685 Disable unused alternative addresses

[David Jander]

changes since v2:
- Extra patch: Make comments more consisent
- Extra patch: Use BIT() macro instead of shift
- Fix typo in commit message

David Jander (3):
  drivers: pwm: pwm-pcs9685.c: Make comments more consistent
  drivers: pwm: pwm-pca9685.c: Use BIT() macro instead of shift
  drivers: pwm: pwm-pca9685.c: Disable unused alternative addresses

 drivers/pwm/pwm-pca9685.c | 45 +++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 18 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] drivers: pwm: pwm-pcs9685.c: Make comments more consistent

[David Jander]

Make all explanatory comments start with an uppercase char.

Signed-off-by: David Jander <david@protonic.nl>
---
 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 76cd22bd6614..0f1a3e07e501 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -91,7 +91,7 @@ static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
 	mutex_lock(&pca->lock);
 	if (pwm_idx >= PCA9685_MAXCHAN) {
 		/*
-		 * "all LEDs" channel:
+		 * "All LEDs" channel:
 		 * pretend already in use if any of the PWMs are requested
 		 */
 		if (!bitmap_empty(pca->pwms_inuse, PCA9685_MAXCHAN)) {
@@ -100,7 +100,7 @@ static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
 		}
 	} else {
 		/*
-		 * regular channel:
+		 * Regular channel:
 		 * pretend already in use if the "all LEDs" channel is requested
 		 */
 		if (test_bit(PCA9685_MAXCHAN, pca->pwms_inuse)) {
@@ -257,7 +257,7 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		if (prescale >= PCA9685_PRESCALE_MIN &&
 			prescale <= PCA9685_PRESCALE_MAX) {
 			/*
-			 * putting the chip briefly into SLEEP mode
+			 * Putting the chip briefly into SLEEP mode
 			 * at this point won't interfere with the
 			 * pm_runtime framework, because the pm_runtime
 			 * state is guaranteed active here.
@@ -475,12 +475,12 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 
 	regmap_write(pca->regmap, PCA9685_MODE2, mode2);
 
-	/* clear all "full off" bits */
+	/* Clear all "full off" bits */
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
 
 	pca->chip.ops = &pca9685_pwm_ops;
-	/* add an extra channel for ALL_LED */
+	/* Add an extra channel for ALL_LED */
 	pca->chip.npwm = PCA9685_MAXCHAN + 1;
 
 	pca->chip.dev = &client->dev;
@@ -496,10 +496,10 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	/* the chip comes out of power-up in the active state */
+	/* The chip comes out of power-up in the active state */
 	pm_runtime_set_active(&client->dev);
 	/*
-	 * enable will put the chip into suspend, which is what we
+	 * Enable will put the chip into suspend, which is what we
 	 * want as all outputs are disabled at this point
 	 */
 	pm_runtime_enable(&client->dev);
-- 
2.25.1

Re: [PATCH v2 1/3] drivers: pwm: pwm-pcs9685.c: Make comments more consistent

[Uwe Kleine-König]

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

On Fri, Aug 28, 2020 at 02:14:13PM +0200, David Jander wrote:
> Make all explanatory comments start with an uppercase char.
> 
> Signed-off-by: David Jander <david@protonic.nl>
> ---
>  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 76cd22bd6614..0f1a3e07e501 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -91,7 +91,7 @@ static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
>  	mutex_lock(&pca->lock);
>  	if (pwm_idx >= PCA9685_MAXCHAN) {
>  		/*
> -		 * "all LEDs" channel:
> +		 * "All LEDs" channel:

I'd not replace this one, assuming "all LEDs" is a term from the
datasheet.

>  		 * pretend already in use if any of the PWMs are requested
>  		 */
>  		if (!bitmap_empty(pca->pwms_inuse, PCA9685_MAXCHAN)) {
> @@ -100,7 +100,7 @@ static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)

Best regards
Uwe

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

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

Re: [PATCH v2 1/3] drivers: pwm: pwm-pcs9685.c: Make comments more consistent

[David Jander]

On Wed, 2 Sep 2020 09:08:44 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> On Fri, Aug 28, 2020 at 02:14:13PM +0200, David Jander wrote:
> > Make all explanatory comments start with an uppercase char.
> > 
> > Signed-off-by: David Jander <david@protonic.nl>
> > ---
> >  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 76cd22bd6614..0f1a3e07e501 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -91,7 +91,7 @@ static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> >  	mutex_lock(&pca->lock);
> >  	if (pwm_idx >= PCA9685_MAXCHAN) {
> >  		/*
> > -		 * "all LEDs" channel:
> > +		 * "All LEDs" channel:  
> 
> I'd not replace this one, assuming "all LEDs" is a term from the
> datasheet.

It is not really a "term" in the datasheet. The string "all LEDs" occurs twice
in the datasheet, but AFAICS in both cases it is just a regular sentence that
does not imply a special meaning to this string:

"LED output frequency (all LEDs) typically varies from..."

and

"Remark: When all LED outputs are configured..." (which isn't strictly the
same complete string anyway (missing plural form "s").

So, do you still want me to change it? (no problem with me if you insist in a
v3 patch for this).

> >  		 * pretend already in use if any of the PWMs are requested
> >  		 */
> >  		if (!bitmap_empty(pca->pwms_inuse, PCA9685_MAXCHAN)) {
> > @@ -100,7 +100,7 @@ static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)  

Best regards,

-- 
David Jander
Protonic Holland.

[PATCH v2 2/3] drivers: pwm: pwm-pca9685.c: Use BIT() macro instead of shift

[David Jander]

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/pwm/pwm-pca9685.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 0f1a3e07e501..9d1d9dece0c0 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -57,10 +57,10 @@
 #define PCA9685_NUMREGS		0xFF
 #define PCA9685_MAXCHAN		0x10
 
-#define LED_FULL		(1 << 4)
-#define MODE1_SLEEP		(1 << 4)
-#define MODE2_INVRT		(1 << 4)
-#define MODE2_OUTDRV		(1 << 2)
+#define LED_FULL		BIT(4)
+#define MODE1_SLEEP		BIT(4)
+#define MODE2_INVRT		BIT(4)
+#define MODE2_OUTDRV		BIT(2)
 
 #define LED_N_ON_H(N)	(PCA9685_LEDX_ON_H + (4 * (N)))
 #define LED_N_ON_L(N)	(PCA9685_LEDX_ON_L + (4 * (N)))
-- 
2.25.1

[PATCH v2 3/3] drivers: pwm: pwm-pca9685.c: Disable unused alternative addresses

[David Jander]

The PCA9685 supports listening to 1 or more alternative I2C chip addresses
for some special features that this driver does not support.
By default the LED ALLCALL address is active (default 0x70), which causes
this chip to respond to address 0x70 in addition to its main address
(0x41). This is not desireable if there is another device on the same bus
that uses this address (like a TMP103 for example).
Since this feature is not supported by this driver, it is best to disable
these addresses in the chip to avoid unsuspected bus collisions.

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/pwm/pwm-pca9685.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 9d1d9dece0c0..ca7d205d6130 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -58,6 +58,10 @@
 #define PCA9685_MAXCHAN		0x10
 
 #define LED_FULL		BIT(4)
+#define MODE1_ALLCALL		BIT(0)
+#define MODE1_SUB3		BIT(1)
+#define MODE1_SUB2		BIT(2)
+#define MODE1_SUB1		BIT(3)
 #define MODE1_SLEEP		BIT(4)
 #define MODE2_INVRT		BIT(4)
 #define MODE2_OUTDRV		BIT(2)
@@ -444,7 +448,7 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 {
 	struct pca9685 *pca;
 	int ret;
-	int mode2;
+	int reg;
 
 	pca = devm_kzalloc(&client->dev, sizeof(*pca), GFP_KERNEL);
 	if (!pca)
@@ -461,19 +465,24 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, pca);
 
-	regmap_read(pca->regmap, PCA9685_MODE2, &mode2);
+	regmap_read(pca->regmap, PCA9685_MODE2, &reg);
 
 	if (device_property_read_bool(&client->dev, "invert"))
-		mode2 |= MODE2_INVRT;
+		reg |= MODE2_INVRT;
 	else
-		mode2 &= ~MODE2_INVRT;
+		reg &= ~MODE2_INVRT;
 
 	if (device_property_read_bool(&client->dev, "open-drain"))
-		mode2 &= ~MODE2_OUTDRV;
+		reg &= ~MODE2_OUTDRV;
 	else
-		mode2 |= MODE2_OUTDRV;
+		reg |= MODE2_OUTDRV;
 
-	regmap_write(pca->regmap, PCA9685_MODE2, mode2);
+	regmap_write(pca->regmap, PCA9685_MODE2, reg);
+
+	/* Disable all LED ALLCALL and SUBx addresses to avoid bus collisions */
+	regmap_read(pca->regmap, PCA9685_MODE1, &reg);
+	reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
+	regmap_write(pca->regmap, PCA9685_MODE1, reg);
 
 	/* Clear all "full off" bits */
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
-- 
2.25.1

Re: [PATCH v2 3/3] drivers: pwm: pwm-pca9685.c: Disable unused alternative addresses

[Uwe Kleine-König]

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

On Fri, Aug 28, 2020 at 02:14:15PM +0200, David Jander wrote:
> The PCA9685 supports listening to 1 or more alternative I2C chip addresses
> for some special features that this driver does not support.
> By default the LED ALLCALL address is active (default 0x70), which causes
> this chip to respond to address 0x70 in addition to its main address
> (0x41). This is not desireable if there is another device on the same bus
> that uses this address (like a TMP103 for example).
> Since this feature is not supported by this driver, it is best to disable
> these addresses in the chip to avoid unsuspected bus collisions.

Is this feature on by default? If the feature is on during boot and the
device at address 0x70 is tryed to probe first, this is unfortunate. (In
this case you might want to disable this feature in the bootloader, too.)
 
> Signed-off-by: David Jander <david@protonic.nl>
> ---
>  drivers/pwm/pwm-pca9685.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 9d1d9dece0c0..ca7d205d6130 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -58,6 +58,10 @@
>  #define PCA9685_MAXCHAN		0x10
>  
>  #define LED_FULL		BIT(4)
> +#define MODE1_ALLCALL		BIT(0)
> +#define MODE1_SUB3		BIT(1)
> +#define MODE1_SUB2		BIT(2)
> +#define MODE1_SUB1		BIT(3)
>  #define MODE1_SLEEP		BIT(4)
>  #define MODE2_INVRT		BIT(4)
>  #define MODE2_OUTDRV		BIT(2)
> @@ -444,7 +448,7 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>  {
>  	struct pca9685 *pca;
>  	int ret;
> -	int mode2;
> +	int reg;

This rename makes the patch considerably bigger. I'd expect you can just
add a mode1 variable, use this to access the MODE1 register and still
get the same compiler output. If so, I'd prefer this way.

>  	pca = devm_kzalloc(&client->dev, sizeof(*pca), GFP_KERNEL);
>  	if (!pca)
> @@ -461,19 +465,24 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>  
>  	i2c_set_clientdata(client, pca);
>  
> -	regmap_read(pca->regmap, PCA9685_MODE2, &mode2);
> +	regmap_read(pca->regmap, PCA9685_MODE2, &reg);
>  
>  	if (device_property_read_bool(&client->dev, "invert"))
> -		mode2 |= MODE2_INVRT;
> +		reg |= MODE2_INVRT;
>  	else
> -		mode2 &= ~MODE2_INVRT;
> +		reg &= ~MODE2_INVRT;
>  
>  	if (device_property_read_bool(&client->dev, "open-drain"))
> -		mode2 &= ~MODE2_OUTDRV;
> +		reg &= ~MODE2_OUTDRV;
>  	else
> -		mode2 |= MODE2_OUTDRV;
> +		reg |= MODE2_OUTDRV;
>  
> -	regmap_write(pca->regmap, PCA9685_MODE2, mode2);
> +	regmap_write(pca->regmap, PCA9685_MODE2, reg);
> +
> +	/* Disable all LED ALLCALL and SUBx addresses to avoid bus collisions */
> +	regmap_read(pca->regmap, PCA9685_MODE1, &reg);
> +	reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
> +	regmap_write(pca->regmap, PCA9685_MODE1, reg);
>  
>  	/* Clear all "full off" bits */
>  	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);

Best regards
Uwe

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

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

Re: [PATCH v2 3/3] drivers: pwm: pwm-pca9685.c: Disable unused alternative addresses

[David Jander]

On Wed, 2 Sep 2020 09:07:24 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> On Fri, Aug 28, 2020 at 02:14:15PM +0200, David Jander wrote:
> > The PCA9685 supports listening to 1 or more alternative I2C chip addresses
> > for some special features that this driver does not support.
> > By default the LED ALLCALL address is active (default 0x70), which causes
> > this chip to respond to address 0x70 in addition to its main address
> > (0x41). This is not desireable if there is another device on the same bus
> > that uses this address (like a TMP103 for example).
> > Since this feature is not supported by this driver, it is best to disable
> > these addresses in the chip to avoid unsuspected bus collisions.  
> 
> Is this feature on by default? If the feature is on during boot and the
> device at address 0x70 is tryed to probe first, this is unfortunate. (In
> this case you might want to disable this feature in the bootloader, too.)

The situation is indeed a little bit more complex. First off, yes, address
0x70 is active by default after power-on, so you could theoretically get ACK
"collisions" before the PCA9685 is probed. But...

While it is considered bad practice to have a PCA9685 and another device that
responds to address 0x70 on the same bus, my specific case involves a device
connected to an extension header which is Raspberry-PI GPIO compatible. So the
I2C bus is not a fixed design, since it can be extended. Drivers are then
possibly loaded as modules.
Also, out of reset (power on), the PCA9685 is in sleep mode, so writing to
address 0x70 will not cause anything to happen to the outputs. Note that 0x70
is NOT like its main address, and it is NOT possible to get it out of sleep
mode accidentally, nor interfere with later startup in any way.
The PCA9685 only ACKs write access to that address, but will never respond to
a read command. So theoretically another device, such as a TMP103 will function
without problems with any probe order between the two, as long as the PCA9685
is NOT woken up before disabling the LED ALLCALL function... which is
precisely what this patch is about.

If this device was activated and used by the bootloader also, then of course
it will be necessary to also patch the bootloader to disable the LED ALLCALL
feature, but that is out of scope for this patch.

Note: The other 3 SUB-addresses are not active by default after power-on. The
reason this patch disables them also, is just because it is no extra effort
and if they were enabled before for whatever reason, that reason is not
relevant since this driver does not support it and either accidentally or
even intentionally messing with the hardware from another piece of software
would only cause conflicts, confusion and hard to debug problems. So it is
safer to disable them also while we are at it.

> > Signed-off-by: David Jander <david@protonic.nl>
> > ---
> >  drivers/pwm/pwm-pca9685.c | 23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 9d1d9dece0c0..ca7d205d6130 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -58,6 +58,10 @@
> >  #define PCA9685_MAXCHAN		0x10
> >  
> >  #define LED_FULL		BIT(4)
> > +#define MODE1_ALLCALL		BIT(0)
> > +#define MODE1_SUB3		BIT(1)
> > +#define MODE1_SUB2		BIT(2)
> > +#define MODE1_SUB1		BIT(3)
> >  #define MODE1_SLEEP		BIT(4)
> >  #define MODE2_INVRT		BIT(4)
> >  #define MODE2_OUTDRV		BIT(2)
> > @@ -444,7 +448,7 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> >  {
> >  	struct pca9685 *pca;
> >  	int ret;
> > -	int mode2;
> > +	int reg;  
> 
> This rename makes the patch considerably bigger. I'd expect you can just
> add a mode1 variable, use this to access the MODE1 register and still
> get the same compiler output. If so, I'd prefer this way.

If you insist, I will add a second variable. But I'd like to offer my humble
opinion about this first in case you want to consider it:
While introducing a second variable makes this patch a little bit smaller,
what remains after it is applied, for the rest of all visitors to this piece
of code, is a situation that is less readable. The more variables are
introduced, the more a reviewer of the code (not this patch) will have to keep
track of. For example, you cannot see as easily that the value of "mode2" is
used only in this scope, and not somewhere later in the code. Re-use of a
variable explicitly in code, conveys information to the human reading it. Not
only to the compiler (which of course is smart enough to figure that out).

> >  	pca = devm_kzalloc(&client->dev, sizeof(*pca), GFP_KERNEL);
> >  	if (!pca)
> > @@ -461,19 +465,24 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> >  
> >  	i2c_set_clientdata(client, pca);
> >  
> > -	regmap_read(pca->regmap, PCA9685_MODE2, &mode2);
> > +	regmap_read(pca->regmap, PCA9685_MODE2, &reg);
> >  
> >  	if (device_property_read_bool(&client->dev, "invert"))
> > -		mode2 |= MODE2_INVRT;
> > +		reg |= MODE2_INVRT;
> >  	else
> > -		mode2 &= ~MODE2_INVRT;
> > +		reg &= ~MODE2_INVRT;
> >  
> >  	if (device_property_read_bool(&client->dev, "open-drain"))
> > -		mode2 &= ~MODE2_OUTDRV;
> > +		reg &= ~MODE2_OUTDRV;
> >  	else
> > -		mode2 |= MODE2_OUTDRV;
> > +		reg |= MODE2_OUTDRV;
> >  
> > -	regmap_write(pca->regmap, PCA9685_MODE2, mode2);
> > +	regmap_write(pca->regmap, PCA9685_MODE2, reg);
> > +
> > +	/* Disable all LED ALLCALL and SUBx addresses to avoid bus collisions */
> > +	regmap_read(pca->regmap, PCA9685_MODE1, &reg);
> > +	reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
> > +	regmap_write(pca->regmap, PCA9685_MODE1, reg);
> >  
> >  	/* Clear all "full off" bits */
> >  	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);  

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v2 0/3] pwm: pwm-pca9685 Disable unused alternative addresses

[Thierry Reding]

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

On Fri, Aug 28, 2020 at 02:14:12PM +0200, David Jander wrote:
> changes since v2:
> - Extra patch: Make comments more consisent
> - Extra patch: Use BIT() macro instead of shift
> - Fix typo in commit message
> 
> David Jander (3):
>   drivers: pwm: pwm-pcs9685.c: Make comments more consistent
>   drivers: pwm: pwm-pca9685.c: Use BIT() macro instead of shift
>   drivers: pwm: pwm-pca9685.c: Disable unused alternative addresses
> 
>  drivers/pwm/pwm-pca9685.c | 45 +++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 18 deletions(-)

I know that you and Uwe disagreed on some aspects of this, but I don't
care much either way and the net result here is positive, so I've
applied these.

Thanks,
Thierry

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