[U-Boot] [PATCHv2] pca953x: support 16-pin devices

[U-Boot] [PATCHv2] pca953x: support 16-pin devices

[Chris Packham]

This adds support for for the PCA9535/PCA9539 family of gpio devices which
have 16 output pins.

To let the driver know which devices are 16-pin it is necessary to define
CONFIG_SYS_I2C_PCA953X_WIDTH in your board config file. This is used to
create an array of {chip, ngpio} tuples that are used to determine the
width of a particular chip. For backwards compatibility it is assumed that
any chip not defined in CONFIG_SYS_I2C_PCA953X_WIDTH has 8 pins.
---

As suggested by Peter I've implemented the 16-pin support in the existing
pca953x driver. So this is pretty much a re-write of the v1 patch. Is the commit
message sufficient to document CONFIG_SYS_I2C_PCA953X_WIDTH or is
there something under doc/ that I should be adding to.

 drivers/gpio/pca953x.c |  107 ++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 85 insertions(+), 22 deletions(-)

diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index 6e82bd6..491b79e 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -17,8 +17,8 @@
  */

 /*
- * Driver for NXP's 4 and 8 bit I2C gpio expanders (eg pca9537, pca9557, etc)
- * TODO: support additional devices with more than 8-bits GPIO
+ * Driver for NXP's 4, 8 and 16 bit I2C gpio expanders (eg pca9537, pca9557,
+ * pca9539, etc)
  */

 #include <common.h>
@@ -38,22 +38,78 @@ enum {
 	PCA953X_CMD_INVERT,
 };

+#ifdef CONFIG_SYS_I2C_PCA953X_WIDTH
+struct chip_ngpio {
+	uint8_t chip;
+	uint8_t ngpio;
+};
+
+static struct chip_ngpio chip_ngpios[] = CONFIG_SYS_I2C_PCA953X_WIDTH;
+#define NUM_CHIP_GPIOS (sizeof(chip_ngpios) / sizeof(struct chip_ngpio))
+
+/*
+ * Determine the number of GPIO pins supported. If we don't know we assume
+ * 8 pins.
+ */
+static int ngpio(uint8_t chip)
+{
+	int i;
+
+	for (i = 0; i < NUM_CHIP_GPIOS; i++) {
+		if (chip_ngpios[i].chip == chip)
+			return chip_ngpios[i].ngpio;
+	}
+	return 8;
+}
+#else
+#define ngpio(chip)	8
+#endif
+
 /*
  * Modify masked bits in register
  */
 static int pca953x_reg_write(uint8_t chip, uint addr, uint mask, uint data)
 {
-	uint8_t val;
+	uint8_t  valb;
+	uint16_t valw;

-	if (i2c_read(chip, addr, 1, &val, 1))
-		return -1;
+	if (ngpio(chip) <= 8) {
+		if (i2c_read(chip, addr, 1, &valb, 1))
+			return -1;
+
+		valb &= ~mask;
+		valb |= data;
+
+		return i2c_write(chip, addr, 1, &valb, 1);
+	} else {
+		if (i2c_read(chip, addr << 1, 1, (u8*)&valw, 2))
+			return -1;
+
+		valw &= ~mask;
+		valw |= data;
+
+		return i2c_write(chip, addr << 1, 1, (u8*)&valw, 2);
+	}
+}

-	val &= ~mask;
-	val |= data;
+static int pca953x_reg_read(uint8_t chip, uint addr, uint *data)
+{
+	uint8_t  valb;
+	uint16_t valw;

-	return i2c_write(chip, addr, 1, &val, 1);
+	if (ngpio(chip) <= 8) {
+		if (i2c_read(chip, addr, 1, &valb, 1))
+			return -1;
+		*data = (int) valb;
+	} else {
+		if (i2c_read(chip, addr << 1, 1, (u8*)&valw, 2))
+			return -1;
+		*data = (int) valw;
+	}
+	return 0;
 }

+
 /*
  * Set output value of IO pins in 'mask' to corresponding value in 'data'
  * 0 = low, 1 = high
@@ -86,9 +142,9 @@ int pca953x_set_dir(uint8_t chip, uint mask, uint data)
  */
 int pca953x_get_val(uint8_t chip)
 {
-	uint8_t val;
+	uint val;

-	if (i2c_read(chip, 0, 1, &val, 1))
+	if (pca953x_reg_read(chip, PCA953X_IN, &val) < 0)
 		return -1;

 	return (int)val;
@@ -102,37 +158,44 @@ int pca953x_get_val(uint8_t chip)
 static int pca953x_info(uint8_t chip)
 {
 	int i;
-	uint8_t data;
+	uint data;
+	int nr_gpio = ngpio(chip);
+	int msb     = nr_gpio - 1;

-	printf("pca953x@ 0x%x:\n\n", chip);
-	printf("gpio pins: 76543210\n");
+	printf("pca953x@ 0x%x (%d pins):\n\n", chip, nr_gpio);
+	printf("gpio pins: ");
+	for (i = msb; i >= 0; i--)
+		printf("%x",i);
+	printf("\n");
+	if (nr_gpio > 8)
+		printf("--------");
 	printf("-------------------\n");

-	if (i2c_read(chip, PCA953X_CONF, 1, &data, 1))
+	if (pca953x_reg_read(chip, PCA953X_CONF, &data) < 0)
 		return -1;
 	printf("conf:      ");
-	for (i = 7; i >= 0; i--)
+	for (i = msb; i >= 0; i--)
 		printf("%c", data & (1 << i) ? 'i' : 'o');
 	printf("\n");

-	if (i2c_read(chip, PCA953X_POL, 1, &data, 1))
+	if (pca953x_reg_read(chip, PCA953X_POL, &data) < 0)
 		return -1;
 	printf("invert:    ");
-	for (i = 7; i >= 0; i--)
+	for (i = msb; i >= 0; i--)
 		printf("%c", data & (1 << i) ? '1' : '0');
 	printf("\n");

-	if (i2c_read(chip, PCA953X_IN, 1, &data, 1))
+	if (pca953x_reg_read(chip, PCA953X_IN, &data) < 0)
 		return -1;
 	printf("input:     ");
-	for (i = 7; i >= 0; i--)
+	for (i = msb; i >= 0; i--)
 		printf("%c", data & (1 << i) ? '1' : '0');
 	printf("\n");

-	if (i2c_read(chip, PCA953X_OUT, 1, &data, 1))
+	if (pca953x_reg_read(chip, PCA953X_OUT, &data) < 0)
 		return -1;
 	printf("output:    ");
-	for (i = 7; i >= 0; i--)
+	for (i = msb; i >= 0; i--)
 		printf("%c", data & (1 << i) ? '1' : '0');
 	printf("\n");

-- 
1.7.3.2

[U-Boot] [PATCHv2] pca953x: support 16-pin devices

[Peter Tyser]

The patch looks good.  I had a few minor nitpicky style comments below:

> As suggested by Peter I've implemented the 16-pin support in the existing
> pca953x driver. So this is pretty much a re-write of the v1 patch. Is the commit
> message sufficient to document CONFIG_SYS_I2C_PCA953X_WIDTH or is
> there something under doc/ that I should be adding to.

You could add a brief description to the top-level README file.  There
is currently a bit of info in the "GPIO Support" section that could be
added to.

>  #include <common.h>
> @@ -38,22 +38,78 @@ enum {
>  	PCA953X_CMD_INVERT,
>  };
> 
> +#ifdef CONFIG_SYS_I2C_PCA953X_WIDTH
> +struct chip_ngpio {
> +	uint8_t chip;
> +	uint8_t ngpio;
> +};

Since this structure is 953x-specific I'd rename chip_ngpio
pca953x_chip_ngpio to clarify its a driver-specific structure and to
prevent the (unlikely) name collision down the road.

The same comment applies to ngpio()->pca953x_ngpio(),
chip_ngpios[]->pca953x_chip_ngpios[].

> +static struct chip_ngpio chip_ngpios[] = CONFIG_SYS_I2C_PCA953X_WIDTH;
> +#define NUM_CHIP_GPIOS (sizeof(chip_ngpios) / sizeof(struct chip_ngpio))
> +
> +/*
> + * Determine the number of GPIO pins supported. If we don't know we assume
> + * 8 pins.
> + */
> +static int ngpio(uint8_t chip)
> +{
> +	int i;
> +
> +	for (i = 0; i < NUM_CHIP_GPIOS; i++) {
> +		if (chip_ngpios[i].chip == chip)
> +			return chip_ngpios[i].ngpio;
> +	}

I'd remove the for loop braces above per the Linux CodingStyle
documentation that U-Boot adheres to.

There should also be an empty line above the "return 8" below.

> +	return 8;
> +}
> +#else
> +#define ngpio(chip)	8
> +#endif
> +
>  /*
>   * Modify masked bits in register
>   */
>  static int pca953x_reg_write(uint8_t chip, uint addr, uint mask, uint data)
>  {
> -	uint8_t val;
> +	uint8_t  valb;
> +	uint16_t valw;

I'd remove one of the spaces between "valb" above.  My understanding is
spaces shouldn't be used for such vertical alignment.

> 
> -	if (i2c_read(chip, addr, 1, &val, 1))
> -		return -1;
> +	if (ngpio(chip) <= 8) {
> +		if (i2c_read(chip, addr, 1, &valb, 1))
> +			return -1;
> +
> +		valb &= ~mask;
> +		valb |= data;
> +
> +		return i2c_write(chip, addr, 1, &valb, 1);
> +	} else {
> +		if (i2c_read(chip, addr << 1, 1, (u8*)&valw, 2))
> +			return -1;
> +
> +		valw &= ~mask;
> +		valw |= data;
> +
> +		return i2c_write(chip, addr << 1, 1, (u8*)&valw, 2);
> +	}
> +}
> 
> -	val &= ~mask;
> -	val |= data;
> +static int pca953x_reg_read(uint8_t chip, uint addr, uint *data)
> +{
> +	uint8_t  valb;
> +	uint16_t valw;
> 
> -	return i2c_write(chip, addr, 1, &val, 1);
> +	if (ngpio(chip) <= 8) {
> +		if (i2c_read(chip, addr, 1, &valb, 1))
> +			return -1;
> +		*data = (int) valb;

The space in "(int) valb" should be removed.  Same below.

> +	} else {
> +		if (i2c_read(chip, addr << 1, 1, (u8*)&valw, 2))
> +			return -1;
> +		*data = (int) valw;
> +	}
> +	return 0;
>  }

<snip>

> +	for (i = msb; i >= 0; i--)
> +		printf("%x",i);
> +	printf("\n");
> +	if (nr_gpio > 8)
> +		printf("--------");
>  	printf("-------------------\n");

What about changing the printing of '-'s to a for loop like
for (i = 19 + nr_gpio; i >0; i++)
	puts("-");

I'll give the next iteration of the patch a shot, it looks like it
should work just fine.

Best,
Peter

[U-Boot] [PATCHv2] pca953x: support 16-pin devices

[Chris Packham]

On Thu, Dec 9, 2010 at 1:08 PM, Peter Tyser <ptyser@xes-inc.com> wrote:
> The patch looks good. ?I had a few minor nitpicky style comments below:
>
>> As suggested by Peter I've implemented the 16-pin support in the existing
>> pca953x driver. So this is pretty much a re-write of the v1 patch. Is the commit
>> message sufficient to document CONFIG_SYS_I2C_PCA953X_WIDTH or is
>> there something under doc/ that I should be adding to.
>
> You could add a brief description to the top-level README file. ?There
> is currently a bit of info in the "GPIO Support" section that could be
> added to.
>
>> ?#include <common.h>
>> @@ -38,22 +38,78 @@ enum {
>> ? ? ? PCA953X_CMD_INVERT,
>> ?};
>>
>> +#ifdef CONFIG_SYS_I2C_PCA953X_WIDTH
>> +struct chip_ngpio {
>> + ? ? uint8_t chip;
>> + ? ? uint8_t ngpio;
>> +};
>
> Since this structure is 953x-specific I'd rename chip_ngpio
> pca953x_chip_ngpio to clarify its a driver-specific structure and to
> prevent the (unlikely) name collision down the road.
>
> The same comment applies to ngpio()->pca953x_ngpio(),
> chip_ngpios[]->pca953x_chip_ngpios[].
>
>> +static struct chip_ngpio chip_ngpios[] = CONFIG_SYS_I2C_PCA953X_WIDTH;
>> +#define NUM_CHIP_GPIOS (sizeof(chip_ngpios) / sizeof(struct chip_ngpio))
>> +
>> +/*
>> + * Determine the number of GPIO pins supported. If we don't know we assume
>> + * 8 pins.
>> + */
>> +static int ngpio(uint8_t chip)
>> +{
>> + ? ? int i;
>> +
>> + ? ? for (i = 0; i < NUM_CHIP_GPIOS; i++) {
>> + ? ? ? ? ? ? if (chip_ngpios[i].chip == chip)
>> + ? ? ? ? ? ? ? ? ? ? return chip_ngpios[i].ngpio;
>> + ? ? }
>
> I'd remove the for loop braces above per the Linux CodingStyle
> documentation that U-Boot adheres to.
>
> There should also be an empty line above the "return 8" below.
>
>> + ? ? return 8;
>> +}
>> +#else
>> +#define ngpio(chip) ?8
>> +#endif
>> +
>> ?/*
>> ? * Modify masked bits in register
>> ? */
>> ?static int pca953x_reg_write(uint8_t chip, uint addr, uint mask, uint data)
>> ?{
>> - ? ? uint8_t val;
>> + ? ? uint8_t ?valb;
>> + ? ? uint16_t valw;
>
> I'd remove one of the spaces between "valb" above. ?My understanding is
> spaces shouldn't be used for such vertical alignment.
>
>>
>> - ? ? if (i2c_read(chip, addr, 1, &val, 1))
>> - ? ? ? ? ? ? return -1;
>> + ? ? if (ngpio(chip) <= 8) {
>> + ? ? ? ? ? ? if (i2c_read(chip, addr, 1, &valb, 1))
>> + ? ? ? ? ? ? ? ? ? ? return -1;
>> +
>> + ? ? ? ? ? ? valb &= ~mask;
>> + ? ? ? ? ? ? valb |= data;
>> +
>> + ? ? ? ? ? ? return i2c_write(chip, addr, 1, &valb, 1);
>> + ? ? } else {
>> + ? ? ? ? ? ? if (i2c_read(chip, addr << 1, 1, (u8*)&valw, 2))
>> + ? ? ? ? ? ? ? ? ? ? return -1;
>> +
>> + ? ? ? ? ? ? valw &= ~mask;
>> + ? ? ? ? ? ? valw |= data;
>> +
>> + ? ? ? ? ? ? return i2c_write(chip, addr << 1, 1, (u8*)&valw, 2);
>> + ? ? }
>> +}
>>
>> - ? ? val &= ~mask;
>> - ? ? val |= data;
>> +static int pca953x_reg_read(uint8_t chip, uint addr, uint *data)
>> +{
>> + ? ? uint8_t ?valb;
>> + ? ? uint16_t valw;
>>
>> - ? ? return i2c_write(chip, addr, 1, &val, 1);
>> + ? ? if (ngpio(chip) <= 8) {
>> + ? ? ? ? ? ? if (i2c_read(chip, addr, 1, &valb, 1))
>> + ? ? ? ? ? ? ? ? ? ? return -1;
>> + ? ? ? ? ? ? *data = (int) valb;
>
> The space in "(int) valb" should be removed. ?Same below.
>
>> + ? ? } else {
>> + ? ? ? ? ? ? if (i2c_read(chip, addr << 1, 1, (u8*)&valw, 2))
>> + ? ? ? ? ? ? ? ? ? ? return -1;
>> + ? ? ? ? ? ? *data = (int) valw;
>> + ? ? }
>> + ? ? return 0;
>> ?}
>
> <snip>
>
>> + ? ? for (i = msb; i >= 0; i--)
>> + ? ? ? ? ? ? printf("%x",i);
>> + ? ? printf("\n");
>> + ? ? if (nr_gpio > 8)
>> + ? ? ? ? ? ? printf("--------");
>> ? ? ? printf("-------------------\n");
>
> What about changing the printing of '-'s to a for loop like
> for (i = 19 + nr_gpio; i >0; i++)
> ? ? ? ?puts("-");
>
> I'll give the next iteration of the patch a shot, it looks like it
> should work just fine.
>
> Best,
> Peter
>

Hi Peter,

I'll try and get an updated patch through as soon as I can. I'm on a
training course today and tomorrow but it won't take me long to make
the changes you've suggested once I find some time.

Thanks,
Chris

[U-Boot] [PATCHv3] pca953x: support 16-pin devices

[Chris Packham]

This adds support for for the PCA9535/PCA9539 family of gpio devices which
have 16 output pins.

To let the driver know which devices are 16-pin it is necessary to define
CONFIG_SYS_I2C_PCA953X_WIDTH in your board config file. This is used to
create an array of {chip, ngpio} tuples that are used to determine the
width of a particular chip. For backwards compatibility it is assumed that
any chip not defined in CONFIG_SYS_I2C_PCA953X_WIDTH has 8 pins.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Changes since v2:
- I've addressed Peters style comments.
- I've added a blurb to README describing the new config option

 README                 |    4 ++
 drivers/gpio/pca953x.c |  111 ++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 92 insertions(+), 23 deletions(-)

diff --git a/README b/README
index 68f5fb0..831c5af 100644
--- a/README
+++ b/README
@@ -746,6 +746,10 @@ The following options need to be configured:
 		CONFIG_PCA953X		- use NXP's PCA953X series I2C GPIO
 		CONFIG_PCA953X_INFO	- enable pca953x info command

+		The CONFIG_SYS_I2C_PCA953X_WIDTH option specifies a list of
+		chip-ngpio pairs that tell the PCA953X driver the number of
+		pins supported by a particular chip.
+
 		Note that if the GPIO device uses I2C, then the I2C interface
 		must also be configured. See I2C Support, below.

diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index 6e82bd6..c8f5403 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -17,8 +17,8 @@
  */

 /*
- * Driver for NXP's 4 and 8 bit I2C gpio expanders (eg pca9537, pca9557, etc)
- * TODO: support additional devices with more than 8-bits GPIO
+ * Driver for NXP's 4, 8 and 16 bit I2C gpio expanders (eg pca9537, pca9557,
+ * pca9539, etc)
  */

 #include <common.h>
@@ -38,20 +38,78 @@ enum {
 	PCA953X_CMD_INVERT,
 };

+#ifdef CONFIG_SYS_I2C_PCA953X_WIDTH
+struct pca953x_chip_ngpio {
+	uint8_t chip;
+	uint8_t ngpio;
+};
+
+static struct pca953x_chip_ngpio pca953x_chip_ngpios[] =
+    CONFIG_SYS_I2C_PCA953X_WIDTH;
+
+#define NUM_CHIP_GPIOS (sizeof(pca953x_chip_ngpios) / \
+			sizeof(struct pca953x_chip_ngpio))
+
+/*
+ * Determine the number of GPIO pins supported. If we don't know we assume
+ * 8 pins.
+ */
+static int pca953x_ngpio(uint8_t chip)
+{
+	int i;
+
+	for (i = 0; i < NUM_CHIP_GPIOS; i++)
+		if (pca953x_chip_ngpios[i].chip == chip)
+			return pca953x_chip_ngpios[i].ngpio;
+
+	return 8;
+}
+#else
+#define pca953x_ngpio(chip)	8
+#endif
+
 /*
  * Modify masked bits in register
  */
 static int pca953x_reg_write(uint8_t chip, uint addr, uint mask, uint data)
 {
-	uint8_t val;
+	uint8_t valb;
+	uint16_t valw;

-	if (i2c_read(chip, addr, 1, &val, 1))
-		return -1;
+	if (pca953x_ngpio(chip) <= 8) {
+		if (i2c_read(chip, addr, 1, &valb, 1))
+			return -1;
+
+		valb &= ~mask;
+		valb |= data;
+
+		return i2c_write(chip, addr, 1, &valb, 1);
+	} else {
+		if (i2c_read(chip, addr << 1, 1, (u8*)&valw, 2))
+			return -1;
+
+		valw &= ~mask;
+		valw |= data;
+
+		return i2c_write(chip, addr << 1, 1, (u8*)&valw, 2);
+	}
+}

-	val &= ~mask;
-	val |= data;
+static int pca953x_reg_read(uint8_t chip, uint addr, uint *data)
+{
+	uint8_t valb;
+	uint16_t valw;

-	return i2c_write(chip, addr, 1, &val, 1);
+	if (pca953x_ngpio(chip) <= 8) {
+		if (i2c_read(chip, addr, 1, &valb, 1))
+			return -1;
+		*data = (int)valb;
+	} else {
+		if (i2c_read(chip, addr << 1, 1, (u8*)&valw, 2))
+			return -1;
+		*data = (int)valw;
+	}
+	return 0;
 }

 /*
@@ -86,9 +144,9 @@ int pca953x_set_dir(uint8_t chip, uint mask, uint data)
  */
 int pca953x_get_val(uint8_t chip)
 {
-	uint8_t val;
+	uint val;

-	if (i2c_read(chip, 0, 1, &val, 1))
+	if (pca953x_reg_read(chip, PCA953X_IN, &val) < 0)
 		return -1;

 	return (int)val;
@@ -102,37 +160,44 @@ int pca953x_get_val(uint8_t chip)
 static int pca953x_info(uint8_t chip)
 {
 	int i;
-	uint8_t data;
+	uint data;
+	int nr_gpio = pca953x_ngpio(chip);
+	int msb = nr_gpio - 1;

-	printf("pca953x@ 0x%x:\n\n", chip);
-	printf("gpio pins: 76543210\n");
-	printf("-------------------\n");
+	printf("pca953x@ 0x%x (%d pins):\n\n", chip, nr_gpio);
+	printf("gpio pins: ");
+	for (i = msb; i >= 0; i--)
+		printf("%x", i);
+	printf("\n");
+	for (i = 11 + nr_gpio; i > 0; i--)
+		printf("-");
+	printf("\n");

-	if (i2c_read(chip, PCA953X_CONF, 1, &data, 1))
+	if (pca953x_reg_read(chip, PCA953X_CONF, &data) < 0)
 		return -1;
 	printf("conf:      ");
-	for (i = 7; i >= 0; i--)
+	for (i = msb; i >= 0; i--)
 		printf("%c", data & (1 << i) ? 'i' : 'o');
 	printf("\n");

-	if (i2c_read(chip, PCA953X_POL, 1, &data, 1))
+	if (pca953x_reg_read(chip, PCA953X_POL, &data) < 0)
 		return -1;
 	printf("invert:    ");
-	for (i = 7; i >= 0; i--)
+	for (i = msb; i >= 0; i--)
 		printf("%c", data & (1 << i) ? '1' : '0');
 	printf("\n");

-	if (i2c_read(chip, PCA953X_IN, 1, &data, 1))
+	if (pca953x_reg_read(chip, PCA953X_IN, &data) < 0)
 		return -1;
 	printf("input:     ");
-	for (i = 7; i >= 0; i--)
+	for (i = msb; i >= 0; i--)
 		printf("%c", data & (1 << i) ? '1' : '0');
 	printf("\n");

-	if (i2c_read(chip, PCA953X_OUT, 1, &data, 1))
+	if (pca953x_reg_read(chip, PCA953X_OUT, &data) < 0)
 		return -1;
 	printf("output:    ");
-	for (i = 7; i >= 0; i--)
+	for (i = msb; i >= 0; i--)
 		printf("%c", data & (1 << i) ? '1' : '0');
 	printf("\n");

-- 
1.7.3.2

[U-Boot] [PATCHv3] pca953x: support 16-pin devices

[Peter Tyser]

On Thu, 2010-12-09 at 22:11 +1300, Chris Packham wrote:
> This adds support for for the PCA9535/PCA9539 family of gpio devices which
> have 16 output pins.
> 
> To let the driver know which devices are 16-pin it is necessary to define
> CONFIG_SYS_I2C_PCA953X_WIDTH in your board config file. This is used to
> create an array of {chip, ngpio} tuples that are used to determine the
> width of a particular chip. For backwards compatibility it is assumed that
> any chip not defined in CONFIG_SYS_I2C_PCA953X_WIDTH has 8 pins.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Looks good to me and works as advertised.

Acked-by: Peter Tyser <ptyser@xes-inc.com>
Tested-by: Peter Tyser <ptyser@xes-inc.com>

[U-Boot] [PATCHv3] pca953x: support 16-pin devices

[Chris Packham]

On Fri, Dec 10, 2010 at 6:08 AM, Peter Tyser <ptyser@xes-inc.com> wrote:
> On Thu, 2010-12-09 at 22:11 +1300, Chris Packham wrote:
>> This adds support for for the PCA9535/PCA9539 family of gpio devices which
>> have 16 output pins.
>>
>> To let the driver know which devices are 16-pin it is necessary to define
>> CONFIG_SYS_I2C_PCA953X_WIDTH in your board config file. This is used to
>> create an array of {chip, ngpio} tuples that are used to determine the
>> width of a particular chip. For backwards compatibility it is assumed that
>> any chip not defined in CONFIG_SYS_I2C_PCA953X_WIDTH has 8 pins.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>
> Looks good to me and works as advertised.
>
> Acked-by: Peter Tyser <ptyser@xes-inc.com>
> Tested-by: Peter Tyser <ptyser@xes-inc.com>
>

There is one minor fixup we might want to squash into v3 (or if
someone wants me to submit a v4 I can). It makes sense to have
pca953x_ngpio as a function in both cases. The compiler will
auto-inline the function so we won't see a increase in size and having
a function instead of a macro allows the compiler to do proper type
checking.

---8<---

From: Chris Packham <chris.packham@alliedtelesis.co.nz>
Date: Wed, 15 Dec 2010 15:44:17 +1300
Subject: [PATCH] fixup! pca953x: support 16-pin devices

---
 drivers/gpio/pca953x.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index c8f5403..359fdee 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -65,7 +65,10 @@ static int pca953x_ngpio(uint8_t chip)
 	return 8;
 }
 #else
-#define pca953x_ngpio(chip)	8
+static int pca953x_ngpio(uint8_t chip)
+{
+	return 8;
+}
 #endif

 /*
-- 
1.7.3.2

[U-Boot] [PATCHv3] pca953x: support 16-pin devices

[Wolfgang Denk]

Dear Chris Packham,

In message <AANLkTi=UrUVinO-Px-yc+7OfskODJADD_QpgRYDRHYxm@mail.gmail.com> you wrote:
>
> There is one minor fixup we might want to squash into v3 (or if
> someone wants me to submit a v4 I can). It makes sense to have
> pca953x_ngpio as a function in both cases. The compiler will
> auto-inline the function so we won't see a increase in size and having
> a function instead of a macro allows the compiler to do proper type
> checking.

Please send a v4.  Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I've seen it. It's rubbish."          - Marvin the Paranoid Android

[U-Boot] [PATCHv4] pca953x: support 16-pin devices

[Chris Packham]

This adds support for for the PCA9535/PCA9539 family of gpio devices which
have 16 output pins.

To let the driver know which devices are 16-pin it is necessary to define
CONFIG_SYS_I2C_PCA953X_WIDTH in your board config file. This is used to
create an array of {chip, ngpio} tuples that are used to determine the
width of a particular chip. For backwards compatibility it is assumed that
any chip not defined in CONFIG_SYS_I2C_PCA953X_WIDTH has 8 pins.

Acked-by: Peter Tyser <ptyser@xes-inc.com>
Tested-by: Peter Tyser <ptyser@xes-inc.com>
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Changes since v3:
- pca953x_ngpio is now a function in all cases.
- Added Peter's ACK to the commit message.

 README                 |    4 ++
 drivers/gpio/pca953x.c |  114 ++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 95 insertions(+), 23 deletions(-)

diff --git a/README b/README
index 68f5fb0..831c5af 100644
--- a/README
+++ b/README
@@ -746,6 +746,10 @@ The following options need to be configured:
 		CONFIG_PCA953X		- use NXP's PCA953X series I2C GPIO
 		CONFIG_PCA953X_INFO	- enable pca953x info command

+		The CONFIG_SYS_I2C_PCA953X_WIDTH option specifies a list of
+		chip-ngpio pairs that tell the PCA953X driver the number of
+		pins supported by a particular chip.
+
 		Note that if the GPIO device uses I2C, then the I2C interface
 		must also be configured. See I2C Support, below.

diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index 6e82bd6..359fdee 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -17,8 +17,8 @@
  */

 /*
- * Driver for NXP's 4 and 8 bit I2C gpio expanders (eg pca9537, pca9557, etc)
- * TODO: support additional devices with more than 8-bits GPIO
+ * Driver for NXP's 4, 8 and 16 bit I2C gpio expanders (eg pca9537, pca9557,
+ * pca9539, etc)
  */

 #include <common.h>
@@ -38,20 +38,81 @@ enum {
 	PCA953X_CMD_INVERT,
 };

+#ifdef CONFIG_SYS_I2C_PCA953X_WIDTH
+struct pca953x_chip_ngpio {
+	uint8_t chip;
+	uint8_t ngpio;
+};
+
+static struct pca953x_chip_ngpio pca953x_chip_ngpios[] =
+    CONFIG_SYS_I2C_PCA953X_WIDTH;
+
+#define NUM_CHIP_GPIOS (sizeof(pca953x_chip_ngpios) / \
+			sizeof(struct pca953x_chip_ngpio))
+
+/*
+ * Determine the number of GPIO pins supported. If we don't know we assume
+ * 8 pins.
+ */
+static int pca953x_ngpio(uint8_t chip)
+{
+	int i;
+
+	for (i = 0; i < NUM_CHIP_GPIOS; i++)
+		if (pca953x_chip_ngpios[i].chip == chip)
+			return pca953x_chip_ngpios[i].ngpio;
+
+	return 8;
+}
+#else
+static int pca953x_ngpio(uint8_t chip)
+{
+	return 8;
+}
+#endif
+
 /*
  * Modify masked bits in register
  */
 static int pca953x_reg_write(uint8_t chip, uint addr, uint mask, uint data)
 {
-	uint8_t val;
+	uint8_t valb;
+	uint16_t valw;

-	if (i2c_read(chip, addr, 1, &val, 1))
-		return -1;
+	if (pca953x_ngpio(chip) <= 8) {
+		if (i2c_read(chip, addr, 1, &valb, 1))
+			return -1;
+
+		valb &= ~mask;
+		valb |= data;
+
+		return i2c_write(chip, addr, 1, &valb, 1);
+	} else {
+		if (i2c_read(chip, addr << 1, 1, (u8*)&valw, 2))
+			return -1;

-	val &= ~mask;
-	val |= data;
+		valw &= ~mask;
+		valw |= data;

-	return i2c_write(chip, addr, 1, &val, 1);
+		return i2c_write(chip, addr << 1, 1, (u8*)&valw, 2);
+	}
+}
+
+static int pca953x_reg_read(uint8_t chip, uint addr, uint *data)
+{
+	uint8_t valb;
+	uint16_t valw;
+
+	if (pca953x_ngpio(chip) <= 8) {
+		if (i2c_read(chip, addr, 1, &valb, 1))
+			return -1;
+		*data = (int)valb;
+	} else {
+		if (i2c_read(chip, addr << 1, 1, (u8*)&valw, 2))
+			return -1;
+		*data = (int)valw;
+	}
+	return 0;
 }

 /*
@@ -86,9 +147,9 @@ int pca953x_set_dir(uint8_t chip, uint mask, uint data)
  */
 int pca953x_get_val(uint8_t chip)
 {
-	uint8_t val;
+	uint val;

-	if (i2c_read(chip, 0, 1, &val, 1))
+	if (pca953x_reg_read(chip, PCA953X_IN, &val) < 0)
 		return -1;

 	return (int)val;
@@ -102,37 +163,44 @@ int pca953x_get_val(uint8_t chip)
 static int pca953x_info(uint8_t chip)
 {
 	int i;
-	uint8_t data;
+	uint data;
+	int nr_gpio = pca953x_ngpio(chip);
+	int msb = nr_gpio - 1;

-	printf("pca953x@ 0x%x:\n\n", chip);
-	printf("gpio pins: 76543210\n");
-	printf("-------------------\n");
+	printf("pca953x@ 0x%x (%d pins):\n\n", chip, nr_gpio);
+	printf("gpio pins: ");
+	for (i = msb; i >= 0; i--)
+		printf("%x", i);
+	printf("\n");
+	for (i = 11 + nr_gpio; i > 0; i--)
+		printf("-");
+	printf("\n");

-	if (i2c_read(chip, PCA953X_CONF, 1, &data, 1))
+	if (pca953x_reg_read(chip, PCA953X_CONF, &data) < 0)
 		return -1;
 	printf("conf:      ");
-	for (i = 7; i >= 0; i--)
+	for (i = msb; i >= 0; i--)
 		printf("%c", data & (1 << i) ? 'i' : 'o');
 	printf("\n");

-	if (i2c_read(chip, PCA953X_POL, 1, &data, 1))
+	if (pca953x_reg_read(chip, PCA953X_POL, &data) < 0)
 		return -1;
 	printf("invert:    ");
-	for (i = 7; i >= 0; i--)
+	for (i = msb; i >= 0; i--)
 		printf("%c", data & (1 << i) ? '1' : '0');
 	printf("\n");

-	if (i2c_read(chip, PCA953X_IN, 1, &data, 1))
+	if (pca953x_reg_read(chip, PCA953X_IN, &data) < 0)
 		return -1;
 	printf("input:     ");
-	for (i = 7; i >= 0; i--)
+	for (i = msb; i >= 0; i--)
 		printf("%c", data & (1 << i) ? '1' : '0');
 	printf("\n");

-	if (i2c_read(chip, PCA953X_OUT, 1, &data, 1))
+	if (pca953x_reg_read(chip, PCA953X_OUT, &data) < 0)
 		return -1;
 	printf("output:    ");
-	for (i = 7; i >= 0; i--)
+	for (i = msb; i >= 0; i--)
 		printf("%c", data & (1 << i) ? '1' : '0');
 	printf("\n");

-- 
1.7.3.2

[U-Boot] [U-Boot,PATCHv4] pca953x: support 16-pin devices

[Heiko Schocher]

Hello Chris,

Sorry for the late reply, but just looked in patchwork and found that
I am responsible for your patch, so ...

Chris Packham wrote:
> This adds support for for the PCA9535/PCA9539 family of gpio devices which
> have 16 output pins.
> 
> To let the driver know which devices are 16-pin it is necessary to define
> CONFIG_SYS_I2C_PCA953X_WIDTH in your board config file. This is used to
> create an array of {chip, ngpio} tuples that are used to determine the
> width of a particular chip. For backwards compatibility it is assumed that
> any chip not defined in CONFIG_SYS_I2C_PCA953X_WIDTH has 8 pins.
> 
> Acked-by: Peter Tyser <ptyser@xes-inc.com>
> Tested-by: Peter Tyser <ptyser@xes-inc.com>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> 
> ---
> Changes since v3:
> - pca953x_ngpio is now a function in all cases.
> - Added Peter's ACK to the commit message.
> 
>  README                 |    4 ++
>  drivers/gpio/pca953x.c |  114 ++++++++++++++++++++++++++++++++++++++----------
>  2 files changed, 95 insertions(+), 23 deletions(-)

Compiles with actual u-boot the xpedite* boards, so added to
u-boot-i2c.git

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [U-Boot,PATCHv4] pca953x: support 16-pin devices

[Chris Packham]

On Mon, Jan 10, 2011 at 8:02 PM, Heiko Schocher <hs@denx.de> wrote:
> Hello Chris,
>
> Sorry for the late reply, but just looked in patchwork and found that
> I am responsible for your patch, so ...
>
> Chris Packham wrote:
>> This adds support for for the PCA9535/PCA9539 family of gpio devices which
>> have 16 output pins.
>>
>> To let the driver know which devices are 16-pin it is necessary to define
>> CONFIG_SYS_I2C_PCA953X_WIDTH in your board config file. This is used to
>> create an array of {chip, ngpio} tuples that are used to determine the
>> width of a particular chip. For backwards compatibility it is assumed that
>> any chip not defined in CONFIG_SYS_I2C_PCA953X_WIDTH has 8 pins.
>>
>> Acked-by: Peter Tyser <ptyser@xes-inc.com>
>> Tested-by: Peter Tyser <ptyser@xes-inc.com>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>
>> ---
>> Changes since v3:
>> - pca953x_ngpio is now a function in all cases.
>> - Added Peter's ACK to the commit message.
>>
>> ?README ? ? ? ? ? ? ? ? | ? ?4 ++
>> ?drivers/gpio/pca953x.c | ?114 ++++++++++++++++++++++++++++++++++++++----------
>> ?2 files changed, 95 insertions(+), 23 deletions(-)
>
> Compiles with actual u-boot the xpedite* boards, so added to
> u-boot-i2c.git
>
> Thanks!
>
> bye,
> Heiko

Thanks I was about to poke the mailing list to find out where things
had got to. Did I miss something on
http://www.denx.de/wiki/U-Boot/Patches that says look <here> list for
a custodian and CC them?

[U-Boot] [U-Boot,PATCHv4] pca953x: support 16-pin devices

[Peter Tyser]

On Thu, 2011-01-13 at 17:02 +1300, Chris Packham wrote:
> On Mon, Jan 10, 2011 at 8:02 PM, Heiko Schocher <hs@denx.de> wrote:
> > Hello Chris,
> >
> > Sorry for the late reply, but just looked in patchwork and found that
> > I am responsible for your patch, so ...
> >
> > Chris Packham wrote:
> >> This adds support for for the PCA9535/PCA9539 family of gpio devices which
> >> have 16 output pins.
> >>
> >> To let the driver know which devices are 16-pin it is necessary to define
> >> CONFIG_SYS_I2C_PCA953X_WIDTH in your board config file. This is used to
> >> create an array of {chip, ngpio} tuples that are used to determine the
> >> width of a particular chip. For backwards compatibility it is assumed that
> >> any chip not defined in CONFIG_SYS_I2C_PCA953X_WIDTH has 8 pins.
> >>
> >> Acked-by: Peter Tyser <ptyser@xes-inc.com>
> >> Tested-by: Peter Tyser <ptyser@xes-inc.com>
> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> >>
> >> ---
> >> Changes since v3:
> >> - pca953x_ngpio is now a function in all cases.
> >> - Added Peter's ACK to the commit message.
> >>
> >>  README                 |    4 ++
> >>  drivers/gpio/pca953x.c |  114 ++++++++++++++++++++++++++++++++++++++----------
> >>  2 files changed, 95 insertions(+), 23 deletions(-)
> >
> > Compiles with actual u-boot the xpedite* boards, so added to
> > u-boot-i2c.git
> >
> > Thanks!
> >
> > bye,
> > Heiko
> 
> Thanks I was about to poke the mailing list to find out where things
> had got to. Did I miss something on
> http://www.denx.de/wiki/U-Boot/Patches that says look <here> list for
> a custodian and CC them?

There's a list of custodians at
http://www.denx.de/wiki/U-Boot/Custodians , but your GPIO driver doesn't
really fall neatly into any one of their areas of responsibility, so
it'd be tough to know who to CC.  I added a comment to
http://www.denx.de/wiki/U-Boot/Patches to mention that appropriate
maintainers should be CC-ed, as well as people who might be familiar
with the change based on past commits for what its worth.

Best,
Peter