at24 eeprom driver (V2) and misc patches

at24 eeprom driver (V2) and misc patches

[Marc Reilly]

Hi,

These patches add an (improved) at24 eeprom driver and some other fixes. The
eeprom driver has improved write handling that polls the device for write 
completion (rather than fixed 10ms delay).

The eeprom driver requires patch 3 and 4. 

The nand patches fix a build error when BBT is not enabled.

Cheers,
Marc


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/7] imx35: 6-bit divider helper

[Marc Reilly]

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 arch/arm/mach-imx/speed-imx35.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-imx/speed-imx35.c b/arch/arm/mach-imx/speed-imx35.c
index 684dc14..905ab47 100644
--- a/arch/arm/mach-imx/speed-imx35.c
+++ b/arch/arm/mach-imx/speed-imx35.c
@@ -97,6 +97,11 @@ static unsigned long get_3_3_div(unsigned long in)
 	return (((in >> 3) & 0x7) + 1) * ((in & 0x7) + 1);
 }
 
+static unsigned long get_6_div(unsigned long in)
+{
+	return ((in & 0x3f) + 1);
+}
+
 static unsigned long imx_get_ipg_perclk(void)
 {
 	ulong pdr0 = readl(IMX_CCM_BASE + CCM_PDR0);
-- 
1.7.7


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/7] imx35: mmc clock has 6 bit divider, not 3_3

[Marc Reilly]

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 arch/arm/mach-imx/speed-imx35.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-imx/speed-imx35.c b/arch/arm/mach-imx/speed-imx35.c
index 905ab47..6d4236a 100644
--- a/arch/arm/mach-imx/speed-imx35.c
+++ b/arch/arm/mach-imx/speed-imx35.c
@@ -170,10 +170,11 @@ unsigned long imx_get_uartclk(void)
 		return imx_get_ppllclk() / div;
 }
 
+/* mmc0 clk only */
 unsigned long imx_get_mmcclk(void)
 {
 	unsigned long pdr3 = readl(IMX_CCM_BASE + CCM_PDR3);
-	unsigned long div = get_3_3_div(pdr3);
+	unsigned long div = get_6_div(pdr3);
 
 	if (pdr3 & (1 << 6))
 		return imx_get_armclk() / div;
-- 
1.7.7


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/7] i2c: add platform_data for i2c_board_info

[Marc Reilly]

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/i2c/i2c.c |    1 +
 include/i2c/i2c.h |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/i2c.c b/drivers/i2c/i2c.c
index 5df0d30..15f5507 100644
--- a/drivers/i2c/i2c.c
+++ b/drivers/i2c/i2c.c
@@ -250,6 +250,7 @@ struct i2c_client *i2c_new_device(struct i2c_adapter *adapter,
 	client = xzalloc(sizeof *client);
 	strcpy(client->dev.name, chip->type);
 	client->dev.type_data = client;
+	client->dev.platform_data = chip->platform_data;
 	client->adapter = adapter;
 	client->addr = chip->addr;
 
diff --git a/include/i2c/i2c.h b/include/i2c/i2c.h
index ccbf518..c3e1763 100644
--- a/include/i2c/i2c.h
+++ b/include/i2c/i2c.h
@@ -99,6 +99,7 @@ struct i2c_client {
 struct i2c_board_info {
 	char		type[I2C_NAME_SIZE];	/**< name of device */
 	unsigned short	addr;			/**< stored in i2c_client.addr */
+	void		*platform_data;		/**< platform data for device */
 };
 
 /**
-- 
1.7.7


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/7] i2c: device id default to -1 for auto assignment

[Marc Reilly]

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/i2c/i2c.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/i2c.c b/drivers/i2c/i2c.c
index 15f5507..c0d8fa2 100644
--- a/drivers/i2c/i2c.c
+++ b/drivers/i2c/i2c.c
@@ -251,6 +251,7 @@ struct i2c_client *i2c_new_device(struct i2c_adapter *adapter,
 	strcpy(client->dev.name, chip->type);
 	client->dev.type_data = client;
 	client->dev.platform_data = chip->platform_data;
+	client->dev.id = -1;
 	client->adapter = adapter;
 	client->addr = chip->addr;
 
-- 
1.7.7


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/7] nand: fix build error when BBT not enabled.

[Marc Reilly]

This fixes following error when bad blocks config option is not enabled:

drivers/built-in.o: In function `nand_erase_nand':
drivers/mtd/nand/nand_write.c:721: undefined reference to `nand_update_bbt'
drivers/built-in.o: In function `nand_default_block_markbad':
drivers/mtd/nand/nand_write.c:76: undefined reference to `nand_update_bbt'
make: *** [barebox] Error 1

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/mtd/nand/nand_write.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nand_write.c b/drivers/mtd/nand/nand_write.c
index 13b6c89..0fd49d6 100644
--- a/drivers/mtd/nand/nand_write.c
+++ b/drivers/mtd/nand/nand_write.c
@@ -72,9 +72,15 @@ int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs)
 		chip->bbt[block >> 2] |= 0x01 << ((block & 0x03) << 1);
 
 	/* Do we have a flash based bad block table ? */
-	if (chip->options & NAND_USE_FLASH_BBT)
+	if (chip->options & NAND_USE_FLASH_BBT) {
+#if defined(CONFIG_NAND_BBT)
 		ret = nand_update_bbt(mtd, ofs);
-	else {
+#else
+		printk(KERN_WARNING "NAND trying to use BBT,"
+				"but not supported\n");
+		ret = 0;
+#endif
+	} else {
 		/* We write two bytes, so we dont have to mess with 16 bit
 		 * access
 		 */
@@ -714,11 +720,13 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
 	for (chipnr = 0; chipnr < chip->numchips; chipnr++) {
 		if (!rewrite_bbt[chipnr])
 			continue;
+#if defined(CONFIG_NAND_BBT)
 		/* update the BBT for chip */
 		MTD_DEBUG(MTD_DEBUG_LEVEL0, "nand_erase_nand: nand_update_bbt "
 		      "(%d:0x%0x 0x%0x)\n", chipnr, rewrite_bbt[chipnr],
 		      chip->bbt_td->pages[chipnr]);
 		nand_update_bbt(mtd, rewrite_bbt[chipnr]);
+#endif
 	}
 
 	/* Return more or less happy */
-- 
1.7.7


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 6/7] nand: Prevent drivers setting NAND_USE_FLASH_BBT if BBT config not enabled.

[Marc Reilly]

This prevents drivers from setting the options flags to use the flash
bab block table when the BBT is not enabled in config.

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/mtd/nand/atmel_nand.c   |    2 +-
 drivers/mtd/nand/nand_imx.c     |    2 +-
 drivers/mtd/nand/nand_s3c24xx.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index 96624a1..cc62df9 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -407,7 +407,7 @@ static int __init atmel_nand_probe(struct device_d *dev)
 		}
 	}
 
-	if (host->board->on_flash_bbt) {
+	if (IS_ENABLED(CONFIG_NAND_BBT) && host->board->on_flash_bbt) {
 		printk(KERN_INFO "atmel_nand: Use On Flash BBT\n");
 		nand_chip->options |= NAND_USE_FLASH_BBT;
 	}
diff --git a/drivers/mtd/nand/nand_imx.c b/drivers/mtd/nand/nand_imx.c
index e75ffbc..67b056b 100644
--- a/drivers/mtd/nand/nand_imx.c
+++ b/drivers/mtd/nand/nand_imx.c
@@ -1142,7 +1142,7 @@ static int __init imxnd_probe(struct device_d *dev)
 		imx_nand_set_layout(0, 16);
 	}
 
-	if (pdata->flash_bbt) {
+	if (IS_ENABLED(CONFIG_NAND_BBT) && pdata->flash_bbt) {
 		this->bbt_td = &bbt_main_descr;
 		this->bbt_md = &bbt_mirror_descr;
 		/* update flash based bbt */
diff --git a/drivers/mtd/nand/nand_s3c24xx.c b/drivers/mtd/nand/nand_s3c24xx.c
index c629701..63a3aac 100644
--- a/drivers/mtd/nand/nand_s3c24xx.c
+++ b/drivers/mtd/nand/nand_s3c24xx.c
@@ -469,7 +469,7 @@ static int s3c24x0_nand_probe(struct device_d *dev)
 		chip->ecc.layout = &nand_hw_eccoob;
 	}
 
-	if (pdata->flash_bbt) {
+	if (IS_ENABLED(CONFIG_NAND_BBT) && pdata->flash_bbt) {
 		/* use a flash based bbt */
 		chip->options |= NAND_USE_FLASH_BBT;
 	}
-- 
1.7.7


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 7/7] drivers/eeprom: at24: add I2C eeprom driver for 24c01/02

[Marc Reilly]

This series adds a driver for at24 eeproms. Much of the guts of the code
was taken from the at24 driver in the linux kernel.

The device is polled for write completion. All the datasheets I looked
at had a max of 10ms for eeprom write time.
The driver automatically wraps the writes to page boundaries, so we don't
write more than is remaining in the page.

The driver can not yet handle addressing offsets > 256 in devices with
larger capacities.

The platform data fields are all optional, if they are zero they are
assigned default values. As the device capabilities can not be probed,
the default assumption is that the device is 256 bytes.

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/eeprom/Kconfig  |    7 ++
 drivers/eeprom/Makefile |    1 +
 drivers/eeprom/at24.c   |  233 +++++++++++++++++++++++++++++++++++++++++++++++
 include/i2c/at24.h      |   13 +++
 4 files changed, 254 insertions(+), 0 deletions(-)
 create mode 100644 drivers/eeprom/at24.c
 create mode 100644 include/i2c/at24.h

diff --git a/drivers/eeprom/Kconfig b/drivers/eeprom/Kconfig
index a0b5489..a2bcaaa 100644
--- a/drivers/eeprom/Kconfig
+++ b/drivers/eeprom/Kconfig
@@ -8,4 +8,11 @@ config EEPROM_AT25
 	  after you configure the board init code to know about each eeprom
 	  on your target board.
 
+config EEPROM_AT24
+	bool "at24 based eeprom"
+	depends on I2C
+	help
+	  Provides read/write for the at24 family of I2C EEPROMS.
+	  Currently only the 2K bit versions are supported.
+
 endmenu
diff --git a/drivers/eeprom/Makefile b/drivers/eeprom/Makefile
index e323bd0..e287eb0 100644
--- a/drivers/eeprom/Makefile
+++ b/drivers/eeprom/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_EEPROM_AT25)	+= at25.o
+obj-$(CONFIG_EEPROM_AT24)	+= at24.o
diff --git a/drivers/eeprom/at24.c b/drivers/eeprom/at24.c
new file mode 100644
index 0000000..fa16d88
--- /dev/null
+++ b/drivers/eeprom/at24.c
@@ -0,0 +1,233 @@
+/*
+ * Copyright (C) 2007 Sascha Hauer, Pengutronix
+ *               2009 Marc Kleine-Budde <mkl@pengutronix.de>
+ *               2010 Marc Reilly, Creative Product Design
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ *
+ */
+
+#include <common.h>
+#include <init.h>
+#include <clock.h>
+#include <driver.h>
+#include <xfuncs.h>
+#include <errno.h>
+
+#include <i2c/i2c.h>
+#include <i2c/at24.h>
+
+#define DRIVERNAME		"at24"
+#define DEVICENAME		"eeprom"
+
+struct at24 {
+	struct cdev		cdev;
+	struct i2c_client	*client;
+	/* size in bytes */
+	unsigned int		size;
+	unsigned int		page_size;
+};
+
+#define to_at24(a)		container_of(a, struct at24, cdev)
+
+static ssize_t at24_read(struct cdev *cdev, void *_buf, size_t count,
+		ulong offset, ulong flags)
+{
+	struct at24 *priv = to_at24(cdev);
+	u8 *buf = _buf;
+	size_t i = count;
+	ssize_t numwritten = 0;
+	int retries = 5;
+	int ret;
+
+	while (i && retries) {
+		ret = i2c_read_reg(priv->client, offset, buf, i);
+		if (ret < 0)
+			return (ssize_t)ret;
+		else if (ret == 0)
+			--retries;
+
+		numwritten += ret;
+		i -= ret;
+		offset += ret;
+		buf += ret;
+	}
+
+	return numwritten;
+}
+
+static int at24_poll_device(struct i2c_client *client)
+{
+	uint64_t start;
+	u8 dummy;
+	int ret;
+
+	/*
+	 * eeprom can take 5-10ms to write, during which time it
+	 * will not respond. Poll it by attempting reads.
+	 */
+	start = get_time_ns();
+	while (1) {
+		ret = i2c_master_recv(client, &dummy, 1);
+		if (ret > 0)
+			return ret;
+
+		if (is_timeout(start, 20 * MSECOND))
+			return -EIO;
+	}
+
+	return 0;
+}
+
+static ssize_t at24_write(struct cdev *cdev, const void *_buf, size_t count,
+		ulong offset, ulong flags)
+{
+	struct at24 *priv = to_at24(cdev);
+	const u8 *buf = _buf;
+	const int pagesize = priv->page_size;
+	ssize_t numwritten = 0;
+
+	while (count) {
+		int ret, numtowrite;
+		int page_remain = pagesize - (offset % pagesize);
+
+		numtowrite = count;
+		if (numtowrite > pagesize)
+			numtowrite = pagesize;
+		/* don't write past page */
+		if (numtowrite > page_remain)
+			numtowrite = page_remain;
+
+		ret = i2c_write_reg(priv->client, offset, buf, numtowrite);
+		if (ret < 0)
+			return (ssize_t)ret;
+
+		numwritten += ret;
+		buf += ret;
+		count -= ret;
+		offset += ret;
+
+		ret = at24_poll_device(priv->client);
+		if (ret < 0)
+			return (ssize_t)ret;
+	}
+
+	return numwritten;
+}
+
+/* max page size of any of the at24 family devices is 16 bytes */
+#define AT24_MAX_PAGE_SIZE	16
+
+static ssize_t at24_erase(struct cdev *cdev, size_t count, unsigned long offset)
+{
+	struct at24 *priv = to_at24(cdev);
+	char erase[AT24_MAX_PAGE_SIZE];
+	const int pagesize = priv->page_size;
+	ssize_t numwritten = 0;
+
+	memset(erase, 0xff, sizeof(erase));
+
+	while (count) {
+		int ret, numtowrite;
+		int page_remain = pagesize - (offset % pagesize);
+
+		numtowrite = count;
+		if (numtowrite > pagesize)
+			numtowrite = pagesize;
+		/* don't write past page */
+		if (numtowrite > page_remain)
+			numtowrite = page_remain;
+
+		ret = i2c_write_reg(priv->client, offset, erase, numtowrite);
+		if (ret < 0)
+			return (ssize_t)ret;
+
+		numwritten += ret;
+		count -= ret;
+		offset += ret;
+
+		ret = at24_poll_device(priv->client);
+		if (ret < 0)
+			return (ssize_t)ret;
+	}
+
+	return 0;
+}
+
+static struct file_operations at24_fops = {
+	.lseek	= dev_lseek_default,
+	.read	= at24_read,
+	.write	= at24_write,
+	.erase	= at24_erase,
+};
+
+static int at24_probe(struct device_d *dev)
+{
+	const struct at24_platform_data *pdata;
+	struct at24 *at24;
+	at24 = xzalloc(sizeof(*at24));
+
+	dev->priv = at24;
+	at24->client = to_i2c_client(dev);
+	at24->cdev.dev = dev;
+	at24->cdev.ops = &at24_fops;
+
+	pdata = dev->platform_data;
+	if (pdata) {
+		at24->cdev.size = pdata->size_bytes;
+		at24->cdev.name = strdup(pdata->name);
+		at24->page_size = pdata->page_size;
+	}
+
+	if (at24->cdev.size == 0)
+		at24->cdev.size = 256;
+	if (!at24->cdev.name || at24->cdev.name[0] == '\0') {
+		char buf[20];
+		sprintf(buf, DEVICENAME"%d", dev->id);
+		at24->cdev.name = strdup(buf);
+	}
+	if (at24->page_size == 0) {
+		switch (at24->cdev.size) {
+		case 512:
+		case 1024:
+		case 2048:
+			at24->page_size = 16;
+			break;
+		case 128:
+		case 256:
+		default:
+			at24->page_size = 8;
+			break;
+		}
+	}
+
+	devfs_create(&at24->cdev);
+
+	return 0;
+}
+
+static struct driver_d at24_driver = {
+	.name  = DRIVERNAME,
+	.probe = at24_probe,
+};
+
+static int at24_init(void)
+{
+	register_driver(&at24_driver);
+	return 0;
+}
+
+device_initcall(at24_init);
diff --git a/include/i2c/at24.h b/include/i2c/at24.h
new file mode 100644
index 0000000..00e4624
--- /dev/null
+++ b/include/i2c/at24.h
@@ -0,0 +1,13 @@
+#ifndef _EEPROM_AT24_H
+#define _EEPROM_AT24_H
+
+struct at24_platform_data {
+	/* preferred device name */
+	const char name[10];
+	/* page write size in bytes */
+	u8 page_size;
+	/* device size in bytes */
+	u16 size_bytes;
+};
+
+#endif /* _EEPROM_AT24_H */
-- 
1.7.7


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 5/7] nand: fix build error when BBT not enabled.

[Jean-Christophe PLAGNIOL-VILLARD]

On 17:41 Sun 29 Jul     , Marc Reilly wrote:
> This fixes following error when bad blocks config option is not enabled:
> 
> drivers/built-in.o: In function `nand_erase_nand':
> drivers/mtd/nand/nand_write.c:721: undefined reference to `nand_update_bbt'
> drivers/built-in.o: In function `nand_default_block_markbad':
> drivers/mtd/nand/nand_write.c:76: undefined reference to `nand_update_bbt'
> make: *** [barebox] Error 1
> 
> Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> ---
>  drivers/mtd/nand/nand_write.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_write.c b/drivers/mtd/nand/nand_write.c
> index 13b6c89..0fd49d6 100644
> --- a/drivers/mtd/nand/nand_write.c
> +++ b/drivers/mtd/nand/nand_write.c
> @@ -72,9 +72,15 @@ int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs)
>  		chip->bbt[block >> 2] |= 0x01 << ((block & 0x03) << 1);
>  
>  	/* Do we have a flash based bad block table ? */
> -	if (chip->options & NAND_USE_FLASH_BBT)
> +	if (chip->options & NAND_USE_FLASH_BBT) {
> +#if defined(CONFIG_NAND_BBT)
use if (IS_ENABLED())

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/7] i2c: device id default to -1 for auto assignment

[Jean-Christophe PLAGNIOL-VILLARD]

On 17:41 Sun 29 Jul     , Marc Reilly wrote:
> Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> ---
>  drivers/i2c/i2c.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/i2c.c b/drivers/i2c/i2c.c
> index 15f5507..c0d8fa2 100644
> --- a/drivers/i2c/i2c.c
> +++ b/drivers/i2c/i2c.c
> @@ -251,6 +251,7 @@ struct i2c_client *i2c_new_device(struct i2c_adapter *adapter,
>  	strcpy(client->dev.name, chip->type);
>  	client->dev.type_data = client;
>  	client->dev.platform_data = chip->platform_data;
> +	client->dev.id = -1;
use the macro DEVICE_ID_DYNAMIC

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 6/7] nand: Prevent drivers setting NAND_USE_FLASH_BBT if BBT config not enabled.

[Jean-Christophe PLAGNIOL-VILLARD]

On 17:41 Sun 29 Jul     , Marc Reilly wrote:
> This prevents drivers from setting the options flags to use the flash
> bab block table when the BBT is not enabled in config.
> 
> Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> ---
>  drivers/mtd/nand/atmel_nand.c   |    2 +-
>  drivers/mtd/nand/nand_imx.c     |    2 +-
>  drivers/mtd/nand/nand_s3c24xx.c |    2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index 96624a1..cc62df9 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -407,7 +407,7 @@ static int __init atmel_nand_probe(struct device_d *dev)
>  		}
>  	}
>  
> -	if (host->board->on_flash_bbt) {
> +	if (IS_ENABLED(CONFIG_NAND_BBT) && host->board->on_flash_bbt) {
>  		printk(KERN_INFO "atmel_nand: Use On Flash BBT\n");
>  		nand_chip->options |= NAND_USE_FLASH_BBT;
driver pass this info to the nand framework which is supposed to handle this
by itself

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

RE: [PATCH 5/7] nand: fix build error when BBT not enabled.

[Marc Reilly]

Hi,

> On 17:41 Sun 29 Jul     , Marc Reilly wrote:
> > This fixes following error when bad blocks config option is not enabled:
> >
> > drivers/built-in.o: In function `nand_erase_nand':
> > drivers/mtd/nand/nand_write.c:721: undefined reference to
> `nand_update_bbt'
> > drivers/built-in.o: In function `nand_default_block_markbad':
> > drivers/mtd/nand/nand_write.c:76: undefined reference to
> `nand_update_bbt'
> > make: *** [barebox] Error 1


 >  	/* Do we have a flash based bad block table ? */
> > -	if (chip->options & NAND_USE_FLASH_BBT)
> > +	if (chip->options & NAND_USE_FLASH_BBT) { #if
> > +defined(CONFIG_NAND_BBT)
> use if (IS_ENABLED())

The intention of this is to avoid referencing nand_update_bbt, I think using
IS_ENABLED() would not avoid that.

Cheers,
Marc


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

RE: [PATCH 6/7] nand: Prevent drivers setting NAND_USE_FLASH_BBT if BBT config not enabled.

[Marc Reilly]

> -----Original Message-----
> From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj@jcrosoft.com]
> Sent: Sunday, 29 July 2012 8:01 PM
> To: Marc Reilly
> Cc: barebox@lists.infradead.org
> Subject: Re: [PATCH 6/7] nand: Prevent drivers setting
> NAND_USE_FLASH_BBT if BBT config not enabled.
> 
> On 17:41 Sun 29 Jul     , Marc Reilly wrote:
> > This prevents drivers from setting the options flags to use the flash
> > bab block table when the BBT is not enabled in config.
> >
> > Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> > ---
> >  drivers/mtd/nand/atmel_nand.c   |    2 +-
> >  drivers/mtd/nand/nand_imx.c     |    2 +-
> >  drivers/mtd/nand/nand_s3c24xx.c |    2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/atmel_nand.c
> > b/drivers/mtd/nand/atmel_nand.c index 96624a1..cc62df9 100644
> > --- a/drivers/mtd/nand/atmel_nand.c
> > +++ b/drivers/mtd/nand/atmel_nand.c
> > @@ -407,7 +407,7 @@ static int __init atmel_nand_probe(struct device_d
> *dev)
> >  		}
> >  	}
> >
> > -	if (host->board->on_flash_bbt) {
> > +	if (IS_ENABLED(CONFIG_NAND_BBT) && host->board-
> >on_flash_bbt) {
> >  		printk(KERN_INFO "atmel_nand: Use On Flash BBT\n");
> >  		nand_chip->options |= NAND_USE_FLASH_BBT;
> driver pass this info to the nand framework which is supposed to handle
this
> by itself

Ok, cool. Assuming the nand framework handles this then please drop this
patch.

Cheers,
Narc



_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/7] i2c: add platform_data for i2c_board_info

[Uwe Kleine-König]

On Sun, Jul 29, 2012 at 05:41:50PM +1000, Marc Reilly wrote:
> Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
We have nearly exactly this patch in one of our customer patch stacks to
get at24 up and running. (I only set client->dev.platform_data two lines
later than you :-)

So:
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 7/7] drivers/eeprom: at24: add I2C eeprom driver for 24c01/02

[Uwe Kleine-König]

Hello Marc,

On Sun, Jul 29, 2012 at 05:41:54PM +1000, Marc Reilly wrote:
> This series adds a driver for at24 eeproms. Much of the guts of the code
> was taken from the at24 driver in the linux kernel.
> 
> The device is polled for write completion. All the datasheets I looked
> at had a max of 10ms for eeprom write time.
> The driver automatically wraps the writes to page boundaries, so we don't
> write more than is remaining in the page.
> 
> The driver can not yet handle addressing offsets > 256 in devices with
> larger capacities.
> 
> The platform data fields are all optional, if they are zero they are
> assigned default values. As the device capabilities can not be probed,
> the default assumption is that the device is 256 bytes.
> 
> Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> ---
>  drivers/eeprom/Kconfig  |    7 ++
>  drivers/eeprom/Makefile |    1 +
>  drivers/eeprom/at24.c   |  233 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/i2c/at24.h      |   13 +++
>  4 files changed, 254 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/eeprom/at24.c
>  create mode 100644 include/i2c/at24.h
> 
> diff --git a/drivers/eeprom/Kconfig b/drivers/eeprom/Kconfig
> index a0b5489..a2bcaaa 100644
> --- a/drivers/eeprom/Kconfig
> +++ b/drivers/eeprom/Kconfig
> @@ -8,4 +8,11 @@ config EEPROM_AT25
>  	  after you configure the board init code to know about each eeprom
>  	  on your target board.
>  
> +config EEPROM_AT24
> +	bool "at24 based eeprom"
> +	depends on I2C
> +	help
> +	  Provides read/write for the at24 family of I2C EEPROMS.
> +	  Currently only the 2K bit versions are supported.
> +
>  endmenu
> diff --git a/drivers/eeprom/Makefile b/drivers/eeprom/Makefile
> index e323bd0..e287eb0 100644
> --- a/drivers/eeprom/Makefile
> +++ b/drivers/eeprom/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_EEPROM_AT25)	+= at25.o
> +obj-$(CONFIG_EEPROM_AT24)	+= at24.o
nitpick: sort at24 before at25?

> diff --git a/drivers/eeprom/at24.c b/drivers/eeprom/at24.c
> new file mode 100644
> index 0000000..fa16d88
> --- /dev/null
> +++ b/drivers/eeprom/at24.c
> @@ -0,0 +1,233 @@
> +/*
> + * Copyright (C) 2007 Sascha Hauer, Pengutronix
> + *               2009 Marc Kleine-Budde <mkl@pengutronix.de>
> + *               2010 Marc Reilly, Creative Product Design
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + *
> + */
> +
> +#include <common.h>
> +#include <init.h>
> +#include <clock.h>
> +#include <driver.h>
> +#include <xfuncs.h>
> +#include <errno.h>
> +
> +#include <i2c/i2c.h>
> +#include <i2c/at24.h>
> +
> +#define DRIVERNAME		"at24"
> +#define DEVICENAME		"eeprom"
why not DEVICENAME == DRIVERNAME?

> +
> +struct at24 {
> +	struct cdev		cdev;
> +	struct i2c_client	*client;
> +	/* size in bytes */
> +	unsigned int		size;
> +	unsigned int		page_size;
> +};
> +
> +#define to_at24(a)		container_of(a, struct at24, cdev)
> +
> +static ssize_t at24_read(struct cdev *cdev, void *_buf, size_t count,
> +		ulong offset, ulong flags)
> +{
> +	struct at24 *priv = to_at24(cdev);
> +	u8 *buf = _buf;
> +	size_t i = count;
> +	ssize_t numwritten = 0;
s/written/read/

> +	int retries = 5;
> +	int ret;
> +
> +	while (i && retries) {
> +		ret = i2c_read_reg(priv->client, offset, buf, i);
> +		if (ret < 0)
> +			return (ssize_t)ret;
This cast is also done implicitly.

> +		else if (ret == 0)
> +			--retries;
> +
> +		numwritten += ret;
> +		i -= ret;
> +		offset += ret;
> +		buf += ret;
> +	}
IMHO this loop should be in a more generic place once instead of
repeating it for each driver. Also I wonder if you saw the eeprom
yielding some but not all requested bytes on a read request.

> +
> +	return numwritten;
> +}
> +
> +static int at24_poll_device(struct i2c_client *client)
> +{
> +	uint64_t start;
> +	u8 dummy;
> +	int ret;
> +
> +	/*
> +	 * eeprom can take 5-10ms to write, during which time it
> +	 * will not respond. Poll it by attempting reads.
> +	 */
> +	start = get_time_ns();
> +	while (1) {
> +		ret = i2c_master_recv(client, &dummy, 1);
I implemented a write of length 0 here. IMHO this is better.

> +		if (ret > 0)
> +			return ret;
> +
> +		if (is_timeout(start, 20 * MSECOND))
> +			return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t at24_write(struct cdev *cdev, const void *_buf, size_t count,
> +		ulong offset, ulong flags)
> +{
> +	struct at24 *priv = to_at24(cdev);
> +	const u8 *buf = _buf;
> +	const int pagesize = priv->page_size;
> +	ssize_t numwritten = 0;
> +
> +	while (count) {
> +		int ret, numtowrite;
> +		int page_remain = pagesize - (offset % pagesize);
> +
> +		numtowrite = count;
> +		if (numtowrite > pagesize)
> +			numtowrite = pagesize;
I think you can skip this if in the presence of the the if below.

> +		/* don't write past page */
> +		if (numtowrite > page_remain)
> +			numtowrite = page_remain;
> +
> +		ret = i2c_write_reg(priv->client, offset, buf, numtowrite);
> +		if (ret < 0)
> +			return (ssize_t)ret;
> +
> +		numwritten += ret;
> +		buf += ret;
> +		count -= ret;
> +		offset += ret;
> +
> +		ret = at24_poll_device(priv->client);
> +		if (ret < 0)
> +			return (ssize_t)ret;
Don't you need to poll before writing instead of after a write?

> +	}
> +
> +	return numwritten;
> +}
> +
> +/* max page size of any of the at24 family devices is 16 bytes */
> +#define AT24_MAX_PAGE_SIZE	16
That is wrong. My eeprom has a page size of 64 bytes.

> +
> +static ssize_t at24_erase(struct cdev *cdev, size_t count, unsigned long offset)
> +{
> +	struct at24 *priv = to_at24(cdev);
> +	char erase[AT24_MAX_PAGE_SIZE];
> +	const int pagesize = priv->page_size;
> +	ssize_t numwritten = 0;
> +
> +	memset(erase, 0xff, sizeof(erase));
> +
> +	while (count) {
> +		int ret, numtowrite;
> +		int page_remain = pagesize - (offset % pagesize);
> +
> +		numtowrite = count;
> +		if (numtowrite > pagesize)
> +			numtowrite = pagesize;
> +		/* don't write past page */
> +		if (numtowrite > page_remain)
> +			numtowrite = page_remain;
> +
> +		ret = i2c_write_reg(priv->client, offset, erase, numtowrite);
> +		if (ret < 0)
> +			return (ssize_t)ret;
> +
> +		numwritten += ret;
> +		count -= ret;
> +		offset += ret;
> +
> +		ret = at24_poll_device(priv->client);
> +		if (ret < 0)
> +			return (ssize_t)ret;
> +	}
> +
> +	return 0;
> +}
I think conceptually you don't need the erase callback for this eeprom.

> +
> +static struct file_operations at24_fops = {
> +	.lseek	= dev_lseek_default,
> +	.read	= at24_read,
> +	.write	= at24_write,
> +	.erase	= at24_erase,
> +};
> +
> +static int at24_probe(struct device_d *dev)
> +{
> +	const struct at24_platform_data *pdata;
> +	struct at24 *at24;
> +	at24 = xzalloc(sizeof(*at24));
> +
> +	dev->priv = at24;
> +	at24->client = to_i2c_client(dev);
> +	at24->cdev.dev = dev;
> +	at24->cdev.ops = &at24_fops;
> +
> +	pdata = dev->platform_data;
> +	if (pdata) {
> +		at24->cdev.size = pdata->size_bytes;
> +		at24->cdev.name = strdup(pdata->name);
> +		at24->page_size = pdata->page_size;
> +	}
> +
> +	if (at24->cdev.size == 0)
> +		at24->cdev.size = 256;
> +	if (!at24->cdev.name || at24->cdev.name[0] == '\0') {
> +		char buf[20];
> +		sprintf(buf, DEVICENAME"%d", dev->id);
> +		at24->cdev.name = strdup(buf);
> +	}
> +	if (at24->page_size == 0) {
> +		switch (at24->cdev.size) {
> +		case 512:
> +		case 1024:
> +		case 2048:
> +			at24->page_size = 16;
> +			break;
> +		case 128:
> +		case 256:
> +		default:
> +			at24->page_size = 8;
> +			break;
> +		}
> +	}
> +
> +	devfs_create(&at24->cdev);
> +
> +	return 0;
> +}
> +
> +static struct driver_d at24_driver = {
> +	.name  = DRIVERNAME,
> +	.probe = at24_probe,
> +};
> +
> +static int at24_init(void)
> +{
> +	register_driver(&at24_driver);
> +	return 0;
return register_driver(&at24_driver) ?

> +}
> +
> +device_initcall(at24_init);
> diff --git a/include/i2c/at24.h b/include/i2c/at24.h
> new file mode 100644
> index 0000000..00e4624
> --- /dev/null
> +++ b/include/i2c/at24.h
> @@ -0,0 +1,13 @@
> +#ifndef _EEPROM_AT24_H
> +#define _EEPROM_AT24_H
> +
> +struct at24_platform_data {
> +	/* preferred device name */
> +	const char name[10];
> +	/* page write size in bytes */
> +	u8 page_size;
> +	/* device size in bytes */
> +	u16 size_bytes;
> +};
> +
> +#endif /* _EEPROM_AT24_H */
> -- 
> 1.7.7
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: at24 eeprom driver (V2) and misc patches

[Sascha Hauer]

Hi Marc,

On Sun, Jul 29, 2012 at 05:41:47PM +1000, Marc Reilly wrote:
> Hi,
> 
> These patches add an (improved) at24 eeprom driver and some other fixes. The
> eeprom driver has improved write handling that polls the device for write 
> completion (rather than fixed 10ms delay).
> 
> The eeprom driver requires patch 3 and 4. 
> 
> The nand patches fix a build error when BBT is not enabled.

For now I applied patch 1 & 2, but I squashed them into a single commit,
because otherwise we would introduce a build warning (unused function)
between those two patches.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/7] i2c: add platform_data for i2c_board_info

[Sascha Hauer]

On Sun, Jul 29, 2012 at 05:41:50PM +1000, Marc Reilly wrote:
> Signed-off-by: Marc Reilly <marc@cpdesign.com.au>

Applied this one aswell.

Sascha

> ---
>  drivers/i2c/i2c.c |    1 +
>  include/i2c/i2c.h |    1 +
>  2 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/i2c.c b/drivers/i2c/i2c.c
> index 5df0d30..15f5507 100644
> --- a/drivers/i2c/i2c.c
> +++ b/drivers/i2c/i2c.c
> @@ -250,6 +250,7 @@ struct i2c_client *i2c_new_device(struct i2c_adapter *adapter,
>  	client = xzalloc(sizeof *client);
>  	strcpy(client->dev.name, chip->type);
>  	client->dev.type_data = client;
> +	client->dev.platform_data = chip->platform_data;
>  	client->adapter = adapter;
>  	client->addr = chip->addr;
>  
> diff --git a/include/i2c/i2c.h b/include/i2c/i2c.h
> index ccbf518..c3e1763 100644
> --- a/include/i2c/i2c.h
> +++ b/include/i2c/i2c.h
> @@ -99,6 +99,7 @@ struct i2c_client {
>  struct i2c_board_info {
>  	char		type[I2C_NAME_SIZE];	/**< name of device */
>  	unsigned short	addr;			/**< stored in i2c_client.addr */
> +	void		*platform_data;		/**< platform data for device */
>  };
>  
>  /**
> -- 
> 1.7.7
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 7/7] drivers/eeprom: at24: add I2C eeprom driver for 24c01/02

[Marc Reilly]

Hi Uwe,

Thanks for comments. You are very thorough, and I mean that in a nice way.
I gather you also have an at24 driver, did you support addressing offsets > 
256?

On Monday, July 30, 2012 10:25:05 AM Uwe Kleine-König wrote:
> Hello Marc,
> 
> On Sun, Jul 29, 2012 at 05:41:54PM +1000, Marc Reilly wrote:
> > This series adds a driver for at24 eeproms. Much of the guts of the code
> > was taken from the at24 driver in the linux kernel.
> > 
> > The device is polled for write completion. All the datasheets I looked
> > at had a max of 10ms for eeprom write time.
> > The driver automatically wraps the writes to page boundaries, so we don't
> > write more than is remaining in the page.
> > 
> > The driver can not yet handle addressing offsets > 256 in devices with
> > larger capacities.
> > 
> > The platform data fields are all optional, if they are zero they are
> > assigned default values. As the device capabilities can not be probed,
> > the default assumption is that the device is 256 bytes.
> > 
> > Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> > ---
> > 
> >  drivers/eeprom/Kconfig  |    7 ++
> >  drivers/eeprom/Makefile |    1 +
> >  drivers/eeprom/at24.c   |  233
> >  +++++++++++++++++++++++++++++++++++++++++++++++ include/i2c/at24.h     
> >  |   13 +++
> >  4 files changed, 254 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/eeprom/at24.c
> >  create mode 100644 include/i2c/at24.h
> > 
> > diff --git a/drivers/eeprom/Kconfig b/drivers/eeprom/Kconfig
> > index a0b5489..a2bcaaa 100644
> > --- a/drivers/eeprom/Kconfig
> > +++ b/drivers/eeprom/Kconfig
> > @@ -8,4 +8,11 @@ config EEPROM_AT25
> > 
> >  	  after you configure the board init code to know about each eeprom
> >  	  on your target board.
> > 
> > +config EEPROM_AT24
> > +	bool "at24 based eeprom"
> > +	depends on I2C
> > +	help
> > +	  Provides read/write for the at24 family of I2C EEPROMS.
> > +	  Currently only the 2K bit versions are supported.
> > +
> > 
> >  endmenu
> > 
> > diff --git a/drivers/eeprom/Makefile b/drivers/eeprom/Makefile
> > index e323bd0..e287eb0 100644
> > --- a/drivers/eeprom/Makefile
> > +++ b/drivers/eeprom/Makefile
> > @@ -1 +1,2 @@
> > 
> >  obj-$(CONFIG_EEPROM_AT25)	+= at25.o
> > 
> > +obj-$(CONFIG_EEPROM_AT24)	+= at24.o
> 
> nitpick: sort at24 before at25?

Cool.

> 
> > diff --git a/drivers/eeprom/at24.c b/drivers/eeprom/at24.c
> > new file mode 100644
> > index 0000000..fa16d88
> > --- /dev/null
> > +++ b/drivers/eeprom/at24.c
> > @@ -0,0 +1,233 @@
> > +/*
> > + * Copyright (C) 2007 Sascha Hauer, Pengutronix
> > + *               2009 Marc Kleine-Budde <mkl@pengutronix.de>
> > + *               2010 Marc Reilly, Creative Product Design
> > + *
> > + * 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.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> > + *
> > + */
> > +
> > +#include <common.h>
> > +#include <init.h>
> > +#include <clock.h>
> > +#include <driver.h>
> > +#include <xfuncs.h>
> > +#include <errno.h>
> > +
> > +#include <i2c/i2c.h>
> > +#include <i2c/at24.h>
> > +
> > +#define DRIVERNAME		"at24"
> > +#define DEVICENAME		"eeprom"
> 
> why not DEVICENAME == DRIVERNAME?

Originally I'd started with DRIVENAME as eeprom, but changed it later as that 
seemed too generic. I wanted to keep the device name as eeprom so that the 
device would be "/dev/eepromX", both for compatibilty with existing board 
setup and as a conceptual abstraction to regard the device as a more generic 
"eeprom" device, rather than a more specific "at24".
(Hope that makes sense).

> 
> > +
> > +struct at24 {
> > +	struct cdev		cdev;
> > +	struct i2c_client	*client;
> > +	/* size in bytes */
> > +	unsigned int		size;
> > +	unsigned int		page_size;
> > +};
> > +
> > +#define to_at24(a)		container_of(a, struct at24, cdev)
> > +
> > +static ssize_t at24_read(struct cdev *cdev, void *_buf, size_t count,
> > +		ulong offset, ulong flags)
> > +{
> > +	struct at24 *priv = to_at24(cdev);
> > +	u8 *buf = _buf;
> > +	size_t i = count;
> > +	ssize_t numwritten = 0;
> 
> s/written/read/
> 
> > +	int retries = 5;
> > +	int ret;
> > +
> > +	while (i && retries) {
> > +		ret = i2c_read_reg(priv->client, offset, buf, i);
> > +		if (ret < 0)
> > +			return (ssize_t)ret;
> 
> This cast is also done implicitly.
> 
> > +		else if (ret == 0)
> > +			--retries;
> > +
> > +		numwritten += ret;
> > +		i -= ret;
> > +		offset += ret;
> > +		buf += ret;
> > +	}
> 
> IMHO this loop should be in a more generic place once instead of
> repeating it for each driver. Also I wonder if you saw the eeprom
> yielding some but not all requested bytes on a read request.

Not that I can remember, but this code is old, and I think was copied from a 
kernel driver and I just left as is.
I considered doing a generic loop, but in my head anything I thought of wasn't 
much better than having similar code 2 or 3 times.

> 
> > +
> > +	return numwritten;
> > +}
> > +
> > +static int at24_poll_device(struct i2c_client *client)
> > +{
> > +	uint64_t start;
> > +	u8 dummy;
> > +	int ret;
> > +
> > +	/*
> > +	 * eeprom can take 5-10ms to write, during which time it
> > +	 * will not respond. Poll it by attempting reads.
> > +	 */
> > +	start = get_time_ns();
> > +	while (1) {
> > +		ret = i2c_master_recv(client, &dummy, 1);
> 
> I implemented a write of length 0 here. IMHO this is better.

I'm not clear whether you are saying that your way is better :)
This way reads just one byte after device responds. I'm thinking that your way 
would write a byte for the address...

> 
> > +		if (ret > 0)
> > +			return ret;
> > +
> > +		if (is_timeout(start, 20 * MSECOND))
> > +			return -EIO;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static ssize_t at24_write(struct cdev *cdev, const void *_buf, size_t
> > count, +		ulong offset, ulong flags)
> > +{
> > +	struct at24 *priv = to_at24(cdev);
> > +	const u8 *buf = _buf;
> > +	const int pagesize = priv->page_size;
> > +	ssize_t numwritten = 0;
> > +
> > +	while (count) {
> > +		int ret, numtowrite;
> > +		int page_remain = pagesize - (offset % pagesize);
> > +
> > +		numtowrite = count;
> > +		if (numtowrite > pagesize)
> > +			numtowrite = pagesize;
> 
> I think you can skip this if in the presence of the the if below.
> 
> > +		/* don't write past page */
> > +		if (numtowrite > page_remain)
> > +			numtowrite = page_remain;
> > +
> > +		ret = i2c_write_reg(priv->client, offset, buf, numtowrite);
> > +		if (ret < 0)
> > +			return (ssize_t)ret;
> > +
> > +		numwritten += ret;
> > +		buf += ret;
> > +		count -= ret;
> > +		offset += ret;
> > +
> > +		ret = at24_poll_device(priv->client);
> > +		if (ret < 0)
> > +			return (ssize_t)ret;
> 
> Don't you need to poll before writing instead of after a write?

That would probably be better.

> 
> > +	}
> > +
> > +	return numwritten;
> > +}
> > +
> > +/* max page size of any of the at24 family devices is 16 bytes */
> > +#define AT24_MAX_PAGE_SIZE	16
> 
> That is wrong. My eeprom has a page size of 64 bytes.

That comment should have had a "that I looked at" after devices. I'll change 
it to 64.

> 
> > +
> > +static ssize_t at24_erase(struct cdev *cdev, size_t count, unsigned long
> > offset) +{
> > +	struct at24 *priv = to_at24(cdev);
> > +	char erase[AT24_MAX_PAGE_SIZE];
> > +	const int pagesize = priv->page_size;
> > +	ssize_t numwritten = 0;
> > +
> > +	memset(erase, 0xff, sizeof(erase));
> > +
> > +	while (count) {
> > +		int ret, numtowrite;
> > +		int page_remain = pagesize - (offset % pagesize);
> > +
> > +		numtowrite = count;
> > +		if (numtowrite > pagesize)
> > +			numtowrite = pagesize;
> > +		/* don't write past page */
> > +		if (numtowrite > page_remain)
> > +			numtowrite = page_remain;
> > +
> > +		ret = i2c_write_reg(priv->client, offset, erase, numtowrite);
> > +		if (ret < 0)
> > +			return (ssize_t)ret;
> > +
> > +		numwritten += ret;
> > +		count -= ret;
> > +		offset += ret;
> > +
> > +		ret = at24_poll_device(priv->client);
> > +		if (ret < 0)
> > +			return (ssize_t)ret;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> I think conceptually you don't need the erase callback for this eeprom.

While it is possible to write 0xff through the device, this was more 
convenient. I'd prefer to keep it, unless theres a reason otherwise. 

> 
> > +
> > +static struct file_operations at24_fops = {
> > +	.lseek	= dev_lseek_default,
> > +	.read	= at24_read,
> > +	.write	= at24_write,
> > +	.erase	= at24_erase,
> > +};
> > +
> > +static int at24_probe(struct device_d *dev)
> > +{
> > +	const struct at24_platform_data *pdata;
> > +	struct at24 *at24;
> > +	at24 = xzalloc(sizeof(*at24));
> > +
> > +	dev->priv = at24;
> > +	at24->client = to_i2c_client(dev);
> > +	at24->cdev.dev = dev;
> > +	at24->cdev.ops = &at24_fops;
> > +
> > +	pdata = dev->platform_data;
> > +	if (pdata) {
> > +		at24->cdev.size = pdata->size_bytes;
> > +		at24->cdev.name = strdup(pdata->name);
> > +		at24->page_size = pdata->page_size;
> > +	}
> > +
> > +	if (at24->cdev.size == 0)
> > +		at24->cdev.size = 256;
> > +	if (!at24->cdev.name || at24->cdev.name[0] == '\0') {
> > +		char buf[20];
> > +		sprintf(buf, DEVICENAME"%d", dev->id);

I have an issue myself with this, as an spi eeprom could also be present or 
even two at24s on different i2c busses.

I remember, a while ago, working on a function that would make up a name for a 
device. I'll try dig that up.


> > +		at24->cdev.name = strdup(buf);
> > +	}
> > +	if (at24->page_size == 0) {
> > +		switch (at24->cdev.size) {
> > +		case 512:
> > +		case 1024:
> > +		case 2048:
> > +			at24->page_size = 16;
> > +			break;
> > +		case 128:
> > +		case 256:
> > +		default:
> > +			at24->page_size = 8;
> > +			break;
> > +		}
> > +	}
> > +
> > +	devfs_create(&at24->cdev);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct driver_d at24_driver = {
> > +	.name  = DRIVERNAME,
> > +	.probe = at24_probe,
> > +};
> > +
> > +static int at24_init(void)
> > +{
> > +	register_driver(&at24_driver);
> > +	return 0;
> 
> return register_driver(&at24_driver) ?

Yup.

Thanks again! I'll rework this soon.

Cheers,
Marc

> 
> > +}
> > +
> > +device_initcall(at24_init);
> > diff --git a/include/i2c/at24.h b/include/i2c/at24.h
> > new file mode 100644
> > index 0000000..00e4624
> > --- /dev/null
> > +++ b/include/i2c/at24.h
> > @@ -0,0 +1,13 @@
> > +#ifndef _EEPROM_AT24_H
> > +#define _EEPROM_AT24_H
> > +
> > +struct at24_platform_data {
> > +	/* preferred device name */
> > +	const char name[10];
> > +	/* page write size in bytes */
> > +	u8 page_size;
> > +	/* device size in bytes */
> > +	u16 size_bytes;
> > +};
> > +
> > +#endif /* _EEPROM_AT24_H */

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 7/7] drivers/eeprom: at24: add I2C eeprom driver for 24c01/02

[Uwe Kleine-König]

Hi Marc,

On Mon, Jul 30, 2012 at 08:36:46PM +1000, Marc Reilly wrote:
> Thanks for comments. You are very thorough, and I mean that in a nice way.
> I gather you also have an at24 driver, did you support addressing offsets > 
> 256?
yes, our driver does. But it hardcodes two address bytes similar to your
driver hardcoding a single byte :-)

> > > +#define DRIVERNAME		"at24"
> > > +#define DEVICENAME		"eeprom"
> > 
> > why not DEVICENAME == DRIVERNAME?
> 
> Originally I'd started with DRIVENAME as eeprom, but changed it later as that 
> seemed too generic. I wanted to keep the device name as eeprom so that the 
> device would be "/dev/eepromX", both for compatibilty with existing board 
> setup and as a conceptual abstraction to regard the device as a more generic 
> "eeprom" device, rather than a more specific "at24".
> (Hope that makes sense).
Ah, I missed that your devices have a number included after eeprom. Then
I'm ok with your approach.
 
> > > +	while (i && retries) {
> > > +		ret = i2c_read_reg(priv->client, offset, buf, i);
> > > +		if (ret < 0)
> > > +			return (ssize_t)ret;
> > 
> > This cast is also done implicitly.
> > 
> > > +		else if (ret == 0)
> > > +			--retries;
> > > +
> > > +		numwritten += ret;
> > > +		i -= ret;
> > > +		offset += ret;
> > > +		buf += ret;
> > > +	}
> > 
> > IMHO this loop should be in a more generic place once instead of
> > repeating it for each driver. Also I wonder if you saw the eeprom
> > yielding some but not all requested bytes on a read request.
> 
> Not that I can remember, but this code is old, and I think was copied from a 
> kernel driver and I just left as is.
> I considered doing a generic loop, but in my head anything I thought of wasn't 
> much better than having similar code 2 or 3 times.
I think it would be worth to have it in generic code. (For our driver I
did it in the command that implements the custom eeprom layout. So I
don't have anything to share.)

> > > +static int at24_poll_device(struct i2c_client *client)
> > > +{
> > > +	uint64_t start;
> > > +	u8 dummy;
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * eeprom can take 5-10ms to write, during which time it
> > > +	 * will not respond. Poll it by attempting reads.
> > > +	 */
> > > +	start = get_time_ns();
> > > +	while (1) {
> > > +		ret = i2c_master_recv(client, &dummy, 1);
> > 
> > I implemented a write of length 0 here. IMHO this is better.
> 
> I'm not clear whether you are saying that your way is better :)
> This way reads just one byte after device responds. I'm thinking that your way
> would write a byte for the address...
/me rechecks ... Hm, our driver uses:

	i2c_write_reg(at24->client, I2C_ADDR_16_BIT, buf, 0);

so it even transfers two bytes for the address, so regarding the
generated overhead, you're right. But "my" datasheet[1] says:

	ACKNOWLEDGE POLLING: Once the internally-timed write cycle has
	started and the EEPROM inputs are disabled, acknowledge polling
	can be initiated. This involves sending a start condition
	followed by the device address word. The read/write bit is
	representative of the operation desired. Only if the internal
	write cycle has completed will the EEPROM respond with a zero,
	allowing the read or write sequence to continue.

So I think you must not do a read operation to check if a write is
possible. That might be a theoretical problem now, but still I prefer
being aligned to the datasheet.

[1] http://www.atmel.com/Images/doc0670.pdf

> > I think conceptually you don't need the erase callback for this eeprom.
> 
> While it is possible to write 0xff through the device, this was more 
> convenient. I'd prefer to keep it, unless theres a reason otherwise. 
I don't care much, but IMHO the erase callback is for devices that need
the erase before writing.
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox