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

[Daniel Drake]

On Wed, Sep 21, 2011 at 2:16 PM, Mark Brown
<broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> 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...)
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;