mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [RFC PATCH] spi: add at25 spi eeprom driver
@ 2011-06-16  8:12 Hubert Feurstein
  2011-06-20  6:45 ` Sascha Hauer
  0 siblings, 1 reply; 4+ messages in thread
From: Hubert Feurstein @ 2011-06-16  8:12 UTC (permalink / raw)
  To: barebox

Signed-off-by: Hubert Feurstein <h.feurstein@gmail.com>
---
 drivers/Kconfig              |    1 +
 drivers/Makefile             |    1 +
 drivers/misc/Kconfig         |   17 +++
 drivers/misc/Makefile        |    1 +
 drivers/misc/eeprom/Kconfig  |   11 ++
 drivers/misc/eeprom/Makefile |    1 +
 drivers/misc/eeprom/at25.c   |  323 ++++++++++++++++++++++++++++++++++++++++++
 drivers/spi/spi.c            |   42 ++++++
 include/spi/eeprom.h         |   22 +++
 include/spi/spi.h            |   78 ++++++++++-
 10 files changed, 495 insertions(+), 2 deletions(-)
 create mode 100644 drivers/misc/Kconfig
 create mode 100644 drivers/misc/Makefile
 create mode 100644 drivers/misc/eeprom/Kconfig
 create mode 100644 drivers/misc/eeprom/Makefile
 create mode 100644 drivers/misc/eeprom/at25.c
 create mode 100644 include/spi/eeprom.h

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 86d8fb5..c54e225 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -13,5 +13,6 @@ source "drivers/mci/Kconfig"
 source "drivers/clk/Kconfig"
 source "drivers/mfd/Kconfig"
 source "drivers/led/Kconfig"
+source "drivers/misc/Kconfig"

 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index b1b402f..9c67239 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_VIDEO) += video/
 obj-y	+= clk/
 obj-y	+= mfd/
 obj-$(CONFIG_LED) += led/
+obj-y	+= misc/
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
new file mode 100644
index 0000000..84d0e95
--- /dev/null
+++ b/drivers/misc/Kconfig
@@ -0,0 +1,17 @@
+#
+# Misc strange devices
+#
+
+menuconfig MISC_DEVICES
+	bool "Misc devices"
+	---help---
+	  Say Y here to get to see options for device drivers from various
+	  different categories. This option alone does not add any kernel code.
+
+	  If you say N, all options in this submenu will be skipped and disabled.
+
+if MISC_DEVICES
+
+source "drivers/misc/eeprom/Kconfig"
+
+endif # MISC_DEVICES
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
new file mode 100644
index 0000000..11e1ea4
--- /dev/null
+++ b/drivers/misc/Makefile
@@ -0,0 +1 @@
+obj-y				+= eeprom/
diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
new file mode 100644
index 0000000..a0b5489
--- /dev/null
+++ b/drivers/misc/eeprom/Kconfig
@@ -0,0 +1,11 @@
+menu "EEPROM support"
+
+config EEPROM_AT25
+	tristate "SPI EEPROMs from most vendors"
+	depends on SPI
+	help
+	  Enable this driver to get read/write support to most SPI EEPROMs,
+	  after you configure the board init code to know about each eeprom
+	  on your target board.
+
+endmenu
diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile
new file mode 100644
index 0000000..e323bd0
--- /dev/null
+++ b/drivers/misc/eeprom/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_EEPROM_AT25)	+= at25.o
diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
new file mode 100644
index 0000000..16eb128
--- /dev/null
+++ b/drivers/misc/eeprom/at25.c
@@ -0,0 +1,323 @@
+/*
+ * at25.c -- support most SPI EEPROMs, such as Atmel AT25 models
+ *
+ * Copyright (C) 2011 Hubert Feurstein <h.feurstein@gmail.com>
+ *
+ * based on linux driver by:
+ * Copyright (C) 2006 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.
+ */
+
+#include <common.h>
+#include <init.h>
+#include <clock.h>
+#include <driver.h>
+#include <errno.h>
+#include <xfuncs.h>
+#include <malloc.h>
+#include <spi/spi.h>
+#include <spi/eeprom.h>
+
+/*
+ * NOTE: this is an *EEPROM* driver.  The vagaries of product naming
+ * mean that some AT25 products are EEPROMs, and others are FLASH.
+ * Handle FLASH chips with the drivers/mtd/devices/m25p80.c driver,
+ * not this one!
+ */
+
+struct at25_data {
+	struct cdev		cdev;
+	struct spi_device	*spi;
+	struct spi_eeprom	chip;
+	unsigned		addrlen;
+};
+
+#define to_at25_data(cdev)		((struct at25_data *)(cdev)->priv)
+
+#define	AT25_WREN	0x06		/* latch the write enable */
+#define	AT25_WRDI	0x04		/* reset the write enable */
+#define	AT25_RDSR	0x05		/* read status register */
+#define	AT25_WRSR	0x01		/* write status register */
+#define	AT25_READ	0x03		/* read byte(s) */
+#define	AT25_WRITE	0x02		/* write byte(s)/sector */
+
+#define	AT25_SR_nRDY	0x01		/* nRDY = write-in-progress */
+#define	AT25_SR_WEN	0x02		/* write enable (latched) */
+#define	AT25_SR_BP0	0x04		/* BP for software writeprotect */
+#define	AT25_SR_BP1	0x08
+#define	AT25_SR_WPEN	0x80		/* writeprotect enable */
+
+
+#define EE_MAXADDRLEN	3		/* 24 bit addresses, up to 2 MBytes */
+
+/* Specs often allow 5 msec for a page write, sometimes 20 msec;
+ * it's important to recover from write timeouts.
+ */
+#define	EE_TIMEOUT	(25 * MSECOND)
+#define DRIVERNAME	"at25x"
+
+/*-------------------------------------------------------------------------*/
+
+#define	io_limit	PAGE_SIZE	/* bytes */
+
+static ssize_t at25_ee_read(struct cdev *cdev,
+			    void *buf,
+			    size_t count,
+			    ulong offset,
+			    ulong flags)
+{
+	u8			command[EE_MAXADDRLEN + 1];
+	u8			*cp;
+	ssize_t			status;
+	struct spi_transfer	t[2];
+	struct spi_message	m;
+	struct at25_data	*at25 = to_at25_data(cdev);
+
+	if (unlikely(offset >= at25->chip.size))
+		return 0;
+	if ((offset + count) > at25->chip.size)
+		count = at25->chip.size - offset;
+	if (unlikely(!count))
+		return count;
+
+	cp = command;
+	*cp++ = AT25_READ;
+
+	/* 8/16/24-bit address is written MSB first */
+	switch (at25->addrlen) {
+	default:	/* case 3 */
+		*cp++ = offset >> 16;
+	case 2:
+		*cp++ = offset >> 8;
+	case 1:
+	case 0:	/* can't happen: for better codegen */
+		*cp++ = offset >> 0;
+	}
+
+	spi_message_init(&m);
+	memset(t, 0, sizeof t);
+
+	t[0].tx_buf = command;
+	t[0].len = at25->addrlen + 1;
+	spi_message_add_tail(&t[0], &m);
+
+	t[1].rx_buf = buf;
+	t[1].len = count;
+	spi_message_add_tail(&t[1], &m);
+
+	/* Read it all at once.
+	 *
+	 * REVISIT that's potentially a problem with large chips, if
+	 * other devices on the bus need to be accessed regularly or
+	 * this chip is clocked very slowly
+	 */
+	status = spi_sync(at25->spi, &m);
+	dev_dbg(at25->cdev.dev,
+		"read %d bytes at %lu --> %d\n",
+		count, offset, (int) status);
+
+	return status ? status : count;
+}
+
+static ssize_t at25_ee_write(struct cdev *cdev,
+			     const void *buf,
+			     size_t count,
+			     ulong off,
+			     ulong flags)
+{
+	ssize_t			status = 0;
+	unsigned		written = 0;
+	unsigned		buf_size;
+	u8			*bounce;
+	struct at25_data	*at25 = to_at25_data(cdev);
+
+	if (unlikely(off >= at25->chip.size))
+		return -EFBIG;
+	if ((off + count) > at25->chip.size)
+		count = at25->chip.size - off;
+	if (unlikely(!count))
+		return count;
+
+	/* Temp buffer starts with command and address */
+	buf_size = at25->chip.page_size;
+	if (buf_size > io_limit)
+		buf_size = io_limit;
+	bounce = xmalloc(buf_size + at25->addrlen + 1);
+
+	/* For write, rollover is within the page ... so we write at
+	 * most one page, then manually roll over to the next page.
+	 */
+	bounce[0] = AT25_WRITE;
+
+	do {
+		uint64_t	start_time;
+		unsigned long	retries;
+		unsigned	segment;
+		unsigned	offset = (unsigned) off;
+		u8		*cp = bounce + 1;
+		int		sr;
+
+		*cp = AT25_WREN;
+		status = spi_write(at25->spi, cp, 1);
+		if (status < 0) {
+			dev_dbg(at25->cdev.dev, "WREN --> %d\n",
+					(int) status);
+			break;
+		}
+
+		/* 8/16/24-bit address is written MSB first */
+		switch (at25->addrlen) {
+		default:	/* case 3 */
+			*cp++ = offset >> 16;
+		case 2:
+			*cp++ = offset >> 8;
+		case 1:
+		case 0:	/* can't happen: for better codegen */
+			*cp++ = offset >> 0;
+		}
+
+		/* Write as much of a page as we can */
+		segment = buf_size - (offset % buf_size);
+		if (segment > count)
+			segment = count;
+		memcpy(cp, buf, segment);
+		status = spi_write(at25->spi, bounce,
+				segment + at25->addrlen + 1);
+		dev_dbg(at25->cdev.dev,
+				"write %u bytes at %u --> %d\n",
+				segment, offset, (int) status);
+		if (status < 0)
+			break;
+
+		/* REVISIT this should detect (or prevent) failed writes
+		 * to readonly sections of the EEPROM...
+		 */
+
+		/* Wait for non-busy status */
+		start_time = get_time_ns();
+
+		retries = 0;
+		do {
+
+			sr = spi_w8r8(at25->spi, AT25_RDSR);
+			if (sr < 0 || (sr & AT25_SR_nRDY)) {
+				dev_dbg(at25->cdev.dev,
+					"rdsr --> %d (%02x)\n", sr, sr);
+				mdelay(1);
+				continue;
+			}
+			if (!(sr & AT25_SR_nRDY))
+				break;
+		} while (retries++ < 3 || !is_timeout(start_time, EE_TIMEOUT));
+
+		if ((sr < 0) || (sr & AT25_SR_nRDY)) {
+			dev_err(at25->cdev.dev,
+				"write %d bytes offset %d, "
+				"timeout after %u msecs\n",
+				segment, offset,
+				(unsigned int)(get_time_ns() - start_time) /
+					1000000);
+			status = -ETIMEDOUT;
+			break;
+		}
+
+		off += segment;
+		buf += segment;
+		count -= segment;
+		written += segment;
+
+	} while (count > 0);
+
+	free(bounce);
+	return written ? written : status;
+}
+
+static off_t at25_ee_lseek(struct cdev *cdev, off_t off)
+{
+	return off;
+}
+
+static struct file_operations at25_fops = {
+	.read	= at25_ee_read,
+	.write	= at25_ee_write,
+	.lseek	= at25_ee_lseek,
+};
+
+static int at25_probe(struct device_d *dev)
+{
+	int err, sr;
+	int addrlen;
+	struct at25_data *at25 = NULL;
+	const struct spi_eeprom *chip;
+
+	/* Chip description */
+	chip = dev->platform_data;
+	if (!chip) {
+		dev_dbg(dev, "no chip description\n");
+		err = -ENODEV;
+		goto fail;
+	}
+
+	/* For now we only support 8/16/24 bit addressing */
+	if (chip->flags & EE_ADDR1) {
+		addrlen = 1;
+	} else if (chip->flags & EE_ADDR2) {
+		addrlen = 2;
+	} else if (chip->flags & EE_ADDR3) {
+		addrlen = 3;
+	} else {
+		dev_dbg(dev, "unsupported address type\n");
+		err = -EINVAL;
+		goto fail;
+	}
+
+	at25 = xzalloc(sizeof(*at25));
+	at25->spi = dev->type_data;
+	at25->spi->mode = SPI_MODE_0;
+	at25->spi->bits_per_word = 8;
+	at25->cdev.ops = &at25_fops;
+	at25->cdev.size = chip->size;
+	at25->cdev.dev = dev;
+	at25->chip = *chip;
+	at25->addrlen = addrlen;
+	at25->cdev.name = at25->chip.name[0] ? at25->chip.name : DRIVERNAME;
+	at25->cdev.priv = at25;
+
+	/* Ping the chip ... the status register is pretty portable,
+	 * unlike probing manufacturer IDs.  We do expect that system
+	 * firmware didn't write it in the past few milliseconds!
+	 */
+	sr = spi_w8r8(at25->spi, AT25_RDSR);
+	if (sr < 0 || sr & AT25_SR_nRDY) {
+		dev_dbg(dev, "rdsr --> %d (%02x)\n", sr, sr);
+		err = -ENXIO;
+		goto fail;
+	}
+
+	dev_dbg(dev, "%s probed\n", at25->cdev.name);
+	devfs_create(&at25->cdev);
+	return 0;
+
+fail:
+	if (at25)
+		free(at25);
+
+	return err;
+}
+
+static struct driver_d at25_driver = {
+	.name  = DRIVERNAME,
+	.probe = at25_probe,
+};
+
+static int at25_init(void)
+{
+	register_driver(&at25_driver);
+	return 0;
+}
+
+device_initcall(at25_init);
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 4560259..6456897 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -75,6 +75,7 @@ struct spi_device *spi_new_device(struct spi_master *master,
 	proxy->chip_select = chip->chip_select;
 	proxy->max_speed_hz = chip->max_speed_hz;
 	proxy->mode = chip->mode;
+	proxy->dev.platform_data = chip->platform_data;
 	strcpy(proxy->dev.name, chip->name);
 	proxy->dev.type_data = proxy;
 	status = register_device(&proxy->dev);
@@ -194,3 +195,44 @@ int spi_sync(struct spi_device *spi, struct spi_message *message)
 	return spi->master->transfer(spi, message);
 }

+/**
+ * spi_write_then_read - SPI synchronous write followed by read
+ * @spi: device with which data will be exchanged
+ * @txbuf: data to be written
+ * @n_tx: size of txbuf, in bytes
+ * @rxbuf: buffer into which data will be read
+ * @n_rx: size of rxbuf, in bytes
+ * Context: can sleep
+ *
+ * This performs a half duplex MicroWire style transaction with the
+ * device, sending txbuf and then reading rxbuf.  The return value
+ * is zero for success, else a negative errno status code.
+ * This call may only be used from a context that may sleep.
+ */
+int spi_write_then_read(struct spi_device *spi,
+		const void *txbuf, unsigned n_tx,
+		void *rxbuf, unsigned n_rx)
+{
+	int			status;
+	struct spi_message	message;
+	struct spi_transfer	x[2];
+
+	spi_message_init(&message);
+	memset(x, 0, sizeof x);
+	if (n_tx) {
+		x[0].len = n_tx;
+		spi_message_add_tail(&x[0], &message);
+	}
+	if (n_rx) {
+		x[1].len = n_rx;
+		spi_message_add_tail(&x[1], &message);
+	}
+
+	x[0].tx_buf = txbuf;
+	x[1].rx_buf = rxbuf;
+
+	/* do the i/o */
+	status = spi_sync(spi, &message);
+	return status;
+}
+EXPORT_SYMBOL(spi_write_then_read);
diff --git a/include/spi/eeprom.h b/include/spi/eeprom.h
new file mode 100644
index 0000000..15495e5
--- /dev/null
+++ b/include/spi/eeprom.h
@@ -0,0 +1,22 @@
+#ifndef _SPI_EEPROM_H
+#define _SPI_EEPROM_H
+
+/*
+ * Put one of these structures in platform_data for SPI EEPROMS handled
+ * by the "at25" driver.  On SPI, most EEPROMS understand the same core
+ * command set.  If you need to support EEPROMs that don't yet fit, add
+ * flags to support those protocol options.  These values all come from
+ * the chip datasheets.
+ */
+struct spi_eeprom {
+	char		name[10];
+	u16		page_size;		/* for writes */
+	u16		flags;
+#define	EE_ADDR1	0x0001			/*  8 bit addrs */
+#define	EE_ADDR2	0x0002			/* 16 bit addrs */
+#define	EE_ADDR3	0x0004			/* 24 bit addrs */
+#define	EE_READONLY	0x0008			/* disallow writes */
+	u32		size;
+};
+
+#endif /* _SPI_EEPROM_H */
diff --git a/include/spi/spi.h b/include/spi/spi.h
index 8dce8db..ac2013a 100644
--- a/include/spi/spi.h
+++ b/include/spi/spi.h
@@ -14,8 +14,8 @@ struct spi_board_info {
 	/* mode becomes spi_device.mode, and is essential for chips
 	 * where the default of SPI_CS_HIGH = 0 is wrong.
 	 */
-	u8		mode;
-
+	u8	mode;
+	void	*platform_data;
 };

 /**
@@ -349,6 +349,80 @@ static inline int spi_register_board_info(struct spi_board_info const *info,
 }
 #endif

+/**
+ * spi_write - SPI synchronous write
+ * @spi: device to which data will be written
+ * @buf: data buffer
+ * @len: data buffer size
+ * Context: can sleep
+ *
+ * This writes the buffer and returns zero or a negative error code.
+ * Callable only from contexts that can sleep.
+ */
+static inline int
+spi_write(struct spi_device *spi, const void *buf, size_t len)
+{
+	struct spi_transfer	t = {
+			.tx_buf		= buf,
+			.len		= len,
+		};
+	struct spi_message	m;
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t, &m);
+	return spi_sync(spi, &m);
+}
+
+/**
+ * spi_read - SPI synchronous read
+ * @spi: device from which data will be read
+ * @buf: data buffer
+ * @len: data buffer size
+ * Context: can sleep
+ *
+ * This reads the buffer and returns zero or a negative error code.
+ * Callable only from contexts that can sleep.
+ */
+static inline int
+spi_read(struct spi_device *spi, void *buf, size_t len)
+{
+	struct spi_transfer	t = {
+			.rx_buf		= buf,
+			.len		= len,
+		};
+	struct spi_message	m;
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t, &m);
+	return spi_sync(spi, &m);
+}
+
+/* this copies txbuf and rxbuf data; for small transfers only! */
+extern int spi_write_then_read(struct spi_device *spi,
+		const void *txbuf, unsigned n_tx,
+		void *rxbuf, unsigned n_rx);
+
+/**
+ * spi_w8r8 - SPI synchronous 8 bit write followed by 8 bit read
+ * @spi: device with which data will be exchanged
+ * @cmd: command to be written before data is read back
+ * Context: can sleep
+ *
+ * This returns the (unsigned) eight bit number returned by the
+ * device, or else a negative error code.  Callable only from
+ * contexts that can sleep.
+ */
+static inline ssize_t spi_w8r8(struct spi_device *spi, u8 cmd)
+{
+	ssize_t			status;
+	u8			result;
+
+	status = spi_write_then_read(spi, &cmd, 1, &result, 1);
+
+	/* return negative errno or unsigned value */
+	return (status < 0) ? status : result;
+}
+
 #endif /* DOXYGEN_SHOULD_SKIP_THIS */

 #endif /* __INCLUDE_SPI_H */
--
1.7.4.1


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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC PATCH] spi: add at25 spi eeprom driver
  2011-06-16  8:12 [RFC PATCH] spi: add at25 spi eeprom driver Hubert Feurstein
@ 2011-06-20  6:45 ` Sascha Hauer
  2011-06-20  7:23   ` Hubert Feurstein
  0 siblings, 1 reply; 4+ messages in thread
From: Sascha Hauer @ 2011-06-20  6:45 UTC (permalink / raw)
  To: Hubert Feurstein; +Cc: barebox

Hi Hubert,

On Thu, Jun 16, 2011 at 10:12:43AM +0200, Hubert Feurstein wrote:
>  obj-y	+= mfd/
>  obj-$(CONFIG_LED) += led/
> +obj-y	+= misc/
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig

I think we should move the driver up to drivers/eeprom and skip the
'misc'. The kernel guys like to get rid of it also.

> +
> +		/* Wait for non-busy status */
> +		start_time = get_time_ns();
> +
> +		retries = 0;
> +		do {
> +
> +			sr = spi_w8r8(at25->spi, AT25_RDSR);
> +			if (sr < 0 || (sr & AT25_SR_nRDY)) {
> +				dev_dbg(at25->cdev.dev,
> +					"rdsr --> %d (%02x)\n", sr, sr);
> +				mdelay(1);
> +				continue;
> +			}
> +			if (!(sr & AT25_SR_nRDY))
> +				break;
> +		} while (retries++ < 3 || !is_timeout(start_time, EE_TIMEOUT));

I don't understand this. The loop is limited by retries++ < 3. Why this
additional is_timeout? Is this the same in the kernel?

> +static int at25_init(void)
> +{
> +	register_driver(&at25_driver);
> +	return 0;
> +}
> +
> +device_initcall(at25_init);
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 4560259..6456897 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -75,6 +75,7 @@ struct spi_device *spi_new_device(struct spi_master *master,
>  	proxy->chip_select = chip->chip_select;
>  	proxy->max_speed_hz = chip->max_speed_hz;
>  	proxy->mode = chip->mode;
> +	proxy->dev.platform_data = chip->platform_data;

This should be a seperate patch.

>  	strcpy(proxy->dev.name, chip->name);
>  	proxy->dev.type_data = proxy;
>  	status = register_device(&proxy->dev);
> @@ -194,3 +195,44 @@ int spi_sync(struct spi_device *spi, struct spi_message *message)
>  	return spi->master->transfer(spi, message);
>  }
> 
> +/**
> + * spi_write_then_read - SPI synchronous write followed by read
> + * @spi: device with which data will be exchanged
> + * @txbuf: data to be written
> + * @n_tx: size of txbuf, in bytes
> + * @rxbuf: buffer into which data will be read
> + * @n_rx: size of rxbuf, in bytes
> + * Context: can sleep
> + *
> + * This performs a half duplex MicroWire style transaction with the
> + * device, sending txbuf and then reading rxbuf.  The return value
> + * is zero for success, else a negative errno status code.
> + * This call may only be used from a context that may sleep.
> + */
> +int spi_write_then_read(struct spi_device *spi,
> +		const void *txbuf, unsigned n_tx,
> +		void *rxbuf, unsigned n_rx)
> +{
> +	int			status;
> +	struct spi_message	message;
> +	struct spi_transfer	x[2];
> +
> +	spi_message_init(&message);
> +	memset(x, 0, sizeof x);
> +	if (n_tx) {
> +		x[0].len = n_tx;
> +		spi_message_add_tail(&x[0], &message);
> +	}
> +	if (n_rx) {
> +		x[1].len = n_rx;
> +		spi_message_add_tail(&x[1], &message);
> +	}
> +
> +	x[0].tx_buf = txbuf;
> +	x[1].rx_buf = rxbuf;
> +
> +	/* do the i/o */
> +	status = spi_sync(spi, &message);
> +	return status;
> +}
> +EXPORT_SYMBOL(spi_write_then_read);

Also a seperate patch.

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC PATCH] spi: add at25 spi eeprom driver
  2011-06-20  6:45 ` Sascha Hauer
@ 2011-06-20  7:23   ` Hubert Feurstein
  2011-06-20  7:37     ` Sascha Hauer
  0 siblings, 1 reply; 4+ messages in thread
From: Hubert Feurstein @ 2011-06-20  7:23 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

Hi Sascha,

2011/6/20 Sascha Hauer <s.hauer@pengutronix.de>:
> Hi Hubert,
>
> On Thu, Jun 16, 2011 at 10:12:43AM +0200, Hubert Feurstein wrote:
>>  obj-y        += mfd/
>>  obj-$(CONFIG_LED) += led/
>> +obj-y        += misc/
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>
> I think we should move the driver up to drivers/eeprom and skip the
> 'misc'. The kernel guys like to get rid of it also.
>
OK

>> +
>> +             /* Wait for non-busy status */
>> +             start_time = get_time_ns();
>> +
>> +             retries = 0;
>> +             do {
>> +
>> +                     sr = spi_w8r8(at25->spi, AT25_RDSR);
>> +                     if (sr < 0 || (sr & AT25_SR_nRDY)) {
>> +                             dev_dbg(at25->cdev.dev,
>> +                                     "rdsr --> %d (%02x)\n", sr, sr);
>> +                             mdelay(1);
>> +                             continue;
>> +                     }
>> +                     if (!(sr & AT25_SR_nRDY))
>> +                             break;
>> +             } while (retries++ < 3 || !is_timeout(start_time, EE_TIMEOUT));
>
> I don't understand this. The loop is limited by retries++ < 3. Why this
> additional is_timeout? Is this the same in the kernel?
>
Hmm, I don't know why we have both here. I simply ported the kernel code.

>> +static int at25_init(void)
>> +{
>> +     register_driver(&at25_driver);
>> +     return 0;
>> +}
>> +
>> +device_initcall(at25_init);
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index 4560259..6456897 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -75,6 +75,7 @@ struct spi_device *spi_new_device(struct spi_master *master,
>>       proxy->chip_select = chip->chip_select;
>>       proxy->max_speed_hz = chip->max_speed_hz;
>>       proxy->mode = chip->mode;
>> +     proxy->dev.platform_data = chip->platform_data;
>
> This should be a seperate patch.
OK

>
>>       strcpy(proxy->dev.name, chip->name);
>>       proxy->dev.type_data = proxy;
>>       status = register_device(&proxy->dev);
>> @@ -194,3 +195,44 @@ int spi_sync(struct spi_device *spi, struct spi_message *message)
>>       return spi->master->transfer(spi, message);
>>  }
>>
>> +/**
>> + * spi_write_then_read - SPI synchronous write followed by read
>> + * @spi: device with which data will be exchanged
>> + * @txbuf: data to be written
>> + * @n_tx: size of txbuf, in bytes
>> + * @rxbuf: buffer into which data will be read
>> + * @n_rx: size of rxbuf, in bytes
>> + * Context: can sleep
>> + *
>> + * This performs a half duplex MicroWire style transaction with the
>> + * device, sending txbuf and then reading rxbuf.  The return value
>> + * is zero for success, else a negative errno status code.
>> + * This call may only be used from a context that may sleep.
>> + */
>> +int spi_write_then_read(struct spi_device *spi,
>> +             const void *txbuf, unsigned n_tx,
>> +             void *rxbuf, unsigned n_rx)
>> +{
>> +     int                     status;
>> +     struct spi_message      message;
>> +     struct spi_transfer     x[2];
>> +
>> +     spi_message_init(&message);
>> +     memset(x, 0, sizeof x);
>> +     if (n_tx) {
>> +             x[0].len = n_tx;
>> +             spi_message_add_tail(&x[0], &message);
>> +     }
>> +     if (n_rx) {
>> +             x[1].len = n_rx;
>> +             spi_message_add_tail(&x[1], &message);
>> +     }
>> +
>> +     x[0].tx_buf = txbuf;
>> +     x[1].rx_buf = rxbuf;
>> +
>> +     /* do the i/o */
>> +     status = spi_sync(spi, &message);
>> +     return status;
>> +}
>> +EXPORT_SYMBOL(spi_write_then_read);
>
> Also a seperate patch.
OK


Hubert

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC PATCH] spi: add at25 spi eeprom driver
  2011-06-20  7:23   ` Hubert Feurstein
@ 2011-06-20  7:37     ` Sascha Hauer
  0 siblings, 0 replies; 4+ messages in thread
From: Sascha Hauer @ 2011-06-20  7:37 UTC (permalink / raw)
  To: Hubert Feurstein; +Cc: barebox

On Mon, Jun 20, 2011 at 09:23:40AM +0200, Hubert Feurstein wrote:
> Hi Sascha,
> 
> 2011/6/20 Sascha Hauer <s.hauer@pengutronix.de>:
> > Hi Hubert,
> >
> > On Thu, Jun 16, 2011 at 10:12:43AM +0200, Hubert Feurstein wrote:
> >>  obj-y        += mfd/
> >>  obj-$(CONFIG_LED) += led/
> >> +obj-y        += misc/
> >> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> >
> > I think we should move the driver up to drivers/eeprom and skip the
> > 'misc'. The kernel guys like to get rid of it also.
> >
> OK
> 
> >> +
> >> +             /* Wait for non-busy status */
> >> +             start_time = get_time_ns();
> >> +
> >> +             retries = 0;
> >> +             do {
> >> +
> >> +                     sr = spi_w8r8(at25->spi, AT25_RDSR);
> >> +                     if (sr < 0 || (sr & AT25_SR_nRDY)) {
> >> +                             dev_dbg(at25->cdev.dev,
> >> +                                     "rdsr --> %d (%02x)\n", sr, sr);
> >> +                             mdelay(1);

We can remove this mdelay. In the kernel it makes sense to use msleep to
give the cpu a chance to sleep. In barebox we are polling anyway, so we
can better poll the status register instead of the timer register. This
gives us the chance to bail out here after 1.1ms instead of 1, 2, ...

> >> +                             continue;
> >> +                     }
> >> +                     if (!(sr & AT25_SR_nRDY))
> >> +                             break;
> >> +             } while (retries++ < 3 || !is_timeout(start_time, EE_TIMEOUT));
> >
> > I don't understand this. The loop is limited by retries++ < 3. Why this
> > additional is_timeout? Is this the same in the kernel?
> >
> Hmm, I don't know why we have both here. I simply ported the kernel code.

Looking at it again it seems like 'poll for EE_TIMEOUT ms but at least
three times'. I think we can skip the retries and just poll for
EE_TIMEOUT ms. The most important thing about timeouts is that we bail
out after reasonable time to give the user a chance to continue without
this driver and to give him a clue where it hangs. The exact amount
of time we poll is not really important.

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2011-06-20  7:37 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-06-16  8:12 [RFC PATCH] spi: add at25 spi eeprom driver Hubert Feurstein
2011-06-20  6:45 ` Sascha Hauer
2011-06-20  7:23   ` Hubert Feurstein
2011-06-20  7:37     ` Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox