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

[Robert Hancock]

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@xxxxxxxxxxxxx>
>> ---
>>  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.

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock@xxxxxxxxxxxxx