[i2c] [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[Jean Delvare]

On Fri, 11 Jul 2008 16:57:36 +0800, eric miao wrote:
> On Fri, Jul 11, 2008 at 4:29 PM, Jean Delvare <khali at linux-fr.org> wrote:
> > On Thu, 10 Jul 2008 14:13:39 +0800, Eric Miao wrote:
> >> + *   - Group A : by I2C address 0b'110xxxx
> >> + *   - Group B : by I2C address 0b'101xxxx
> >> + *
> >> + * where 'xxxx' is decided by the connections of pin AD2/AD0.
> >
> > AD2-AD0 (assuming there there is an AD1 pin)
> 
> Unfortunately, no AD1 pin

Ah, these are 2 four-state address pins, I get it now.

> >> (...)
> >> +#define PORT_NONE    0x0     /* '/' No Port */
> >
> > You don't use this define anywhere.
> 
> Just defined here to illustrate the purpose of the "0" here means
> NO PORT exist in that bit position, it helps people to better
> understand the port organization and initialization sequence

Could be a comment rather than a define, but up to you.

> >> (...)
> >> +     nr_port = port;
> >
> > Why do you need 2 variables for that?
> 
> Again, better readability :-)
> 
> I can remove that if you mind.

Personally I tend to think that it makes the readability worse not
better. Looking at just the end of the function, I see:

	if (nr_port > 7) {
(...)
	gc->ngpio = port;

And I have to scroll up a bit to find out that "nr_port" and "port"
always have the same value by construction. So indeed I would suggest
to drop "port" and use "nr_port" everywhere for clarity.

Oh, and one more thing as I just notice it:

> +static inline int is_group_a(struct max732x_chip *chip, unsigned off)
> +{
> +	return (1u << off) & chip->mask_group_a;
> +}

Given the way you use it, can't you just define this function as:

static inline int is_group_a(struct max732x_chip *chip, unsigned off)
{
	return (off < 8);
}

? As this is the only place where you use chip->mask_group_a, you would
be able to get rid of it.

-- 
Jean Delvare