[U-Boot] [PATCHv2] pca953x: support 16-pin devices

[Chris Packham]

On Thu, Dec 9, 2010 at 1:08 PM, Peter Tyser <ptyser at 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