[PATCH] leds: pca963x: Fix open-drain initialization

[PATCH] leds: pca963x: Fix open-drain initialization

[Zahari Petkov]

OUTDRV setting (bit 2) of Mode register 2 has a default value of 1.
During initialization when open-drain is used, instead of setting
OUTDRV to 0, the driver keeps it as 1. OUTDRV setting is now correctly
initialized to 0 when open-drain is used.

Additionally the BIT macro is used for improved readibility.

Fixes: bb29b9cccd95 ("leds: pca963x: Add bindings to invert polarity")
Signed-off-by: Zahari Petkov <zahari@balena.io>
---
 drivers/leds/leds-pca963x.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-pca963x.c b/drivers/leds/leds-pca963x.c
index 4afc317901a8..e3da2156b385 100644
--- a/drivers/leds/leds-pca963x.c
+++ b/drivers/leds/leds-pca963x.c
@@ -438,12 +438,12 @@ static int pca963x_probe(struct i2c_client *client,
 						    PCA963X_MODE2);
 		/* Configure output: open-drain or totem pole (push-pull) */
 		if (pdata->outdrv == PCA963X_OPEN_DRAIN)
-			mode2 |= 0x01;
+			mode2 &= ~BIT(2);
 		else
-			mode2 |= 0x05;
+			mode2 |= BIT(2);
 		/* Configure direction: normal or inverted */
 		if (pdata->dir == PCA963X_INVERTED)
-			mode2 |= 0x10;
+			mode2 |= BIT(4);
 		i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2,
 					  mode2);
 	}
-- 
2.23.0

Re: [PATCH] leds: pca963x: Fix open-drain initialization

[Pavel Machek]

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

On Mon 2019-10-14 15:36:04, Zahari Petkov wrote:
> OUTDRV setting (bit 2) of Mode register 2 has a default value of 1.
> During initialization when open-drain is used, instead of setting
> OUTDRV to 0, the driver keeps it as 1. OUTDRV setting is now correctly
> initialized to 0 when open-drain is used.
> 
> Additionally the BIT macro is used for improved readibility.

You change more than you describe in the changelog.

> +++ b/drivers/leds/leds-pca963x.c
> @@ -438,12 +438,12 @@ static int pca963x_probe(struct i2c_client *client,
>  						    PCA963X_MODE2);
>  		/* Configure output: open-drain or totem pole (push-pull) */
>  		if (pdata->outdrv == PCA963X_OPEN_DRAIN)
> -			mode2 |= 0x01;
> +			mode2 &= ~BIT(2);

| 0 -> & ~0x04;

>  		else
> -			mode2 |= 0x05;
> +			mode2 |= BIT(2);

| 5 -> | 0x04;

Are you sure?

Additionaly, we already have defines for bits in mode2 register:

#define PCA963X_MODE2_DMBLNK    0x20    /* Enable blinking */

So if you care about readability, perhaps you should add defines for
invert/ open drain there, and then use them?

Please keep using 0xab instead of BIT() for consistency with the rest
of the driver.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

Re: [PATCH] leds: pca963x: Fix open-drain initialization

[Zahari Petkov]

On Mon, Nov 04, 2019 at 10:09:53AM +0100, Pavel Machek wrote:
> On Mon 2019-10-14 15:36:04, Zahari Petkov wrote:
> > OUTDRV setting (bit 2) of Mode register 2 has a default value of 1.
> > During initialization when open-drain is used, instead of setting
> > OUTDRV to 0, the driver keeps it as 1. OUTDRV setting is now correctly
> > initialized to 0 when open-drain is used.
> > 
> > Additionally the BIT macro is used for improved readibility.
> 
> You change more than you describe in the changelog.

You are indeed correct. I will provide a more detailed and precise description.

> 
> > +++ b/drivers/leds/leds-pca963x.c
> > @@ -438,12 +438,12 @@ static int pca963x_probe(struct i2c_client *client,
> >  						    PCA963X_MODE2);
> >  		/* Configure output: open-drain or totem pole (push-pull) */
> >  		if (pdata->outdrv == PCA963X_OPEN_DRAIN)
> > -			mode2 |= 0x01;
> > +			mode2 &= ~BIT(2);
> 
> | 0 -> & ~0x04;
> 
> >  		else
> > -			mode2 |= 0x05;
> > +			mode2 |= BIT(2);
> 
> | 5 -> | 0x04;
> 
> Are you sure?

Yes, I need to explain this better in the updated description.

> 
> Additionaly, we already have defines for bits in mode2 register:
> 
> #define PCA963X_MODE2_DMBLNK    0x20    /* Enable blinking */
> 
> So if you care about readability, perhaps you should add defines for
> invert/ open drain there, and then use them?
> 
> Please keep using 0xab instead of BIT() for consistency with the rest
> of the driver.

I will update the code to use new defines instead of BIT().

Thanks a lot!
Zahari