[U-Boot] [PATCH 1/2] gpio: tca642x: Add the tca642x gpio expander driver

[Dan Murphy <dmurphy@ti.com>]

Nishanth
Thanks for the feedback

On 07/08/2013 04:27 PM, Nishanth Menon wrote:
> On 07/08/2013 03:53 PM, Dan Murphy wrote:
>> Add the tca642x gpio expander driver
> http://www.ti.com/sitesearch/docs/universalsearch.tsp?searchTerm=tca642&linkId=1 
>
> I think there are just TCA6424 ?
> giving a link to the product helps I suppose.
Done.  Next patchset
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   drivers/gpio/Makefile  |    1 +
>>   drivers/gpio/tca642x.c |  312 
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/tca642x.h      |   64 ++++++++++
>>   3 files changed, 377 insertions(+)
>>   create mode 100644 drivers/gpio/tca642x.c
>>   create mode 100644 include/tca642x.h
>>
>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>> index f77c1ec..7e74dfe 100644
>> --- a/drivers/gpio/Makefile
>> +++ b/drivers/gpio/Makefile
>> @@ -49,6 +49,7 @@ COBJS-$(CONFIG_BCM2835_GPIO)    += bcm2835_gpio.o
>>   COBJS-$(CONFIG_S3C2440_GPIO)    += s3c2440_gpio.o
>>   COBJS-$(CONFIG_XILINX_GPIO)    += xilinx_gpio.o
>>   COBJS-$(CONFIG_ADI_GPIO2)    += adi_gpio2.o
>> +COBJS-$(CONFIG_TCA642X)        += tca642x.o
>>
>>   COBJS    := $(COBJS-y)
>>   SRCS     := $(COBJS:.o=.c)
>> diff --git a/drivers/gpio/tca642x.c b/drivers/gpio/tca642x.c
>> new file mode 100644
>> index 0000000..740714d
>> --- /dev/null
>> +++ b/drivers/gpio/tca642x.c
>> @@ -0,0 +1,312 @@
>> +/*
>> + * Copyright 2013 Texas Instruments, Inc.
>> + *
>> + * Derived work from the pca953x.c driver
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * Version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>> + */
>> +
>> +#include <common.h>
>> +#include <i2c.h>
>> +#include <tca642x.h>
>> +
>> +/* tca642x register address definitions */
>> +struct tca642x_bank_info tca642x_banks[] = {
>> +    {0x00, 0x04, 0x08, 0x0c},
>> +    {0x01, 0x05, 0x09, 0x0d},
>> +    {0x02, 0x06, 0x0a, 0x0e},
> could we use explicit map of the struct params? helps any future 
> expansion.
>
Not sure what you are looking for here.  These are the register 
addresses within the chip.
>> +};
>> +
>> +/*
>> + * Modify masked bits in register
>> + */
>> +static int tca642x_reg_write(uint8_t chip, uint addr, uint mask, 
>> uint data)
>> +{
>> +    uint16_t valw;
> why not use u8 instead? same for mask and data?
Doh.  That was carried over from the derivation work. Done.  Next patchset
>> +    int org_bus_num;
>> +    int ret;
>> +
>> +    org_bus_num = i2c_get_bus_num();
>> +    i2c_set_bus_num(CONFIG_SYS_I2C_TCA642X_BUS_NUM);
>> +
>> +    if (i2c_read(chip, addr, 1, (u8 *)&valw, 1)) {
>> +        printf("Could not read before writing\n");
>> +        ret = -1;
>> +        goto error;
>> +    }
>> +    valw &= ~mask;
> data &= mask ?
Well in this case the mask is not being passed in.  The bit to flip is 
being passed and then the actual mask
is created here.

This code should be fine unless others object as well.
>> +    valw |= data;
>> +
>> +    ret = i2c_write(chip, addr, 1, (u8 *)&valw, 1);
>> +
>> +error:
>> +    i2c_set_bus_num(org_bus_num);
>> +    return ret;
>> +
> extra eol?
Done.  Next patchset
>> +}
>> +
>> +static int tca642x_reg_read(uint8_t chip, uint addr, uint *data)
> same here? addr is u8, data is u8 *?
Done.  Next patchset
>> +{
>> +    uint16_t valw;
> val as u8?
Done.  Next patchset
>> +    int org_bus_num;
>> +    int ret = 0;
>> +
>> +    org_bus_num = i2c_get_bus_num();
>> +    i2c_set_bus_num(CONFIG_SYS_I2C_TCA642X_BUS_NUM);
>> +    if (i2c_read(chip, addr, 1, (u8 *)&valw, 1)) {
>> +        ret = -1;
>> +        goto error;
>> +    }
>> +
>> +    *data = (int)valw;
>> +
>> +error:
>> +    i2c_set_bus_num(org_bus_num);
>> +    return ret;
>> +}
> [...]


-- 
----------------------------------------------
Dan Murphy