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