On Mon, 13 Nov 2017, Dmitry Torokhov wrote: > 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. I am not familiar with the device_* OF implementation, so find it hard to provide a solid, knowledgeable review. It looks okay in principle. I'd appreciate it if Rob or one of the other DT guys could cast an eye though. > If this is somewhat acceptable I can untangle core from twl6040 > changes. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
