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

[Wolfram Sang]

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@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
> 
> 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: signature.asc
Description: Digital signature