mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* Add basic driver for 24clxx eeproms
@ 2011-01-14  0:06 Marc Reilly
  2011-01-14  0:07 ` [PATCH 1/2] i2c: add platform_data for i2c_board_info Marc Reilly
  2011-01-14  0:07 ` [PATCH 2/2] at24: add I2C eeprom for 24cl02 Marc Reilly
  0 siblings, 2 replies; 9+ messages in thread
From: Marc Reilly @ 2011-01-14  0:06 UTC (permalink / raw)
  To: barebox

Hi,

I did this driver a while back, and almost forgot about it. The first
patch is required so that the board can pass in the size of the eeprom when the
device is probed.

Cheers
Marc


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

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

* [PATCH 1/2] i2c: add platform_data for i2c_board_info
  2011-01-14  0:06 Add basic driver for 24clxx eeproms Marc Reilly
@ 2011-01-14  0:07 ` Marc Reilly
  2011-01-14  0:07 ` [PATCH 2/2] at24: add I2C eeprom for 24cl02 Marc Reilly
  1 sibling, 0 replies; 9+ messages in thread
From: Marc Reilly @ 2011-01-14  0:07 UTC (permalink / raw)
  To: barebox

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 2507d68..da61d01 100644
--- a/include/i2c/i2c.h
+++ b/include/i2c/i2c.h
@@ -98,6 +98,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.1


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

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

* [PATCH 2/2] at24: add I2C eeprom for 24cl02
  2011-01-14  0:06 Add basic driver for 24clxx eeproms Marc Reilly
  2011-01-14  0:07 ` [PATCH 1/2] i2c: add platform_data for i2c_board_info Marc Reilly
@ 2011-01-14  0:07 ` Marc Reilly
  2011-01-14  9:19   ` Sascha Hauer
  1 sibling, 1 reply; 9+ messages in thread
From: Marc Reilly @ 2011-01-14  0:07 UTC (permalink / raw)
  To: barebox

This series adds a driver for at24 eeproms. Much of the guts of the code
is taken from the at24 driver in the linux kernel.
The files are located under a new misc directory, as per the kernel also.

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/Kconfig       |    1 +
 drivers/Makefile      |    1 +
 drivers/misc/Kconfig  |   10 ++++
 drivers/misc/Makefile |    1 +
 drivers/misc/at24.c   |  130 +++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 143 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/Kconfig
 create mode 100644 drivers/misc/Makefile
 create mode 100644 drivers/misc/at24.c

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..2127857 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-$(CONFIG_MISC) += misc/
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
new file mode 100644
index 0000000..7da647f
--- /dev/null
+++ b/drivers/misc/Kconfig
@@ -0,0 +1,10 @@
+menuconfig MISC
+	bool "Misc devices support"
+
+if MISC
+
+config AT24
+	bool "at24 based eeprom"
+	depends on I2C
+
+endif
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
new file mode 100644
index 0000000..45d6553
--- /dev/null
+++ b/drivers/misc/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_AT24) += at24.o
diff --git a/drivers/misc/at24.c b/drivers/misc/at24.c
new file mode 100644
index 0000000..7bb085a
--- /dev/null
+++ b/drivers/misc/at24.c
@@ -0,0 +1,130 @@
+/*
+ * 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 <driver.h>
+#include <xfuncs.h>
+#include <errno.h>
+
+#include <i2c/i2c.h>
+#include <misc/at24.h>
+
+#define DRIVERNAME		"at24"
+
+#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 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 maxwrite = 8;
+	int numtowrite;
+	ssize_t numwritten = 0;
+
+	while (count) {
+		int ret;
+
+		numtowrite = count;
+		if (numtowrite > maxwrite)
+			numtowrite = maxwrite;
+
+		ret = i2c_write_reg(priv->client, offset, buf, numtowrite);
+		if (ret < 0)
+			return (ssize_t)ret;
+
+		mdelay(10);
+
+		numwritten += ret;
+		buf += ret;
+		count -= ret;
+		offset += ret;
+	}
+
+	return numwritten;
+}
+
+static struct file_operations at24_fops = {
+	.lseek	= dev_lseek_default,
+	.read	= at24_read,
+	.write	= at24_write,
+};
+
+static int at24_probe(struct device_d *dev)
+{
+	struct at24 *at24;
+	struct at24_platform_data *pdata;
+	at24 = xzalloc(sizeof(*at24));
+
+	dev->priv = at24;
+	pdata = dev->platform_data;
+
+	at24->cdev.name = DRIVERNAME;
+	at24->client = to_i2c_client(dev);
+	at24->cdev.size = pdata->size;
+	at24->cdev.dev = dev;
+	at24->cdev.ops = &at24_fops;
+
+	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);
-- 
1.7.1


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

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

* Re: [PATCH 2/2] at24: add I2C eeprom for 24cl02
  2011-01-14  0:07 ` [PATCH 2/2] at24: add I2C eeprom for 24cl02 Marc Reilly
@ 2011-01-14  9:19   ` Sascha Hauer
  2011-01-14  9:39     ` Sascha Hauer
  0 siblings, 1 reply; 9+ messages in thread
From: Sascha Hauer @ 2011-01-14  9:19 UTC (permalink / raw)
  To: Marc Reilly; +Cc: barebox

Hi Marc,

On Fri, Jan 14, 2011 at 11:07:01AM +1100, Marc Reilly wrote:
> This series adds a driver for at24 eeproms. Much of the guts of the code
> is taken from the at24 driver in the linux kernel.
> The files are located under a new misc directory, as per the kernel also.
> 
> Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> ---
>  drivers/Kconfig       |    1 +
>  drivers/Makefile      |    1 +
>  drivers/misc/Kconfig  |   10 ++++
>  drivers/misc/Makefile |    1 +
>  drivers/misc/at24.c   |  130 +++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 143 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/misc/Kconfig
>  create mode 100644 drivers/misc/Makefile
>  create mode 100644 drivers/misc/at24.c
> 
> 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..2127857 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-$(CONFIG_MISC) += misc/
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> new file mode 100644
> index 0000000..7da647f
> --- /dev/null
> +++ b/drivers/misc/Kconfig
> @@ -0,0 +1,10 @@
> +menuconfig MISC
> +	bool "Misc devices support"
> +
> +if MISC
> +
> +config AT24
> +	bool "at24 based eeprom"
> +	depends on I2C
> +
> +endif
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> new file mode 100644
> index 0000000..45d6553
> --- /dev/null
> +++ b/drivers/misc/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_AT24) += at24.o
> diff --git a/drivers/misc/at24.c b/drivers/misc/at24.c
> new file mode 100644
> index 0000000..7bb085a
> --- /dev/null
> +++ b/drivers/misc/at24.c
> @@ -0,0 +1,130 @@
> +/*
> + * 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 <driver.h>
> +#include <xfuncs.h>
> +#include <errno.h>
> +
> +#include <i2c/i2c.h>
> +#include <misc/at24.h>
> +
> +#define DRIVERNAME		"at24"
> +
> +#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 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 maxwrite = 8;
> +	int numtowrite;
> +	ssize_t numwritten = 0;
> +
> +	while (count) {
> +		int ret;
> +
> +		numtowrite = count;
> +		if (numtowrite > maxwrite)
> +			numtowrite = maxwrite;
> +
> +		ret = i2c_write_reg(priv->client, offset, buf, numtowrite);
> +		if (ret < 0)
> +			return (ssize_t)ret;
> +
> +		mdelay(10);
> +
> +		numwritten += ret;
> +		buf += ret;
> +		count -= ret;
> +		offset += ret;
> +	}
> +
> +	return numwritten;
> +}
> +
> +static struct file_operations at24_fops = {
> +	.lseek	= dev_lseek_default,
> +	.read	= at24_read,
> +	.write	= at24_write,
> +};
> +
> +static int at24_probe(struct device_d *dev)
> +{
> +	struct at24 *at24;
> +	struct at24_platform_data *pdata;
> +	at24 = xzalloc(sizeof(*at24));
> +
> +	dev->priv = at24;
> +	pdata = dev->platform_data;
> +
> +	at24->cdev.name = DRIVERNAME;

You should use at24->cdev.name = asprintf("%s%d", DRIVERNAME, dev->id);
here to support multiple eeproms. Note that this afaics needs patching
i2c_new_device to initialize client->dev.id to -1 to make sure the i2c
device gets an autoassigned id.

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] 9+ messages in thread

* Re: [PATCH 2/2] at24: add I2C eeprom for 24cl02
  2011-01-14  9:19   ` Sascha Hauer
@ 2011-01-14  9:39     ` Sascha Hauer
  2011-01-16  1:04       ` [PATCH] devfs: new func make_cdev_name: allocate unique cdev name Marc Reilly
       [not found]       ` <1295139858-9193-1-git-send-email-marc@cpdesign.com.au>
  0 siblings, 2 replies; 9+ messages in thread
From: Sascha Hauer @ 2011-01-14  9:39 UTC (permalink / raw)
  To: Marc Reilly; +Cc: barebox

On Fri, Jan 14, 2011 at 10:19:12AM +0100, Sascha Hauer wrote:
> Hi Marc,
> 
> > +
> > +static int at24_probe(struct device_d *dev)
> > +{
> > +	struct at24 *at24;
> > +	struct at24_platform_data *pdata;
> > +	at24 = xzalloc(sizeof(*at24));
> > +
> > +	dev->priv = at24;
> > +	pdata = dev->platform_data;
> > +
> > +	at24->cdev.name = DRIVERNAME;
> 
> You should use at24->cdev.name = asprintf("%s%d", DRIVERNAME, dev->id);
> here to support multiple eeproms. Note that this afaics needs patching
> i2c_new_device to initialize client->dev.id to -1 to make sure the i2c
> device gets an autoassigned id.

This reminds me of something I should do in the next time.
A cdev is not in any way related to a struct device_d anymore.
Nevertheless we more than once use the device id to construct a
cdev name like above. Instead we should have a
get_cdev_name(char *template) function which creates a name suitable for
cdevs. This way both a spi and a i2c eeprom driver could use "eeprom" as
template and both would get a valid name.

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] 9+ messages in thread

* [PATCH] devfs: new func make_cdev_name: allocate unique cdev name
  2011-01-14  9:39     ` Sascha Hauer
@ 2011-01-16  1:04       ` Marc Reilly
       [not found]       ` <1295139858-9193-1-git-send-email-marc@cpdesign.com.au>
  1 sibling, 0 replies; 9+ messages in thread
From: Marc Reilly @ 2011-01-16  1:04 UTC (permalink / raw)
  To: s.hauer; +Cc: barebox

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

diff --git a/fs/devfs.c b/fs/devfs.c
index 7019c8d..c1196d1 100644
--- a/fs/devfs.c
+++ b/fs/devfs.c
@@ -38,6 +38,47 @@
 
 static LIST_HEAD(cdev_list);
 
+/**
+ * Allocates a unique name for a cdev based on existing cdevs.
+ *
+ * @param template Starting point for the name of the device.
+ * The name can optionally contain a "%d" marker to designate where the
+ * device number should be inserted into the device string. If no marker
+ * is specified then one is appended.
+ *
+ * @param prefid Preferred device id. Any id less than zero will default
+ * to start from 0.
+ */
+char *make_cdev_name(const char *template, int prefid)
+{
+	char *name = 0;
+	char *temp;
+	int id;
+	struct cdev *cdev;
+
+	/* if there is no template for number, append one */
+	if (!strstr(template, "%d"))
+		temp = asprintf("%s%%d", template);
+	else
+		temp = strdup(template);
+
+	id = (prefid < 0) ? 0 : prefid;
+
+	do {
+		free(name);
+		name = asprintf(temp, id);
+
+		cdev = cdev_by_name(name);
+		if (cdev && (id == prefid))
+			printf("WARN: preferred device name %s already used\n",
+					name);
+		++id;
+	} while (cdev);
+
+	free(temp);
+	return name;
+}
+
 struct cdev *cdev_by_name(const char *filename)
 {
 	struct cdev *cdev;
diff --git a/include/driver.h b/include/driver.h
index b9edca0..1712637 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -313,6 +313,7 @@ struct cdev {
 
 int devfs_create(struct cdev *);
 int devfs_remove(struct cdev *);
+char *make_cdev_name(const char *template, int prefid);
 struct cdev *cdev_by_name(const char *filename);
 ssize_t cdev_read(struct cdev *cdev, void *buf, size_t count, ulong offset, ulong flags);
 ssize_t cdev_write(struct cdev *cdev, const void *buf, size_t count, ulong offset, ulong flags);
-- 
1.7.1


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

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

* Re: cdev name generation
       [not found]       ` <1295139858-9193-1-git-send-email-marc@cpdesign.com.au>
@ 2011-01-17 18:01         ` Sascha Hauer
  2011-01-17 22:22           ` Marc Reilly
  0 siblings, 1 reply; 9+ messages in thread
From: Sascha Hauer @ 2011-01-17 18:01 UTC (permalink / raw)
  To: Marc Reilly; +Cc: barebox

Hi Marc,

On Sun, Jan 16, 2011 at 12:04:17PM +1100, Marc Reilly wrote:
> Hi Sascha,
> 
> Do you mean something like this attached patch?

Yes

> I preferred having the possibilty for assigning the id, so that boards can have
> some expectations of what the device name will be.

Generally a good idea, but how do you want to pass previd to the drivers
using it? via platform_data? I haven't looked into it to see if this
works well.

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] 9+ messages in thread

* Re: cdev name generation
  2011-01-17 18:01         ` cdev name generation Sascha Hauer
@ 2011-01-17 22:22           ` Marc Reilly
  2011-01-19  8:41             ` Sascha Hauer
  0 siblings, 1 reply; 9+ messages in thread
From: Marc Reilly @ 2011-01-17 22:22 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

Hi Sascha,

> > 
> > Do you mean something like this attached patch?
> 
> Yes
> 
> > I preferred having the possibilty for assigning the id, so that boards
> > can have some expectations of what the device name will be.
> 
> Generally a good idea, but how do you want to pass previd to the drivers
> using it? via platform_data? I haven't looked into it to see if this
> works well.
> 

That's what I did for the at24, (although preferred_id would be a better name 
than just id)

static struct at24_platform_data at24_data = {
	.size	= 2048 / 8,
	.id		= 1,
};


static int at24_probe(struct device_d *dev)
{
	struct at24 *at24;
	struct at24_platform_data *pdata;
	at24 = xzalloc(sizeof(*at24));

	dev->priv = at24;
	pdata = dev->platform_data;

	at24->cdev.name = make_cdev_name("eeprom", pdata->id);
	at24->client = to_i2c_client(dev);
	at24->cdev.size = pdata->size;
	at24->cdev.dev = dev;


Maybe the platform data should have the preferred cdev name also? In the 
example above, the driver just blindly calls it "eeprom", ie a more generic 
term than "at24", the driver name.
If the desired end result is to have for example eeprom0, eeprom1 and eeprom2 
cdevs all from potentially different drivers then I makes sense to also be 
able to specify the "eeprom" part. (and then we'd probably want to pass in the 
device_d* to make_cdev_name in case the user doesn't specify a cdev name - the 
device name, and id, could be used).

Seems like too much thinking for so little code :)

Cheers,
Marc

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

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

* Re: cdev name generation
  2011-01-17 22:22           ` Marc Reilly
@ 2011-01-19  8:41             ` Sascha Hauer
  0 siblings, 0 replies; 9+ messages in thread
From: Sascha Hauer @ 2011-01-19  8:41 UTC (permalink / raw)
  To: Marc Reilly; +Cc: barebox

On Tue, Jan 18, 2011 at 09:22:20AM +1100, Marc Reilly wrote:
> Hi Sascha,
> 
> > > 
> > > Do you mean something like this attached patch?
> > 
> > Yes
> > 
> > > I preferred having the possibilty for assigning the id, so that boards
> > > can have some expectations of what the device name will be.
> > 
> > Generally a good idea, but how do you want to pass previd to the drivers
> > using it? via platform_data? I haven't looked into it to see if this
> > works well.
> > 
> 
> That's what I did for the at24, (although preferred_id would be a better name 
> than just id)
> 
> static struct at24_platform_data at24_data = {
> 	.size	= 2048 / 8,
> 	.id		= 1,
> };
> 
> 
> static int at24_probe(struct device_d *dev)
> {
> 	struct at24 *at24;
> 	struct at24_platform_data *pdata;
> 	at24 = xzalloc(sizeof(*at24));
> 
> 	dev->priv = at24;
> 	pdata = dev->platform_data;
> 
> 	at24->cdev.name = make_cdev_name("eeprom", pdata->id);
> 	at24->client = to_i2c_client(dev);
> 	at24->cdev.size = pdata->size;
> 	at24->cdev.dev = dev;
> 
> 
> Maybe the platform data should have the preferred cdev name also? In the 
> example above, the driver just blindly calls it "eeprom", ie a more generic 
> term than "at24", the driver name.
> If the desired end result is to have for example eeprom0, eeprom1 and eeprom2 
> cdevs all from potentially different drivers then I makes sense to also be 
> able to specify the "eeprom" part. (and then we'd probably want to pass in the 
> device_d* to make_cdev_name in case the user doesn't specify a cdev name - the 
> device name, and id, could be used).
> 
> Seems like too much thinking for so little code :)

I don't think so. This is one of the places where we'll regret a wrong
decision once we have to change it in the future and have to deal with
loads of breaking environments.

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] 9+ messages in thread

end of thread, other threads:[~2011-01-19  8:41 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-14  0:06 Add basic driver for 24clxx eeproms Marc Reilly
2011-01-14  0:07 ` [PATCH 1/2] i2c: add platform_data for i2c_board_info Marc Reilly
2011-01-14  0:07 ` [PATCH 2/2] at24: add I2C eeprom for 24cl02 Marc Reilly
2011-01-14  9:19   ` Sascha Hauer
2011-01-14  9:39     ` Sascha Hauer
2011-01-16  1:04       ` [PATCH] devfs: new func make_cdev_name: allocate unique cdev name Marc Reilly
     [not found]       ` <1295139858-9193-1-git-send-email-marc@cpdesign.com.au>
2011-01-17 18:01         ` cdev name generation Sascha Hauer
2011-01-17 22:22           ` Marc Reilly
2011-01-19  8:41             ` Sascha Hauer

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