Re: [PATCH 1/2] mfd: core: Support multiple OF child devices of the same type

[Lee Jones <lee.jones@linaro.org>]

On Wed, 05 Jun 2019, Robert Hancock wrote:

> On 2019-06-05 12:31 a.m., Lee Jones wrote:
> > On Tue, 04 Jun 2019, Robert Hancock wrote:
> > 
> >> Previously the MFD core supported assigning OF nodes to created MFD
> >> devices, but relied solely on matching the of_compatible string. This
> >> would result in devices being potentially assigned the wrong node if
> >> there are multiple devices with the same compatible string within a
> >> multifunction device.
> >>
> >> Add support for matching the full name of the node in the MFD cell
> >> definition, so that we can match against a specific instance of a
> >> device. If this is not specified, we match just based on the
> >> compatible string, as before.
> >>
> >> Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
> >> ---
> >>  drivers/mfd/mfd-core.c   | 5 ++++-
> >>  include/linux/mfd/core.h | 3 +++
> >>  2 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> >> index 1ade4c8..74bc895 100644
> >> --- a/drivers/mfd/mfd-core.c
> >> +++ b/drivers/mfd/mfd-core.c
> >> @@ -177,7 +177,10 @@ static int mfd_add_device(struct device *parent, int id,
> >>  
> >>  	if (parent->of_node && cell->of_compatible) {
> >>  		for_each_child_of_node(parent->of_node, np) {
> >> -			if (of_device_is_compatible(np, cell->of_compatible)) {
> >> +			if (of_device_is_compatible(np, cell->of_compatible) &&
> >> +			    (!cell->of_full_name ||
> >> +			     !strcmp(cell->of_full_name,
> >> +				     of_node_full_name(np)))) {
> >>  				pdev->dev.of_node = np;
> >>  				break;
> > 
> > That is some ugly, squashed up code.
> > 
> > If we end up accepting this, I suggest flattening this out a bit.
> > 
> > ... but we'll cross that bridge when we come to it.
> 
> Yes, that if statement could be broken up to make it more readable. Will
> fix in a next version assuming the concept is acceptable.
> 
> > 
> >> diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> >> index 99c0395..470f6cb 100644
> >> --- a/include/linux/mfd/core.h
> >> +++ b/include/linux/mfd/core.h
> >> @@ -55,6 +55,9 @@ struct mfd_cell {
> >>  	 */
> >>  	const char		*of_compatible;
> >>  
> >> +	/* Optionally match against a specific device of a given type */
> >> +	const char		*of_full_name;
> >> +
> > 
> > Can you give me an example for when this might be useful?
> 
> This is an example of some device tree entries for our MFD device:
> 
>             axi_iic_0: i2c@c0000 {
>                 compatible = "xlnx,xps-iic-2.00.a";
>                 clocks = <&axi_clk>;
>                 clock-frequency = <100000>;
>                 interrupts = <7>;
>                 #size-cells = <0>;
>                 #address-cells = <1>;
>             };
> 
>             axi_iic_1: i2c@d0000 {
>                 compatible = "xlnx,xps-iic-2.00.a";
>                 clocks = <&axi_clk>;
>                 clock-frequency = <100000>;
>                 interrupts = <8>;
>                 #size-cells = <0>;
>                 #address-cells = <1>;
>             };
> 
> and the corresponding MFD cells:
> 
> {
> 	.name		= "axi_iic_0",
> 	.of_compatible	= "xlnx,xps-iic-2.00.a",
> 	.of_full_name	= "i2c@c0000",
> 	.num_resources	= ARRAY_SIZE(dbe_i2c1_resources),
> 	.resources	= dbe_i2c1_resources
> },
> {
> 	.name		= "axi_iic_1",
> 	.of_compatible	= "xlnx,xps-iic-2.00.a",
> 	.of_full_name	= "i2c@d0000",
> 	.num_resources	= ARRAY_SIZE(dbe_i2c2_resources),
> 	.resources	= dbe_i2c2_resources
> },
> 
> Without having the .of_full_name support, both MFD cells ended up
> wrongly matching against the i2c@c0000 device tree node since we just
> picked the first one where of_compatible matched.

What is contained in each of their resources?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog