Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

Dmitry Torokhov
2017-11-13T21:54:52Z

On Mon, Nov 13, 2017 at 12:51:02PM +0100, Johan Hovold wrote:
> On Mon, Nov 13, 2017 at 10:20:28AM +0000, Lee Jones wrote:
> > On Mon, 13 Nov 2017, Johan Hovold wrote:
> >
> > > On Mon, Nov 13, 2017 at 09:11:44AM +0000, Lee Jones wrote:
> > > > On Sun, 12 Nov 2017, Johan Hovold wrote:
> > > >
> > > > > [ +CC: Lee, Rob and device-tree list ]
> > > > >
> > > > > On Sat, Nov 11, 2017 at 09:50:59AM -0800, Dmitry Torokhov wrote:
> > > > > > On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> > > > > > > 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.
> > > >
> > > > > 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()?
> > > >
> > > > 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
> >
> > 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).
>
> > > > > 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?
> >
> > 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.
>
> > > 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.
> >
> > > 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.
>
> 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.

Thanks.
>
> Thanks,
> Johan

--
Dmitry

Attach fwnode to MFD cells

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/misc/twl6040-vibra.c | 32 ++++++++++----------------------
drivers/mfd/mfd-core.c | 2 ++
drivers/mfd/twl6040.c | 14 +++++++-------
include/linux/mfd/core.h | 6 ++++++
include/linux/mfd/twl6040.h | 2 ++
5 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c
index 5690eb7ff954..bd9507cf5608 100644
--- a/drivers/input/misc/twl6040-vibra.c
+++ b/drivers/input/misc/twl6040-vibra.c
@@ -242,23 +242,13 @@ static SIMPLE_DEV_PM_OPS(twl6040_vibra_pm_ops, twl6040_vibra_suspend, NULL);
static int twl6040_vibra_probe(struct platform_device *pdev)
{
struct device *twl6040_core_dev = pdev->dev.parent;
- struct device_node *twl6040_core_node;
struct vibra_info *info;
int vddvibl_uV = 0;
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,
- "vibra");
- if (!twl6040_core_node) {
- dev_err(&pdev->dev, "parent of node is missing?\n");
- return -EINVAL;
- }
-
info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
if (!info) {
- of_node_put(twl6040_core_node);
dev_err(&pdev->dev, "couldn't allocate memory\n");
return -ENOMEM;
}
@@ -267,18 +257,16 @@ static int twl6040_vibra_probe(struct platform_device *pdev)

info->twl6040 = dev_get_drvdata(pdev->dev.parent);

- of_property_read_u32(twl6040_core_node, "ti,vibldrv-res",
- &info->vibldrv_res);
- of_property_read_u32(twl6040_core_node, "ti,vibrdrv-res",
- &info->vibrdrv_res);
- of_property_read_u32(twl6040_core_node, "ti,viblmotor-res",
- &info->viblmotor_res);
- of_property_read_u32(twl6040_core_node, "ti,vibrmotor-res",
- &info->vibrmotor_res);
- of_property_read_u32(twl6040_core_node, "ti,vddvibl-uV", &vddvibl_uV);
- of_property_read_u32(twl6040_core_node, "ti,vddvibr-uV", &vddvibr_uV);
-
- of_node_put(twl6040_core_node);
+ device_property_read_u32(&pdev->dev, "ti,vibldrv-res",
+ &info->vibldrv_res);
+ device_property_read_u32(&pdev->dev, "ti,vibrdrv-res",
+ &info->vibrdrv_res);
+ device_property_read_u32(&pdev->dev, "ti,viblmotor-res",
+ &info->viblmotor_res);
+ device_property_read_u32(&pdev->dev, "ti,vibrmotor-res",
+ &info->vibrmotor_res);
+ device_property_read_u32(&pdev->dev, "ti,vddvibl-uV", &vddvibl_uV);
+ device_property_read_u32(&pdev->dev, "ti,vddvibr-uV", &vddvibr_uV);

if ((!info->vibldrv_res && !info->viblmotor_res) ||
(!info->vibrdrv_res && !info->vibrmotor_res)) {
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index c57e407020f1..2792fe40e2e4 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -186,6 +186,8 @@ static int mfd_add_device(struct device *parent, int id,

mfd_acpi_add_device(cell, pdev);

+ set_secondary_fwnode(&pdev->dev, cell->fwnode);
+
if (cell->pdata_size) {
ret = platform_device_add_data(pdev,
cell->platform_data, cell->pdata_size);
diff --git a/drivers/mfd/twl6040.c b/drivers/mfd/twl6040.c
index d66502d36ba0..2994a8885c7e 100644
--- a/drivers/mfd/twl6040.c
+++ b/drivers/mfd/twl6040.c
@@ -97,13 +97,9 @@ static struct reg_sequence twl6040_patch[] = {
};

-static bool twl6040_has_vibra(struct device_node *node)
+static struct device_node *twl6040_get_vibra(struct device_node *node)
{
-#ifdef CONFIG_OF
- if (of_find_node_by_name(node, "vibra"))
- return true;
-#endif
- return false;
+ return node ? of_get_child_by_name(node, "vibra") : NULL;
}

int twl6040_reg_read(struct twl6040 *twl6040, unsigned int reg)
@@ -766,11 +762,13 @@ static int twl6040_probe(struct i2c_client *client,
children++;

/* Vibra input driver support */
- if (twl6040_has_vibra(node)) {
+ twl6040->vibra_node = twl6040_get_vibra(node);
+ if (twl6040->vibra_node) {
irq = regmap_irq_get_virq(twl6040->irq_data, TWL6040_IRQ_VIB);

cell = &twl6040->cells[children];
cell->name = "twl6040-vibra";
+ cell->fwnode = &twl6040->vibra_node->fwnode;
twl6040_vibra_rsrc[0].start = irq;
twl6040_vibra_rsrc[0].end = irq;
cell->resources = twl6040_vibra_rsrc;
@@ -817,6 +815,8 @@ static int twl6040_remove(struct i2c_client *client)

mfd_remove_devices(&client->dev);

+ of_node_put(twl6040->vibra_node);
+
regulator_bulk_disable(TWL6040_NUM_SUPPLIES, twl6040->supplies);

return 0;
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 99c0395fe1f9..452f9d4de98a 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -58,6 +58,12 @@ struct mfd_cell {
/* Matches ACPI */
const struct mfd_cell_acpi_match *acpi_match;

+ /*
+ * Secondary firmware node that should be associated with the
+ * platform device corresponding to the cell.
+ */
+ struct fwnode_handle *fwnode;
+
/*
* These resources can be specified relative to the parent device.
* For accessing hardware you should use resources from the platform dev
diff --git a/include/linux/mfd/twl6040.h b/include/linux/mfd/twl6040.h
index a2e88761c09f..9eee44c4fc1f 100644
--- a/include/linux/mfd/twl6040.h
+++ b/include/linux/mfd/twl6040.h
@@ -232,6 +232,8 @@ struct twl6040 {
struct mfd_cell cells[TWL6040_CELLS];
struct completion ready;

+ struct device_node *vibra_node;
+
int audpwron;
int power_count;
int rev;