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

[Lee Jones <lee.jones@linaro.org>]

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