Fwd: [PATCH 2/3] I2C: at24: add kernel interface for reading/writing EEPROM

Fwd: [PATCH 2/3] I2C: at24: add kernel interface for reading/writing EEPROM

[David Brownell]

My own comments on this:

	- I'd like to see at24.c use something running before
	  device_initcall, so suitably configured system can
	  have drivers calling platform_device_probe() from
	  their own initcalls and yet have access to the config
	  data from the EEPROMs.

	- Seems to me that "struct at24_iface" should be more
	  generic ... the same notion works for SPI eeproms,
	  NVRAM as found in RTCs, etc.

Comments?

- Dave

----------  Forwarded Message  ----------

Subject: [PATCH 2/3] I2C: at24: add kernel interface for reading/writing EEPROM
Date: Monday 25 August 2008
From: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
To: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org

This patch adds an interface by which other kernel code can read/write
detected EEPROM.

The platform code registers a 'setup' callback with the
at24_platform_data.  When the at24 driver detects an EEPROM, it fills
out the read and write functions of at24_iface and calls the setup
callback.  The platform code can then use the read/write functions in
the at24_iface struct for reading and writing the EEPROM.

Original idea, review and updates by David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>

Signed-off-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
---
 drivers/i2c/chips/at24.c |   42 +++++++++++++++++++++++++++++++++++-------
 include/linux/i2c/at24.h |   10 ++++++++++
 2 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/chips/at24.c b/drivers/i2c/chips/at24.c
index 2a4acb2..ce6423d 100644
--- a/drivers/i2c/chips/at24.c
+++ b/drivers/i2c/chips/at24.c
@@ -53,6 +53,7 @@
 
 struct at24_data {
 	struct at24_platform_data chip;
+	struct at24_iface iface;
 	bool use_smbus;
 
 	/*
@@ -264,13 +265,6 @@ static ssize_t at24_bin_read(struct kobject *kobj, struct bin_attribute *attr,
 
 
 /*
- * REVISIT: export at24_bin{read,write}() to let other kernel code use
- * eeprom data. For example, it might hold a board's Ethernet address, or
- * board-specific calibration data generated on the manufacturing floor.
- */
-
-
-/*
  * Note that if the hardware write-protect pin is pulled high, the whole
  * chip is normally write protected. But there are plenty of product
  * variants here, including OTP fuses and partial chip protect.
@@ -386,6 +380,30 @@ static ssize_t at24_bin_write(struct kobject *kobj, struct bin_attribute *attr,
 
 /*-------------------------------------------------------------------------*/
 
+/*
+ * This lets other kernel code access the eeprom data. For example, it
+ * might hold a board's Ethernet address, or board-specific calibration
+ * data generated on the manufacturing floor.
+ */
+
+static ssize_t at24_iface_read(struct at24_iface *iface, char *buf,
+			      off_t offset, size_t count)
+{
+	struct at24_data *at24 = container_of(iface, struct at24_data, iface);
+
+	return at24_eeprom_read(at24, buf, offset, count);
+}
+
+static ssize_t at24_iface_write(struct at24_iface *iface, char *buf,
+			       off_t offset, size_t count)
+{
+	struct at24_data *at24 = container_of(iface, struct at24_data, iface);
+
+	return at24_eeprom_write(at24, buf, offset, count);
+}
+
+/*-------------------------------------------------------------------------*/
+
 static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct at24_platform_data chip;
@@ -413,6 +431,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		 * is recommended anyhow.
 		 */
 		chip.page_size = 1;
+
+		chip.setup = NULL;
+		chip.context = NULL;
 	}
 
 	if (!is_power_of_2(chip.byte_len))
@@ -449,6 +470,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		goto err_out;
 	}
 
+	at24->iface.read = at24_iface_read;
+	at24->iface.write = at24_iface_write;
+
 	mutex_init(&at24->lock);
 	at24->use_smbus = use_smbus;
 	at24->chip = chip;
@@ -521,6 +545,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		at24->write_max,
 		use_smbus ? ", use_smbus" : "");
 
+	/* export data to kernel code */
+	if (chip.setup)
+		chip.setup(&at24->iface, chip.context);
+
 	return 0;
 
 err_clients:
diff --git a/include/linux/i2c/at24.h b/include/linux/i2c/at24.h
index f6edd52..50008cc 100644
--- a/include/linux/i2c/at24.h
+++ b/include/linux/i2c/at24.h
@@ -15,6 +15,13 @@
  * is bigger than what the chip actually supports!
  */
 
+struct at24_iface {
+	ssize_t (*read)(struct at24_iface *, char *buf, off_t offset,
+			size_t count);
+	ssize_t (*write)(struct at24_iface *, char *buf, off_t offset,
+			 size_t count);
+};
+
 struct at24_platform_data {
 	u32		byte_len;		/* size (sum of all addr) */
 	u16		page_size;		/* for writes */
@@ -23,6 +30,9 @@ struct at24_platform_data {
 #define AT24_FLAG_READONLY	0x40	/* sysfs-entry will be read-only */
 #define AT24_FLAG_IRUGO		0x20	/* sysfs-entry will be world-readable */
 #define AT24_FLAG_TAKE8ADDR	0x10	/* take always 8 addresses (24c00) */
+
+	int             (*setup)(struct at24_iface *, void *context);
+	void		*context;
 };
 
 #endif /* _LINUX_AT24_H */
-- 
1.5.6.4


-------------------------------------------------------

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Fwd: [PATCH 2/3] I2C: at24: add kernel interface for reading/writing EEPROM

[Jon Smirl]

On 8/25/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> My own comments on this:
>
>         - I'd like to see at24.c use something running before
>           device_initcall, so suitably configured system can
>           have drivers calling platform_device_probe() from
>           their own initcalls and yet have access to the config
>           data from the EEPROMs.
>
>         - Seems to me that "struct at24_iface" should be more
>           generic ... the same notion works for SPI eeproms,
>           NVRAM as found in RTCs, etc.
>
>  Comments?

Would it make sense to use bus notifiers to track the detection of a
at24 chip? I have some MMC code in my tree that is using them.

+static int of_mmc_spi_notify(struct notifier_block *nb, unsigned long action,
+			     void *_dev)
+{
+	struct device *dev = _dev;
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		return of_mmc_spi_add(dev);
+	case BUS_NOTIFY_DEL_DEVICE:
+		return of_mmc_spi_del(dev);
+	};
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block of_mmc_spi_notifier = {
+	.notifier_call = of_mmc_spi_notify,
+};
+

-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Fwd: [PATCH 2/3] I2C: at24: add kernel interface for reading/writing EEPROM

[David Brownell]

On Monday 25 August 2008, Jon Smirl wrote:
> On 8/25/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > My own comments on this:
> >
> >         - I'd like to see at24.c use something running before
> >           device_initcall, so suitably configured system can
> >           have drivers calling platform_device_probe() from
> >           their own initcalls and yet have access to the config
> >           data from the EEPROMs.
> >
> >         - Seems to me that "struct at24_iface" should be more
> >           generic ... the same notion works for SPI eeproms,
> >           NVRAM as found in RTCs, etc.
> >
> >  Comments?
> 
> Would it make sense to use bus notifiers to track the detection of a
> at24 chip? 

Not to me ... but then, I'm no fan of notifiers in such cases.
Some differences associated with these setup callbacks:
 
  - Notifiers would have to poke at the parameters to sort
    out (a) BUS_NOTIFY_BOUND_DRIVER (b) with a device bound
    to this specific driver, and in fact (c) which specific
    devices is involved, maybe platform_data pointer checks;
    and then (d) use some API that the driver exports, which
    implies (e) link-time dependencies.

  - setup() can be chip-specific, eliminating (a,b,c);

  - at24_iface just uses function pointers, eliminating (d,e).

Architecturally, the elimination of (d,e) seems significant;
but eliminating (a,b,c) is certainly convenient when writing
code to hook it all up!

- Dave

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Fwd: [PATCH 2/3] I2C: at24: add kernel interface for reading/writing EEPROM

[Jon Smirl]

On 8/26/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> On Monday 25 August 2008, Jon Smirl wrote:
>  > On 8/25/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
>  > > My own comments on this:
>  > >
>  > >         - I'd like to see at24.c use something running before
>  > >           device_initcall, so suitably configured system can
>  > >           have drivers calling platform_device_probe() from
>  > >           their own initcalls and yet have access to the config
>  > >           data from the EEPROMs.
>  > >
>  > >         - Seems to me that "struct at24_iface" should be more
>  > >           generic ... the same notion works for SPI eeproms,
>  > >           NVRAM as found in RTCs, etc.
>  > >
>  > >  Comments?
>  >
>  > Would it make sense to use bus notifiers to track the detection of a
>  > at24 chip?
>
>
> Not to me ... but then, I'm no fan of notifiers in such cases.
>  Some differences associated with these setup callbacks:
>
>   - Notifiers would have to poke at the parameters to sort
>     out (a) BUS_NOTIFY_BOUND_DRIVER (b) with a device bound
>     to this specific driver, and in fact (c) which specific
>     devices is involved, maybe platform_data pointer checks;
>     and then (d) use some API that the driver exports, which
>     implies (e) link-time dependencies.
>
>   - setup() can be chip-specific, eliminating (a,b,c);
>
>   - at24_iface just uses function pointers, eliminating (d,e).
>
>  Architecturally, the elimination of (d,e) seems significant;
>  but eliminating (a,b,c) is certainly convenient when writing
>  code to hook it all up!

Another solution is call_userspace_helper. GregKH convinced me to
change some things like this to small user space apps in the graphics
code. Is is easy to do the read/write from sysfs.

It doesn't say what driver it is that wants this capability. PC based
video and network cards want to do this but in those cases the i2c bus
is located on the card.

The embedded system I'm working could probably use this. The eeprom
holding the MAC address is on the RTC chip. There is no direct way
from the network driver to change the stored MAC address. I just set
it from uboot.

-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Fwd: [PATCH 2/3] I2C: at24: add kernel interface for reading/writing EEPROM

[David Brownell]

On Tuesday 26 August 2008, Jon Smirl wrote:
> 
> Another solution is call_userspace_helper. GregKH convinced me to
> change some things like this to small user space apps in the graphics
> code. Is is easy to do the read/write from sysfs.

Some of us don't like the implication that the relevant hardware
can't properly boot without initramfs/initrd tools.  Even if it
doesn't need anything the least bit fancy like a RAID for rootfs.


> It doesn't say what driver it is that wants this capability. PC based
> video and network cards want to do this but in those cases the i2c bus
> is located on the card.
> 
> The embedded system I'm working could probably use this. The eeprom
> holding the MAC address is on the RTC chip. There is no direct way
> from the network driver to change the stored MAC address. I just set
> it from uboot.

In this case, manufacturing assigns the MAC address by writing it into
part of the EEPROM.  Board-specific code can know which part, and then
this patch lets it read the address and hand it to the MAC driver.

Last I heard, the U-Boot policy was not to touch controllers that are
not required during boot.  Despite relatively common cases like needing
to set up MAC addresses ... maybe this patch of Kevin's can make it a
bunch easier to cope with that U-Boot policy.  :)

- Dave


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Fwd: [PATCH 2/3] I2C: at24: add kernel interface for reading/writing EEPROM

[Trent Piepho]

On Tue, 26 Aug 2008, David Brownell wrote:
> On Tuesday 26 August 2008, Jon Smirl wrote:
> > Another solution is call_userspace_helper. GregKH convinced me to
> > change some things like this to small user space apps in the graphics
> > code. Is is easy to do the read/write from sysfs.
>
> Some of us don't like the implication that the relevant hardware
> can't properly boot without initramfs/initrd tools.  Even if it
> doesn't need anything the least bit fancy like a RAID for rootfs.

Having the kernel use a userspace helper to read an eeprom via the kernel
eeprom driver does seem ridiculously round-about and overly complicated.

Lots of tv cards have eeproms with configuration information.  They use an
I2C driver called tveeprom that exports this:
int tveeprom_read(struct i2c_client *c, unsigned char *eedata, int len)

> Last I heard, the U-Boot policy was not to touch controllers that are
> not required during boot.  Despite relatively common cases like needing
> to set up MAC addresses ... maybe this patch of Kevin's can make it a
> bunch easier to cope with that U-Boot policy.  :)

U-boot can put the MAC address into a property of the MAC's device node in
the OF device tree (which u-boot passes to the kernel).  The Linux mac
driver can then use that to set the address.  U-boot doesn't actually need
to touch the mac.  It does need to read the eeprom then (which could store
u-boot environment).

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Fwd: [PATCH 2/3] I2C: at24: add kernel interface for reading/writing EEPROM

[David Brownell]

On Tuesday 26 August 2008, Trent Piepho wrote:
> Lots of tv cards have eeproms with configuration information.  They use an
> I2C driver called tveeprom that exports this:
> 
> int tveeprom_read(struct i2c_client *c, unsigned char *eedata, int len)

That presumes the driver somehow has access to that I2C client.

(Also, that it's an I2C EEPROM.  I keep wanting to have something
usable for SPI EEPROMs and NVRAM too.  But maybe nobody else cares.)

 
> > Last I heard, the U-Boot policy was not to touch controllers that are
> > not required during boot.  Despite relatively common cases like needing
> > to set up MAC addresses ... maybe this patch of Kevin's can make it a
> > bunch easier to cope with that U-Boot policy.  :)
> 
> U-boot can put the MAC address into a property of the MAC's device node in
> the OF device tree (which u-boot passes to the kernel).

Maybe with PowerPC it does.  Not on ARM.  ;)



_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Fwd: [PATCH 2/3] I2C: at24: add kernel interface for reading/writing EEPROM

[Jon Smirl]

On 8/27/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> On Tuesday 26 August 2008, Trent Piepho wrote:
>  > Lots of tv cards have eeproms with configuration information.  They use an
>  > I2C driver called tveeprom that exports this:
>  >
>  > int tveeprom_read(struct i2c_client *c, unsigned char *eedata, int len)
>
>
> That presumes the driver somehow has access to that I2C client.

How about using bus_find_device? PowerPC uses it to locate the i2c
device using pointers in the device tree.

+static int of_dev_node_match(struct device *dev, void *data)
+{
+        return dev->archdata.of_node == data;
+}
+
+struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
+{
+	struct device *dev;
+	
+	dev = bus_find_device(&i2c_bus_type, NULL, node,
+					 of_dev_node_match);
+	if (!dev)
+		return NULL;
+		
+	return to_i2c_client(dev);
+}
+EXPORT_SYMBOL(of_find_i2c_device_by_node);
+




>
>  (Also, that it's an I2C EEPROM.  I keep wanting to have something
>  usable for SPI EEPROMs and NVRAM too.  But maybe nobody else cares.)
>
>
>
>  > > Last I heard, the U-Boot policy was not to touch controllers that are
>  > > not required during boot.  Despite relatively common cases like needing
>  > > to set up MAC addresses ... maybe this patch of Kevin's can make it a
>  > > bunch easier to cope with that U-Boot policy.  :)
>  >
>  > U-boot can put the MAC address into a property of the MAC's device node in
>  > the OF device tree (which u-boot passes to the kernel).
>
>
> Maybe with PowerPC it does.  Not on ARM.  ;)
>
>
>


-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Fwd: [PATCH 2/3] I2C: at24: add kernel interface for reading/writing EEPROM

[David Brownell]

On Wednesday 27 August 2008, Jon Smirl wrote:
> On 8/27/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > On Tuesday 26 August 2008, Trent Piepho wrote:
> >  > Lots of tv cards have eeproms with configuration information.  They use an
> >  > I2C driver called tveeprom that exports this:
> >  >
> >  > int tveeprom_read(struct i2c_client *c, unsigned char *eedata, int len)
> >
> >
> > That presumes the driver somehow has access to that I2C client.
> 
> How about using bus_find_device? PowerPC uses it to locate the i2c
> device using pointers in the device tree.

Still making too many assumptions for my taste ... like using
I2C, and in this case also OF.  What I want is an approach that
can work with all the drivers with EEPROM and NVRAM support
that I've happenened on so far:

	drivers/i2c/chips/at24.c
	drivers/spi/chips/at25.c
	drivers/rtc/rtc-cmos.c
	drivers/rtc/rtc-ds1305.c
	drivers/rtc/rtc-ds1307.c
	drivers/rtc/rtc-ds1511.c
	drivers/rtc/rtc-ds1553.c
	drivers/rtc/rtc-ds1742.c
	drivers/rtc/rtc-m48t59.c
	drivers/rtc/rtc-stk17ta8.c

All those could easily export a (renamed) "struct at24_iface *" so
their persistent (and updatable) memory could be exported to kernel
code using the very same (simple) interface.

Discussion about how to use a different interface than what's shown
in the $SUBJECT patch seems to want to promote (a) each of those ten
drivers having a *different* interface exposed by EXPORT_SYMBOL(),
or else don't support in-kernel access at all, and (b) a bunch of
extra glue code to support it, like the OF-specific bits you showed
below and then going to the code that knows to use those bits to
get which device, and how to ask those devices for their data.

Moreover, IMO the basic question is still whether there is a good
reason not to build on the $SUBJECT patch.  (So far:  no.)

Sure it *could* be done differently -- especially if think (a) is
good but being able to reuse interfaces is bad -- but it's like
spending so much time choosing what color to paint the bikeshed
that the bikeshed itself never gets built...

- Dave

p.s. The only nontrivial technical issue I have with $PATCH is that
     "struct at24_iface" needs to cope better with "rmmod at24".
     An optional teardown(...) call would suffice.


> +static int of_dev_node_match(struct device *dev, void *data)
> +{
> +        return dev->archdata.of_node == data;
> +}
> +
> +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
> +{
> +       struct device *dev;
> +       
> +       dev = bus_find_device(&i2c_bus_type, NULL, node,
> +                                        of_dev_node_match);
> +       if (!dev)
> +               return NULL;
> +               
> +       return to_i2c_client(dev);
> +}
> +EXPORT_SYMBOL(of_find_i2c_device_by_node);
> +


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: Fwd: [PATCH 2/3] I2C: at24: add kernel interface for reading/writing EEPROM

[Jon Smirl]

On 8/28/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> On Wednesday 27 August 2008, Jon Smirl wrote:
>  > On 8/27/08, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
>  > > On Tuesday 26 August 2008, Trent Piepho wrote:
>  > >  > Lots of tv cards have eeproms with configuration information.  They use an
>  > >  > I2C driver called tveeprom that exports this:
>  > >  >
>  > >  > int tveeprom_read(struct i2c_client *c, unsigned char *eedata, int len)
>  > >
>  > >
>  > > That presumes the driver somehow has access to that I2C client.
>  >
>  > How about using bus_find_device? PowerPC uses it to locate the i2c
>  > device using pointers in the device tree.
>
>
> Still making too many assumptions for my taste ... like using
>  I2C, and in this case also OF.  What I want is an approach that
>  can work with all the drivers with EEPROM and NVRAM support
>  that I've happenened on so far:
>
>
>         drivers/i2c/chips/at24.c
>
>         drivers/spi/chips/at25.c
>         drivers/rtc/rtc-cmos.c
>         drivers/rtc/rtc-ds1305.c
>         drivers/rtc/rtc-ds1307.c
>         drivers/rtc/rtc-ds1511.c
>         drivers/rtc/rtc-ds1553.c
>         drivers/rtc/rtc-ds1742.c
>         drivers/rtc/rtc-m48t59.c
>         drivers/rtc/rtc-stk17ta8.c
>
>  All those could easily export a (renamed) "struct at24_iface *" so
>  their persistent (and updatable) memory could be exported to kernel
>  code using the very same (simple) interface.
>
>  Discussion about how to use a different interface than what's shown
>  in the $SUBJECT patch seems to want to promote (a) each of those ten
>  drivers having a *different* interface exposed by EXPORT_SYMBOL(),
>  or else don't support in-kernel access at all, and (b) a bunch of
>  extra glue code to support it, like the OF-specific bits you showed
>  below and then going to the code that knows to use those bits to
>  get which device, and how to ask those devices for their data.
>
>  Moreover, IMO the basic question is still whether there is a good
>  reason not to build on the $SUBJECT patch.  (So far:  no.)
>
>  Sure it *could* be done differently -- especially if think (a) is
>  good but being able to reuse interfaces is bad -- but it's like
>  spending so much time choosing what color to paint the bikeshed
>  that the bikeshed itself never gets built...


There are two parts to the puzzle.
1) A common way to export the interface. That can be addressed by
applying the technique in the initial patch.

2) How do the drivers find each other. Device trees already have a way
for drivers to find each other. This is an example of a audio codec
that lives on both the i2c and i2s bus. The codec-handle is used to
retrieve the tas0 node pointer. of_find_i2c_device_by_node then
searches the archdata for that pointer.

struct i2c_client *of_find_i2c_device_by_node(struct device_node
*node) could be generalized by having it search multiple buses for the
device. dev->archdata.of_node is just a cookie, it is only used to
match against. The pointer could be a void*.

I didn't like setup call part of the proposal. It is building another
way for drivers to find each other. How can you generate an equivalent
to the archdata.of_node cookie for other platforms? Another problem
with the setup callback, how do you tell identical devices apart?

		i2s@2200 { /* PSC2 in i2s mode */
			compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
			cell-index = <1>;
			reg = <0x2200 0x100>;
			interrupts = <0x2 0x2 0x0>;
			interrupt-parent = <&mpc5200_pic>;
			codec-handle = <&tas0>;
		};

		i2c@3d00 {
			#address-cells = <1>;
			#size-cells = <0>;
			compatible = "fsl,mpc5200b-i2c","fsl,mpc5200-i2c","fsl-i2c";
			cell-index = <0>;
			reg = <0x3d00 0x40>;
			interrupts = <0x2 0xf 0x0>;
			interrupt-parent = <&mpc5200_pic>;
			fsl5200-clocking;

			tas0:codec@1b {
				compatible = "ti,tas5504";
				reg = <0x1b>;
			};
			clock0:clock@68 {
				compatible = "maxim,max9485";
				reg = <0x68>;
			};
		};




>
>  - Dave
>
>  p.s. The only nontrivial technical issue I have with $PATCH is that
>      "struct at24_iface" needs to cope better with "rmmod at24".
>      An optional teardown(...) call would suffice.
>
>
>
>  > +static int of_dev_node_match(struct device *dev, void *data)
>  > +{
>  > +        return dev->archdata.of_node == data;
>  > +}
>  > +
>  > +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
>  > +{
>  > +       struct device *dev;
>  > +
>  > +       dev = bus_find_device(&i2c_bus_type, NULL, node,
>  > +                                        of_dev_node_match);
>  > +       if (!dev)
>  > +               return NULL;
>  > +
>  > +       return to_i2c_client(dev);
>  > +}
>  > +EXPORT_SYMBOL(of_find_i2c_device_by_node);
>  > +
>
>


-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c