[PATCH RFC] misc/at24: distinguish between eeprom and fram chips

[PATCH RFC] misc/at24: distinguish between eeprom and fram chips

[Lars Poeschel]

Hello!

I wanted to use a fm24c04 i2c fram chip with linux. I grepped the source
and found nothing. I later found that my chip can be handled by at24
eeprom driver. It creates a sysfs file called eeprom to read from and
write to the chip. Userspace has no chance to distinguish if it is
writing an eeprom or a fram chip.

I present this patch for 3 reasons:
1. For other people grepping finding a little more reference.
2. For userspace being able to distinguish eeprom and fram.
3. Raising the bytes per write for fram chips.

What do you kernel developers think ?

Cheers,
Lars
-- >8 --
>From 4fab49fae62390995868e3b6dee7e0693fce5be9 Mon Sep 17 00:00:00 2001
From: Lars Poeschel <poeschel@lemonage.de>
Date: Tue, 4 Dec 2012 15:41:40 +0100
Subject: [PATCH] misc/at24: distinguish between eeprom and fram chips

Add a AT24_FLAGS_FRAM state to the flags to make userspace able to
distinguish if it is using eeprom or fram. The sysfs entry gets the
name "fram" instead of "eeprom".
For frams the bytes/write can be at least 128 bytes, since these
chips have no need to internally buffer writes.

Signed-off-by: Lars Poeschel <poeschel@lemonage.de>

diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index c9e695e..55948a5 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -12,6 +12,12 @@ config EEPROM_AT24
 	     24c00, 24c01, 24c02, spd (readonly 24c02), 24c04, 24c08,
 	     24c16, 24c32, 24c64, 24c128, 24c256, 24c512, 24c1024
 
+	  This driver also supports I2C FRAM chips that are feature
+	  compatible to the 24cxx ones. In your at24_platform_data set
+	  .flags = AT24_FLAG_FRAM. These generic names are supported:
+
+	     fm24c04
+
 	  Unless you like data loss puzzles, always be sure that any chip
 	  you configure as a 24c32 (32 kbit) or larger is NOT really a
 	  24c16 (16 kbit) or smaller, and vice versa. Marking the chip
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index ab1ad41..02a03a1 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -125,6 +125,7 @@ static const struct i2c_device_id at24_ids[] = {
 	{ "24c256", AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) },
 	{ "24c512", AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16) },
 	{ "24c1024", AT24_DEVICE_MAGIC(1048576 / 8, AT24_FLAG_ADDR16) },
+	{ "fm24c04", AT24_DEVICE_MAGIC(4096 / 8, AT24_FLAG_FRAM) },
 	{ "at24", 0 },
 	{ /* END OF LIST */ }
 };
@@ -504,8 +505,13 @@ static int at24_probe(struct i2c_client *client, const 
struct i2c_device_id *id)
 		 * This is slow, but we can't know all eeproms, so we better
 		 * play safe. Specifying custom eeprom-types via platform_data
 		 * is recommended anyhow.
+		 * For fram chips, we can allow minmum 128 bytes, as there is
+		 * no page size and 128 is the smallest so far seen chip.
 		 */
-		chip.page_size = 1;
+		if (chip.flags & AT24_FLAG_FRAM)
+			chip.page_size = 128;
+		else
+			chip.page_size = 1;
 
 		/* update chipdata if OF is present */
 		at24_get_ofdata(client, &chip);
@@ -570,7 +576,10 @@ static int at24_probe(struct i2c_client *client, const 
struct i2c_device_id *id)
 	 * By default, only root should see the data (maybe passwords etc)
 	 */
 	sysfs_bin_attr_init(&at24->bin);
-	at24->bin.attr.name = "eeprom";
+	if (chip.flags & AT24_FLAG_FRAM)
+		at24->bin.attr.name = "fram";
+	else
+		at24->bin.attr.name = "eeprom";
 	at24->bin.attr.mode = chip.flags & AT24_FLAG_IRUGO ? S_IRUGO : S_IRUSR;
 	at24->bin.read = at24_bin_read;
 	at24->bin.size = chip.byte_len;
diff --git a/include/linux/i2c/at24.h b/include/linux/i2c/at24.h
index 285025a..d786b71 100644
--- a/include/linux/i2c/at24.h
+++ b/include/linux/i2c/at24.h
@@ -47,6 +47,7 @@ struct at24_platform_data {
 #define AT24_FLAG_READONLY	0x40	/* sysfs-entry will be read-only */
 #define AT24_FLAG_IRUGO		0x20	/* sysfs-entry will be world-readable */
 #define AT24_FLAG_TAKE8ADDR	0x10	/* take always 8 addresses (24c00) */
+#define AT24_FLAG_FRAM          0x08    /* chip is fram not eeprom */
 
 	void		(*setup)(struct memory_accessor *, void *context);
 	void		*context;
-- 
1.7.10.4

Re: [PATCH RFC] misc/at24: distinguish between eeprom and fram chips

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 5712 bytes --]

Hi,

> I wanted to use a fm24c04 i2c fram chip with linux. I grepped the source
> and found nothing. I later found that my chip can be handled by at24
> eeprom driver. It creates a sysfs file called eeprom to read from and
> write to the chip. Userspace has no chance to distinguish if it is
> writing an eeprom or a fram chip.

Why should it?

> I present this patch for 3 reasons:
> 1. For other people grepping finding a little more reference.

Yes. The Kconfig entry could be updated to mention not only EEPROMs.

> 2. For userspace being able to distinguish eeprom and fram.

Why?

> 3. Raising the bytes per write for fram chips.

Properly configuring the chip (including proper page_size) is the way to
go IMO. Read on...

> 
> What do you kernel developers think ?
> 
> Cheers,
> Lars
> -- >8 --
> From 4fab49fae62390995868e3b6dee7e0693fce5be9 Mon Sep 17 00:00:00 2001
> From: Lars Poeschel <poeschel@lemonage.de>
> Date: Tue, 4 Dec 2012 15:41:40 +0100
> Subject: [PATCH] misc/at24: distinguish between eeprom and fram chips
> 
> Add a AT24_FLAGS_FRAM state to the flags to make userspace able to
> distinguish if it is using eeprom or fram. The sysfs entry gets the
> name "fram" instead of "eeprom".
> For frams the bytes/write can be at least 128 bytes, since these
> chips have no need to internally buffer writes.
> 
> Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> 
> diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
> index c9e695e..55948a5 100644
> --- a/drivers/misc/eeprom/Kconfig
> +++ b/drivers/misc/eeprom/Kconfig
> @@ -12,6 +12,12 @@ config EEPROM_AT24
>  	     24c00, 24c01, 24c02, spd (readonly 24c02), 24c04, 24c08,
>  	     24c16, 24c32, 24c64, 24c128, 24c256, 24c512, 24c1024
>  
> +	  This driver also supports I2C FRAM chips that are feature
> +	  compatible to the 24cxx ones. In your at24_platform_data set
> +	  .flags = AT24_FLAG_FRAM. These generic names are supported:
> +
> +	     fm24c04
> +

The method of accessing EEPROMs is used by way more chips than FRAMs.
So, I'd prefer to have the text updated more generic like "EEPROMs and
similar devices like RAMs, ROMs, etc...". Describing setting .flags in
Kconfig is overkill.


>  	  Unless you like data loss puzzles, always be sure that any chip
>  	  you configure as a 24c32 (32 kbit) or larger is NOT really a
>  	  24c16 (16 kbit) or smaller, and vice versa. Marking the chip
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index ab1ad41..02a03a1 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -125,6 +125,7 @@ static const struct i2c_device_id at24_ids[] = {
>  	{ "24c256", AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) },
>  	{ "24c512", AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16) },
>  	{ "24c1024", AT24_DEVICE_MAGIC(1048576 / 8, AT24_FLAG_ADDR16) },
> +	{ "fm24c04", AT24_DEVICE_MAGIC(4096 / 8, AT24_FLAG_FRAM) },

The 24cxxx entries are last resort fallbacks to give you minimal access.
They also do this for your FRAM correctly. For proper setup, you need
custom configuration anyhow. There, you can define the page_size as well
as the permissions for the created file. So, no to this change.

>  	{ "at24", 0 },
>  	{ /* END OF LIST */ }
>  };
> @@ -504,8 +505,13 @@ static int at24_probe(struct i2c_client *client, const 
> struct i2c_device_id *id)
>  		 * This is slow, but we can't know all eeproms, so we better
>  		 * play safe. Specifying custom eeprom-types via platform_data
>  		 * is recommended anyhow.
> +		 * For fram chips, we can allow minmum 128 bytes, as there is
> +		 * no page size and 128 is the smallest so far seen chip.
>  		 */
> -		chip.page_size = 1;
> +		if (chip.flags & AT24_FLAG_FRAM)
> +			chip.page_size = 128;
> +		else
> +			chip.page_size = 1;

I'd think most FRAMs can have the chip_size as the page_size since they
probably don't do buffering. But do you know for all the chips out
there? So, let's still play safe. If you want performance, you need to
setup the driver properly.

>  
>  		/* update chipdata if OF is present */
>  		at24_get_ofdata(client, &chip);
> @@ -570,7 +576,10 @@ static int at24_probe(struct i2c_client *client, const 
> struct i2c_device_id *id)
>  	 * By default, only root should see the data (maybe passwords etc)
>  	 */
>  	sysfs_bin_attr_init(&at24->bin);
> -	at24->bin.attr.name = "eeprom";
> +	if (chip.flags & AT24_FLAG_FRAM)
> +		at24->bin.attr.name = "fram";
> +	else
> +		at24->bin.attr.name = "eeprom";

Again no. Applications would need to check for this file or that file.
The name "eeprom" is indeed a bit unfortunate, but is historic cruft.

>  	at24->bin.attr.mode = chip.flags & AT24_FLAG_IRUGO ? S_IRUGO : S_IRUSR;
>  	at24->bin.read = at24_bin_read;
>  	at24->bin.size = chip.byte_len;
> diff --git a/include/linux/i2c/at24.h b/include/linux/i2c/at24.h
> index 285025a..d786b71 100644
> --- a/include/linux/i2c/at24.h
> +++ b/include/linux/i2c/at24.h
> @@ -47,6 +47,7 @@ struct at24_platform_data {
>  #define AT24_FLAG_READONLY	0x40	/* sysfs-entry will be read-only */
>  #define AT24_FLAG_IRUGO		0x20	/* sysfs-entry will be world-readable */
>  #define AT24_FLAG_TAKE8ADDR	0x10	/* take always 8 addresses (24c00) */
> +#define AT24_FLAG_FRAM          0x08    /* chip is fram not eeprom */
>  
>  	void		(*setup)(struct memory_accessor *, void *context);
>  	void		*context;

Thanks for your interest nonetheless,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH RFC] misc/at24: distinguish between eeprom and fram chips

[Lars Poeschel]

I see there where to much "no"s to get anything in, but thank you for
your comments and explanations.

> > I wanted to use a fm24c04 i2c fram chip with linux. I grepped the source
> > and found nothing. I later found that my chip can be handled by at24
> > eeprom driver. It creates a sysfs file called eeprom to read from and
> > write to the chip. Userspace has no chance to distinguish if it is
> > writing an eeprom or a fram chip.
> 
> Why should it?

Because writes are much faster and it doesn't have to take care on erase
cycles. It could use other write strategies on such devices and update
informations that have to survive power downs more often.
 
> > diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
> > index c9e695e..55948a5 100644
> > --- a/drivers/misc/eeprom/Kconfig
> > +++ b/drivers/misc/eeprom/Kconfig
> > @@ -12,6 +12,12 @@ config EEPROM_AT24
> > 
> >  	     24c00, 24c01, 24c02, spd (readonly 24c02), 24c04, 24c08,
> >  	     24c16, 24c32, 24c64, 24c128, 24c256, 24c512, 24c1024
> > 
> > +	  This driver also supports I2C FRAM chips that are feature
> > +	  compatible to the 24cxx ones. In your at24_platform_data set
> > +	  .flags = AT24_FLAG_FRAM. These generic names are supported:
> > +
> > +	     fm24c04
> > +
> 
> The method of accessing EEPROMs is used by way more chips than FRAMs.
> So, I'd prefer to have the text updated more generic like "EEPROMs and
> similar devices like RAMs, ROMs, etc...". Describing setting .flags in
> Kconfig is overkill.

A patch updating Kconfig is below.
 
> > -		chip.page_size = 1;
> > +		if (chip.flags & AT24_FLAG_FRAM)
> > +			chip.page_size = 128;
> > +		else
> > +			chip.page_size = 1;
> 
> I'd think most FRAMs can have the chip_size as the page_size since they
> probably don't do buffering. But do you know for all the chips out
> there? So, let's still play safe. If you want performance, you need to
> setup the driver properly.

No one knows all chips out there.
For the fm24c04 I use page_size != chip_size. It does not buffer but it has 
two pages, 256 bytes each.

-- >8 --
>From 82f77ade238755b7a1196b24f41a03a0e7344d89 Mon Sep 17 00:00:00 2001
From: Lars Poeschel <poeschel@lemonage.de>
Date: Wed, 5 Dec 2012 10:08:07 +0100
Subject: [PATCH] misc/at24: Update Kconfig to mention FRAMs, SRAMs and ROMs

As the at24 driver is able handle a bunch of serial storage chips other than
EEPROMs this is now mentioned in Kconfig.

Signed-off-by: Lars Poeschel <poeschel@lemonage.de>

diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index c9e695e..04f2e1f 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -1,13 +1,14 @@
 menu "EEPROM support"
 
 config EEPROM_AT24
-	tristate "I2C EEPROMs from most vendors"
+	tristate "I2C EEPROMs / RAMs / ROMs from most vendors"
 	depends on I2C && SYSFS
 	help
-	  Enable this driver to get read/write support to most I2C EEPROMs,
-	  after you configure the driver to know about each EEPROM on
-	  your target board.  Use these generic chip names, instead of
-	  vendor-specific ones like at24c64 or 24lc02:
+	  Enable this driver to get read/write support to most I2C EEPROMs
+	  and compatible devices like FRAMs, SRAMs, ROMs etc. After you
+	  configure the driver to know about each chip on your target
+	  board.  Use these generic chip names, instead of vendor-specific
+	  ones like at24c64, 24lc02 or fm24c04:
 
 	     24c00, 24c01, 24c02, spd (readonly 24c02), 24c04, 24c08,
 	     24c16, 24c32, 24c64, 24c128, 24c256, 24c512, 24c1024
-- 
1.7.10.4

Re: [PATCH RFC] misc/at24: distinguish between eeprom and fram chips

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1667 bytes --]

Hi,

On Wed, Dec 05, 2012 at 10:43:07AM +0100, Lars Poeschel wrote:
> I see there where to much "no"s to get anything in, but thank you for
> your comments and explanations.

Not necessarily, just not in this form :)

> 
> > > I wanted to use a fm24c04 i2c fram chip with linux. I grepped the source
> > > and found nothing. I later found that my chip can be handled by at24
> > > eeprom driver. It creates a sysfs file called eeprom to read from and
> > > write to the chip. Userspace has no chance to distinguish if it is
> > > writing an eeprom or a fram chip.
> > 
> > Why should it?
> 
> Because writes are much faster and it doesn't have to take care on erase
> cycles. It could use other write strategies on such devices and update
> informations that have to survive power downs more often.

I agree. I think that a seperate attribute named e.g. 'page_size' would
be more helpful than renaming the binary file to fram?

> > The method of accessing EEPROMs is used by way more chips than FRAMs.
> > So, I'd prefer to have the text updated more generic like "EEPROMs and
> > similar devices like RAMs, ROMs, etc...". Describing setting .flags in
> > Kconfig is overkill.
> 
> A patch updating Kconfig is below.

Looks good from a glimpse, will apply it later.

> No one knows all chips out there.
> For the fm24c04 I use page_size != chip_size. It does not buffer but it has 
> two pages, 256 bytes each.

Yup, it uses two I2C adresses...

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH RFC] misc/at24: distinguish between eeprom and fram chips

[Lars Poeschel]

> > > > I wanted to use a fm24c04 i2c fram chip with linux. I grepped the
> > > > source and found nothing. I later found that my chip can be handled
> > > > by at24 eeprom driver. It creates a sysfs file called eeprom to
> > > > read from and write to the chip. Userspace has no chance to
> > > > distinguish if it is writing an eeprom or a fram chip.
> > > 
> > > Why should it?
> > 
> > Because writes are much faster and it doesn't have to take care on erase
> > cycles. It could use other write strategies on such devices and update
> > informations that have to survive power downs more often.
> 
> I agree. I think that a seperate attribute named e.g. 'page_size' would
> be more helpful than renaming the binary file to fram?

Yes, this is a much better solution! Adding a seperate sysfs file page_size 
and a file for the type of device which would read eeprom, fram, etc then.
If you also think this is the way to go, I would spent one of my next free 
timeslots to this.

> > > The method of accessing EEPROMs is used by way more chips than FRAMs.
> > > So, I'd prefer to have the text updated more generic like "EEPROMs and
> > > similar devices like RAMs, ROMs, etc...". Describing setting .flags in
> > > Kconfig is overkill.
> > 
> > A patch updating Kconfig is below.
> 
> Looks good from a glimpse, will apply it later.

Thank you!

Lars

Re: [PATCH RFC] misc/at24: distinguish between eeprom and fram chips

[Wolfram Sang]

On Fri, Dec 07, 2012 at 11:14:28AM +0100, Lars Poeschel wrote:
> > > > > I wanted to use a fm24c04 i2c fram chip with linux. I grepped the
> > > > > source and found nothing. I later found that my chip can be handled
> > > > > by at24 eeprom driver. It creates a sysfs file called eeprom to
> > > > > read from and write to the chip. Userspace has no chance to
> > > > > distinguish if it is writing an eeprom or a fram chip.
> > > > 
> > > > Why should it?
> > > 
> > > Because writes are much faster and it doesn't have to take care on erase
> > > cycles. It could use other write strategies on such devices and update
> > > informations that have to survive power downs more often.
> > 
> > I agree. I think that a seperate attribute named e.g. 'page_size' would
> > be more helpful than renaming the binary file to fram?
> 
> Yes, this is a much better solution! Adding a seperate sysfs file page_size 
> and a file for the type of device which would read eeprom, fram, etc then.
> If you also think this is the way to go, I would spent one of my next free 
> timeslots to this.

Oops, this mail seems to have dropped off :(

I am all for the 'page_size' attribute, but still not convinced what
gain the 'type' attribute would allow. For FRAM, the page size will be
large. Isn't this enough information?

Regards,

   Wolfram

Re: [PATCH RFC] misc/at24: distinguish between eeprom and fram chips

[Lars Poeschel]

On Thursday 24 January 2013 at 08:27:01, Wolfram Sang wrote:
> > > > > > I wanted to use a fm24c04 i2c fram chip with linux. I grepped
> > > > > > the source and found nothing. I later found that my chip can be
> > > > > > handled by at24 eeprom driver. It creates a sysfs file called
> > > > > > eeprom to read from and write to the chip. Userspace has no
> > > > > > chance to distinguish if it is writing an eeprom or a fram
> > > > > > chip.
> > > > > 
> > > > > Why should it?
> > > > 
> > > > Because writes are much faster and it doesn't have to take care on
> > > > erase cycles. It could use other write strategies on such devices
> > > > and update informations that have to survive power downs more
> > > > often.
> > > 
> > > I agree. I think that a seperate attribute named e.g. 'page_size'
> > > would be more helpful than renaming the binary file to fram?
> > 
> > Yes, this is a much better solution! Adding a seperate sysfs file
> > page_size and a file for the type of device which would read eeprom,
> > fram, etc then. If you also think this is the way to go, I would spent
> > one of my next free timeslots to this.
> 
> Oops, this mail seems to have dropped off :(

Luckily I did not have a free timeslot to invest yet. ;)

> I am all for the 'page_size' attribute, but still not convinced what
> gain the 'type' attribute would allow. For FRAM, the page size will be
> large. Isn't this enough information?

Yes, this would be enough information and I think this is the way we should 
go.
I set this on my todo list. Although the change will be quite simple, I think 
I will not find the time to hit the upcoming merge window.

Lars

Re: [PATCH RFC] misc/at24: distinguish between eeprom and fram chips

[Lars Poeschel]

On Wednesday 05 December 2012 at 17:41:53, Wolfram Sang wrote:

> > > The method of accessing EEPROMs is used by way more chips than FRAMs.
> > > So, I'd prefer to have the text updated more generic like "EEPROMs and
> > > similar devices like RAMs, ROMs, etc...". Describing setting .flags in
> > > Kconfig is overkill.
> > 
> > A patch updating Kconfig is below.
> 
> Looks good from a glimpse, will apply it later.

Hello Wolfram,

what happend to this one ? It was a patch updating Kconfig help for at24.

https://patchwork.kernel.org/patch/1840591/

Regards,
Lars

Re: [PATCH RFC] misc/at24: distinguish between eeprom and fram chips

[Wolfram Sang]

> what happend to this one ? It was a patch updating Kconfig help for at24.

Do you know linux-next? Have a look there...

Re: [PATCH RFC] misc/at24: distinguish between eeprom and fram chips

[Lars Poeschel]

On Sunday 10 February 2013 at 17:30:47, Wolfram Sang wrote:
> > what happend to this one ? It was a patch updating Kconfig help for
> > at24.
> 
> Do you know linux-next? Have a look there...

Thank you for the hint and for commiting it.

Lars