[i2c] [PATCH] Add a new-style driver for most I2C EEPROMs

Jean Delvare khali at linux-fr.org
Mon Apr 14 14:39:25 CEST 2008


Hi Robert, hi Wolfram,

On Mon, 14 Apr 2008 09:22:27 +0200, Robert Schwebel wrote:
> On Fri, Apr 11, 2008 at 02:24:55PM +0200, Wolfram Sang wrote:
> > > + * at24.c - handle most I2C EEPROMs
> > Maybe rename the driver? at24 is vendor-specific. 24xx? 24cxx?
> > eeprom-ng? I'd go for 24xx.
> 
> Don't use to many "xx" characters. Last time someone did that, he was
> blocked by rmk's spam filter :-) Maybe "i2c-eeprom"?

Definitely not. We use i2c-* for I2C _bus_ drivers, not I2C chip
drivers, so "i2c-eeprom" would be confusing. "at24" is just fine and I
see no reason to change the name. The Linux driver tree is full of
vendor-specific names, and this has never been a problem.

<snip>

> > To support the X24645, it would be necessary to raise AT24_MAX_CLIENTS
> > to 32 (what a beast!). Then again, most eeproms will just need one
> > client, so this would cause quite some overhead in most use-cases.
> > Maybe it pays off to hande this dynamically?
> 
> As eeproms are normally slow things anyway, would it be a big
> performance impact?

I agree that going dynamic makes sense. Not that it has anything to do
with the speed of EEPROMs though - the memory allocation will be done
at device initialization time, and after that dynamic or not should
perform just the same.

> > > +static struct i2c_client *at24_ee_address(
> > > +	struct at24_data	*at24,
> > > +	u16			*addr,
> > > +	unsigned		*offset
> 
> Minor nit: the kernel usually doesn't aling the variables vertically; if
> you add more and longer data types, it might be necessary to move around
> all other lines, which clutters patches. So I'd write this as
> 
> 	struct at24_data *at24,
> 	u16 *addr,
> 	unsigned *offset

Or even better, everything on the same line until you hit the 80 column
limit, etc.

-- 
Jean Delvare



More information about the i2c mailing list