[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