Hi Marc,
On Mon, Jul 30, 2012 at 08:36:46PM +1000, Marc Reilly wrote:
> Thanks for comments. You are very thorough, and I mean that in a nice way.
> I gather you also have an at24 driver, did you support addressing offsets >
> 256?
yes, our driver does. But it hardcodes two address bytes similar to your
driver hardcoding a single byte :-)
> > > +#define DRIVERNAME "at24"
> > > +#define DEVICENAME "eeprom"
> >
> > why not DEVICENAME == DRIVERNAME?
>
> Originally I'd started with DRIVENAME as eeprom, but changed it later as that
> seemed too generic. I wanted to keep the device name as eeprom so that the
> device would be "/dev/eepromX", both for compatibilty with existing board
> setup and as a conceptual abstraction to regard the device as a more generic
> "eeprom" device, rather than a more specific "at24".
> (Hope that makes sense).
Ah, I missed that your devices have a number included after eeprom. Then
I'm ok with your approach.
> > > + while (i && retries) {
> > > + ret = i2c_read_reg(priv->client, offset, buf, i);
> > > + if (ret < 0)
> > > + return (ssize_t)ret;
> >
> > This cast is also done implicitly.
> >
> > > + else if (ret == 0)
> > > + --retries;
> > > +
> > > + numwritten += ret;
> > > + i -= ret;
> > > + offset += ret;
> > > + buf += ret;
> > > + }
> >
> > IMHO this loop should be in a more generic place once instead of
> > repeating it for each driver. Also I wonder if you saw the eeprom
> > yielding some but not all requested bytes on a read request.
>
> Not that I can remember, but this code is old, and I think was copied from a
> kernel driver and I just left as is.
> I considered doing a generic loop, but in my head anything I thought of wasn't
> much better than having similar code 2 or 3 times.
I think it would be worth to have it in generic code. (For our driver I
did it in the command that implements the custom eeprom layout. So I
don't have anything to share.)
> > > +static int at24_poll_device(struct i2c_client *client)
> > > +{
> > > + uint64_t start;
> > > + u8 dummy;
> > > + int ret;
> > > +
> > > + /*
> > > + * eeprom can take 5-10ms to write, during which time it
> > > + * will not respond. Poll it by attempting reads.
> > > + */
> > > + start = get_time_ns();
> > > + while (1) {
> > > + ret = i2c_master_recv(client, &dummy, 1);
> >
> > I implemented a write of length 0 here. IMHO this is better.
>
> I'm not clear whether you are saying that your way is better :)
> This way reads just one byte after device responds. I'm thinking that your way
> would write a byte for the address...
/me rechecks ... Hm, our driver uses:
i2c_write_reg(at24->client, I2C_ADDR_16_BIT, buf, 0);
so it even transfers two bytes for the address, so regarding the
generated overhead, you're right. But "my" datasheet[1] says:
ACKNOWLEDGE POLLING: Once the internally-timed write cycle has
started and the EEPROM inputs are disabled, acknowledge polling
can be initiated. This involves sending a start condition
followed by the device address word. The read/write bit is
representative of the operation desired. Only if the internal
write cycle has completed will the EEPROM respond with a zero,
allowing the read or write sequence to continue.
So I think you must not do a read operation to check if a write is
possible. That might be a theoretical problem now, but still I prefer
being aligned to the datasheet.
[1] http://www.atmel.com/Images/doc0670.pdf
> > I think conceptually you don't need the erase callback for this eeprom.
>
> While it is possible to write 0xff through the device, this was more
> convenient. I'd prefer to keep it, unless theres a reason otherwise.
I don't care much, but IMHO the erase callback is for devices that need
the erase before writing.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
