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

[PATCH] 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 unsuspecting 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 76cd22bd6614..63ff1f22246d 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -58,6 +58,10 @@
 #define PCA9685_MAXCHAN		0x10
 
 #define LED_FULL		(1 << 4)
+#define MODE1_ALLCALL		(1 << 0)
+#define MODE1_SUB3		(1 << 1)
+#define MODE1_SUB2		(1 << 2)
+#define MODE1_SUB1		(1 << 3)
 #define MODE1_SLEEP		(1 << 4)
 #define MODE2_INVRT		(1 << 4)
 #define MODE2_OUTDRV		(1 << 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] drivers: pwm: pwm-pca9685.c: Disable unused alternative addresses

[Lee Jones]

On Thu, 20 Aug 2020, 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 unsuspecting bus collisions.

Nit: s/unsuspecting/unsuspected/

"Those unsuspecting bus collisions didn't know what hit them!"

> 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 76cd22bd6614..63ff1f22246d 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -58,6 +58,10 @@
>  #define PCA9685_MAXCHAN		0x10
>  
>  #define LED_FULL		(1 << 4)
> +#define MODE1_ALLCALL		(1 << 0)
> +#define MODE1_SUB3		(1 << 1)
> +#define MODE1_SUB2		(1 << 2)
> +#define MODE1_SUB1		(1 << 3)
>  #define MODE1_SLEEP		(1 << 4)
>  #define MODE2_INVRT		(1 << 4)
>  #define MODE2_OUTDRV		(1 << 2)

Nit: Consider converting these to BIT() in a subsequent patch.

> @@ -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 */

Nit: Consider drafting a patch to make commenting more consistent in
     this file.  I suggest always starting with an uppercase char.

>  	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);

The patch itself is generally okay though:

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog