[PATCH 1/3] mfd: allow mfd_cell association with device tree node

[PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Daniel Drake]

This allows a mfd_cell to be linked with a device tree node, which
then allows child drivers to have easy access to that handle.

Signed-off-by: Daniel Drake <dsd-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org>
---
 drivers/mfd/mfd-core.c   |    1 +
 include/linux/mfd/core.h |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)

I think this is what is being suggested at 
http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-September/008235.html

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 0902523..7d22dcd 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -87,6 +87,7 @@ static int mfd_add_device(struct device *parent, int id,
 		goto fail_device;
 
 	pdev->dev.parent = parent;
+	pdev->dev.of_node = cell->of_node;
 
 	if (cell->pdata_size) {
 		ret = platform_device_add_data(pdev,
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 4e76163..9b836f9 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -15,6 +15,7 @@
 #define MFD_CORE_H
 
 #include <linux/platform_device.h>
+#include <linux/of.h>
 
 /*
  * This struct describes the MFD part ("cell").
@@ -37,6 +38,9 @@ struct mfd_cell {
 	void			*platform_data;
 	size_t			pdata_size;
 
+	/* association with device tree node (optional) */
+	struct device_node	*of_node;
+
 	/*
 	 * These resources can be specified relative to the parent device.
 	 * For accessing hardware you should use resources from the platform dev
-- 
1.7.6.2

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Mark Brown]

On Wed, Sep 21, 2011 at 01:01:48PM +0100, Daniel Drake wrote:

> @@ -37,6 +38,9 @@ struct mfd_cell {
>  	void			*platform_data;
>  	size_t			pdata_size;
>  
> +	/* association with device tree node (optional) */
> +	struct device_node	*of_node;
> +

This doesn't seem great as it means that the mfd_cells can't be static
data, they have to be allocated per device.  Why can't the MFD cells get
at the OF node by using the parent device in the same way as they
(mostly) get platform data at the minute?

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Daniel Drake]

On Wed, Sep 21, 2011 at 1:49 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Wed, Sep 21, 2011 at 01:01:48PM +0100, Daniel Drake wrote:
>
>> @@ -37,6 +38,9 @@ struct mfd_cell {
>>       void                    *platform_data;
>>       size_t                  pdata_size;
>>
>> +     /* association with device tree node (optional) */
>> +     struct device_node      *of_node;
>> +
>
> This doesn't seem great as it means that the mfd_cells can't be static
> data, they have to be allocated per device.  Why can't the MFD cells get
> at the OF node by using the parent device in the same way as they
> (mostly) get platform data at the minute?

Thanks for reviewing.
The data can still be static, not needing allocation, it just has to
be modified at runtime. See patch 2.

Not sure how the MFD cells could get at the OF node by using the
parent device - if the parent device had a OF node, wouldn't this
correspond to the parent instead of the child? Also, as far as I can
see, platform data is passed to the child in exactly the same way - by
including it in the mfd_cell definition - see mfd_add_device():

	if (cell->pdata_size) {
		ret = platform_device_add_data(pdev,
					cell->platform_data, cell->pdata_size);
		if (ret)
			goto fail_res;
	}

Daniel

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Mark Brown]

On Wed, Sep 21, 2011 at 02:02:43PM +0100, Daniel Drake wrote:

> Thanks for reviewing.
> The data can still be static, not needing allocation, it just has to
> be modified at runtime. See patch 2.

Right, but that means that you need to take a copy before using it
otherwise two devices of the same type might get into a fight with each
other.

> Not sure how the MFD cells could get at the OF node by using the
> parent device - if the parent device had a OF node, wouldn't this
> correspond to the parent instead of the child? Also, as far as I can

Well, obviously.  But then with a lot of MFDs (including this one, the
GPIO driver is entirely specific to the parent) it's not clear that we
should be splitting the device up in the device tree in the first place
- our use of MFDs is a Linux implementation detail.  If the child is
specific to the parent it can cooperate with the parent device happily.

My suspicion is that for device tree in cases where the MFD really is
totally independent of the parent we shouldn't need explicit MFD code to
instantiate the child at all any more in the same way that we should be
avoiding this for the SoCs.

> see, platform data is passed to the child in exactly the same way - by
> including it in the mfd_cell definition - see mfd_add_device():

Indeed, I have the same concerns there.  

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Daniel Drake]

[-- Attachment #1: Type: text/plain, Size: 2878 bytes --]

On Wed, Sep 21, 2011 at 2:16 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
>> Not sure how the MFD cells could get at the OF node by using the
>> parent device - if the parent device had a OF node, wouldn't this
>> correspond to the parent instead of the child? Also, as far as I can
>
> Well, obviously.  But then with a lot of MFDs (including this one, the
> GPIO driver is entirely specific to the parent) it's not clear that we
> should be splitting the device up in the device tree in the first place
> - our use of MFDs is a Linux implementation detail.  If the child is
> specific to the parent it can cooperate with the parent device happily.
>
> My suspicion is that for device tree in cases where the MFD really is
> totally independent of the parent we shouldn't need explicit MFD code to
> instantiate the child at all any more in the same way that we should be
> avoiding this for the SoCs.

The VX855 is somewhat a SoC if you ignore the fact that the processor
is separate.
The software-visible architecture is somewhat messy and may hide this however.

The fact that it exposes some things as individual PCI devices and
some as behind the ISA bridge (which the mfd driver latches onto) is
probably just there for legacy reasons. Once you get behind that
cover, you see that the VX855 register space is really quite
disorganised with individual bits within the same byte of
configuration space used for vastly different system components (e.g.
gpio bits mixed with acpi and audio stuff in the same byte) and you
get the feeling that this really is a lot of hardware combined. So the
discussion of "independence" is particularly tricky in this case.

Anyway, I think I have come up with an approach on these lines. The
mfd driver latches onto the ISA bridge, and the documented
architecture suggests that a whole bunch of stuff comes off this:
8042, interrupt controller, dma controller, rtc, serial, timer, gpio,
spi, ...

We already have this accurately laid out in the device tree hierarchy:
/pci/isa/ has a child node for each of the above mentioned things
(e.g. /pci/isa/timer , /pci/isa/rtc and so on)

So, the /pci/isa node nicely matches the vx855-mfd driver, so we can
assign its of_node to that. Then, the vx855-gpio driver can consult
the parent and then look at its children in order to find the of_node
for the gpio controller. I think this nicely crosses with Mark's
request that the ability to have constant mfd definitions isn't broken
any more than it is already, and with Grant's preference that the
parent resource has some contribution in helping the child gpio
controller driver find its of_node. How does the attached patch look?

Grant, what do you think of this, and the rest of the discussion so far?

Daniel (just trying to make our gpio-based HDD LED go blinky blink...)

[-- Attachment #2: vx855.txt --]
[-- Type: text/plain, Size: 2261 bytes --]

diff --git a/drivers/gpio/gpio-vx855.c b/drivers/gpio/gpio-vx855.c
index ef5aabd..fd24213 100644
--- a/drivers/gpio/gpio-vx855.c
+++ b/drivers/gpio/gpio-vx855.c
@@ -31,6 +31,7 @@
 #include <linux/platform_device.h>
 #include <linux/pci.h>
 #include <linux/io.h>
+#include <linux/of.h>
 
 #define MODULE_NAME "vx855_gpio"
 
@@ -201,7 +202,27 @@ static const char *vx855gpio_names[NR_VX855_GP] = {
 	"VX855_GPIO12", "VX855_GPIO13", "VX855_GPIO14"
 };
 
-static void vx855gpio_gpio_setup(struct vx855_gpio *vg)
+static void vx855gpio_set_of_node(struct platform_device *pdev,
+				  struct gpio_chip *c)
+{
+#ifdef CONFIG_OF
+	struct device_node *parent_node = pdev->dev.parent->of_node;
+	struct device_node *child;
+
+	if (!parent_node)
+		return;
+
+	for_each_child_of_node(parent_node, child) {
+		if (of_device_is_compatible(child, "via,vx855-gpio")) {
+			c->of_node = child;
+			break;
+		}
+	}
+#endif
+}
+
+static void vx855gpio_gpio_setup(struct vx855_gpio *vg,
+				 struct platform_device *pdev)
 {
 	struct gpio_chip *c = &vg->gpio;
 
@@ -216,6 +237,7 @@ static void vx855gpio_gpio_setup(struct vx855_gpio *vg)
 	c->ngpio = NR_VX855_GP;
 	c->can_sleep = 0;
 	c->names = vx855gpio_names;
+	vx855gpio_set_of_node(pdev, c);
 }
 
 /* This platform device is ordinarily registered by the vx855 mfd driver */
@@ -264,7 +286,7 @@ static __devinit int vx855gpio_probe(struct platform_device *pdev)
 	else
 		vg->gpo_reserved = true;
 
-	vx855gpio_gpio_setup(vg);
+	vx855gpio_gpio_setup(vg, pdev);
 
 	ret = gpiochip_add(&vg->gpio);
 	if (ret) {
diff --git a/drivers/mfd/vx855.c b/drivers/mfd/vx855.c
index d698703..aee9a5f 100644
--- a/drivers/mfd/vx855.c
+++ b/drivers/mfd/vx855.c
@@ -30,6 +30,7 @@
 #include <linux/platform_device.h>
 #include <linux/pci.h>
 #include <linux/mfd/core.h>
+#include <linux/of.h>
 
 /* offset into pci config space indicating the 16bit register containing
  * the power management IO space base */
@@ -90,6 +91,10 @@ static __devinit int vx855_probe(struct pci_dev *pdev,
 		goto out;
 	}
 
+#ifdef CONFIG_OF
+	pdev->dev.of_node = of_find_compatible_node(NULL, NULL, "via,vx855-isa");
+#endif
+
 	/* mask out the lowest seven bits, as they are always zero, but
 	 * hardware returns them as 0x01 */
 	gpio_io_offset &= 0xff80;

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Grant Likely]

On Tue, Sep 27, 2011 at 03:44:56PM +0100, Daniel Drake wrote:
> On Wed, Sep 21, 2011 at 2:16 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
> >> Not sure how the MFD cells could get at the OF node by using the
> >> parent device - if the parent device had a OF node, wouldn't this
> >> correspond to the parent instead of the child? Also, as far as I can
> >
> > Well, obviously.  But then with a lot of MFDs (including this one, the
> > GPIO driver is entirely specific to the parent) it's not clear that we
> > should be splitting the device up in the device tree in the first place
> > - our use of MFDs is a Linux implementation detail.  If the child is
> > specific to the parent it can cooperate with the parent device happily.
> >
> > My suspicion is that for device tree in cases where the MFD really is
> > totally independent of the parent we shouldn't need explicit MFD code to
> > instantiate the child at all any more in the same way that we should be
> > avoiding this for the SoCs.

Right. MFD seems to be most useful when IP blocks are used in multiple
places and can be instantiated by multiple parents.  Sometimes a
driver really should just register the interfaces that the device
provides without the MFD framework.

> The VX855 is somewhat a SoC if you ignore the fact that the processor
> is separate.
> The software-visible architecture is somewhat messy and may hide this however.
> 
> The fact that it exposes some things as individual PCI devices and
> some as behind the ISA bridge (which the mfd driver latches onto) is
> probably just there for legacy reasons. Once you get behind that
> cover, you see that the VX855 register space is really quite
> disorganised with individual bits within the same byte of
> configuration space used for vastly different system components (e.g.
> gpio bits mixed with acpi and audio stuff in the same byte) and you
> get the feeling that this really is a lot of hardware combined. So the
> discussion of "independence" is particularly tricky in this case.
> 
> Anyway, I think I have come up with an approach on these lines. The
> mfd driver latches onto the ISA bridge, and the documented
> architecture suggests that a whole bunch of stuff comes off this:
> 8042, interrupt controller, dma controller, rtc, serial, timer, gpio,
> spi, ...
> 
> We already have this accurately laid out in the device tree hierarchy:
> /pci/isa/ has a child node for each of the above mentioned things
> (e.g. /pci/isa/timer , /pci/isa/rtc and so on)
> 
> So, the /pci/isa node nicely matches the vx855-mfd driver, so we can
> assign its of_node to that. Then, the vx855-gpio driver can consult
> the parent and then look at its children in order to find the of_node
> for the gpio controller. I think this nicely crosses with Mark's
> request that the ability to have constant mfd definitions isn't broken
> any more than it is already, and with Grant's preference that the
> parent resource has some contribution in helping the child gpio
> controller driver find its of_node. How does the attached patch look?
> 
> Grant, what do you think of this, and the rest of the discussion so far?

MFD bindings really need to be designed on a device-by-device basis.
Sometimes it will make more sense to have a single node, sometimes it
needs to be split up.  There is no hard rule here.

What is a hard rule is that the code creating the children needs to
know what the binding is and populate the child's of_node
appropriately.  Similarly, the child driver needs to know something
about the kinds of nodes it will be passed (can be tested by the
compatible property).

One warning however, the current platform_bus_type DT code won't deal
well with multiple platform_devices pointing to the same of_node.  It
may result in the wrong driver getting bound since the probe code
first looks at .compatible to decide which driver to probe.  If 3
separate devices are intended to bind to 3 different drivers, you'll
probably end up with all three matching against the first one unless
the drivers have some additional explicit test.

g.

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Mark Brown]

On Tue, Sep 27, 2011 at 09:05:55AM -0600, Grant Likely wrote:
> On Tue, Sep 27, 2011 at 03:44:56PM +0100, Daniel Drake wrote:
> > On Wed, Sep 21, 2011 at 2:16 PM, Mark Brown

> > > My suspicion is that for device tree in cases where the MFD really is
> > > totally independent of the parent we shouldn't need explicit MFD code to
> > > instantiate the child at all any more in the same way that we should be
> > > avoiding this for the SoCs.

> Right. MFD seems to be most useful when IP blocks are used in multiple
> places and can be instantiated by multiple parents.  Sometimes a
> driver really should just register the interfaces that the device
> provides without the MFD framework.

Well, if you need a bunch of platform devices it's a good way of
creating them especially in the current world.  There's also generally
some core logic, for example routing interrupt lines, that can usefully
be provided by the MFD part of the driver.

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Daniel Drake]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

On Tue, Sep 27, 2011 at 4:05 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> What is a hard rule is that the code creating the children needs to
> know what the binding is and populate the child's of_node
> appropriately.  Similarly, the child driver needs to know something
> about the kinds of nodes it will be passed (can be tested by the
> compatible property).

Thanks for your input.
Here is an updated patch which takes account of this hard rule.

It uses the dynamic mfd cell creation like before. Mark isn't keen on
this, but its how the driver works already, so I don't think it should
be a blocker. I don't know how else we can make the mfd framework meet
Grant's hard rule.

However, it does take Mark's suggestion into account that the mfd
should have some clear representation in the device tree. For us it
already did have (naturally), but this wasn't reflected in my earlier
patch. It is now - the location of the vx855-gpio node is based on
finding the mfd node and going from there.

Comments?
It obviously needs to be tested and split up into individual patches -
I'll do that shortly if this doesn't get shot down.

cheers
Daniel

[-- Attachment #2: vx855-2.txt --]
[-- Type: text/plain, Size: 3286 bytes --]

diff --git a/drivers/gpio/gpio-vx855.c b/drivers/gpio/gpio-vx855.c
index ef5aabd..2c300c9 100644
--- a/drivers/gpio/gpio-vx855.c
+++ b/drivers/gpio/gpio-vx855.c
@@ -201,7 +201,8 @@ static const char *vx855gpio_names[NR_VX855_GP] = {
 	"VX855_GPIO12", "VX855_GPIO13", "VX855_GPIO14"
 };
 
-static void vx855gpio_gpio_setup(struct vx855_gpio *vg)
+static void vx855gpio_gpio_setup(struct vx855_gpio *vg,
+				 struct platform_device *pdev)
 {
 	struct gpio_chip *c = &vg->gpio;
 
@@ -216,6 +217,10 @@ static void vx855gpio_gpio_setup(struct vx855_gpio *vg)
 	c->ngpio = NR_VX855_GP;
 	c->can_sleep = 0;
 	c->names = vx855gpio_names;
+
+#ifdef CONFIG_OF_GPIO
+	c->of_node = pdev->dev.of_node;
+#endif
 }
 
 /* This platform device is ordinarily registered by the vx855 mfd driver */
@@ -264,7 +269,7 @@ static __devinit int vx855gpio_probe(struct platform_device *pdev)
 	else
 		vg->gpo_reserved = true;
 
-	vx855gpio_gpio_setup(vg);
+	vx855gpio_gpio_setup(vg, pdev);
 
 	ret = gpiochip_add(&vg->gpio);
 	if (ret) {
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 0902523..7d22dcd 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -87,6 +87,7 @@ static int mfd_add_device(struct device *parent, int id,
 		goto fail_device;
 
 	pdev->dev.parent = parent;
+	pdev->dev.of_node = cell->of_node;
 
 	if (cell->pdata_size) {
 		ret = platform_device_add_data(pdev,
diff --git a/drivers/mfd/vx855.c b/drivers/mfd/vx855.c
index d698703..d223840 100644
--- a/drivers/mfd/vx855.c
+++ b/drivers/mfd/vx855.c
@@ -30,6 +30,7 @@
 #include <linux/platform_device.h>
 #include <linux/pci.h>
 #include <linux/mfd/core.h>
+#include <linux/of.h>
 
 /* offset into pci config space indicating the 16bit register containing
  * the power management IO space base */
@@ -72,6 +73,27 @@ static struct mfd_cell vx855_cells[] = {
 	},
 };
 
+static __devinit void vx855_set_of_nodes(struct pci_dev *pdev)
+{
+#ifdef CONFIG_OF
+	struct device_node *isa_node;
+	struct device_node *child;
+
+	isa_node = of_find_compatible_node(NULL, NULL, "via,vx855-isa");
+	pdev->dev.of_node = isa_node;
+
+	if (!isa_node)
+		return;
+
+	for_each_child_of_node(isa_node, child) {
+		if (of_device_is_compatible(child, "via,vx855-gpio")) {
+			vx855_cells[0].of_node = child;
+			break;
+		}
+	}
+#endif
+}
+
 static __devinit int vx855_probe(struct pci_dev *pdev,
 				 const struct pci_device_id *id)
 {
@@ -90,6 +112,8 @@ static __devinit int vx855_probe(struct pci_dev *pdev,
 		goto out;
 	}
 
+	vx855_set_of_nodes(pdev);
+
 	/* mask out the lowest seven bits, as they are always zero, but
 	 * hardware returns them as 0x01 */
 	gpio_io_offset &= 0xff80;
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 4e76163..9b836f9 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -15,6 +15,7 @@
 #define MFD_CORE_H
 
 #include <linux/platform_device.h>
+#include <linux/of.h>
 
 /*
  * This struct describes the MFD part ("cell").
@@ -37,6 +38,9 @@ struct mfd_cell {
 	void			*platform_data;
 	size_t			pdata_size;
 
+	/* association with device tree node (optional) */
+	struct device_node	*of_node;
+
 	/*
 	 * These resources can be specified relative to the parent device.
 	 * For accessing hardware you should use resources from the platform dev

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Mark Brown]

On Tue, Sep 27, 2011 at 05:44:04PM +0100, Daniel Drake wrote:

> However, it does take Mark's suggestion into account that the mfd
> should have some clear representation in the device tree. For us it
> already did have (naturally), but this wasn't reflected in my earlier
> patch. It is now - the location of the vx855-gpio node is based on
> finding the mfd node and going from there.

Why not just kmemdup() the template you're using rather than modifying
it in place?

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Daniel Drake]

On Tue, Sep 27, 2011 at 7:14 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Sep 27, 2011 at 05:44:04PM +0100, Daniel Drake wrote:
>
>> However, it does take Mark's suggestion into account that the mfd
>> should have some clear representation in the device tree. For us it
>> already did have (naturally), but this wasn't reflected in my earlier
>> patch. It is now - the location of the vx855-gpio node is based on
>> finding the mfd node and going from there.
>
> Why not just kmemdup() the template you're using rather than modifying
> it in place?

Could do, but is there any point in this case? I'm not seeing it. The
"template" is already being modified in this way in the current driver
as well.

Daniel

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Mark Brown]

On Tue, Sep 27, 2011 at 07:25:03PM +0100, Daniel Drake wrote:
> On Tue, Sep 27, 2011 at 7:14 PM, Mark Brown

> > Why not just kmemdup() the template you're using rather than modifying
> > it in place?

> Could do, but is there any point in this case? I'm not seeing it. The
> "template" is already being modified in this way in the current driver
> as well.

That seems like a bug that just happened to get noticed while reviewing
this change, though...

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Daniel Drake]

On Tue, Sep 27, 2011 at 7:26 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> That seems like a bug that just happened to get noticed while reviewing
> this change, though...

Sorry if I'm missing something obvious but I'm not seeing the issue.
What exactly is this bug and what effects could it cause?

Thanks,
Daniel

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Mark Brown]

On Tue, Sep 27, 2011 at 07:28:40PM +0100, Daniel Drake wrote:
> On Tue, Sep 27, 2011 at 7:26 PM, Mark Brown

> > That seems like a bug that just happened to get noticed while reviewing
> > this change, though...

> Sorry if I'm missing something obvious but I'm not seeing the issue.
> What exactly is this bug and what effects could it cause?

You're modifying global data which should really be const and is shared
between multiple devices in place.  Probably you'll not notice any
practical effects, especially if you don't happen to have multiple
devices in the same system.

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Daniel Drake]

On Tue, Sep 27, 2011 at 7:38 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> You're modifying global data which should really be const and is shared
> between multiple devices in place.  Probably you'll not notice any
> practical effects, especially if you don't happen to have multiple
> devices in the same system.

I see. In this case, it would be impossible to have two VX855s in the
same system, and if you did, you would have bigger problems. In this
context what we're doing is safe.

Thanks,
Daniel

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Mark Brown]

On Wed, Sep 28, 2011 at 10:07:43AM +0100, Daniel Drake wrote:
> On Tue, Sep 27, 2011 at 7:38 PM, Mark Brown

> > You're modifying global data which should really be const and is shared
> > between multiple devices in place.  Probably you'll not notice any
> > practical effects, especially if you don't happen to have multiple
> > devices in the same system.

> I see. In this case, it would be impossible to have two VX855s in the
> same system, and if you did, you would have bigger problems. In this
> context what we're doing is safe.

Right, but your modification was to the MFD core so it's going to affect
other devices...

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Daniel Drake]

On Wed, Sep 28, 2011 at 1:31 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> Right, but your modification was to the MFD core so it's going to affect
> other devices...

But only when they start using the newly added functionality. At this
point, it affects nobody except vx855.

Grant, any chance I could have your comments on the latest patches?
http://marc.info/?l=linux-kernel&m=131720353308737&w=2
http://marc.info/?l=linux-kernel&m=131720341808574&w=2
http://marc.info/?l=linux-kernel&m=131720093305636&w=2

Thanks,
Daniel

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Mark Brown]

On Mon, Oct 03, 2011 at 09:40:10AM +0100, Daniel Drake wrote:
> On Wed, Sep 28, 2011 at 1:31 PM, Mark Brown
> > Right, but your modification was to the MFD core so it's going to affect
> > other devices...

> But only when they start using the newly added functionality. At this
> point, it affects nobody except vx855.

I'd really expect that if we're adding stuff to the framework then it
should be suitable for random drivers to use.  To be honest I don't
really understand why you're changing the framework at all here, the
child driver is entirely specific to the parent as far as I can see.

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Daniel Drake]

On Mon, Oct 3, 2011 at 11:28 AM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> I'd really expect that if we're adding stuff to the framework then it
> should be suitable for random drivers to use.

It is suitable. If other drivers would otherwise run into the data
reuse problem you have described, they can use the kmemdup solution
you have described.

> To be honest I don't
> really understand why you're changing the framework at all here, the
> child driver is entirely specific to the parent as far as I can see.

Because I'm trying to get devicetree-driven HDD LED support driven,
and Grant stated that doing it this way is a hard rule:

"What is a hard rule is that the code creating the children needs to
know what the binding is and populate the child's of_node
appropriately."

I also confirmed that extending the mfd_cell layer is exactly what
Grant was suggesting:
http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-September/008480.html

And Grant is undoubtedly a source of authority on device tree
implementation stuff.

So I seem to be stuck between two people giving me conflicting
requirements for merge of my work (which commenced in July, and has
already seen several discussions and rounds of patches). Any help
appreciated - I'm just trying to make a HDD LED blink.

Thanks,
Daniel

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Mark Brown]

On Mon, Oct 03, 2011 at 11:39:47AM +0100, Daniel Drake wrote:
> On Mon, Oct 3, 2011 at 11:28 AM, Mark Brown

> > I'd really expect that if we're adding stuff to the framework then it
> > should be suitable for random drivers to use.

> It is suitable. If other drivers would otherwise run into the data
> reuse problem you have described, they can use the kmemdup solution
> you have described.

This seems the wrong way round - we're working

> > To be honest I don't
> > really understand why you're changing the framework at all here, the
> > child driver is entirely specific to the parent as far as I can see.

> Because I'm trying to get devicetree-driven HDD LED support driven,
> and Grant stated that doing it this way is a hard rule:

> "What is a hard rule is that the code creating the children needs to
> know what the binding is and populate the child's of_node
> appropriately."

> I also confirmed that extending the mfd_cell layer is exactly what
> Grant was suggesting:
> http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-September/008480.html

That's in the context of adding a binding for the vx855 GPIO module
that's distinct from the binding for the rest of the chip.  It's not
massively clear to me that this is a good idea.

> So I seem to be stuck between two people giving me conflicting
> requirements for merge of my work (which commenced in July, and has
> already seen several discussions and rounds of patches). Any help
> appreciated - I'm just trying to make a HDD LED blink.

It seems to me like either the IP block is heavily dependant on the core
and shouldn't be split out in the device tree at all (but should instead
be part of the core node) or the IP block is very isolated from the core
(in which case we should just be able to instantiate the device from the
device tree without using explict code in the core driver).

This all feels like there's some abstraction violation going on.

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Daniel Drake]

On Mon, Oct 3, 2011 at 1:16 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> It seems to me like either the IP block is heavily dependant on the core
> and shouldn't be split out in the device tree at all (but should instead
> be part of the core node) or the IP block is very isolated from the core
> (in which case we should just be able to instantiate the device from the
> device tree without using explict code in the core driver).
>
> This all feels like there's some abstraction violation going on.

I guess it is a matter of opinion. To me, the abstraction is sensible
and representative of the hardware.

The gpio controller (and several other components) are abstracted at
the hardware level behind an ISA bridge.
In the device tree, we have the ISA bridge represented as /pci/isa.

Then we have various child nodes of that ISA bridge, such as the gpio
controller at /pci/isa/gpios or the timer controller at /pci/isa/timer

This also matches the way that the hardware is described by VIA in the
specs, and it matches the way that Linux has its own device hierachy
laid out (i.e. the ISA bridge driven by the mfd driver, which then
spawns off a child device for the GPIO controller).

It would not be possible to fold all of the isa bridge child
components into the same device tree node without dealing with various
namespace collisions.

Grant, could we have your opinion on whether this is a good
abstraction or not? and generally how we go forward.

Thanks,
Daniel

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Mark Brown]

On Mon, Oct 03, 2011 at 01:30:15PM +0100, Daniel Drake wrote:
> On Mon, Oct 3, 2011 at 1:16 PM, Mark Brown

> > It seems to me like either the IP block is heavily dependant on the core
> > and shouldn't be split out in the device tree at all (but should instead
> > be part of the core node) or the IP block is very isolated from the core
> > (in which case we should just be able to instantiate the device from the
> > device tree without using explict code in the core driver).

> > This all feels like there's some abstraction violation going on.

> I guess it is a matter of opinion. To me, the abstraction is sensible
> and representative of the hardware.

> This also matches the way that the hardware is described by VIA in the
> specs, and it matches the way that Linux has its own device hierachy
> laid out (i.e. the ISA bridge driven by the mfd driver, which then
> spawns off a child device for the GPIO controller).

> It would not be possible to fold all of the isa bridge child
> components into the same device tree node without dealing with various
> namespace collisions.

So, I made two suggestions above and it sounds like you want the second
one but you've only responded to the first one without commenting on the
second.  My second suggestion was that if the block is sufficiently
isoltated from the core we should be able instantiate it from the device
tree without requiring explicit code in the core driver.

Re: [PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Daniel Drake]

On Mon, Oct 3, 2011 at 1:40 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> So, I made two suggestions above and it sounds like you want the second
> one but you've only responded to the first one without commenting on the
> second.  My second suggestion was that if the block is sufficiently
> isoltated from the core we should be able instantiate it from the device
> tree without requiring explicit code in the core driver.

Sorry, hadn't quite grasped what you meant there. I understand now.

It is an interesting idea but leaves me with some questions/problems:

The GPIO controller needs to find its register space by looking at the
PCI device (the ISA bridge). So probing it independently could maybe
be viewed by some as a hierarchy violation as it would have to then
hunt around for its PCI dev.

According to Grant's hard rule, the parent device needs to be the
thing that passes the of_node to the child.
So we would still need a driver for the parent ISA bridge
instantiating the child GPIO controller. Wouldn't that bring us
straight back to the same problem (that the "core" needs code to
instantiate the child)?

Also, not an argument against the direction, but an outstanding
problem that would need to be resolved: the x86 device tree
implementation doesn't seem to follow Grant's design for how things
should work (or maybe I misunderstood something), so work would be
needed there first. See the unfinished discussion at
http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-July/006853.html

Thanks,
Daniel

[PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Daniel Drake]

This allows a mfd_cell to be linked with a device tree node, which
then allows child drivers to have easy access to that handle.

Signed-off-by: Daniel Drake <dsd-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org>
---
 drivers/mfd/mfd-core.c   |    1 +
 include/linux/mfd/core.h |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 0902523..7d22dcd 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -87,6 +87,7 @@ static int mfd_add_device(struct device *parent, int id,
 		goto fail_device;
 
 	pdev->dev.parent = parent;
+	pdev->dev.of_node = cell->of_node;
 
 	if (cell->pdata_size) {
 		ret = platform_device_add_data(pdev,
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 4e76163..9b836f9 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -15,6 +15,7 @@
 #define MFD_CORE_H
 
 #include <linux/platform_device.h>
+#include <linux/of.h>
 
 /*
  * This struct describes the MFD part ("cell").
@@ -37,6 +38,9 @@ struct mfd_cell {
 	void			*platform_data;
 	size_t			pdata_size;
 
+	/* association with device tree node (optional) */
+	struct device_node	*of_node;
+
 	/*
 	 * These resources can be specified relative to the parent device.
 	 * For accessing hardware you should use resources from the platform dev
-- 
1.7.6.2

[PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Daniel Drake]

This allows a mfd_cell to be linked with a device tree node, which
then allows child drivers to have easy access to that handle.

Signed-off-by: Daniel Drake <dsd@laptop.org>
---
 drivers/mfd/mfd-core.c   |    1 +
 include/linux/mfd/core.h |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 0902523..7d22dcd 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -87,6 +87,7 @@ static int mfd_add_device(struct device *parent, int id,
 		goto fail_device;
 
 	pdev->dev.parent = parent;
+	pdev->dev.of_node = cell->of_node;
 
 	if (cell->pdata_size) {
 		ret = platform_device_add_data(pdev,
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 4e76163..9b836f9 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -15,6 +15,7 @@
 #define MFD_CORE_H
 
 #include <linux/platform_device.h>
+#include <linux/of.h>
 
 /*
  * This struct describes the MFD part ("cell").
@@ -37,6 +38,9 @@ struct mfd_cell {
 	void			*platform_data;
 	size_t			pdata_size;
 
+	/* association with device tree node (optional) */
+	struct device_node	*of_node;
+
 	/*
 	 * These resources can be specified relative to the parent device.
 	 * For accessing hardware you should use resources from the platform dev
-- 
1.7.6.2

[PATCH 1/3] mfd: allow mfd_cell association with device tree node

[Daniel Drake]

This allows a mfd_cell to be linked with a device tree node, which
then allows child drivers to have easy access to that handle.

Signed-off-by: Daniel Drake <dsd@laptop.org>
---
 drivers/mfd/mfd-core.c   |    1 +
 include/linux/mfd/core.h |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)

I think this is what is being suggested at 
http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-September/008235.html

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 0902523..7d22dcd 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -87,6 +87,7 @@ static int mfd_add_device(struct device *parent, int id,
 		goto fail_device;
 
 	pdev->dev.parent = parent;
+	pdev->dev.of_node = cell->of_node;
 
 	if (cell->pdata_size) {
 		ret = platform_device_add_data(pdev,
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 4e76163..9b836f9 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -15,6 +15,7 @@
 #define MFD_CORE_H
 
 #include <linux/platform_device.h>
+#include <linux/of.h>
 
 /*
  * This struct describes the MFD part ("cell").
@@ -37,6 +38,9 @@ struct mfd_cell {
 	void			*platform_data;
 	size_t			pdata_size;
 
+	/* association with device tree node (optional) */
+	struct device_node	*of_node;
+
 	/*
 	 * These resources can be specified relative to the parent device.
 	 * For accessing hardware you should use resources from the platform dev
-- 
1.7.6.2