Input: twl4030-vibra: fix sibling-node lookup

Fix child-node lookup during probe, which ended up searching the whole device tree depth-first starting at parent rather than just matching on its children.

Later sanity checks on node properties (which would likely be missing) should prevent this from causing much trouble however, especially as the original premature free of the parent node has already been fixed separately (but that "fix" was apparently never backported to stable).

Fixes e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")

Fixes c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")

Cc stable <[email protected]> # 3.6

Cc Peter Ujfalusi <[email protected]>

Cc H. Nikolaus Schaller <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/input/misc/twl6040-vibra.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/input/misc/twl6040-vibra.c +0 -0

index 5690eb7ff954..15e0d352c4cc 100644

Hi Johan,

Quoted 11 lines

Am 11.11.2017 um 16:43 schrieb Johan Hovold <[email protected]>:

Fix child-node lookup during probe, which ended up searching the whole device tree depth-first starting at parent rather than just matching on its children.

Later sanity checks on node properties (which would likely be missing) should prevent this from causing much trouble however, especially as the original premature free of the parent node has already been fixed separately (but that "fix" was apparently never backported to stable).

Quoted 6 lines

Fixes e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")

Fixes c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")

Cc stable <[email protected]> # 3.6

Cc Peter Ujfalusi <[email protected]>

Cc H. Nikolaus Schaller <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

Tested-by H. Nikolaus Schaller <[email protected]> (on Pyra OMAP5 hardware)

I also tested your twl4030-vibra patch on GTA04.

No regressions observed.

Fix child-node lookup during probe, which ended up searching the whole device tree depth-first starting at parent rather than just matching on its children.

Later sanity checks on node properties (which would likely be missing) should prevent this from causing much trouble however, especially as the original premature free of the parent node has already been fixed separately (but that "fix" was apparently never backported to stable).

Fixes e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")

Fixes c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")

Cc stable <[email protected]> # 3.6

Cc Peter Ujfalusi <[email protected]>

Cc H. Nikolaus Schaller <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/input/misc/twl6040-vibra.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/input/misc/twl6040-vibra.c +0 -0

index 5690eb7ff954..15e0d352c4cc 100644

Quoted 9 lines · Johan Hovold

Fix child-node lookup during probe, which ended up searching the whole device tree depth-first starting at parent rather than just matching on its children.

Later sanity checks on node properties (which would likely be missing) should prevent this from causing much trouble however, especially as the original premature free of the parent node has already been fixed separately (but that "fix" was apparently never backported to stable).

Quoted 3 lines

Fixes e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")

Fixes c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")

Cc stable <[email protected]> # 3.6

Note that of_get_child_by_name() is not present in 3.6, this was the reason the original implementation was using of_find_node_by_name()

Am 13.11.2017 um 15:19 schrieb Johan Hovold <[email protected]>:

Quoted 14 lines · Peter Ujfalusi

Quoted 12 lines · Johan Hovold

Fix child-node lookup during probe, which ended up searching the whole device tree depth-first starting at parent rather than just matching on its children.

Later sanity checks on node properties (which would likely be missing) should prevent this from causing much trouble however, especially as the original premature free of the parent node has already been fixed separately (but that "fix" was apparently never backported to stable).

Fixes e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")

Fixes c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")

Cc stable <[email protected]> # 3.6

Quoted 2 lines

Note that of_get_child_by_name() is not present in 3.6, this was the reason the original implementation was using of_find_node_by_name()

Ah, ok. So the lookup would have needed to be open-coded using for_each_child_of_node() back then, or if anyone wants to backport any of these fixes to before v3.7.

Hi,

Quoted 18 lines

Am 13.11.2017 um 15:19 schrieb Johan Hovold <[email protected]>:

Quoted 14 lines · Peter Ujfalusi

Quoted 12 lines · Johan Hovold

Fix child-node lookup during probe, which ended up searching the whole device tree depth-first starting at parent rather than just matching on its children.

Later sanity checks on node properties (which would likely be missing) should prevent this from causing much trouble however, especially as the original premature free of the parent node has already been fixed separately (but that "fix" was apparently never backported to stable).

Fixes e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")

Fixes c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")

Cc stable <[email protected]> # 3.6

Quoted 6 lines

Quoted 2 lines

Note that of_get_child_by_name() is not present in 3.6, this was the reason the original implementation was using of_find_node_by_name()

Ah, ok. So the lookup would have needed to be open-coded using for_each_child_of_node() back then, or if anyone wants to backport any of these fixes to before v3.7.

Just let me note that intensive DT support of OMAP4&5/ARM based boards didn't start before ca. v3.13. So it is very unlikely that anyone is running a twl6040 based board with older kernels. It is different for twl4030/omap3 which was used by several devices (N900, Pandora, GTA04) back in 2.6. So I don't see a reason to backport this change to any kernel older than 3.16.

Fix child-node lookup during probe, which ended up searching the whole device tree depth-first starting at parent rather than just matching on its children.

Later sanity checks on node properties (which would likely be missing) should prevent this from causing much trouble however, especially as the original premature free of the parent node has already been fixed separately (but that "fix" was apparently never backported to stable).

Fixes e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")

Fixes c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")

Cc stable <[email protected]> # 3.6

Cc Peter Ujfalusi <[email protected]>

Cc H. Nikolaus Schaller <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/input/misc/twl6040-vibra.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/input/misc/twl6040-vibra.c +1 -2

index 5690eb7ff954..15e0d352c4cc 100644
@@ -248,8 +248,7 @@ static int twl6040_vibra_probe(struct platform_device *pdev)
 	int vddvibr_uV = 0;
 	int error;
 
-	of_node_get(twl6040_core_dev->of_node);
-	twl6040_core_node = of_find_node_by_name(twl6040_core_dev->of_node,
+	twl6040_core_node = of_get_child_by_name(twl6040_core_dev->of_node,
 						 "vibra");
 	if (!twl6040_core_node) {
 		dev_err(&pdev->dev, "parent of node is missing?\n");

Signature · 1 line

2.15.0

Fix child node-lookup during probe, which ended up searching the whole device tree depth-first starting at parent rather than just matching on its children.

To make things worse, the parent node was prematurely freed, while the child node was leaked.

Fixes 2e57d56747e6 ("mfd: 88pm860x: Device tree support")

Cc stable <[email protected]> # 3.7

Cc Haojian Zhuang <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/input/touchscreen/88pm860x-ts.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/input/touchscreen/88pm860x-ts.c +12 -4

index 7ed828a51f4c..3486d9403805 100644
@@ -126,7 +126,7 @@ static int pm860x_touch_dt_init(struct platform_device *pdev,
 	int data, n, ret;
 	if (!np)
 		return -ENODEV;
-	np = of_find_node_by_name(np, "touch");
+	np = of_get_child_by_name(np, "touch");
 	if (!np) {
 		dev_err(&pdev->dev, "Can't find touch node\n");
 		return -EINVAL;

@@ -144,13 +144,13 @@ static int pm860x_touch_dt_init(struct platform_device *pdev,
 	if (data) {
 		ret = pm860x_reg_write(i2c, PM8607_GPADC_MISC1, data);
 		if (ret < 0)
-			return -EINVAL;
+			goto err_put_node;
 	}
 	/* set tsi prebias time */
 	if (!of_property_read_u32(np, "marvell,88pm860x-tsi-prebias", &data)) {
 		ret = pm860x_reg_write(i2c, PM8607_TSI_PREBIAS, data);
 		if (ret < 0)
-			return -EINVAL;
+			goto err_put_node;
 	}
 	/* set prebias & prechg time of pen detect */
 	data = 0;

@@ -161,10 +161,18 @@ static int pm860x_touch_dt_init(struct platform_device *pdev,
 	if (data) {
 		ret = pm860x_reg_write(i2c, PM8607_PD_PREBIAS, data);
 		if (ret < 0)
-			return -EINVAL;
+			goto err_put_node;
 	}
 	of_property_read_u32(np, "marvell,88pm860x-resistor-X", res_x);
+
+	of_node_put(np);
+
 	return 0;
+
+err_put_node:
+	of_node_put(np);
+
+	return -EINVAL;
 }
 #else
 #define pm860x_touch_dt_init(x, y, z)	(-1)

Signature · 1 line

2.15.0

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Fixes 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")

Fixes e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")

Cc stable <[email protected]> # 3.7

Cc Peter Ujfalusi <[email protected]>

Cc Marek Belisko <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/input/misc/twl4030-vibra.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/input/misc/twl4030-vibra.c +4 -2

index 6c51d404874b..c37aea9ac272 100644
@@ -178,12 +178,14 @@ static SIMPLE_DEV_PM_OPS(twl4030_vibra_pm_ops,
 			 twl4030_vibra_suspend, twl4030_vibra_resume);
 
 static bool twl4030_vibra_check_coexist(struct twl4030_vibra_data *pdata,
-			      struct device_node *node)
+			      struct device_node *parent)
 {
+	struct device_node *node;
+
 	if (pdata && pdata->coexist)
 		return true;
 
-	node = of_find_node_by_name(node, "codec");
+	node = of_get_child_by_name(parent, "codec");
 	if (node) {
 		of_node_put(node);
 		return true;

Signature · 1 line

2.15.0

Quoted 4 lines · Johan Hovold

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Ugh, this all is pretty ugly business. Can we teach MFD to allow specifying firmware node to be attached to the platform devices it creates in mfd_add_device() so that the leaf drivers simply call device_property_read_XXX() on their own device and not be bothered with weird OF refcount issues or what node they need to locate and parse?

[ +CC: Lee, Rob and device-tree list ]

Quoted 11 lines · Dmitry Torokhov

Quoted 4 lines · Johan Hovold

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Ugh, this all is pretty ugly business. Can we teach MFD to allow specifying firmware node to be attached to the platform devices it creates in mfd_add_device() so that the leaf drivers simply call device_property_read_XXX() on their own device and not be bothered with weird OF refcount issues or what node they need to locate and parse?

Yeah, that may have helped. You can actually specify a compatible string in struct mfd_cell today which does make mfd_add_device() associate a matching child node.

Some best practice regarding how to deal with MFD and device tree would be good to determine and document too. For example, when should of_platform_populate() be used in favour of mfd_add_device()?

And how best to deal with sibling nodes, which is part of the problem here (I think the mfd should have provided a flag rather than having subdrivers deal with sibling nodes, for example).

That said, driver authors using the wrong of-helper could possibly have been avoided by amending the kernel docs (I'll do that as a follow up), but once these incorrect usages get in, only review can prevent them from being reproduced through copy-paste coding.

Quoted 14 lines · Johan Hovold

[ +CC: Lee, Rob and device-tree list ]

Quoted 11 lines · Dmitry Torokhov

Quoted 4 lines · Johan Hovold

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Ugh, this all is pretty ugly business. Can we teach MFD to allow specifying firmware node to be attached to the platform devices it creates in mfd_add_device() so that the leaf drivers simply call device_property_read_XXX() on their own device and not be bothered with weird OF refcount issues or what node they need to locate and parse?

If a child compatible is provided, we already set the child's of_node. It's then up to the driver (set) author(s) to use it in the correct manner.

Quoted 4 lines

Yeah, that may have helped. You can actually specify a compatible string in struct mfd_cell today which does make mfd_add_device() associate a matching child node.

Quoted 3 lines

Some best practice regarding how to deal with MFD and device tree would be good to determine and document too. For example, when should of_platform_populate() be used in favour of mfd_add_device()?

When the device supports DT and its entire hierarchical layout, along with all of its attributes can be expressed in DT.

Quoted 3 lines

And how best to deal with sibling nodes, which is part of the problem here (I think the mfd should have provided a flag rather than having subdrivers deal with sibling nodes, for example).

I disagree. The only properties the MFD (parent) driver is interested in is ones which are shared across multiple child devices. *Everything* which pertains to only a single child device should be handled by its accompanying driver.

Quoted 25 lines · Lee Jones

Quoted 14 lines · Johan Hovold

[ +CC: Lee, Rob and device-tree list ]

Quoted 11 lines · Dmitry Torokhov

Quoted 4 lines · Johan Hovold

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Ugh, this all is pretty ugly business. Can we teach MFD to allow specifying firmware node to be attached to the platform devices it creates in mfd_add_device() so that the leaf drivers simply call device_property_read_XXX() on their own device and not be bothered with weird OF refcount issues or what node they need to locate and parse?

If a child compatible is provided, we already set the child's of_node. It's then up to the driver (set) author(s) to use it in the correct manner.

Quoted 4 lines

Yeah, that may have helped. You can actually specify a compatible string in struct mfd_cell today which does make mfd_add_device() associate a matching child node.

Quoted 6 lines

Quoted 3 lines

Some best practice regarding how to deal with MFD and device tree would be good to determine and document too. For example, when should of_platform_populate() be used in favour of mfd_add_device()?

When the device supports DT and its entire hierarchical layout, along with all of its attributes can be expressed in DT.

Ok, a follow up: When there are different variants of an MFD and that affects the child drivers, then that should be expressed in in the child node compatibles rather than having the child match on the parent node?

I'm asking because this came up recently during review and their seems to be no precedent for matching on the parent compatible in child drivers:

	https://lkml.kernel.org/r/20171105154725.GA11226@localhost

Quoted 8 lines

Quoted 3 lines

And how best to deal with sibling nodes, which is part of the problem here (I think the mfd should have provided a flag rather than having subdrivers deal with sibling nodes, for example).

I disagree. The only properties the MFD (parent) driver is interested in is ones which are shared across multiple child devices. *Everything* which pertains to only a single child device should be handled by its accompanying driver.

Even if that means leaking details of one child driver into a sibling?

Isn't it then cleaner to use the parent MFD to coordinate between the cells, just as we do for IO?

In this case a child driver looked up a sibling node based on name, but

that doesn't mean the node is active, that there's a driver bound, or that the sibling node has some other random property for example. The parent could be used for such coordination, if only to pass information from one sibling to another.

Quoted 41 lines · Johan Hovold

Quoted 25 lines · Lee Jones

Quoted 14 lines · Johan Hovold

[ +CC: Lee, Rob and device-tree list ]

Quoted 11 lines · Dmitry Torokhov

Quoted 4 lines · Johan Hovold

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Ugh, this all is pretty ugly business. Can we teach MFD to allow specifying firmware node to be attached to the platform devices it creates in mfd_add_device() so that the leaf drivers simply call device_property_read_XXX() on their own device and not be bothered with weird OF refcount issues or what node they need to locate and parse?

If a child compatible is provided, we already set the child's of_node. It's then up to the driver (set) author(s) to use it in the correct manner.

Quoted 4 lines

Yeah, that may have helped. You can actually specify a compatible string in struct mfd_cell today which does make mfd_add_device() associate a matching child node.

Quoted 6 lines

Quoted 3 lines

Some best practice regarding how to deal with MFD and device tree would be good to determine and document too. For example, when should of_platform_populate() be used in favour of mfd_add_device()?

When the device supports DT and its entire hierarchical layout, along with all of its attributes can be expressed in DT.

Ok, a follow up: When there are different variants of an MFD and that affects the child drivers, then that should be expressed in in the child node compatibles rather than having the child match on the parent node?

I'm asking because this came up recently during review and their seems to be no precedent for matching on the parent compatible in child drivers:

Quoted 1 line

	https://lkml.kernel.org/r/20171105154725.GA11226@localhost

Accessing the parent's of_device_id .data directly doesn't sit well with me. The parent driver should pass this type of configuration though pdata IMHO.

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

And how best to deal with sibling nodes, which is part of the problem here (I think the mfd should have provided a flag rather than having subdrivers deal with sibling nodes, for example).

I disagree. The only properties the MFD (parent) driver is interested in is ones which are shared across multiple child devices. *Everything* which pertains to only a single child device should be handled by its accompanying driver.

Quoted 1 line

Even if that means leaking details of one child driver into a sibling?

Not sure what you mean here. Could you please elaborate or provide an example?

Quoted 4 lines

Isn't it then cleaner to use the parent MFD to coordinate between the cells, just as we do for IO?

In this case a child driver looked up a sibling node based on name, but

This should not be allowed. If >1 sibling requires access to a particular property, this is normally evidence enough that this property should be shared and handled by the parent.

Quoted 4 lines

that doesn't mean the node is active, that there's a driver bound, or that the sibling node has some other random property for example. The parent could be used for such coordination, if only to pass information from one sibling to another.

Right.

Quoted 43 lines · Lee Jones

Quoted 41 lines · Johan Hovold

Quoted 25 lines · Lee Jones

Quoted 14 lines · Johan Hovold

[ +CC: Lee, Rob and device-tree list ]

Quoted 11 lines · Dmitry Torokhov

Quoted 4 lines · Johan Hovold

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Ugh, this all is pretty ugly business. Can we teach MFD to allow specifying firmware node to be attached to the platform devices it creates in mfd_add_device() so that the leaf drivers simply call device_property_read_XXX() on their own device and not be bothered with weird OF refcount issues or what node they need to locate and parse?

If a child compatible is provided, we already set the child's of_node. It's then up to the driver (set) author(s) to use it in the correct manner.

Quoted 4 lines

Yeah, that may have helped. You can actually specify a compatible string in struct mfd_cell today which does make mfd_add_device() associate a matching child node.

Quoted 6 lines

Quoted 3 lines

Some best practice regarding how to deal with MFD and device tree would be good to determine and document too. For example, when should of_platform_populate() be used in favour of mfd_add_device()?

When the device supports DT and its entire hierarchical layout, along with all of its attributes can be expressed in DT.

Ok, a follow up: When there are different variants of an MFD and that affects the child drivers, then that should be expressed in in the child node compatibles rather than having the child match on the parent node?

I'm asking because this came up recently during review and their seems to be no precedent for matching on the parent compatible in child drivers:

Quoted 5 lines

Quoted 1 line

	https://lkml.kernel.org/r/20171105154725.GA11226@localhost

Accessing the parent's of_device_id .data directly doesn't sit well with me. The parent driver should pass this type of configuration though pdata IMHO.

The child driver is only matching on the parent-node compatible string IIRC, and therefore keeps its own table of all parent compatibles with its own set of (child) private match data (i.e. the parent compatible property is matched first by the parent driver, and then again by the child).

Passing through pdata here is not possible since mfd_add_device() isn't used, right? It could of course be described using properties of the child node (e.g. by using different child compatible strings).

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

And how best to deal with sibling nodes, which is part of the problem here (I think the mfd should have provided a flag rather than having subdrivers deal with sibling nodes, for example).

I disagree. The only properties the MFD (parent) driver is interested in is ones which are shared across multiple child devices. *Everything* which pertains to only a single child device should be handled by its accompanying driver.

Quoted 4 lines

Quoted 1 line

Even if that means leaking details of one child driver into a sibling?

Not sure what you mean here. Could you please elaborate or provide an example?

I mean that the sibling node needs to be aware of the name, compatible string, or other node properties of its sibling node to be able to parse sibling nodes itself (rather than the sibling or parent doing so on its behalf). But it seems you answer this below.

Quoted 14 lines

Quoted 4 lines

Isn't it then cleaner to use the parent MFD to coordinate between the cells, just as we do for IO?

In this case a child driver looked up a sibling node based on name, but

This should not be allowed. If >1 sibling requires access to a particular property, this is normally evidence enough that this property should be shared and handled by the parent.

Quoted 4 lines

that doesn't mean the node is active, that there's a driver bound, or that the sibling node has some other random property for example. The parent could be used for such coordination, if only to pass information from one sibling to another.

Quoted 1 line

Right.

Ok, it seems we're in agreement here.

Given that MFD has evolved over time and device-tree support has been added retroactively to some drivers, we've ended up with a multitude of different ways of dealing with such issues. I think it may still be a good idea to jot down some best practices for future driver developers.

Thanks,
Johan

Quoted 97 lines · Johan Hovold

Quoted 43 lines · Lee Jones

Quoted 41 lines · Johan Hovold

Quoted 25 lines · Lee Jones

Quoted 14 lines · Johan Hovold

[ +CC: Lee, Rob and device-tree list ]

Quoted 11 lines · Dmitry Torokhov

Quoted 4 lines · Johan Hovold

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Ugh, this all is pretty ugly business. Can we teach MFD to allow specifying firmware node to be attached to the platform devices it creates in mfd_add_device() so that the leaf drivers simply call device_property_read_XXX() on their own device and not be bothered with weird OF refcount issues or what node they need to locate and parse?

If a child compatible is provided, we already set the child's of_node. It's then up to the driver (set) author(s) to use it in the correct manner.

Quoted 4 lines

Yeah, that may have helped. You can actually specify a compatible string in struct mfd_cell today which does make mfd_add_device() associate a matching child node.

Quoted 6 lines

Quoted 3 lines

Some best practice regarding how to deal with MFD and device tree would be good to determine and document too. For example, when should of_platform_populate() be used in favour of mfd_add_device()?

When the device supports DT and its entire hierarchical layout, along with all of its attributes can be expressed in DT.

Ok, a follow up: When there are different variants of an MFD and that affects the child drivers, then that should be expressed in in the child node compatibles rather than having the child match on the parent node?

I'm asking because this came up recently during review and their seems to be no precedent for matching on the parent compatible in child drivers:

Quoted 5 lines

Quoted 1 line

	https://lkml.kernel.org/r/20171105154725.GA11226@localhost

Accessing the parent's of_device_id .data directly doesn't sit well with me. The parent driver should pass this type of configuration though pdata IMHO.

The child driver is only matching on the parent-node compatible string IIRC, and therefore keeps its own table of all parent compatibles with its own set of (child) private match data (i.e. the parent compatible property is matched first by the parent driver, and then again by the child).

Passing through pdata here is not possible since mfd_add_device() isn't used, right? It could of course be described using properties of the child node (e.g. by using different child compatible strings).

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

And how best to deal with sibling nodes, which is part of the problem here (I think the mfd should have provided a flag rather than having subdrivers deal with sibling nodes, for example).

I disagree. The only properties the MFD (parent) driver is interested in is ones which are shared across multiple child devices. *Everything* which pertains to only a single child device should be handled by its accompanying driver.

Quoted 4 lines

Quoted 1 line

Even if that means leaking details of one child driver into a sibling?

Not sure what you mean here. Could you please elaborate or provide an example?

I mean that the sibling node needs to be aware of the name, compatible string, or other node properties of its sibling node to be able to parse sibling nodes itself (rather than the sibling or parent doing so on its behalf). But it seems you answer this below.

Quoted 9 lines

Quoted 4 lines

Isn't it then cleaner to use the parent MFD to coordinate between the cells, just as we do for IO?

In this case a child driver looked up a sibling node based on name, but

This should not be allowed. If >1 sibling requires access to a particular property, this is normally evidence enough that this property should be shared and handled by the parent.

Quoted 6 lines

Quoted 4 lines

that doesn't mean the node is active, that there's a driver bound, or that the sibling node has some other random property for example. The parent could be used for such coordination, if only to pass information from one sibling to another.

Right.

Ok, it seems we're in agreement here.

Quoted 4 lines

Given that MFD has evolved over time and device-tree support has been added retroactively to some drivers, we've ended up with a multitude of different ways of dealing with such issues. I think it may still be a good idea to jot down some best practices for future driver developers.

FWIW here is the patch allowing attaching fwnode to an MFD cell that is not using of_compatible (because if historical reasons). Completely untested as I do not have this hardware.

If this is somewhat acceptable I can untangle core from twl6040 changes.

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Fixes 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")

Fixes e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")

Cc stable <[email protected]> # 3.7

Cc Peter Ujfalusi <[email protected]>

Cc Marek Belisko <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/input/misc/twl4030-vibra.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/input/misc/twl4030-vibra.c +1 -1

index 6c51d404874b..c37aea9ac272 100644
@@ -178,12 +178,14 @@ static SIMPLE_DEV_PM_OPS(twl4030_vibra_pm_ops,
 			 twl4030_vibra_suspend, twl4030_vibra_resume);
 
 static bool twl4030_vibra_check_coexist(struct twl4030_vibra_data *pdata,
-			      struct device_node *node)
+			      struct device_node *parent)
 {

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Fixes 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")

Fixes e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")

Cc stable <[email protected]> # 3.7

Cc Peter Ujfalusi <[email protected]>

Cc Marek Belisko <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Quoted 5 lines · Johan Hovold

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Quoted 7 lines

Fixes 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")

Fixes e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")

Cc stable <[email protected]> # 3.7

Cc Peter Ujfalusi <[email protected]>

Cc Marek Belisko <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Just wanted to send a reminder about this series. I believe these bugs need to be fixes regardless of whether MFD-core eventually provides some means of avoiding such lookups in cell drivers (i.e. the discussion which appears to have stalled).

Quoted 14 lines · Johan Hovold

Quoted 12 lines · Johan Hovold

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Fixes 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")

Fixes e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")

Cc stable <[email protected]> # 3.7

Cc Peter Ujfalusi <[email protected]>

Cc Marek Belisko <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Quoted 4 lines

Just wanted to send a reminder about this series. I believe these bugs need to be fixes regardless of whether MFD-core eventually provides some means of avoiding such lookups in cell drivers (i.e. the discussion which appears to have stalled).

Another month, another reminder: Will you pick this series up for 4.16, Dmitry?

Quoted 20 lines · Johan Hovold

Quoted 18 lines · Johan Hovold

Quoted 5 lines · Johan Hovold

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Quoted 7 lines

Fixes 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")

Fixes e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")

Cc stable <[email protected]> # 3.7

Cc Peter Ujfalusi <[email protected]>

Cc Marek Belisko <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Just wanted to send a reminder about this series. I believe these bugs need to be fixes regardless of whether MFD-core eventually provides some means of avoiding such lookups in cell drivers (i.e. the discussion which appears to have stalled).

Quoted 2 lines

Another month, another reminder: Will you pick this series up for 4.16, Dmitry?

Sorry, I was hoping we'd have some movement in MFD... Applied all 3, thank you.

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Fixes 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")

Fixes e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")

Cc stable <[email protected]> # 3.7

Cc Peter Ujfalusi <[email protected]>

Cc Marek Belisko <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/input/misc/twl4030-vibra.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/input/misc/twl4030-vibra.c +4 -2

index 6c51d404874b..c37aea9ac272 100644
@@ -178,12 +178,14 @@ static SIMPLE_DEV_PM_OPS(twl4030_vibra_pm_ops,
 			 twl4030_vibra_suspend, twl4030_vibra_resume);
 
 static bool twl4030_vibra_check_coexist(struct twl4030_vibra_data *pdata,
-			      struct device_node *node)
+			      struct device_node *parent)
 {
+	struct device_node *node;
+
 	if (pdata && pdata->coexist)
 		return true;
 
-	node = of_find_node_by_name(node, "codec");
+	node = of_get_child_by_name(parent, "codec");
 	if (node) {
 		of_node_put(node);
 		return true;

Signature · 1 line

2.15.0

A helper purported to look up a child node based on its name was using the wrong of-helper and ended up prematurely freeing the parent of-node while searching the whole device tree depth-first starting at the parent node.

Fixes 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")

Fixes e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")

Cc stable <[email protected]> # 3.7

Cc Peter Ujfalusi <[email protected]>

Cc Marek Belisko <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/input/misc/twl4030-vibra.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/input/misc/twl4030-vibra.c +4 -2

index 6c51d404874b..c37aea9ac272 100644
@@ -178,12 +178,14 @@ static SIMPLE_DEV_PM_OPS(twl4030_vibra_pm_ops,
 			 twl4030_vibra_suspend, twl4030_vibra_resume);
 
 static bool twl4030_vibra_check_coexist(struct twl4030_vibra_data *pdata,
-			      struct device_node *node)
+			      struct device_node *parent)
 {
+	struct device_node *node;
+
 	if (pdata && pdata->coexist)
 		return true;
 
-	node = of_find_node_by_name(node, "codec");
+	node = of_get_child_by_name(parent, "codec");
 	if (node) {
 		of_node_put(node);
 		return true;

Signature · 1 line

2.15.0