Re: [PATCH v4] New-style EEPROM driver using device_ids (superseding the old eeprom driver)

[Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2521 bytes --]

On Wed, Jul 02, 2008 at 04:30:04PM +0200, Jean Delvare wrote:

> Great. The code looks very nice now, I think it's ready for merge.
Hooray! \o/

> > +/*
> > + * Specs often allow 5 msec for a page write, sometimes 20 msec;
> > + * it's important to recover from write timeouts.
> > + */
> > +static unsigned write_timeout = 25;
> > +module_param(write_timeout, uint, 0);
> 
> No longer changeable at run-time? I don't mind, but I'm curious why you
> changed it.

Sorry, forgot to document it. When wearing my paranoia-hat, I wondered
what happened if you start writing to an EEPROM with a sufficent value
and change it to an insufficent one while still writing to it. You will
most probably end up with broken eeprom content. This is why I thought
one should better measuer a good value once, and then keep it. Measuring
can easily be done by reloading the module with a different parameter
(or hacking the source if you want to change it run-time).

> > +	{ "24c02", AT24_DEVICE_MAGIC(2048 / 8, 0) },
> > +	/* spd is a 24c02 in memory DIMMs */
> > +	{ "spd", AT24_DEVICE_MAGIC(2048 / 8,
> > +		AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
> > +	/* pcf8570 has SRAM only, write it all */
> > +	{ "pcf8570", AT24_DEVICE_MAGIC(2048 / 8, 0) },
> 
> Now that you use page-size of 1 in all cases, this comment is a bit
> confusing. And I am curious if anyone will use this entry, given that
> its the same as "24c02", and anyone who is certain to have a PCF8570
> would provide platform data to take benefit of the lack of pages of
> this chip.
> 
> I would at least remove the second part of the comment, and maybe even
> the entry, unless you add a new flag AT2C_FLAG_SRAM which automatically
> sets a large page size (if that makes sense) or restore the possibility
> to set the page size, just for this entry.

ACK. I'd say, delete this entry for now. I guess the 'page_size'-issue
will arise sooner or later anyhow, maybe then this entry can be
reactivated.

> > +	if (chip.flags & AT24_FLAG_TAKE8ADDR)
> > +		num_addresses = 8;
> > +	else
> > +		num_addresses =	DIV_ROUND_UP(chip.byte_len,
> > +			chip.flags & AT24_FLAG_ADDR16 ? 65536 : 256);
> 
> Extra parentheses wouldn't hurt... I wouldn't know off the top of my
> head, which of & and ?: has greater precedence.
Okay. Seems I got used to it that ?: is really low.

All the best,

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c