From mboxrd@z Thu Jan  1 00:00:00 1970
From: Nishanth Menon <nm@ti.com>
Date: Tue, 9 Jul 2013 11:21:04 -0500
Subject: [U-Boot] [PATCH 1/2] gpio: tca642x: Add the tca642x gpio
 expander driver
In-Reply-To: <51DC3790.60104@ti.com>
References: <1373316840-28436-1-git-send-email-dmurphy@ti.com>
	<51DB2EDD.5040107@ti.com> <51DC3790.60104@ti.com>
Message-ID: <51DC3870.4080408@ti.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 07/09/2013 11:17 AM, Dan Murphy wrote:

> On 07/08/2013 04:27 PM, Nishanth Menon wrote:
>> On 07/08/2013 03:53 PM, Dan Murphy wrote:
[...]
>>> +/* 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.

something like
{.input_reg = 0x00, .polarity_reg = 0x04, ....

It helps readability and future modification to bank_info structure.

[..]
>>> +    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.

reg_setbits? instead then? following the standard api convention - maybe 
make this as a i2c generic api?
-- 
Regards,
Nishanth Menon