[U-Boot] [PATCHv2] pca953x: support 16-pin devices
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");
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("--------");
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
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
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");
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
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
/*
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
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");
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
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?
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
participants (4)
-
Chris Packham -
Heiko Schocher -
Peter Tyser -
Wolfgang Denk