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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
