Discussion:
at24c i2c EEPROM driver
(too old to reply)
David Brownell
2005-07-12 00:37:11 UTC
Permalink
I ran into one too many board with an I2C EEPROM that Linux handled poorly,
and got a hack attack. Here's the result ... tested on at24c04 but it
should work on most other chips in the at24c series without much effort.
(Including on the at24c1024, 1 MBit of EEPROM... more than enough to
hold say "u-Boot", along with product config data.)

If someone wants to pick this up and do things with this, feel free; there's
an obvious gap in how it initializes, since these chips can't very usefully
be probed. I have no current plans to submit this, but feel free to fling
patches my way.

This is modeled more or less after "eeprom" but of course it handles not
only read/write access, but also chips with more than 2 Kbits of storage.
So there's the notion that on some systems it might replace "eeprom".

- Dave



-------------- next part --------------
A non-text attachment was scrubbed...
Name: at24c.patch
Type: text/x-diff
Size: 14851 bytes
Desc: not available
URL: <http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050711/10f5b0bc/attachment.bin>
David Brownell
2005-07-12 11:50:54 UTC
Permalink
Post by David Brownell
I ran into one too many board with an I2C EEPROM that Linux handled poorly,
and got a hack attack. Here's the result ... tested on at24c04 but it
should work on most other chips in the at24c series without much effort.
Hmm, and it appears most other I2C EEPROMs too; I happened across a
chart with about eight vendors showing part number equivalence.
And the AT24C column was big, but maybe not the biggest.

So now what I need is some 8 pin DIP sockets on a Linux machine
and then I can use this driver to program the 24LCxx EEPROMs
used on some 8 bit microcontrollers I've got sitting around (on
the grounds that they won't run Linux). Hmm. :)

- Dave
Jean Delvare
2005-07-12 13:54:42 UTC
Permalink
Hi David,
Post by David Brownell
I ran into one too many board with an I2C EEPROM that Linux handled poorly,
and got a hack attack.
Could you please define "handled poorly"?
Post by David Brownell
If someone wants to pick this up and do things with this, feel free;
there's an obvious gap in how it initializes, since these chips can't
very usefully be probed. I have no current plans to submit this, but
feel free to fling patches my way.
You are not the first one complaining about the limitations of the simple
eeprom driver. So far we have been able to work around them, typically
by using user-space tools to program the eeproms. The question is
whether your case is different or not. Is eeprog not sufficient for your
needs?

http://www2.lm-sensors.nu/~lm78/cvs/browse.cgi/lm_sensors2/prog/eepromer

If you followed the story of Ben Gardner's max6875 driver, you must
realize that drivers for I2C devices in the 0x50-0x57 range should be
VERY careful not to accidentally write to an innocent EEPROM. The
I2C/SMBus protocols make it happen very easily, unfortunately.
Post by David Brownell
This is modeled more or less after "eeprom" but of course it handles not
only read/write access, but also chips with more than 2 Kbits of storage.
So there's the notion that on some systems it might replace "eeprom".
The eeprom driver *did* support EEPROMs of more than 2 Kbits (actually up
to 16 Kbits) although maybe not in a very elegant way.

If your driver is to replace the eeprom driver (you will have to convince
me it is needed and safe), then it better replace it completely on all
systems. Let's not duplicate such a simple functionality.
Post by David Brownell
Tested only with AT24C04 so far, but support for many others is
coded and ready for you once you decide how you want to handle
the board-specific configuration issues.
What "board-specific configuration issues" are you thinking of?

Now, my comments on the patch itself.
Post by David Brownell
+obj-$(CONFIG_I2C_AT24C) += at24c.o
FWIW, CONFIG_I2C_* have always been used for i2c core drivers and i2c
adapter drivers (aka i2c bus drivers), not for i2c chip drivers. Those
were using either CONFIG_SENSORS_* for hardware monitoring drivers (and
some others, but this is a mistake), or no particular prefix for the
other ones. Now that we moved the hardware monitoring drivers outside of
the i2c/chips subdirectory, I'd like to make things a bit more
standard. I have no strong opinion on what naming scheme we should use,
but I sure would like us to pick one and stick to it.
Post by David Brownell
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ osk/drivers/i2c/chips/at24c.c 2005-07-11 17:06:28.000000000 -0700
@@ -0,0 +1,502 @@
+/*
+ * drivers/i2c/chips/at24c.c -- support some Atmel AT24C EEPROMs
Having the full path in the comment is redundant and error-prone. We know
where the file currently is, but that may change in the future, as was
seen recently with all the hardware monitoring drivers.
Post by David Brownell
+#undef DEBUG
Needless.
Post by David Brownell
+ * - for one 4Kbit EEPROM with A0..A2 grounded, kernel params
+ * at24c.force=0,0x50,-1 at24c.sizes=4
+ * - one 2Kbit EEPROM with A0..A2 grounded; one with them pulled high
+ * at24c.force=0,0x50,0,0x57,-1 at24c.sizes=2,2
I don't know where you got these "-1" from but they are not correct.
i2c "force" parameters should always be given by pair.

As the legacy parameter names were all singular, I would suggest "size"
rather than "sizes" for the new parameter name.

Also, why do you use "force" rather than "probe" here?
Post by David Brownell
+ * It's also OK to leave off the 0x5 prefix.
I consider this a bad idea, as this adds code with no benefit and is even
error-prone as it breaks the compatibility with the way all other i2c
chip drivers handle that parameter.
Post by David Brownell
+/* to a first approximation: kbits = chiptype */
+enum chiptype {
+ /* generic 2Kbit, read-only; as seen on memory DIMMS etc */
+ generic = 0,
+ /* single byte addressing, sometimes using I2C addresses too */
+ at24c01 = 1,
+ at24c02 = 2,
What you call "generic" are perfectly standard at24c02-like EEPROMs, so
I see no point in making them a particular case. If it's just to make
them read-only and all the other ones read-write, this looks a bad idea
to merge the writability with the size in a single parameter. Sooner or
later someone will need a read-only access to a larger EEPROM, and this
will break.

What do you mean with "sometimes using I2C addresses too"?
Post by David Brownell
+struct at24c_data {
+ struct i2c_client client;
+ struct semaphore lock;
+ struct bin_attribute bin;
+ u32 next_addr;
+ u16 chiptype;
+ u16 pagesize;
+ u8 addr_len;
+};
Maybe some comments on what next_addr, pagesize and addr_len are for?
(The mix of member names using underscores with names not using them
isn't exactly nice, BTW.)
Post by David Brownell
+static int at24c_ee_address(
+ unsigned chiptype,
+ struct i2c_msg *msg,
+ unsigned *offset,
+ u32 *next_addr
+)
+{
Eek. What's this coding style?
Post by David Brownell
/* advantages of not using i2c_smbus_read_i2c_block_data() here
* (a) uses addresses after at24c->client.addr, as needed;
* (b) handles two-byte "register" addresses;
* (c) this can often omit the initial dummy write;
* (d) reads everything at once, not in 32-byte niblets;
* (e) less data copying.
*/
Points (a) and (c) would need clarification.

Point (b): You could as well implement a generic function in i2c-core.
I2C_FUNC_SMBUS_READ_I2C_BLOCK_2 is already reserved for that in i2c.h,
which suggests that SMBus 2.0 defines it (although I can't remember of
any hardware implementimg it).

Points (d) and (e) suggest that the i2c-core API is not adapted to the
needs. You are not the first one to complain about the i2c block
transfers API, yet everyone always go coding its own implementation on
its side. I'd like someone to properly analyze the situation and make
concrete proposals for a change.

Note that your implementation requires a plain I2C bus master. SMBus
masters won't possibly work with it. This is a significant drawback. If
you want this driver to ever replace the eeprom driver, you will have to
make it work on SMBus as well.
Post by David Brownell
at24c_bin_write(struct kobject *kobj, char *buf, loff_t off, size_t count)
Could you possibly split the i2c-write part out of this function, just
like you did for the read part? This would make it more modular and
readable.
Post by David Brownell
/* don't know how to handle this chip type yet! */
return -ENOSYS;
You have a memory leak here, and also a few lines later in the same
function. You really should centralize your failure path to prevent that.
Post by David Brownell
strcpy(at24c->client.name, "generic EEPROM");
Please use strlcpy instead of strcpy.
Post by David Brownell
/* some chips take two or more addresses ... register using the
* first one, and hope no other driver claims the others!
*/
No, don't just hope. Properly register the other addresses as
subclients. See the asb100 driver for an example of how to do that.
Post by David Brownell
address |= 0x50;
As said earlier, this is dangerous and shouldn't be done at all.
Post by David Brownell
/* FIXME poke it, to see if there's actually a chip there! */
This is the i2c-core job and you would have it for free if you were using
"probe" rather than "force" as suggested above. Note that using
"probe" would require you to include the 24RF08 corruption preventer
(until I find some time to work around the problem more globally).
Post by David Brownell
static int at24c_attach_adapter(struct i2c_adapter *adapter)
{
if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) {
dev_dbg(&adapter->dev, "%s probe, no I2C\n",
at24c_driver.name);
return 0;
}
return i2c_probe(adapter, &addr_data, at24c_probe);
}
This is interesting. In all other drivers we are checking for the
functionality inside the *_probe (or *_detect) function rather than in
the *_attach_adapter function. Now that I think of it, this sounds
sub-optimal, your approach should be more efficient. Unless I'm missing
something, we should probably change all other drivers to match this one.
Post by David Brownell
kfree(container_of(client, struct at24c_data, client));
You should use i2c_get_clientdata() (just like you should have used
i2c_set_clientdata() earlier.)

No MODULE_AUTHOR?

Note that your driver does not cache the reads to the EEPROM. While it
may be on purpose, it raises concerns because you gave read access to
everyone through sysfs, which means that any random user could saturate
the I2C bus the EEPROM is on. If there are other critical chips on the
bus (think battery controller, hardware monitor...) then we are in
trouble.

I really appreciate that you have made your driver non-intrusive, in that
it won't attach to any device by default. However, I would go one step
further, by having a "writable" attribute separate from the concept of
"default eeprom" which I don't really like. This could either be a
module parameter, or a second sysfs file (I think we had a proposal in
that direction some times ago).

--
Jean Delvare
david-b
2005-07-12 17:48:47 UTC
Permalink
Date: Tue, 12 Jul 2005 15:54:42 +0200 (CEST)
From: "Jean Delvare" <khali at linux-fr.org>
Hi David,
Post by David Brownell
I ran into one too many board with an I2C EEPROM that Linux handled poorly,
and got a hack attack.
Could you please define "handled poorly"?
In the case of the board with an AT24C1024, no kernel support at all.
(That's got 2-byte addressing.) I wanted to put U-Boot on that, and
have some way to access the leftover space, so the NOR flash (at a
whopping 2 MBytes!) had more space available for Real Stuff.

For the AT24C04, it was misdetected as two chips (by "eeprom"), and
there was no write support.
You are not the first one complaining about the limitations of the simple
eeprom driver.
Those weren't exactly the words I used. In public, anyway. :)
So far we have been able to work around them, typically
by using user-space tools to program the eeproms. The question is
whether your case is different or not. Is eeprog not sufficient for your
needs?
http://www2.lm-sensors.nu/~lm78/cvs/browse.cgi/lm_sensors2/prog/eepromer
Interesting, it might be worth mentioning that in the Kconfig helptext
for the "eeprom" driver. I did some googling, and that never came up.
(Mention that along with the fact that "eeprom" only handles chips of up
to about 16 Kbits, and misdetects many of those as multiple chips ... )

I've got a few applications in mind. If I stay un-demanding, "eeprog"
could suffice -- though clearly it'd be slower. The moment I want
other kernel drivers to use that state, it wouldn't be enough.
If you followed the story of Ben Gardner's max6875 driver, you must
realize that drivers for I2C devices in the 0x50-0x57 range should be
VERY careful not to accidentally write to an innocent EEPROM. The
I2C/SMBus protocols make it happen very easily, unfortunately.
Didn't follow that, but I'm fully aware of the issue. That's why I
set this driver up to use only static configuration, no probing.

It's unrealistic to expect the kernel to defend against user error,
but it's highly appropriate to expect it not to misconfigure.
If your driver is to replace the eeprom driver (you will have to convince
me it is needed and safe), then it better replace it completely on all
systems. Let's not duplicate such a simple functionality.
I only noted that such a thing was a possibility. For the moment,
I've met my own goals with respect to this driver. Anyone wanting
to take that next step now has a better tool to start with. :)


The main thing I'd put on the agenda if anyone were were to pursue
that route would be how to provide a decent static declaration of
boards' I2C configuration, to prevent needing to probe things that
don't probe well. Such a thing might be done as part of updating
the I2C stack for better driver model support, whenever time for
such a thing becomes available.

For example, the "platform_bus" is often prepared, in board-specific
setup code, with a platform_device for each device that's present,
plus platform_data with information needed by the driver (like what
GPIOs it uses on this board). That'd be a useful model for this
case: "at address N on this bus there is/may-be a B Kilobit EEPROM
to expose as read-only".

I'll follow up on your code comments in a separate email/thread;
thanks for the feedback!

- Dave
david-b
2005-07-12 19:54:34 UTC
Permalink
Date: Tue, 12 Jul 2005 15:54:42 +0200 (CEST)
From: "Jean Delvare" <khali at linux-fr.org>
...
Post by David Brownell
Tested only with AT24C04 so far, but support for many others is
coded and ready for you once you decide how you want to handle
the board-specific configuration issues.
What "board-specific configuration issues" are you thinking of?
How to declare to the driver -- ideally as part of board-specific init,
rather than by modifying the driver -- what chips a given board has,
and where. That shouldn't live in the driver, since so many boards
have I2C EEPROMS for various purposes.

This does _not_ look very amenable to probing, and from what you
wrote I suspect that other folk have confirmed that in practice.
Now, my comments on the patch itself.
Post by David Brownell
+obj-$(CONFIG_I2C_AT24C) += at24c.o
FWIW, CONFIG_I2C_* have always been used for i2c core drivers and i2c
adapter drivers (aka i2c bus drivers), not for i2c chip drivers. Those
were using either CONFIG_SENSORS_* for hardware monitoring drivers (and
some others, but this is a mistake), or no particular prefix for the
other ones. Now that we moved the hardware monitoring drivers outside of
the i2c/chips subdirectory, I'd like to make things a bit more
standard. I have no strong opinion on what naming scheme we should use,
but I sure would like us to pick one and stick to it.
Me too. Though another option is to have no prefix at all.

I hadn't realized CONFIG_I2C_ was "claimed" in any sense; in
USB-land, the CONFIG_USB_ prefix is used for most everything,
both host side and peripheral side.


I did decide to NOT use the CONFIG_SENSORS_* stuff though. :)

How would for example SPI sensors fit in? I recently noticed
that chips like the TSC2101 and ADS7846 aren't just used for
touchscreen X/Y/presure sensors ... they also have temperature
and battery voltage sensors. SENSOR should not imply I2C...
Post by David Brownell
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ osk/drivers/i2c/chips/at24c.c 2005-07-11 17:06:28.000000000 -0700
@@ -0,0 +1,502 @@
+/*
+ * drivers/i2c/chips/at24c.c -- support some Atmel AT24C EEPROMs
Having the full path in the comment is redundant and error-prone. We know
where the file currently is, but that may change in the future, as was
seen recently with all the hardware monitoring drivers.
Easily changed. (Background noise: keyboard clicking.) There.
Post by David Brownell
+#undef DEBUG
Needless.
Though conventional in many drivers, highlighting the fact that
to get _selective_ debugging (this driver only!) you just change
that one line ...
Post by David Brownell
+ * - for one 4Kbit EEPROM with A0..A2 grounded, kernel params
+ * at24c.force=0,0x50,-1 at24c.sizes=4
+ * - one 2Kbit EEPROM with A0..A2 grounded; one with them pulled high
+ * at24c.force=0,0x50,0,0x57,-1 at24c.sizes=2,2
I don't know where you got these "-1" from but they are not correct.
i2c "force" parameters should always be given by pair.
I'm still not used to the way the I2C parameters are done. The
notion of pre-allocating several large arrays, pre-initialized
with "end" tags, still feels odd to me.
As the legacy parameter names were all singular, I would suggest "size"
rather than "sizes" for the new parameter name.
Good enough, that part's not yet implemented anyway.
There should also be per-chip "is it readonly?" flag.
And maybe a non-I2C parameter limiting bus utilization.
Also, why do you use "force" rather than "probe" here?
That's the model: the driver will do NOTHING unless it's explicitly
told to. Initializing "probe" (like "eeprom" does) would mean the
driver will mis-detect some chips.

And wasn't that 24RF08 problem a consequence of using probing?
Post by David Brownell
+ * It's also OK to leave off the 0x5 prefix.
I consider this a bad idea, as this adds code with no benefit and is even
error-prone as it breaks the compatibility with the way all other i2c
chip drivers handle that parameter.
Thing is, the addresses do need to be filtered/munged since they
are user inputs and not all values are legit. The code I used
to munge the things forced the 0x5 rather than create error cases.
Not that it couldn't be a bit cleaner, but that's what it is now.
Post by David Brownell
+/* to a first approximation: kbits = chiptype */
+enum chiptype {
+ /* generic 2Kbit, read-only; as seen on memory DIMMS etc */
+ generic = 0,
+ /* single byte addressing, sometimes using I2C addresses too */
+ at24c01 = 1,
+ at24c02 = 2,
What you call "generic" are perfectly standard at24c02-like EEPROMs, so
I see no point in making them a particular case. If it's just to make
them read-only and all the other ones read-write, this looks a bad idea
to merge the writability with the size in a single parameter.
The issue was how to handle backward compat with "eeprom"...
and the plan was to treat all uninitialized sizes as "generic".

Which is _exactly_ what "eeprom" does. If it's good enough
for that driver, it should be good enough here ... though as
I commented, "eeprom" mis-handles at least double byte address
cases, also chips of size != 2 Kbits, so maybe it's not really
"good enough" there. ;)
Sooner or
later someone will need a read-only access to a larger EEPROM, and this
will break.
As I noted. The whole board-specific config issue is incomplete,
and that's clearly one issue in that mix.
What do you mean with "sometimes using I2C addresses too"?
Should have read "multiple" I2C addresses. This is your issue
with (a) below. I updated the code to have a comment at the
top explaining this a bit, using the xample of an at24c08
which uses four I2C address for one chip.
Post by David Brownell
+struct at24c_data {
+ struct i2c_client client;
+ struct semaphore lock;
+ struct bin_attribute bin;
+ u32 next_addr;
+ u16 chiptype;
+ u16 pagesize;
+ u8 addr_len;
+};
Maybe some comments on what next_addr, pagesize and addr_len are for?
Comments are fine things. I avoid documenting data structures
much while they're in flux though.
(The mix of member names using underscores with names not using them
isn't exactly nice, BTW.)
There must be a lot of code you count as "not exactly nice" then!!
I added underscores to chip_type and page_size.
Post by David Brownell
+static int at24c_ee_address(
+ unsigned chiptype,
+ struct i2c_msg *msg,
+ unsigned *offset,
+ u32 *next_addr
+)
+{
Eek. What's this coding style?
You mean, "wrap lines at 80 characters"? Or are you maybe
feeling religious about where the right paren goes? :)

One could argue that function parameters are just single-purpose
structures that (often) live on the stack ... many style guidelines
say to format long parameter lists like structures. The kernel
guidelines are agnostic on that point, and I've always found
that structured lists like that are easier to make sense of.
Post by David Brownell
/* advantages of not using i2c_smbus_read_i2c_block_data() here
* (a) uses addresses after at24c->client.addr, as needed;
* (b) handles two-byte "register" addresses;
* (c) this can often omit the initial dummy write;
* (d) reads everything at once, not in 32-byte niblets;
* (e) less data copying.
*/
Points (a) and (c) would need clarification.
Done.
Point (b): You could as well implement a generic function in i2c-core.
I2C_FUNC_SMBUS_READ_I2C_BLOCK_2 is already reserved for that in i2c.h,
which suggests that SMBus 2.0 defines it (although I can't remember of
any hardware implementimg it).
If no SMBUS hardware implements it, I'm not sure what the point of
such a function would be! For now, this version is portable enough.
Points (d) and (e) suggest that the i2c-core API is not adapted to the
needs. You are not the first one to complain about the i2c block
transfers API, yet everyone always go coding its own implementation on
its side. I'd like someone to properly analyze the situation and make
concrete proposals for a change.
Hmm? I thought I _was_ using the I2C block transfer API. The
limitations I'm thinking of are SMBUS policy (32 byte niblets)
or from its API. (Note that this driver doesn't escape copying
things written to EEPROM -- but that's relatively rare, and not
a case where zerocopy arguments should apply.)

You touched on a minor downside to (d) later: this can mean
some BIG reads, denying the bus to other drivers.. Easily
addressed by putting a ceiling on the amount of data read
at once; and better a per-system ceiling than a global one.
Note that your implementation requires a plain I2C bus master. SMBus
masters won't possibly work with it. This is a significant drawback. If
you want this driver to ever replace the eeprom driver, you will have to
make it work on SMBus as well.
All the Linux systems I do I2C on seem to have full I2C controllers,
and don't need SMBUS limits. They'll have no problem with this driver;
after all, it's for "I2C EEPROMs" not "SMBUS EEPROMs". :)

Maybe there should be drivers that in Kconfig depend on SMBUS, and
others that depend on Real I2C(tm). Configs that don't enable a
real I2C controller wouldn't be able to configure I2C-only drivers.
Post by David Brownell
at24c_bin_write(struct kobject *kobj, char *buf, loff_t off, size_t count)
Could you possibly split the i2c-write part out of this function, just
like you did for the read part? This would make it more modular and
readable.
Maybe later; ideally, that'd be done as part of exporting some API
to other kernel drivers to let them maintain their persistent state.
I'm not sure having a kobject vs an at24c_data parameter helps much.
Post by David Brownell
/* don't know how to handle this chip type yet! */
return -ENOSYS;
You have a memory leak here, and also a few lines later in the same
function. You really should centralize your failure path to prevent that.
Done, thanks.
Post by David Brownell
strcpy(at24c->client.name, "generic EEPROM");
Please use strlcpy instead of strcpy.
I changed that a bit differently.
Post by David Brownell
/* some chips take two or more addresses ... register using the
* first one, and hope no other driver claims the others!
*/
No, don't just hope. Properly register the other addresses as
subclients. See the asb100 driver for an example of how to do that.
For now, this suffices. Especially if the model is that all the
addresses in that range are statically configured. It may be
worth cleaning that up at some point though, yes.
Post by David Brownell
address |= 0x50;
As said earlier, this is dangerous and shouldn't be done at all.
Dangerous how? More dangerous than any other user error?
It's not as if an at24c08 has flexibility about the addresses
it will be using.
Post by David Brownell
/* FIXME poke it, to see if there's actually a chip there! */
This is the i2c-core job and you would have it for free if you were using
"probe" rather than "force" as suggested above. Note that using
"probe" would require you to include the 24RF08 corruption preventer
(until I find some time to work around the problem more globally).
I see two models for system configuration here, loosely "hotplug"
and "static config". They aren't always in conflict, but in the
case of these chips the "hotplug" dynamic config model fails since
the chips aren't self-identifying.

This driver uses _only_ static configuration since there's no safe
way to detect basics like whether the chip at the address needs one
byte addressing or two byte addressing; or what size it is...
Post by David Brownell
static int at24c_attach_adapter(struct i2c_adapter *adapter)
{
if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) {
dev_dbg(&adapter->dev, "%s probe, no I2C\n",
at24c_driver.name);
return 0;
}
return i2c_probe(adapter, &addr_data, at24c_probe);
}
This is interesting. In all other drivers we are checking for the
functionality inside the *_probe (or *_detect) function rather than in
the *_attach_adapter function. Now that I think of it, this sounds
sub-optimal, your approach should be more efficient. Unless I'm missing
something, we should probably change all other drivers to match this one.
Well, tps65010 and isp1301_omap do it in attach(), so maybe not
quite all drivers! :)

But yes, I obviously agree this is a more natural place to
do this particular checking. Luckily those changes will
be simple.

Maybe an even better place for it would be in the I2C core,
by having the I2C drivers declare what level of functionality
it needs from the adapter? One word of data in each driver
(taken from struct padding?) replacing several instructions
and fault paths ... moving to one fault path in the core.
Post by David Brownell
kfree(container_of(client, struct at24c_data, client));
You should use i2c_get_clientdata() (just like you should have used
i2c_set_clientdata() earlier.)
I don't see why. It's not incorrect; it's just a simpler style.

In this case it's less expensive; the compiler will generate
less code since it will notice that the two pointer values are
always the same.
No MODULE_AUTHOR?
Redundant given the copyright statement; unless there's something
special about showing up in "modinfo" output?
Note that your driver does not cache the reads to the EEPROM. While it
may be on purpose, it raises concerns because you gave read access to
everyone through sysfs, which means that any random user could saturate
the I2C bus the EEPROM is on. If there are other critical chips on the
bus (think battery controller, hardware monitor...) then we are in
trouble.
I'll remove group and "other" read access then; good point.
And add a parameter to constrain the read sizes.

I don't want to maintain any caches for data that's not
performance-critical.
I really appreciate that you have made your driver non-intrusive, in that
it won't attach to any device by default. However, I would go one step
further, by having a "writable" attribute separate from the concept of
"default eeprom" which I don't really like. This could either be a
module parameter, or a second sysfs file (I think we had a proposal in
that direction some times ago).
I'd been thinking of doing this with a module parameter.

However, the REALLY nice way to do this would be if the I2C bus
framework had a clean way to handle static device configuration
(like "platform_bus" does). Then such a flag could be just one
of several bits of information provided to driver using driver
model infrastructure like platform_data.

- Dave
--
Jean Delvare
Jean Delvare
2005-07-13 09:14:10 UTC
Permalink
Hi David,
Post by david-b
For the AT24C04, it was misdetected as two chips (by "eeprom"), and
there was no write support.
"Misdetected as two chips" is a view of the mind. The driver just
doesn't care whether there are two physical 24C02 EEPROMs or one
physical 24C04 EEPROM. It will export the EEPROM space as chucks of 256
bytes, regardless of the physical reality. If might not be convenient,
but that's the way it was designed (for the sake of simplicity).

Keep in mind that the eeprom driver was a test/debug thing in the first
place. But it happens to be used by almost everyone now, due to a lack
of alternative or sufficient publicizing thereof.

That being said, it is still not clear to me whether we really need to
use a kernel driver for writing to EEPROMs. EEPROMs programming doesn't
sound like something you need to do very frequently, which is why
user-space would make sense (and was used so far as a matter of fact).
More on that below.
Post by david-b
Post by Jean Delvare
So far we have been able to work around them, typically
by using user-space tools to program the eeproms. The question is
whether your case is different or not. Is eeprog not sufficient for
your needs?
Interesting, it might be worth mentioning that in the Kconfig helptext
for the "eeprom" driver. I did some googling, and that never came up.
(Mention that along with the fact that "eeprom" only handles chips of up
to about 16 Kbits, and misdetects many of those as multiple chips ... )
Good idea. Care to propose a patch?
Post by david-b
I've got a few applications in mind. If I stay un-demanding, "eeprog"
could suffice -- though clearly it'd be slower. The moment I want
other kernel drivers to use that state, it wouldn't be enough.
That's right, this is something the current i2c chip drivers (and the
i2c subsystem as a whole, I fear) are not good at. They are meant as
individual drivers, only interacting with user-space. They do not
integrate with the rest of the kernel, as would sometimes be needed.
That being said, the video i2c chip drivers are working differently and
seem to succeed here, it might be worth taking a look.
Post by david-b
It's unrealistic to expect the kernel to defend against user error,
but it's highly appropriate to expect it not to misconfigure.
When an error can prevent a machine from ever booting, we better add some
serious checks that the user did not misconfigure. It's not unusual for
users to try random module parameters to see if it helps. This is the
reason why I insist that the writability should be handles through an
explicit feature (module parameter, sysfs file, whatever) rather than
associated with a size parameter.
Post by david-b
The main thing I'd put on the agenda if anyone were were to pursue
that route would be how to provide a decent static declaration of
boards' I2C configuration, to prevent needing to probe things that
don't probe well.
Agreed, this would be helpful. Note however that this won't cover all
cases. Some EEPROMs may or may not be present on a given system (think
SPD), and you can't list all motherboard models for x86 anyway. So some
probing or at least manual setting will still be needed.
Post by david-b
Such a thing might be done as part of updating the I2C stack for
better driver model support, whenever time for such a thing becomes
available.
For example, the "platform_bus" is often prepared, in board-specific
setup code, with a platform_device for each device that's present,
plus platform_data with information needed by the driver (like what
GPIOs it uses on this board). That'd be a useful model for this
case: "at address N on this bus there is/may-be a B Kilobit EEPROM
to expose as read-only".
Agreed as well. That's beyong my own knowledge at least at the moment,
but I'd always comment on patches if they were available.

Thanks,
--
Jean Delvare
Jean Delvare
2005-07-13 11:12:06 UTC
Permalink
Hi David,

A few comments on top of your comments to my comments ;)
Post by david-b
This does _not_ look very amenable to probing, and from what you
wrote I suspect that other folk have confirmed that in practice.
There are several levels of problems. First, the quick writes used for
I2C probing aren't 24RF08-safe. This could be worked around (I have
plans). Second, you can really only probe for the existence of an EEPROM
at a given address. You can't reliably detect the size of the EEPROM,
as you found yourself. Third, there are a few chips living in the
0x50-0x57 range which are NOT EEPROMs. Hopefully we can do some positive
discrimination for these, but that's tricky.

Overall I agree that doing as much as possible based on the platform
makes full sense, especially for all these systems heavily relying on
large EEPROMs.
Post by david-b
Post by Jean Delvare
I have no strong opinion on what naming scheme we should use,
but I sure would like us to pick one and stick to it.
Me too. Though another option is to have no prefix at all.
"No prefix at all" is one possible naming scheme ;)
Post by david-b
I hadn't realized CONFIG_I2C_ was "claimed" in any sense; in
USB-land, the CONFIG_USB_ prefix is used for most everything,
both host side and peripheral side.
I didn't say it was claimed, not officially anyway. It just happens to
be that way at the moment. I am fine with changing this, but then we'd
do it consistently. Now that the hardware mnonitoring drivers have gone
away, there are really only a few chip drivers remaining.
Post by david-b
I did decide to NOT use the CONFIG_SENSORS_* stuff though. :)
And quite rightly so, thanks :) I will have to fix the few ones using it
improperly (which is why I insist on agreeing on a naming scheme now
rather sooner than later).
Post by david-b
How would for example SPI sensors fit in? I recently noticed
that chips like the TSC2101 and ADS7846 aren't just used for
touchscreen X/Y/presure sensors ... they also have temperature
and battery voltage sensors. SENSOR should not imply I2C...
This is the very reason why we just moved all the hardware monitoring
stuff (AKA sensors) to a different place. Drivers for W1 or SPI hardware
monitoring decices will be welcome there. There are many articicial
links between the hardware monitoring drivers and the i2c subsystem,
mostly for historical reasons. These prevent us from doing a number of
cleanups. I will work on separating both sides as clearly as possible in
the next few weeks. Hopefully this will open the doors to better
integration and lower footprint as well.
Post by david-b
I'm still not used to the way the I2C parameters are done. The
notion of pre-allocating several large arrays, pre-initialized
with "end" tags, still feels odd to me.
True. Historical. On my list of things to cleanup.
Post by david-b
There should also be per-chip "is it readonly?" flag.
Yup, either as a module parameter or a second sysfs file.
Post by david-b
And maybe a non-I2C parameter limiting bus utilization.
I would make it mandatory, like the original eeprom does. Reading from or
writing to an EEPROM isn't time-critical enough that anyone wouldn't
possibly cope with a limit.
Post by david-b
Post by Jean Delvare
Also, why do you use "force" rather than "probe" here?
That's the model: the driver will do NOTHING unless it's explicitly
told to. Initializing "probe" (like "eeprom" does) would mean the
driver will mis-detect some chips.
The only difference between probe and force at the i2c-core level is that
force won't even try to check whether there is anything responsive at
the given address. Then the driver itself may or may not want to make a
difference between both (using the kind parameter of the detect
function), but your driver doesn't.
Post by david-b
And wasn't that 24RF08 problem a consequence of using probing?
True, although a weak workaround exist.

I am fine with you using force for now, but once I have come up with a
real, safe workaround, you may consider switching to probe, as it offers
an additional check of the EEPROM presence.
Post by david-b
Post by Jean Delvare
What you call "generic" are perfectly standard at24c02-like EEPROMs, so
I see no point in making them a particular case. If it's just to make
them read-only and all the other ones read-write, this looks a bad idea
to merge the writability with the size in a single parameter.
The issue was how to handle backward compat with "eeprom"...
and the plan was to treat all uninitialized sizes as "generic".
Which is _exactly_ what "eeprom" does. If it's good enough
for that driver, it should be good enough here ...
This ain't compatible, as you still need to provide the addresses while
they are built-in for the "eeprom" driver. A really backward
compatible driver would need to behave like the old driver did when no
module parameter are provided.
Post by david-b
though as I commented, "eeprom" mis-handles at least double byte
address cases, also chips of size != 2 Kbits, so maybe it's not really
"good enough" there. ;)
I think we really only need to be backward compatible with "eeprom" for
the 24C02. This is the most popular EEPROM in consumer PCs. It's
probably not a problem to break comaptibility for larger EEPROMs, as
there are less users and these users (hopefully) knew better support
could come later, which would imply incompatibility.
Post by david-b
There must be a lot of code you count as "not exactly nice" then!!
You have no idea. People hate me for that. It's not really important and
you might as well ignore my pointless rants.
Post by david-b
Post by Jean Delvare
Post by David Brownell
+static int at24c_ee_address(
+ unsigned chiptype,
+ struct i2c_msg *msg,
+ unsigned *offset,
+ u32 *next_addr
+)
+{
Eek. What's this coding style?
You mean, "wrap lines at 80 characters"? Or are you maybe
feeling religious about where the right paren goes? :)
I mean that, among the various coding styles that exist for function
declarations, I had never seen this one, neither in the kernel code nor
even elsewhere. And this isn't exactly easy to read, as it ressembles a
structure declaration.
Post by david-b
Post by Jean Delvare
Point (b): You could as well implement a generic function in i2c-core.
I2C_FUNC_SMBUS_READ_I2C_BLOCK_2 is already reserved for that in i2c.h,
which suggests that SMBus 2.0 defines it (although I can't remember of
any hardware implementimg it).
If no SMBUS hardware implements it, I'm not sure what the point of
such a function would be! For now, this version is portable enough.
If nothing else, this could avoid code duplication (the max6875 driver
needs something similar, for example). And hardware support might be
added later, and we get immediate benefit.
Post by david-b
Post by Jean Delvare
Points (d) and (e) suggest that the i2c-core API is not adapted to the
needs. You are not the first one to complain about the i2c block
transfers API, yet everyone always go coding its own implementation on
its side. I'd like someone to properly analyze the situation and make
concrete proposals for a change.
Hmm? I thought I _was_ using the I2C block transfer API. The
limitations I'm thinking of are SMBUS policy (32 byte niblets)
or from its API. (Note that this driver doesn't escape copying
things written to EEPROM -- but that's relatively rare, and not
a case where zerocopy arguments should apply.)
Yup, I misexpressed myself. The API offered by i2c-core is really the
SMBus one, as I2C doesn't exactly have an API. It happens that most I2C
devices do work with I2C commands that have a name in the SMBus API, so
we easily mix both.

Still, what I meant is that this SMBus API doesn't seem to make everyone
happy, especially the block transfer functions. I am wondering if
something better could be done so that people actually use that API
rather than implementing their own.
Post by david-b
All the Linux systems I do I2C on seem to have full I2C controllers,
and don't need SMBUS limits. They'll have no problem with this driver;
after all, it's for "I2C EEPROMs" not "SMBUS EEPROMs". :)
There is no such thing as SMBus EEPROMS, all EEPROMs I know of are I2C
chips, and it happens that EEPROMs up to 24C16 can be accessed using
standard SMBus commands, and manufacturers are relying on this. All x86
motherboards have an SMBus master and most use it to give access to some
24C02 EEPROMs, be it SPD on memory modules, laptop proprietary ones or
ones on network adapters. Most EEPROMs I have access to myself are on
SMBus (except for EDID/DDC).
Post by david-b
Maybe there should be drivers that in Kconfig depend on SMBUS, and
others that depend on Real I2C(tm). Configs that don't enable a
real I2C controller wouldn't be able to configure I2C-only drivers.
This is handled by i2c capabilities, which are more flexible. There is no
such thing as a SMBus master, each physical device (and each driver on
top of that) may implement a subset of the SMBus commands. Even Real
I2C(tm) masters can differ, some can do protocol mangling, some can't.

What you propose would prevent some I2C/SMBus interactions which exist
now and we are perfectly happy with. Additionally, a given system can
have both an SMBus and an I2C master, with different devices on them.
Your approach wouldn't play well with this.

Now, I agree that mixing I2C and SMBus led us to a rather complex
situation in the Linux kernel. Blame it to Philips and Intel, as we only
reproduced the mess that physically exist.
Post by david-b
Post by Jean Delvare
Post by David Brownell
/* some chips take two or more addresses ... register using the
* first one, and hope no other driver claims the others!
*/
No, don't just hope. Properly register the other addresses as
subclients. See the asb100 driver for an example of how to do that.
For now, this suffices. Especially if the model is that all the
addresses in that range are statically configured. It may be
worth cleaning that up at some point though, yes.
Sorry but no, it doesn't suffice, not even for now.

If you don't request all addresses for a given device, it means that
anyone else may do. This includes: the eeprom driver, and user-space
through i2c-dev, for example the sensors-detect driver.

If both your driver and a third party access the chip at the same time,
the internal address register of the EEPROM will be overwritten and/or
reads will be interlaced and the read/written data will be corrupted in
no time. You don't want this to happen.
Post by david-b
Post by Jean Delvare
Post by David Brownell
address |= 0x50;
As said earlier, this is dangerous and shouldn't be done at all.
Dangerous how? More dangerous than any other user error?
It's not as if an at24c08 has flexibility about the addresses
it will be using.
Dangerous in that someone asking for address 0x30 will end up with 0x70,
which is irrational, and someone asking for 0x5C (which happens to be
the page protection address of the 24RF08) will get it. As much as I'd
like to agree with you that people are responsible for the module
parameter they pass, I see no point in not checking strictly for the
parameter value. These checks are cheap, and can prevent huge damage.
Post by david-b
Post by Jean Delvare
This is interesting. In all other drivers we are checking for the
functionality inside the *_probe (or *_detect) function rather than in
the *_attach_adapter function. Now that I think of it, this sounds
sub-optimal, your approach should be more efficient. Unless I'm missing
something, we should probably change all other drivers to match this one.
Well, tps65010 and isp1301_omap do it in attach(), so maybe not
quite all drivers! :)
But yes, I obviously agree this is a more natural place to
do this particular checking. Luckily those changes will
be simple.
Maybe an even better place for it would be in the I2C core,
by having the I2C drivers declare what level of functionality
it needs from the adapter? One word of data in each driver
(taken from struct padding?) replacing several instructions
and fault paths ... moving to one fault path in the core.
Exactly the conclusion I had come to. Just like adapter drivers can say
what they can do, device drivers would say what they require for proper
operation (but are still free to use more later for improved speed or
whatever). I need to think about it some more. but that sounds promising
for sure.

(But I want to kill i2c-isa first, as it's in the way somehow.)
Post by david-b
Post by Jean Delvare
You should use i2c_get_clientdata() (just like you should have used
i2c_set_clientdata() earlier.)
I don't see why. It's not incorrect; it's just a simpler style.
For maintainability I suppose. I'd guess there is a reason why Greg
introduced i2c_get/set_clientdata in the first place. I admit I don't
exactly know myself.
Post by david-b
In this case it's less expensive; the compiler will generate
less code since it will notice that the two pointer values are
always the same.
I don't think we really care, speed is not really important here. You
don't unload kernel modules every now and then.
Post by david-b
Post by Jean Delvare
No MODULE_AUTHOR?
Redundant given the copyright statement; unless there's something
special about showing up in "modinfo" output?
Not every user reads the source code, while simple users may want to get
in touch with a driver author for some reason. If not, MODULE_AUTHOR
wouldn't even exist, as all drivers do have their author name in the
source.
Post by david-b
Post by Jean Delvare
Note that your driver does not cache the reads to the EEPROM. While it
may be on purpose, it raises concerns because you gave read access to
everyone through sysfs, which means that any random user could saturate
the I2C bus the EEPROM is on. If there are other critical chips on the
bus (think battery controller, hardware monitor...) then we are in
trouble.
I'll remove group and "other" read access then; good point.
If you do, you will never be compatible with the legacy eeprom driver.
Post by david-b
I don't want to maintain any caches for data that's not
performance-critical.
The cache isn't about performance, although it incidentally helps on
that front. It's about preventing local DoS through bus saturation.
Post by david-b
However, the REALLY nice way to do this would be if the I2C bus
framework had a clean way to handle static device configuration
(like "platform_bus" does). Then such a flag could be just one
of several bits of information provided to driver using driver
model infrastructure like platform_data.
Where are the patches? ;)

You might want to wait for the i2c-isa cleanup to happen first, as the
i2c subsystem will be much cleaner and somewhat less sensors-centric
after I am done.

Thanks,
--
Jean Delvare
david-b
2005-07-13 16:38:39 UTC
Permalink
Here's a refreshed version of the patch ... addressing much of
your feedback and replacing those nasty switch statements with
some more reasonable table driven logic. It's more generic.

It now has working chip configuration, using probing as you
suggested (simpler etc) including that AT28RF04 workaround.

Modulo the need for more testing, probably the biggest potential
problem with this version is that it still doesn't claim all the
addresses some chips use. Safe in controlled environments...

This is a better version for anyone else to hack on. :)

- Dave


Add a simple driver for most I2C EEPROMs, giving sysfs read/write
access to their contents.

Tested only with AT24C04 so far, but support for many others is
coded and ready for you.

Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>

--- osk.orig/drivers/i2c/chips/Kconfig 2005-07-11 10:37:39.000000000 -0700
+++ osk/drivers/i2c/chips/Kconfig 2005-07-13 09:09:27.000000000 -0700
@@ -406,6 +406,25 @@
menu "Other I2C Chip support"
depends on I2C

+config I2C_AT24C
+ tristate "Atmel AT24C series EEPROMs (and compatibles)"
+ depends on I2C && EXPERIMENTAL
+ help
+ This driver should handle most I2C EEPROMS, including the AT24C
+ series and compatible ones like those from Microchip and other
+ vendors. For example, most memory DIMMS have 24c02 EEPROMs.
+
+ 24c00, 24c01, 24c02, 24c04, 24c08, 24c16,
+ 24c32, 24c64, 24c128, 24c256, 24c512, 24c1024
+
+ If you say yes here you get support for most Atmel AT24C series
+ serial EEPROMS. The driver provides read and write access to the
+ EEPROM data through sysfs, once you've configured it to know about
+ the particular chips on your target board.
+
+ This driver can also be built as a module. If so, the module
+ will be called at24c.
+
config SENSORS_DS1337
tristate "Dallas Semiconductor DS1337 and DS1339 Real Time Clock"
depends on I2C && EXPERIMENTAL
--- osk.orig/drivers/i2c/chips/Makefile 2005-07-11 10:37:39.000000000 -0700
+++ osk/drivers/i2c/chips/Makefile 2005-07-11 10:51:01.000000000 -0700
@@ -47,6 +47,7 @@
obj-$(CONFIG_SENSORS_W83627EHF) += w83627ehf.o
obj-$(CONFIG_SENSORS_W83L785TS) += w83l785ts.o

+obj-$(CONFIG_I2C_AT24C) += at24c.o
obj-$(CONFIG_ISP1301_OMAP) += isp1301_omap.o
obj-$(CONFIG_TPS65010) += tps65010.o
obj-$(CONFIG_SENSORS_TLV320AIC23) += tlv320aic23.o
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ osk/drivers/i2c/chips/at24c.c 2005-07-13 09:24:00.000000000 -0700
@@ -0,0 +1,610 @@
+/*
+ * at24c.c -- support many I2C EEPROMs
+ *
+ * Copyright (C) 2005 David Brownell
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+// #define DEBUG
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+
+#ifdef CONFIG_ARM
+#include <asm/mach-types.h>
+#endif
+
+
+/* If all the bus drivers are statically linked, I2C drivers often won't
+ * need hotplug support. Iff that's true in your config, you can #define
+ * NO_I2C_DYNAMIC_BUSSES and shrink this driver's runtime footprint.
+ */
+//#define NO_I2C_DYNAMIC_BUSSES
+
+
+/* I2C EEPROMs from most vendors are reasonably interchangeable; vendors
+ * have their own parts (like Atmel AT24C, MicroChip 24LC, etc) but the
+ * I2C behavior is normally the same.
+ *
+ * Dynamic typing of these chips isn't really possible; there's no way to
+ * reliably distingush the chips. And guessing wrong can be dangerous.
+ * If you issue the request to set a 16-bit address, and the chip that
+ * receves that is one using only 8-bit addresses, that request writes
+ * a byte to the EEPROM instead. Address size is only one of several
+ * chip characteristics that the driver needs to be told.
+ *
+ * So the best way to configure I2C EEPROMS uses static configuration
+ * descriptions. Right now /etc/modprobe.conf can hold this easily.
+ *
+ * modprobe at24c chip_names=24c02,24c64 probe=0,50,1,50
+ *
+ * CONFIGURATION MECHANISM IS SUBJECT TO CHANGE!!
+ */
+static unsigned short ignore[] = { I2C_CLIENT_END };
+I2C_CLIENT_MODULE_PARM(probe,
+ "List of adapter,address pairs for I2C eeproms");
+static struct i2c_client_address_data addr_data = {
+ .normal_i2c = ignore,
+ .probe = probe,
+ .ignore = ignore,
+ .force = ignore,
+};
+
+static char *chip_names[I2C_CLIENT_MAX_OPTS / 2];
+static unsigned n_chip_names;
+module_param_array(chip_names, charp, &n_chip_names, 0);
+MODULE_PARM_DESC(chip_names,
+ "Type of chip; one chip name per probe pair");
+
+static int readonly[I2C_CLIENT_MAX_OPTS / 2];
+static unsigned n_readonly;
+module_param_array(readonly, bool, &n_readonly, 0);
+MODULE_PARM_DESC(chip_names, "Readonly flags, one per probe pair");
+
+
+#ifdef NO_I2C_DYNAMIC_BUSSES
+#undef __devinit
+#undef __devinitdata
+#undef __devexit
+#undef __devexit_p
+/* no I2C devices will be hotplugging, so modules can shrink */
+#define __devinit __init
+#define __devinitdata __initdata
+#define __devexit __exit
+#define __devexit_p __exit_p
+#endif
+
+
+/* As seen through Linux I2C, differences between the most common types
+ * of I2C memory include:
+ * - How many I2C addresses the chip consumes: 1, 2, 4, or 8?
+ * - Memory address space for one I2C address: 256 bytes, or 64 KB?
+ * - How full that memory space is: 16 bytes, 256, 32Kb, etc?
+ * - What write page size does it support?
+ */
+struct chip_desc {
+ u32 byte_len; /* of 1..8 i2c addrs, total */
+ char name[10];
+ u16 page_size; /* for writes */
+ u8 i2c_addr_mask; /* for multi-addr chips */
+ u8 flags;
+#define EE_ADDR2 0x0002 /* 16 bit addrs; <= 64 KB */
+};
+
+struct at24c_data {
+ struct i2c_client client;
+ struct semaphore lock;
+ struct bin_attribute bin;
+
+ struct chip_desc chip;
+
+ /* each chip has an internal "memory address" pointer;
+ * we remember it for faster read access.
+ */
+ u32 next_addr;
+};
+
+/*-------------------------------------------------------------------------*/
+
+static const struct chip_desc __devinitdata chips[] = {
+
+/* this first block of EEPROMS uses 8 bit memory addressing
+ * and can be accessed using standard SMBUS requests.
+ */
+{
+ /* 128 bit chip */
+ .name = "24c00",
+ .byte_len = 128 / 8,
+ .i2c_addr_mask = 0x07, /* A0-A2 ignored */
+ .page_size = 1,
+}, {
+ /* 1 Kbit chip */
+ .name = "24c01",
+ .byte_len = 1024 / 8,
+ /* some have 16 byte pages: */
+ .page_size = 8,
+}, {
+ /* 2 Kbit chip */
+ .name = "24c02", /* common in memory DIMMs etc */
+ .byte_len = 2048 / 8,
+ /* some have 16 byte pages: */
+ .page_size = 8,
+}, {
+ /* 4 Kbit chip */
+ .name = "24c04",
+ .byte_len = 4096 / 8,
+ .page_size = 16,
+ .i2c_addr_mask = 0x01, /* I2C A0 is MEM A8 */
+}, {
+ /* 8 Kbit chip */
+ .name = "24c08",
+ .byte_len = 8192 / 8,
+ .page_size = 16,
+ .i2c_addr_mask = 0x03, /* I2C A1-A0 is MEM A9-A8 */
+}, {
+ /* 16 Kbit chip */
+ .name = "24c16",
+ .byte_len = 16384 / 8,
+ .page_size = 16,
+ .i2c_addr_mask = 0x07, /* I2C A2-A0 is MEM A10-A8 */
+},
+
+/* this second block of EEPROMS uses 16 bit memory addressing
+ * and can't be accessed using standard SMBUS requests.
+ * REVISIT maybe SMBUS 2.0 requests would work ...
+ */
+{
+ /* 32 Kbits */
+ .name = "24c32",
+ .byte_len = 32768 / 8,
+ .flags = EE_ADDR2,
+ .page_size = 32,
+}, {
+ /* 64 Kbits */
+ .name = "24c64",
+ .byte_len = 65536 / 8,
+ .flags = EE_ADDR2,
+ .page_size = 32,
+}, {
+ /* 128 Kbits */
+ .name = "24c128",
+ .byte_len = 131072 / 8,
+ .flags = EE_ADDR2,
+ .page_size = 64,
+}, {
+ /* 256 Kbits */
+ .name = "24c256",
+ .byte_len = 262144 / 8,
+ .flags = EE_ADDR2,
+ .page_size = 64,
+}, {
+ /* 512 Kbits */
+ .name = "24c512",
+ .byte_len = 524288 / 8,
+ .flags = EE_ADDR2,
+ .page_size = 128,
+}, {
+ /* 1 Mbits */
+ .name = "24c1024",
+ .byte_len = 1048576 / 8,
+ .flags = EE_ADDR2,
+ .page_size = 256,
+ .i2c_addr_mask = 0x01, /* I2C A0 is MEM A16 */
+}
+
+};
+#define NUM_EEPROM_CHIPS ARRAY_SIZE(chips)
+
+static inline const struct chip_desc *__devinit
+find_chip(const char *s)
+{
+ unsigned i;
+
+ for (i = 0; i < NUM_EEPROM_CHIPS; i++)
+ if (strnicmp(s, chips[i].name, 16) == 0)
+ return &chips[i];
+ return NULL;
+}
+
+static inline const struct chip_desc *__devinit
+which_chip(struct i2c_adapter *adapter, int address, int *writable)
+{
+ unsigned i;
+
+ for (i = 0; i < I2C_CLIENT_MAX_OPTS; i++) {
+ const struct chip_desc *chip;
+ char *name;
+
+ if (probe[i++] != adapter->id)
+ continue;
+ if (probe[i] != address)
+ continue;
+
+ i >>= 1;
+ if ((name = chip_names[i]) == NULL) {
+ dev_err(&adapter->dev, "no chipname for addr %d\n",
+ address);
+ return NULL;
+
+ }
+ chip = find_chip(name);
+ if (chip == NULL)
+ dev_err(&adapter->dev, "unknown chipname %s\n", name);
+ /* default to writable */
+ *writable = !readonly[i];
+ return chip;
+ }
+
+ /* "can't happen" */
+ dev_dbg(&adapter->dev, "addr %d not in probe[]\n", address);
+ return NULL;
+}
+
+/*-------------------------------------------------------------------------*/
+
+static inline int at24c_ee_address(
+ const struct chip_desc *chip,
+ struct i2c_msg *msg,
+ unsigned *offset,
+ u32 *next_addr
+)
+{
+ unsigned per_address = 256;
+
+ if (*offset >= chip->byte_len)
+ return -EINVAL;
+
+ /* Nothing to do unless accessing that memory would go to some
+ * later I2C address. In that case, modify the output params.
+ * Yes, it can loop more than once on 24c08 and 24c16.
+ */
+ if (chip->flags & EE_ADDR2)
+ per_address = 64 * 1024;
+ while (*offset >= per_address) {
+ msg->addr++;
+ *offset -= per_address;
+ *next_addr -= per_address;
+ }
+ return 0;
+}
+
+/* This parameter is to help this driver avoid blocking other drivers
+ * out of I2C for potentially troublesome amounts of time. With a
+ * 100 KHz I2C clock, one 256 byte read takes about 1/43 second.
+ */
+static unsigned io_limit = 128;
+module_param(io_limit, uint, 0);
+MODULE_PARM_DESC(io_limit, "maximum bytes per i/o (default 128)");
+
+static ssize_t
+at24c_ee_read(struct at24c_data *at24c, char *buf, unsigned offset, size_t count)
+{
+ struct i2c_msg msg;
+ ssize_t status;
+ u32 next_addr;
+
+ down(&at24c->lock);
+ msg.addr = at24c->client.addr;
+ msg.flags = 0;
+
+ next_addr = at24c->next_addr;
+ status = at24c_ee_address(&at24c->chip, &msg, &offset, &next_addr);
+ if (status < 0)
+ goto done;
+
+ /* FIXME switching to the next i2c address confuses the
+ * record keeping for the next memory address ...
+ */
+ next_addr = at24c->bin.size;
+
+ /* advantages of not using i2c_smbus_read_i2c_block_data() here
+ * include both functionality and performance:
+ * (a) uses i2c addresses other than at24c->client.addr, as needed;
+ * (b) handles two-byte "register" addresses;
+ * (c) this can often rely on the chip's internal address pointer,
+ * and omit the initial dummy write;
+ * (d) bigger reads than 32-byte niblets;
+ * (e) less data copying.
+ */
+
+ /* Maybe change the current on-chip address with a dummy write */
+ if (next_addr != offset) {
+ u8 tmp[2];
+
+ msg.buf = tmp;
+ tmp[1] = (u8) offset;
+ tmp[0] = (u8) (offset >> 8);
+ if (at24c->chip.flags & EE_ADDR2) {
+ msg.len = 2;
+ } else {
+ msg.len = 1;
+ msg.buf++;
+ }
+ dev_dbg(&at24c->client.dev, "addr %02x, set current to %d\n",
+ msg.addr, offset);
+ status = i2c_transfer(at24c->client.adapter, &msg, 1);
+ if (status < 0)
+ goto done;
+ }
+
+ /* issue sequential read for the data bytes, knowing that read
+ * page rollover will go to the next page and/or memory block.
+ */
+ if (count > io_limit)
+ count = io_limit;
+
+ msg.len = count;
+ msg.buf = buf;
+ msg.flags = I2C_M_RD;
+ status = i2c_transfer(at24c->client.adapter, &msg, 1);
+done:
+ dev_dbg(&at24c->client.dev, "read %d bytes --> %d\n", count, status);
+ if (status < 0)
+ at24c->next_addr = at24c->bin.size;
+ else
+ at24c->next_addr = offset + count;
+ up(&at24c->lock);
+ return status < 0 ? status : count;
+}
+
+static ssize_t
+at24c_bin_read(struct kobject *kobj, char *buf, loff_t off, size_t count)
+{
+ struct i2c_client *client;
+ struct at24c_data *at24c;
+
+ client = to_i2c_client(container_of(kobj, struct device, kobj));
+ at24c = container_of(client, struct at24c_data, client);
+
+ if (unlikely(off > at24c->bin.size))
+ return 0;
+ if ((off + count) > at24c->bin.size)
+ count = at24c->bin.size - off;
+ if (unlikely(!count))
+ return count;
+
+ /* we don't maintain caches for any data: simpler, cheaper */
+ return at24c_ee_read(at24c, buf, off, count);
+
+}
+
+/* Note that if the hardware write-protect pin is pulled high, the whole
+ * chip is normally write protected.
+ */
+static ssize_t
+at24c_bin_write(struct kobject *kobj, char *buf, loff_t off, size_t count)
+{
+ struct i2c_client *client;
+ struct at24c_data *at24c;
+ struct i2c_msg msg;
+ ssize_t status = 0;
+ unsigned written = 0;
+ u32 scratch;
+ unsigned buf_size;
+
+ client = to_i2c_client(container_of(kobj, struct device, kobj));
+ at24c = container_of(client, struct at24c_data, client);
+
+ if (unlikely(off > at24c->bin.size))
+ return 0;
+ if ((off + count) > at24c->bin.size)
+ count = at24c->bin.size - off;
+
+ /* temp buffer lets us stick the address at the beginning */
+ buf_size = at24c->chip.page_size;
+ if (buf_size > io_limit)
+ buf_size = io_limit;
+ msg.buf = kmalloc(buf_size + 2, GFP_KERNEL);
+ if (!msg.buf)
+ return -ENOMEM;
+ msg.flags = 0;
+
+ /* FIXME split this write logic out separately, so it's easier to
+ * call from non-sysfs contexts... the read side code does this,
+ * but "struct at24c *" isn't really better than a kobject.
+ */
+
+ /* for write, rollover is within the page ... so we must write
+ * at most one page at a time, and manually roll over to the
+ * next page and/or block.
+ */
+ down(&at24c->lock);
+ while (count > 0) {
+ unsigned segment;
+ unsigned offset = (unsigned) off;
+
+ msg.addr = at24c->client.addr;
+ status = at24c_ee_address(&at24c->chip, &msg, &offset, &scratch);
+ if (status < 0)
+ break;
+
+ /* write as much of a page as we can */
+ segment = buf_size - (offset % buf_size);
+ if (segment > count)
+ segment = count;
+ msg.len = segment + 1;
+ if (at24c->chip.flags & EE_ADDR2) {
+ msg.len++;
+ msg.buf[1] = (u8) offset;
+ msg.buf[0] = (u8) (offset >> 8);
+ memcpy(&msg.buf[2], buf, segment);
+ } else {
+ msg.buf[0] = offset;
+ memcpy(&msg.buf[1], buf, segment);
+ }
+ status = i2c_transfer(at24c->client.adapter, &msg, 1);
+ dev_dbg(&at24c->client.dev,
+ "write %d bytes to %02x at %d --> %d\n",
+ segment, msg.addr, offset, status);
+
+ /* REVISIT while we're advancing, the chip is finishing up its
+ * write. We should wait for that; chips often spec a max of
+ * 5 msec (sometimes 20 msec).
+ */
+ off += segment;
+ buf += segment;
+ count -= segment;
+ written += segment;
+ if (status < 0 || count == 0) {
+ if (written != 0)
+ status = written;
+ break;
+ }
+ }
+ at24c->next_addr = at24c->bin.size;
+ up(&at24c->lock);
+
+ kfree(msg.buf);
+ return status;
+}
+
+/*-------------------------------------------------------------------------*/
+
+static struct i2c_driver at24c_driver;
+
+static int __devinit at24c_probe(struct i2c_adapter *adapter, int address, int kind)
+{
+ struct at24c_data *at24c;
+ int err = 0;
+ int writable;
+ const struct chip_desc *chip;
+
+ if (!(at24c = kcalloc(1, sizeof(struct at24c_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto done;
+ }
+ init_MUTEX(&at24c->lock);
+
+ /* if there's no param saying what kind of chip this is,
+ * don't guess! just ignore it.
+ */
+ chip = which_chip(adapter, address, &writable);
+ if (!chip) {
+ err = -ENOENT;
+ goto done;
+ }
+ at24c->chip = *chip;
+ snprintf(at24c->client.name, I2C_NAME_SIZE,
+ "%s I2C EEPROM", at24c->chip.name);
+
+ /* Export the EEPROM bytes through sysfs, since that's convenient */
+ at24c->bin.attr.name = "eeprom";
+ at24c->bin.attr.mode = S_IRUGO;
+ at24c->bin.attr.owner = THIS_MODULE;
+ at24c->bin.read = at24c_bin_read;
+
+ at24c->bin.size = at24c->chip.byte_len;
+ at24c->next_addr = at24c->bin.size;
+ if (writable) {
+ at24c->bin.write = at24c_bin_write;
+ at24c->bin.attr.mode |= S_IWUSR;
+ }
+
+ /* some chips use several addresses ... register the first one.
+ *
+ * FIXME register the others, to prevent various misbehaviors.
+ */
+ address &= ~at24c->chip.i2c_addr_mask;
+
+ at24c->client.addr = address;
+ at24c->client.adapter = adapter;
+ at24c->client.driver = &at24c_driver;
+ at24c->client.flags = 0;
+
+ /* prevent AT24RF08 corruption, a possible consequence of the
+ * probe done by the i2c core (verifying a chip is present).
+ */
+ i2c_smbus_write_quick(&at24c->client, 0);
+
+ /* connect to I2C layer */
+ if ((err = i2c_attach_client(&at24c->client)))
+ goto done;
+
+ sysfs_create_bin_file(&at24c->client.dev.kobj, &at24c->bin);
+
+ dev_info(&at24c->client.dev, "%d byte %s%s\n",
+ at24c->bin.size, at24c->client.name,
+ writable ? " (writable)" : "");
+ dev_dbg(&at24c->client.dev, "page_size %d, i2c_addr_mask %d\n",
+ at24c->chip.page_size, at24c->chip.i2c_addr_mask);
+done:
+ if (err) {
+ dev_dbg(&adapter->dev, "%s probe, err %d\n",
+ at24c_driver.name, err);
+ kfree(at24c);
+ }
+ return 0;
+}
+
+static int __devinit at24c_attach_adapter(struct i2c_adapter *adapter)
+{
+ /* REVISIT: using SMBUS calls would improve portability, though
+ * maybe at the cost of support for 16 bit memory addressing...
+ */
+ if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) {
+ dev_dbg(&adapter->dev, "%s probe, no I2C\n",
+ at24c_driver.name);
+ return 0;
+ }
+ return i2c_probe(adapter, &addr_data, at24c_probe);
+}
+
+static int __devexit at24c_detach_client(struct i2c_client *client)
+{
+ int err;
+
+ err = i2c_detach_client(client);
+ if (err) {
+ dev_err(&client->dev, "deregistration failed, %d\n", err);
+ return err;
+ }
+
+ kfree(container_of(client, struct at24c_data, client));
+ return 0;
+}
+
+/*-------------------------------------------------------------------------*/
+
+static struct i2c_driver at24c_driver = {
+ .owner = THIS_MODULE,
+ .name = "at24c",
+ .flags = I2C_DF_NOTIFY,
+ .attach_adapter = at24c_attach_adapter,
+ .detach_client = __devexit_p(at24c_detach_client),
+};
+
+static int __init at24c_init(void)
+{
+#ifdef CONFIG_OMAP_OSK_MISTRAL
+ /* REVISIT This is a bit hackish. But until we have a cleaner way
+ * to provide compile-time descriptions of the I2C devices on a
+ * given system (platform_data for something?), it suffices.
+ */
+ if (machine_is_omap_osk()) {
+ n_chip_names = 1;
+
+ probe[0] = 0;
+ probe[1] = 0x50;
+ chip_names[0] = "24c04";
+ }
+#endif
+ return i2c_add_driver(&at24c_driver);
+}
+module_init(at24c_init);
+
+static void __exit at24c_exit(void)
+{
+ i2c_del_driver(&at24c_driver);
+}
+module_exit(at24c_exit);
+
+MODULE_DESCRIPTION("Driver for AT24C series (and compatible) I2C EEPROMs");
+MODULE_AUTHOR("David Brownell");
+MODULE_LICENSE("GPL");
+
david-b
2005-07-13 23:06:48 UTC
Permalink
Post by Jean Delvare
Post by david-b
Which is _exactly_ what "eeprom" does. If it's good enough
for that driver, it should be good enough here ...
This ain't compatible, as you still need to provide the addresses while
they are built-in for the "eeprom" driver. A really backward
compatible driver would need to behave like the old driver did when no
module parameter are provided.
I'm thinking full backward compat with "eeprom" is not necessarily
worth having as a goal.

One aspect of that is size sensing thing, where "eeprom" assumes
everything it sees is a 24c02 EEPROM (vs being told what's where),
and can use 8 bit addressing. This is the wrong idea. :)

Another is the various additional tweaks, like special support
to hide the VAIO password. Simpler would be just to make that
data readable only by root.
Post by Jean Delvare
Post by david-b
though as I commented, "eeprom" mis-handles at least double byte
address cases, also chips of size != 2 Kbits, so maybe it's not really
"good enough" there. ;)
I think we really only need to be backward compatible with "eeprom" for
the 24C02. This is the most popular EEPROM in consumer PCs.
What do you mean "compatible" ... never being writable?
Supporting the VAIO hack? Having the same driver name?

Folk have said on this list before they don't think that the
"eeprom" driver is much more than an example. So I'm reasonably
sure that there's no point in being identical to it. If both
drivers can provide an "eeprom" binary file -- that should be
close enough.
Post by Jean Delvare
It's
probably not a problem to break comaptibility for larger EEPROMs, as
there are less users and these users (hopefully) knew better support
could come later, which would imply incompatibility.
Ones using 16 bit addressing never worked before, so breaking
compatibility there is the topmost goal! :)

Chips of 4, 8, or 16 Kbits being handled as one chip rather than
several chips is surely the better model, but that's the main
incompatibility I can see.

Other than saner support for smaller eeproms ... e.g. right now
the driver assumes everything's got 2Kbit capacity, which means
smaller eeproms (notably 1 Kbit and 128 bits) misbehave.
Post by Jean Delvare
Post by david-b
Post by Jean Delvare
Point (b): You could as well implement a generic function in i2c-core.
I2C_FUNC_SMBUS_READ_I2C_BLOCK_2 is already reserved for that in i2c.h,
which suggests that SMBus 2.0 defines it (although I can't remember of
any hardware implementimg it).
If no SMBUS hardware implements it, I'm not sure what the point of
such a function would be! For now, this version is portable enough.
If nothing else, this could avoid code duplication (the max6875 driver
needs something similar, for example). And hardware support might be
added later, and we get immediate benefit.
So that'd be a capability that drivers would need to advertise,
and which would have a straightforward implementation in terms
of I2C primitives. Sure. No problem; that seems like a fine
thing to have.

If someone were to submit patches for this in the core, and
teaching my at24c driver how to work on SMBUS, then I suspect
we'd both be happier. :)
Post by Jean Delvare
Post by david-b
Maybe an even better place for it would be in the I2C core,
by having the I2C drivers declare what level of functionality
it needs from the adapter? One word of data in each driver
(taken from struct padding?) replacing several instructions
and fault paths ... moving to one fault path in the core.
Exactly the conclusion I had come to. Just like adapter drivers can say
what they can do, device drivers would say what they require for proper
operation (but are still free to use more later for improved speed or
whatever). I need to think about it some more. but that sounds promising
for sure.
(But I want to kill i2c-isa first, as it's in the way somehow.)
Take your time. :)

Random side note: I think an SPI driver framework might need the
same sort of thing, wherein drivers say what SPI mode(s) they need.
Post by Jean Delvare
Post by david-b
I'll remove group and "other" read access then; good point.
If you do, you will never be compatible with the legacy eeprom driver.
That wasn't necessarily a good default choice. FLASH eeproms
aren't normally world readable, for example. Why should I2C
ones be?
Post by Jean Delvare
Post by david-b
However, the REALLY nice way to do this would be if the I2C bus
framework had a clean way to handle static device configuration
(like "platform_bus" does). Then such a flag could be just one
of several bits of information provided to driver using driver
model infrastructure like platform_data.
Where are the patches? ;)
The only bright idea I've had so far involves board-specific code
providing a platform pseudo-device whose platform_data would be
morphed (by a "pseudo-driver", registered and deregistered before
the "at24c" i2c driver) into the addr_data (and coupled fields) that
are currently used to init that i2c driver.

That's a bit hacky though. Better if there were some real device
node that the at24c driver would bind to ... which would sort of
turn the current I2C model on its head.

Or maybe not ... maybe a static I2C device tree could just be set
up early by board-specific code, and the I2C infrastructure could
view it as an alternate way to specify more "probe" or "force"
entries, without using module params.
Post by Jean Delvare
You might want to wait for the i2c-isa cleanup to happen first, as the
i2c subsystem will be much cleaner and somewhat less sensors-centric
after I am done.
Wasn't there some work afoot (Greg in his copious spare time?) to
make I2C play better in the driver model world?

- Dave
david-b
2005-07-16 01:21:30 UTC
Permalink
Date: Wed, 13 Jul 2005 16:06:48 -0700
From: david-b at pacbell.net
Post by Jean Delvare
Post by david-b
However, the REALLY nice way to do this would be if the I2C bus
framework had a clean way to handle static device configuration
(like "platform_bus" does). Then such a flag could be just one
of several bits of information provided to driver using driver
model infrastructure like platform_data.
Where are the patches? ;)
The only bright idea I've had so far involves board-specific code
providing a platform pseudo-device whose platform_data would be
morphed (by a "pseudo-driver", registered and deregistered before
the "at24c" i2c driver) into the addr_data (and coupled fields) that
are currently used to init that i2c driver.
That's a bit hacky though. Better if there were some real device
node that the at24c driver would bind to ... which would sort of
turn the current I2C model on its head.
Or maybe not ... maybe a static I2C device tree could just be set
up early by board-specific code, and the I2C infrastructure could
view it as an alternate way to specify more "probe" or "force"
entries, without using module params.
Yeah, the best way to do that is really going to involve turning
the I2C stack a bit on its head ... making it work more like most
other Linux driver stacks.

That is, significant API surgery ... stuff I suspect folk believe
is needed in any case, but still it's a lot of change.

One way to start thinking about that is to look at the attached
patch (NOT suitable for merging! Though it does solve the problem)
and see how the new probe() gets used. And then observe, "hey, if
i2c core creates and registers the i2c client, like it would in
other Linux bus frameworks, and then passed it into this routine...
then the second two parameters would be implicit." A significant
change to the driver's role.

- Dave


This is an experimental patch for system-specific static config
support in the I2C framework.

- New i2c core interfaces:
* New driver probe() method, with platform string data
* Board-specific data storage for i2c
- OSK board init provides static i2c config data
- at24c implements the new probe()
- i2c-core uses it
* retrieves the new device descriptions
* new probe only after attach() gets a first whack
- new static i2c code implements it
* optional, CONFIG_I2C_BOOTDATA
* get/set device record array

It might be better push the driver model deeper into this patch, but
that would involve formally adding a lifecycle where the device
drivers are no longer responsible for creating device nodes (which
are embedded in a "struct i2c_client").

--- h2.orig/include/linux/i2c.h 2005-07-15 17:50:08.000000000 -0700
+++ h2/include/linux/i2c.h 2005-07-15 17:50:14.000000000 -0700
@@ -129,6 +129,11 @@
*/
int (*command)(struct i2c_client *client,unsigned int cmd, void *arg);

+ /* set up devices and drivers according to system description
+ * tables; see <linux/i2c-config.h>
+ */
+ void (*probe)(struct i2c_adapter *adapter, int address, char *data);
+
struct device_driver driver;
struct list_head list;
};
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ h2/include/linux/i2c-config.h 2005-07-15 17:50:14.000000000 -0700
@@ -0,0 +1,56 @@
+#ifndef __I2C_CONFIG_H
+#define __I2C_CONFIG_H
+
+
+ /* DRAFT/RFC for some static config support for I2C
+ *
+ * This preserves current I2C probe model characteristic that
+ * i2c device drivers are responsible for creating devices.
+ *
+ * Otherwise, platform_device.dev.platform_data is the usage
+ * model for the new data being provided to drivers from
+ * board-specific initialization.
+ */
+
+/* Board init code can supply i2c core a table describing devices,
+ * and providing driver-specific setup data for a given device.
+ *
+ * These act like "probe" entries in addr_data, except that they
+ * include device-specific configuration data.
+ */
+struct i2c_device_record {
+ char driver_name[32];
+ int adapter_id;
+ u16 dev_num;
+
+ /* data[] will be passed to a new probe() driver entrypoint.
+ * - if probe of address dev_num at adapter_id succeeds
+ * - and the driver named "driver_name" is registered
+ */
+ char data[26];
+
+ /* REVISIT: for passing a string, this is probably enough.
+ * But functions and binary data will be needed too...
+ */
+};
+
+#ifdef CONFIG_I2C_BOOTDATA
+extern int i2c_set_boot_data(const struct i2c_device_record *, unsigned);
+extern unsigned i2c_get_boot_data(struct i2c_device_record **);
+
+#else
+static inline int
+i2c_set_boot_data(const struct i2c_device_record *data, unsigned n)
+{
+ return -ENOSYS;
+}
+
+static inline unsigned i2c_get_boot_data(struct i2c_device_record **pp)
+{
+ *pp = NULL;
+ return 0;
+}
+
+#endif /* I2C_BOOTDATA */
+
+#endif /* __I2C_CONFIG_H */
--- h2.orig/drivers/i2c/chips/at24c.c 2005-07-15 17:50:08.000000000 -0700
+++ h2/drivers/i2c/chips/at24c.c 2005-07-15 17:50:14.000000000 -0700
@@ -639,6 +639,7 @@
}


+/* "legacy" i2c driver api entry: bind to bus, not device */
static int __devinit at24c_attach_adapter(struct i2c_adapter *adapter)
{
/* REVISIT: using SMBUS calls would improve portability, though
@@ -652,6 +653,20 @@
return i2c_probe(adapter, &addr_data, at24c_old_probe);
}

+/* experimental static config support: bind to device */
+static void __devinit
+at24c_probe(struct i2c_adapter *adapter, int address, char *data)
+{
+ const struct chip_desc *chip;
+
+ chip = find_chip(data);
+ if (!chip)
+ dev_err(&adapter->dev, "unknown chipname %s\n", data);
+ else
+ at24c_activate(adapter, address, chip,
+ !(chip->flags & EE_READONLY));
+}
+
static int __devexit at24c_detach_client(struct i2c_client *client)
{
int err;
@@ -678,11 +693,13 @@
.flags = I2C_DF_NOTIFY,
.attach_adapter = at24c_attach_adapter,
.detach_client = __devexit_p(at24c_detach_client),
+ .probe = at24c_probe,
};

static int __init at24c_init(void)
{
-#ifdef CONFIG_OMAP_OSK_MISTRAL
+#if 0
+//#ifdef CONFIG_OMAP_OSK_MISTRAL
/* REVISIT ... */
if (machine_is_omap_osk()) {
n_chip_names = 1;
--- h2.orig/arch/arm/mach-omap1/board-osk.c 2005-07-15 17:50:08.000000000 -0700
+++ h2/arch/arm/mach-omap1/board-osk.c 2005-07-15 17:50:14.000000000 -0700
@@ -34,6 +34,8 @@
#include <linux/mtd/mtd.h>
#include <linux/mtd/partitions.h>

+#include <linux/i2c-config.h>
+
#include <asm/hardware.h>
#include <asm/mach-types.h>
#include <asm/mach/arch.h>
@@ -191,6 +193,31 @@
{ OMAP_TAG_USB, &osk_usb_config },
};

+
+static struct i2c_device_record i2c_devs[] __initdata = {
+{
+ /* this autprobes OK, but there are several chip variants */
+ .driver_name = "tps65010",
+ .adapter_id = 0,
+ .dev_num = 0x48,
+ .data = "tps65010", /* vs 65013 etc */
+}, {
+ .driver_name = "tlv320aic23",
+ .adapter_id = 0,
+ .dev_num = 0x1b,
+ // clock setup is board-specific ...
+},
+#ifdef CONFIG_OMAP_OSK_MISTRAL
+{
+ .driver_name = "at24c",
+ .adapter_id = 0,
+ .dev_num = 0x50,
+ .data = "24c04",
+},
+ /* the camera connector may also have an ov9640 sensor */
+#endif
+};
+
#ifdef CONFIG_OMAP_OSK_MISTRAL

#ifdef CONFIG_PM
@@ -248,6 +275,8 @@
omap_board_config_size = ARRAY_SIZE(osk_config);
USB_TRANSCEIVER_CTRL_REG |= (3 << 1);

+ i2c_set_boot_data(i2c_devs, ARRAY_SIZE(i2c_devs));
+
osk_mistral_init();
}

--- h2.orig/drivers/i2c/i2c-core.c 2005-07-15 17:50:08.000000000 -0700
+++ h2/drivers/i2c/i2c-core.c 2005-07-15 17:50:14.000000000 -0700
@@ -26,6 +26,7 @@
#include <linux/errno.h>
#include <linux/slab.h>
#include <linux/i2c.h>
+#include <linux/i2c-config.h>
#include <linux/init.h>
#include <linux/idr.h>
#include <linux/seq_file.h>
@@ -138,6 +139,47 @@
* ---------------------------------------------------
*/

+#ifdef CONFIG_I2C_BOOTDATA
+
+/* static board config data -- e.g. what type of eeprom chip, how
+ * the chip's irqs are wired, etc -- can be passed to drivers.
+ */
+static struct i2c_device_record *records;
+static unsigned nrecords;
+
+static void maybe_probe(struct i2c_adapter *adap, struct i2c_driver *driver)
+{
+ unsigned i;
+ u16 addr;
+
+ for (i = 0; i < nrecords; i++) {
+ if (records[i].adapter_id != i2c_adapter_id(adap))
+ continue;
+ if (strncmp(records[i].driver_name, driver->name,
+ sizeof driver->name) != 0)
+ continue;
+
+ /* Skip if already in use */
+ addr = records[i].dev_num;
+ if (i2c_check_addr(adap, addr))
+ continue;
+
+ /* if hardware probe succeeds, then tell the software */
+ if (i2c_smbus_xfer(adap,addr,0,0,0,I2C_SMBUS_QUICK,NULL) >= 0)
+ driver->probe(adap, addr, records[i].data);
+ }
+}
+
+#else
+
+static inline void
+maybe_probe(struct i2c_adapter *adap, struct i2c_driver *driver)
+{
+ /* NOP */
+}
+
+#endif
+
/* -----
* i2c_add_adapter is called from within the algorithm layer,
* when a new hw adapter registers. A new device is register to be
@@ -194,6 +236,11 @@
if (driver->flags & I2C_DF_NOTIFY)
/* We ignore the return code; if it fails, too bad */
driver->attach_adapter(adap);
+
+ /* fallback to static config records, where available */
+ if (!driver->probe)
+ continue;
+ maybe_probe(adap, driver);
}

dev_dbg(&adap->dev, "registered as adapter #%d\n", adap->nr);
@@ -313,6 +360,14 @@
}
}

+ /* fallback to static config records, where relevant */
+ if (driver->probe) {
+ list_for_each(item,&adapters) {
+ adapter = list_entry(item, struct i2c_adapter, list);
+ maybe_probe(adapter, driver);
+ }
+ }
+
out_unlock:
up(&core_lists);
return res;
@@ -557,6 +612,10 @@
{
int retval;

+#ifdef CONFIG_I2C_BOOTDATA
+ nrecords = i2c_get_boot_data(&records);
+#endif
+
retval = bus_register(&i2c_bus_type);
if (retval)
return retval;
--- h2.orig/drivers/i2c/Kconfig 2005-07-15 17:50:08.000000000 -0700
+++ h2/drivers/i2c/Kconfig 2005-07-15 17:50:14.000000000 -0700
@@ -74,5 +74,13 @@
a problem with I2C support and want to see more of what is going
on.

+# for testing, this is always forced on ... systems that want to
+# pass board data this way should eventually enable it selectively
+
+config I2C_BOOTDATA
+ bool
+ depends on I2C
+ default y
+
endmenu

--- h2.orig/drivers/i2c/Makefile 2005-07-15 17:50:08.000000000 -0700
+++ h2/drivers/i2c/Makefile 2005-07-15 17:50:14.000000000 -0700
@@ -5,6 +5,7 @@
obj-$(CONFIG_I2C) += i2c-core.o
obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o
obj-$(CONFIG_I2C_SENSOR) += i2c-sensor.o
+obj-$(CONFIG_I2C_BOOTDATA) += i2c-boot.o
obj-y += busses/ chips/ algos/

i2c-sensor-objs := i2c-sensor-detect.o i2c-sensor-vid.o
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ h2/drivers/i2c/i2c-boot.c 2005-07-15 17:57:23.000000000 -0700
@@ -0,0 +1,45 @@
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/i2c-config.h>
+
+static struct i2c_device_record *records;
+static unsigned nrecords;
+
+int __init i2c_set_boot_data(const struct i2c_device_record *data, unsigned len)
+{
+ if (nrecords)
+ return -EBUSY;
+
+ /* boot code should call this with __initdata; must copy */
+ records = kcalloc(nrecords, sizeof *records, SLAB_KERNEL);
+ if (!records)
+ return -ENOMEM;
+ nrecords = len;
+ memcpy(records, data, nrecords * sizeof (*records));
+ return 0;
+}
+EXPORT_SYMBOL(i2c_set_boot_data);
+
+unsigned i2c_get_boot_data(struct i2c_device_record **datap)
+{
+ *datap = records;
+ return nrecords;
+}
+EXPORT_SYMBOL(i2c_get_boot_data);
+
+/*
+ * That's not much code, but it could be generalized something like
+ *
+ * static inline unsigned i2c_get_boot_data(struct ... **datap)
+ * {
+ * return bus_get_boot_data("i2c", datap);
+ * }
+ *
+ * and likewise for setting it. Another approach would use an
+ * add_boot_data() model. Maybe the best approach would be to
+ * use the driver model in more typical ways, and store this
+ * information in i2c_client.dev.platform_data from the moment
+ * the board-specific code creates that i2c_client node.
+ */
david-b
2005-07-18 00:31:54 UTC
Permalink
Post by david-b
Here's a refreshed version of the patch ... addressing much of
your feedback and replacing those nasty switch statements with
some more reasonable table driven logic. It's more generic.
And here's what will be the last update for a while. The main
patch I'd like to see someone contribute is SMBUS support so
this can run in PC type hardware too.

- Dave


Add a simple driver for most I2C EEPROMs, giving sysfs read/write
access to their contents. Tested with 24LC00, AT24C04, and 24LC64
so far; at both 100 and 400 KHz.

Changes from the last snapshot include bugfixes to address chip write
delays (an msleep, making things veeerrry slow at HZ=100), cleanups,
and claiming all a chip's addresses, to lock out any other drivers.
Also Kconfig doesn't headline Atmel AT24C series any more, since
they're far from the only chips supported.

Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>

--- osk.orig/drivers/i2c/chips/Kconfig 2005-07-17 17:28:00.000000000 -0700
+++ osk/drivers/i2c/chips/Kconfig 2005-07-17 17:28:02.000000000 -0700
@@ -406,6 +406,29 @@
menu "Other I2C Chip support"
depends on I2C

+config I2C_AT24C
+ tristate "EEPROMs from most vendors"
+ depends on I2C && EXPERIMENTAL
+ help
+ Enable this driver to get read/write support to most I2C EEPROMs,
+ after you configure the driver to know about each eeprom on on
+ your target board. Use these generic chip names, instead of
+ vendor-specific ones like at24c64 or 24lc02:
+
+ 24c00, 24c01, 24c02, dimm (readonly 24c02), 24c04, 24c08,
+ 24c16, 24c32, 24c64, 24c128, 24c256, 24c512, 24c1024
+
+ Unless you like data loss puzzles, always be sure that any chip
+ you configure as a 24c32 (32 Kbits) or larger is NOT really a
+ 24c16 (16 Kbits) or smaller. (Marking the chip as readonly won't
+ help recover from this.)
+
+ The current version of this driver demands full I2C bus support,
+ so it won't yet work on most PCs (limited to SMBUS).
+
+ This driver can also be built as a module. If so, the module
+ will be called at24c.
+
config SENSORS_DS1337
tristate "Dallas Semiconductor DS1337 and DS1339 Real Time Clock"
depends on I2C && EXPERIMENTAL
--- osk.orig/drivers/i2c/chips/Makefile 2005-07-17 17:28:00.000000000 -0700
+++ osk/drivers/i2c/chips/Makefile 2005-07-17 17:28:02.000000000 -0700
@@ -7,6 +7,9 @@
obj-$(CONFIG_SENSORS_W83627HF) += w83627hf.o
obj-$(CONFIG_SENSORS_W83781D) += w83781d.o

+# let 'at24c' get a chance before the less powerful 'eeprom'
+obj-$(CONFIG_I2C_AT24C) += at24c.o
+
obj-$(CONFIG_SENSORS_ADM1021) += adm1021.o
obj-$(CONFIG_SENSORS_ADM1025) += adm1025.o
obj-$(CONFIG_SENSORS_ADM1026) += adm1026.o
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ osk/drivers/i2c/chips/at24c.c 2005-07-17 17:28:02.000000000 -0700
@@ -0,0 +1,737 @@
+/*
+ * at24c.c -- support many I2C EEPROMs, including Atmel AT24C models
+ *
+ * Copyright (C) 2005 David Brownell
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+// #define DEBUG
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+
+#ifdef CONFIG_ARM
+#include <asm/mach-types.h>
+#endif
+
+
+/* If all the bus drivers are statically linked, I2C drivers often won't
+ * need hotplug support. Iff that's true in your config, you can #define
+ * NO_I2C_DYNAMIC_BUSSES and shrink this driver's runtime footprint.
+ */
+#define NO_I2C_DYNAMIC_BUSSES
+
+#ifdef NO_I2C_DYNAMIC_BUSSES
+#undef __devinit
+#undef __devinitdata
+#undef __devexit
+#undef __devexit_p
+/* no I2C devices will be hotplugging */
+#define __devinit __init
+#define __devinitdata __initdata
+#define __devexit __exit
+#define __devexit_p __exit_p
+#endif
+
+
+/* I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.
+ * Differences between different vendor product lines (like Atmel AT24C or
+ * MicroChip 24LC, etc) won't much matter for typical read/write access.
+ *
+ * However, misconfiguration can lose data (e.g. "set 16 bit memory address"
+ * could be interpreted as "write data at 8 bit address", or writing with too
+ * big a page size), so board-specific EEPROM configuration should use static
+ * board descriptions rather than dynamic probing when there's much potential
+ * for confusion.
+ *
+ * Using /etc/modprobe.conf you might configure a system with several EEPROMs
+ * something like this:
+ *
+ * options at24c chip_names=24c00,dimm,dimm,24c64 \
+ * probe=0,50,1,50,1,51,2,52 \
+ * readonly=Y,Y,Y,N
+ *
+ * Or, add some board-specific code at the end of this driver, before
+ * registering the driver.
+ *
+ * CONFIGURATION MECHANISM IS SUBJECT TO CHANGE!!
+ *
+ * Key current(*) differences from "eeprom" driver:
+ * (a) "at24c" also supports write access
+ * (b) "at24c" handles all common eeproms, not just 24c02 compatibles
+ * (c) "eeprom" will probe i2c and set up a 24c02 at each address;
+ * "at24c" expects a static config, with part types and addresses
+ * (d) "eeprom" also works on SMBUS-only pc type hardware
+ *
+ * (*) Patches accepted...
+ */
+static unsigned short probe[I2C_CLIENT_MAX_OPTS] __devinitdata
+ = I2C_CLIENT_DEFAULTS;
+static unsigned n_probe __devinitdata;
+module_param_array(probe, ushort, &n_probe, 0);
+MODULE_PARM_DESC(probe, "List of adapter,address pairs for I2C eeproms");
+
+static unsigned short ignore[] __devinitdata = { I2C_CLIENT_END };
+
+static struct i2c_client_address_data addr_data __devinitdata = {
+ .normal_i2c = ignore,
+ .probe = probe,
+ .ignore = ignore,
+ .force = ignore,
+};
+
+static char *chip_names[I2C_CLIENT_MAX_OPTS / 2] __devinitdata;
+static unsigned n_chip_names __devinitdata;
+module_param_array(chip_names, charp, &n_chip_names, 0);
+MODULE_PARM_DESC(chip_names,
+ "Type of chip; one chip name per probe pair");
+
+static int readonly[I2C_CLIENT_MAX_OPTS / 2] __devinitdata;
+static unsigned n_readonly __devinitdata;
+module_param_array(readonly, bool, &n_readonly, 0);
+MODULE_PARM_DESC(chip_names, "Readonly flags, one per probe pair");
+
+
+/* As seen through Linux I2C, differences between the most common types
+ * of I2C memory include:
+ * - How many I2C addresses the chip consumes: 1, 2, 4, or 8?
+ * - Memory address space for one I2C address: 256 bytes, or 64 KB?
+ * - How full that memory space is: 16 bytes, 256, 32Kb, etc?
+ * - What write page size does it support?
+ */
+struct chip_desc {
+ u32 byte_len; /* of 1..8 i2c addrs, total */
+ char name[10];
+ u16 page_size; /* for writes */
+ u8 i2c_addr_mask; /* for multi-addr chips */
+ u8 flags;
+#define EE_ADDR2 0x0080 /* 16 bit addrs; <= 64 KB */
+#define EE_READONLY 0x0040
+#define EE_24RF08 0x0001
+};
+
+struct at24c_data {
+ struct i2c_client client;
+ struct semaphore lock;
+ struct bin_attribute bin;
+
+ struct chip_desc chip;
+
+ /* each chip has an internal "memory address" pointer;
+ * we remember it for faster read access.
+ */
+ u32 next_addr;
+
+ /* some chips tie up multiple I2C addresses ... */
+ int users;
+ struct i2c_client extras[];
+};
+
+/* Specs often allow 5 msec for a page write, sometimes 20 msec;
+ * it's important to recover from write timeouts.
+ */
+#define EE_TIMEOUT 25
+
+
+/*-------------------------------------------------------------------------*/
+
+static struct chip_desc chips[] __devinitdata = {
+
+/* this first block of EEPROMS uses 8 bit memory addressing
+ * and can be accessed using standard SMBUS requests.
+ */
+{
+ /* 128 bit chip */
+ .name = "24c00",
+ .byte_len = 128 / 8,
+ .i2c_addr_mask = 0x07, /* I2C A0-A2 ignored */
+ .page_size = 1,
+}, {
+ /* 1 Kbit chip */
+ .name = "24c01",
+ .byte_len = 1024 / 8,
+ /* some have 16 byte pages: */
+ .page_size = 8,
+}, {
+ /* 2 Kbit chip */
+ .name = "24c02",
+ .byte_len = 2048 / 8,
+ /* some have 16 byte pages: */
+ .page_size = 8,
+}, {
+ /* 2 Kbit chip */
+ .name = "dimm", /* 24c02 in memory DIMMs */
+ .byte_len = 2048 / 8,
+ .flags = EE_READONLY,
+ /* some have 16 byte pages: */
+ .page_size = 8,
+}, {
+ /* 4 Kbit chip */
+ .name = "24c04",
+ .byte_len = 4096 / 8,
+ .page_size = 16,
+ .i2c_addr_mask = 0x01, /* I2C A0 is MEM A8 */
+}, {
+ /* 8 Kbit chip */
+ .name = "24c08",
+ .byte_len = 8192 / 8,
+ .page_size = 16,
+ .i2c_addr_mask = 0x03, /* I2C A1-A0 is MEM A9-A8 */
+}, {
+ /* 8 Kbit chip */
+ .name = "24rf08",
+ .byte_len = 8192 / 8,
+ .flags = EE_24RF08,
+ .page_size = 16,
+ .i2c_addr_mask = 0x03, /* I2C A1-A0 is MEM A9-A8 */
+}, {
+ /* 16 Kbit chip */
+ .name = "24c16",
+ .byte_len = 16384 / 8,
+ .page_size = 16,
+ .i2c_addr_mask = 0x07, /* I2C A2-A0 is MEM A10-A8 */
+},
+
+/* this second block of EEPROMS uses 16 bit memory addressing
+ * and can't be accessed using standard SMBUS requests.
+ * REVISIT maybe SMBUS 2.0 requests could work ...
+ */
+{
+ /* 32 Kbits */
+ .name = "24c32",
+ .byte_len = 32768 / 8,
+ .flags = EE_ADDR2,
+ .page_size = 32,
+}, {
+ /* 64 Kbits */
+ .name = "24c64",
+ .byte_len = 65536 / 8,
+ .flags = EE_ADDR2,
+ .page_size = 32,
+}, {
+ /* 128 Kbits */
+ .name = "24c128",
+ .byte_len = 131072 / 8,
+ .flags = EE_ADDR2,
+ .page_size = 64,
+}, {
+ /* 256 Kbits */
+ .name = "24c256",
+ .byte_len = 262144 / 8,
+ .flags = EE_ADDR2,
+ .page_size = 64,
+}, {
+ /* 512 Kbits */
+ .name = "24c512",
+ .byte_len = 524288 / 8,
+ .flags = EE_ADDR2,
+ .page_size = 128,
+}, {
+ /* 1 Mbits */
+ .name = "24c1024",
+ .byte_len = 1048576 / 8,
+ .flags = EE_ADDR2,
+ .page_size = 256,
+ .i2c_addr_mask = 0x01, /* I2C A0 is MEM A16 */
+}
+
+};
+
+static inline const struct chip_desc *__devinit
+find_chip(const char *s)
+{
+ unsigned i;
+
+ for (i = 0; i < ARRAY_SIZE(chips); i++)
+ if (strnicmp(s, chips[i].name, sizeof chips[i].name) == 0)
+ return &chips[i];
+ return NULL;
+}
+
+static inline const struct chip_desc *__devinit
+which_chip(struct i2c_adapter *adapter, int address, int *writable)
+{
+ unsigned i;
+
+ for (i = 0; i < n_probe; i++) {
+ const struct chip_desc *chip;
+ char *name;
+
+ if (probe[i++] != adapter->id)
+ continue;
+ if (probe[i] != address)
+ continue;
+
+ i >>= 1;
+ if ((name = chip_names[i]) == NULL) {
+ dev_err(&adapter->dev, "no chipname for addr %d\n",
+ address);
+ return NULL;
+
+ }
+ chip = find_chip(name);
+ if (chip == NULL)
+ dev_err(&adapter->dev, "unknown chipname %s\n", name);
+
+ /* user specified r/o value will overide the default */
+ if (i < n_readonly)
+ *writable = !readonly[i];
+ else
+ *writable = !(chip->flags & EE_READONLY);
+ return chip;
+ }
+
+ /* "can't happen" */
+ dev_dbg(&adapter->dev, "addr %d not in probe[]\n", address);
+ return NULL;
+}
+
+/*-------------------------------------------------------------------------*/
+
+/* This parameter is to help this driver avoid blocking other drivers
+ * out of I2C for potentially troublesome amounts of time. With a
+ * 100 KHz I2C clock, one 256 byte read takes about 1/43 second.
+ * VALUE MUST BE A POWER OF TWO so writes will align on pages!!
+ * Note that SMBUS has a 32 byte ceiling...
+ */
+static unsigned io_limit = 128;
+module_param(io_limit, uint, 0);
+MODULE_PARM_DESC(io_limit, "maximum bytes per i/o (default 128)");
+
+static inline int at24c_ee_address(
+ const struct chip_desc *chip,
+ struct i2c_msg *msg,
+ unsigned *offset,
+ u32 *next_addr
+)
+{
+ unsigned per_address = 256;
+
+ if (*offset >= chip->byte_len)
+ return -EINVAL;
+
+ /* Nothing to do unless accessing that memory would go to some
+ * later I2C address. In that case, modify the output params.
+ * Yes, it can loop more than once on 24c08 and 24c16.
+ */
+ if (chip->flags & EE_ADDR2)
+ per_address = 64 * 1024;
+ while (*offset >= per_address) {
+ msg->addr++;
+ *offset -= per_address;
+ *next_addr -= per_address;
+ }
+ return 0;
+}
+
+static ssize_t
+at24c_ee_read(
+ struct at24c_data *at24c,
+ char *buf,
+ unsigned offset,
+ size_t count
+)
+{
+ struct i2c_msg msg;
+ ssize_t status;
+ u32 next_addr;
+ size_t transferred = 0;
+
+ down(&at24c->lock);
+ msg.addr = at24c->client.addr;
+ msg.flags = 0;
+
+ /* maybe adjust i2c address and offset
+ * NOTE: we could now search at24c->extras to choose use
+ * some i2c_client other than the main one...
+ */
+ next_addr = at24c->next_addr;
+ status = at24c_ee_address(&at24c->chip, &msg, &offset, &next_addr);
+ if (status < 0)
+ goto done;
+
+ /* REVISIT at least some key cases here can use just the
+ * SMBUS subset; one issue is 16 byte "register" writes.
+ */
+
+ /* Maybe change the current on-chip address with a dummy write */
+ if (next_addr != offset) {
+ u8 tmp[2];
+
+ msg.buf = tmp;
+ tmp[1] = (u8) offset;
+ tmp[0] = (u8) (offset >> 8);
+ if (at24c->chip.flags & EE_ADDR2) {
+ msg.len = 2;
+ } else {
+ msg.len = 1;
+ msg.buf++;
+ }
+ status = i2c_transfer(at24c->client.adapter, &msg, 1);
+ dev_dbg(&at24c->client.dev,
+ "addr %02x, set current to %d --> %d\n",
+ msg.addr, offset, status);
+ if (status < 0)
+ goto done;
+ next_addr = at24c->next_addr = offset;
+ }
+
+ /* issue bounded sequential reads for the data bytes, knowing that
+ * read page rollover goes to the next page and/or memory block.
+ */
+ while (count > 0) {
+ unsigned segment;
+
+ if (count > io_limit)
+ segment = io_limit;
+ else
+ segment = count;
+
+ msg.len = segment;
+ msg.buf = buf;
+ msg.flags = I2C_M_RD;
+ status = i2c_transfer(at24c->client.adapter, &msg, 1);
+ dev_dbg(&at24c->client.dev, "read %d, %d --> %d\n",
+ transferred, count, status);
+
+ if (status < 0)
+ break;
+ if (status != segment) {
+ status = -EIO;
+ break;
+ }
+
+ count -= segment;
+
+ at24c->next_addr += segment;
+ offset += segment;
+ buf += segment;
+ transferred += segment;
+ }
+done:
+ if (status <= 0)
+ at24c->next_addr = at24c->bin.size;
+ up(&at24c->lock);
+ return transferred ? transferred : status;
+}
+
+static ssize_t
+at24c_bin_read(struct kobject *kobj, char *buf, loff_t off, size_t count)
+{
+ struct i2c_client *client;
+ struct at24c_data *at24c;
+
+ client = to_i2c_client(container_of(kobj, struct device, kobj));
+ at24c = i2c_get_clientdata(client);
+
+ if (unlikely(off >= at24c->bin.size))
+ return 0;
+ if ((off + count) > at24c->bin.size)
+ count = at24c->bin.size - off;
+ if (unlikely(!count))
+ return count;
+
+ /* we don't maintain caches for any data: simpler, cheaper */
+ return at24c_ee_read(at24c, buf, off, count);
+}
+
+
+/* Note that if the hardware write-protect pin is pulled high, the whole
+ * chip is normally write protected. But there are plenty of product
+ * variants here, including OTP fuses and partial chip protect.
+ */
+static ssize_t
+at24c_ee_write(struct at24c_data *at24c, char *buf, loff_t off, size_t count)
+{
+ struct i2c_msg msg;
+ ssize_t status = 0;
+ unsigned written = 0;
+ u32 scratch;
+ unsigned buf_size;
+ unsigned long timeout, retries;
+
+ /* temp buffer lets us stick the address at the beginning */
+ buf_size = at24c->chip.page_size;
+ if (buf_size > io_limit)
+ buf_size = io_limit;
+ msg.buf = kmalloc(buf_size + 2, GFP_KERNEL);
+ if (!msg.buf)
+ return -ENOMEM;
+ msg.flags = 0;
+
+ /* For write, rollover is within the page ... so we write at
+ * most one page, then manually roll over to the next page.
+ */
+ down(&at24c->lock);
+ timeout = jiffies + msecs_to_jiffies(EE_TIMEOUT);
+ retries = 0;
+ do {
+ unsigned segment;
+ unsigned offset = (unsigned) off;
+
+ /* maybe adjust i2c address and offset */
+ msg.addr = at24c->client.addr;
+ status = at24c_ee_address(&at24c->chip, &msg,
+ &offset, &scratch);
+ if (status < 0)
+ break;
+
+ /* write as much of a page as we can */
+ segment = buf_size - (offset % buf_size);
+ if (segment > count)
+ segment = count;
+ msg.len = segment + 1;
+ if (at24c->chip.flags & EE_ADDR2) {
+ msg.len++;
+ msg.buf[1] = (u8) offset;
+ msg.buf[0] = (u8) (offset >> 8);
+ memcpy(&msg.buf[2], buf, segment);
+ } else {
+ msg.buf[0] = offset;
+ memcpy(&msg.buf[1], buf, segment);
+ }
+
+ /* The chip may take a while to finish its previous write;
+ * this write also polls for completion of the last one.
+ * So we always retry a few times.
+ */
+ status = i2c_transfer(at24c->client.adapter, &msg, 1);
+ dev_dbg(&at24c->client.dev,
+ "write %d bytes to %02x at %d --> %d (%ld)\n",
+ segment, msg.addr, offset, status, jiffies);
+ if (status < 0) {
+ if (retries++ < 3 || time_after(timeout, jiffies)) {
+ /* REVISIT: at HZ=100, this is sloooow */
+ msleep(1);
+ continue;
+ }
+ dev_err(&at24c->client.dev,
+ "write %d bytes offset %d to %02x, "
+ "%ld ticks err %d\n",
+ segment, offset, msg.addr,
+ jiffies - (timeout - EE_TIMEOUT),
+ status);
+ status = -ETIMEDOUT;
+ break;
+ }
+
+ off += segment;
+ buf += segment;
+ count -= segment;
+ written += segment;
+ if (status < 0 || count == 0) {
+ if (written != 0)
+ status = written;
+ break;
+ }
+
+ /* yielding may avoid the losing msleep() path above */
+ yield();
+ timeout = jiffies + msecs_to_jiffies(EE_TIMEOUT);
+ retries = 0;
+ } while (count > 0);
+ at24c->next_addr = at24c->bin.size;
+ up(&at24c->lock);
+
+ kfree(msg.buf);
+ return status;
+}
+
+static ssize_t
+at24c_bin_write(struct kobject *kobj, char *buf, loff_t off, size_t count)
+{
+ struct i2c_client *client;
+ struct at24c_data *at24c;
+
+ client = to_i2c_client(container_of(kobj, struct device, kobj));
+ at24c = i2c_get_clientdata(client);
+
+ if (unlikely(off >= at24c->bin.size))
+ return -EFBIG;
+ if ((off + count) > at24c->bin.size)
+ count = at24c->bin.size - off;
+ if (unlikely(!count))
+ return count;
+
+ return at24c_ee_write(at24c, buf, off, count);
+}
+
+/*-------------------------------------------------------------------------*/
+
+static struct i2c_driver at24c_driver;
+
+static void __devinit
+at24c_activate(
+ struct i2c_adapter *adapter,
+ int address,
+ const struct chip_desc *chip,
+ int writable
+)
+{
+ struct at24c_data *at24c;
+ int err = -ENOMEM;
+
+ if (!(at24c = kcalloc(1, sizeof(struct at24c_data)
+ + chip->i2c_addr_mask * sizeof(struct i2c_client),
+ GFP_KERNEL)))
+ goto fail;
+
+ init_MUTEX(&at24c->lock);
+ at24c->chip = *chip;
+ snprintf(at24c->client.name, I2C_NAME_SIZE,
+ "%s I2C EEPROM", at24c->chip.name);
+
+ /* Export the EEPROM bytes through sysfs, since that's convenient.
+ * By default, only root should see the data (maybe passwords etc)
+ */
+ at24c->bin.attr.name = "eeprom";
+ at24c->bin.attr.mode = S_IRUSR;
+ at24c->bin.attr.owner = THIS_MODULE;
+ at24c->bin.read = at24c_bin_read;
+
+ at24c->bin.size = at24c->chip.byte_len;
+ at24c->next_addr = at24c->bin.size;
+ if (writable) {
+ at24c->bin.write = at24c_bin_write;
+ at24c->bin.attr.mode |= S_IWUSR;
+ }
+
+ /* register the first chip address */
+ address &= ~at24c->chip.i2c_addr_mask;
+
+ at24c->client.addr = address;
+ at24c->client.adapter = adapter;
+ at24c->client.driver = &at24c_driver;
+ at24c->client.flags = 0;
+ i2c_set_clientdata(&at24c->client, at24c);
+
+ /* prevent AT24RF08 corruption, a possible consequence of the
+ * probe done by the i2c core (verifying a chip is present).
+ */
+ if (chip->flags & EE_24RF08)
+ i2c_smbus_write_quick(&at24c->client, 0);
+
+ if ((err = i2c_attach_client(&at24c->client)))
+ goto fail;
+ at24c->users++;
+
+ /* then register any other addresses, so other drivers can't */
+ if (chip->i2c_addr_mask) {
+ unsigned i;
+ struct i2c_client *c;
+
+ for (i = 0; i < chip->i2c_addr_mask; i++) {
+ c = &at24c->extras[i];
+ c->addr = address + i + 1;
+ c->adapter = adapter;
+ c->driver = &at24c_driver;
+
+ i2c_set_clientdata(c, at24c);
+ snprintf(c->name, sizeof c->name,
+ "%s address %d", chip->name, i + 2);
+ err = i2c_attach_client(c);
+ if (err == 0)
+ at24c->users++;
+ }
+ }
+
+ sysfs_create_bin_file(&at24c->client.dev.kobj, &at24c->bin);
+
+ dev_info(&at24c->client.dev, "%d byte %s%s\n",
+ at24c->bin.size, at24c->client.name,
+ writable ? " (writable)" : "");
+ dev_dbg(&at24c->client.dev, "page_size %d, i2c_addr_mask %d\n",
+ at24c->chip.page_size, at24c->chip.i2c_addr_mask);
+ return;
+fail:
+ dev_dbg(&adapter->dev, "%s probe, err %d\n", at24c_driver.name, err);
+ kfree(at24c);
+}
+
+static int __devinit
+at24c_old_probe(struct i2c_adapter *adapter, int address, int kind)
+{
+ const struct chip_desc *chip;
+ int writable;
+
+ chip = which_chip(adapter, address, &writable);
+ if (chip)
+ at24c_activate(adapter, address, chip, writable);
+ return 0;
+}
+
+
+static int __devinit at24c_attach_adapter(struct i2c_adapter *adapter)
+{
+ /* REVISIT: using SMBUS calls would improve portability, though
+ * maybe at the cost of support for 16 bit memory addressing...
+ */
+ if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) {
+ dev_dbg(&adapter->dev, "%s probe, no I2C\n",
+ at24c_driver.name);
+ return 0;
+ }
+ return i2c_probe(adapter, &addr_data, at24c_old_probe);
+}
+
+static int __devexit at24c_detach_client(struct i2c_client *client)
+{
+ int err;
+ struct at24c_data *at24c;
+
+ err = i2c_detach_client(client);
+ if (err) {
+ dev_err(&client->dev, "deregistration failed, %d\n", err);
+ return err;
+ }
+
+ /* up to eight clients per chip, detached in any order... */
+ at24c = i2c_get_clientdata(client);
+ if (at24c->users-- == 0)
+ kfree(at24c);
+ return 0;
+}
+
+/*-------------------------------------------------------------------------*/
+
+static struct i2c_driver at24c_driver = {
+ .owner = THIS_MODULE,
+ .name = "at24c",
+ .flags = I2C_DF_NOTIFY,
+ .attach_adapter = at24c_attach_adapter,
+ .detach_client = __devexit_p(at24c_detach_client),
+};
+
+static int __init at24c_init(void)
+{
+#ifdef CONFIG_OMAP_OSK_MISTRAL
+ if (machine_is_omap_osk()) {
+ n_chip_names = 1;
+ n_probe = 1;
+ chip_names[0] = "24c04";
+ probe[0] = 0;
+ probe[1] = 0x50;
+ }
+
+#endif
+ return i2c_add_driver(&at24c_driver);
+}
+module_init(at24c_init);
+
+static void __exit at24c_exit(void)
+{
+ i2c_del_driver(&at24c_driver);
+}
+module_exit(at24c_exit);
+
+MODULE_DESCRIPTION("Driver for AT24C series (and compatible) I2C EEPROMs");
+MODULE_AUTHOR("David Brownell");
+MODULE_LICENSE("GPL");
+
Jean Delvare
2005-07-24 16:10:56 UTC
Permalink
Hi David,
Post by david-b
And here's what will be the last update for a while. The main
patch I'd like to see someone contribute is SMBUS support so
this can run in PC type hardware too.
I've linked from our "new drivers" page to this post in the archive so
that people looking for 2-byte addressed eeprom support will hopefully
find it.
--
Jean Delvare
Loading...