| Message ID | AANLkTik6U8tm44XfKZrpYYT9g2dCw-Tsx8kr7y8mjOF6@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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");
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(-)