Re: [PATCH v2 04/22] of/platform: add of_platform_device_find()

[Tomeu Vizoso]

On 29 July 2015 at 13:20, Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx> wrote:
> On 29 July 2015 at 08:14, Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx> wrote:
>> On 28 July 2015 at 17:31, Rob Herring <robherring2@xxxxxxxxx> wrote:
>>> On Tue, Jul 28, 2015 at 8:54 AM, Tomeu Vizoso
>>> <tomeu.vizoso@xxxxxxxxxxxxx> wrote:
>>>> On 28 July 2015 at 15:39, Rob Herring <robherring2@xxxxxxxxx> wrote:
>>>>> On Tue, Jul 28, 2015 at 8:19 AM, Tomeu Vizoso
>>>>> <tomeu.vizoso@xxxxxxxxxxxxx> wrote:
>>>>>> From an arbitrary node in the tree, find the enclosing node that
>>>>>> corresponds to a platform device, as registered by
>>>>>> of_platform_populate().
>>>>>>
>>>>>> This can be used to find out what device needs to be probed so the
>>>>>> dependency described by a given node is made available.
>>>>>>
>>>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Move the logic for finding a platform device from its firmware node to
>>>>>>   of/platform.c as it's not needed for ACPI nodes.
>>>>>>
>>>>>>  drivers/of/platform.c       | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  include/linux/of_platform.h |  1 +
>>>>>>  2 files changed, 61 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>>>>> index ff27494cda8c..89c5cd513027 100644
>>>>>> --- a/drivers/of/platform.c
>>>>>> +++ b/drivers/of/platform.c
>>>>>> @@ -501,6 +501,66 @@ void of_platform_depopulate(struct device *parent)
>>>>>>  }
>>>>>>  EXPORT_SYMBOL_GPL(of_platform_depopulate);
>>>>>>
>>>>>> +static bool of_is_platform(struct device_node *np)
>>>>>> +{
>>>>>> +       int count;
>>>>>> +
>>>>>> +       count = of_property_count_strings(np, "compatible");
>>>>>> +
>>>>>> +       /* The node has to have a compatible string */
>>>>>> +       if (!count)
>>>>>> +               return false;
>>>>>> +
>>>>>> +       /* But it cannot be just a simple memory-mapped bus */
>>>>>> +       if (count == 1 && of_match_node(of_default_bus_match_table, np))
>>>>>> +               return false;
>>>>>> +
>>>>>> +       /* But AMBA devices aren't platform devices */
>>>>>> +       if (of_device_is_compatible(np, "arm,primecell"))
>>>>>> +               return false;
>>>>>> +
>>>>>> +       /* Node is immediately below root */
>>>>>> +       if (!np->parent || !np->parent->parent)
>>>>>> +               return true;
>>>>>> +
>>>>>> +       /* If it's a node in a simple memory-mapped bus */
>>>>>> +       if (of_match_node(of_default_bus_match_table, np->parent))
>>>>>> +               return true;
>>>>>
>>>>> This seems really fragile.
>>>>
>>>> I think this finding logic matches the logic for registering platform
>>>> devices in of_platform_populate and also what is documented in
>>>> Documentation/devicetree/usage-model.txt.
>>>
>>> Right. So now we have that logic in 2 places. One is descending from
>>> the root and one is walking up from the child. That's an opportunity
>>> for some mismatch.
>>
>> Definitely.
>>
>>>>> What about platform devices which are
>>>>> children of MFDs but are not "simple-mfd"?
>>>>
>>>> This code should deal fine with those (the boards I tested with do
>>>> have them). It probes the mfd master, and that in turn will call
>>>> mfd_add_devices causing the target device to be probed.
>>>
>>> I don't see how this function would ever return true in this case
>>> unless you put the MFD at the root level. The only other way to return
>>> true is matching on of_default_bus_match_table for the parent (i.e.
>>> the MFD).
>>
>> Oops, you are right.
>>
>>>>> Does of_find_device_by_node not work for you?
>>>>
>>>> Well, the dependencies aren't always platform devices, that's why I
>>>> need to go up the tree until I find a node that corresponds to a
>>>> platform device that I can query and probe.
>>>>
>>>> If I had a way to get, say, a i2c device from its fwnode then I would
>>>> just need to make sure that a device's parent is probed before probing
>>>> it and everything would be cleaner in the OF case.
>>>
>>> If you have the struct device from the device_node, then you should be
>>> able to do this, right?
>>
>> Yes, if I could go back from the device_node to the struct device that
>> was registered from it, for all buses, then all this would be much
>> simpler and more robust. It would basically work like in the ACPI
>> case.
>>
>> I will play with this idea.
>>
>>>>> That is probably not the
>>>>> most efficient search, but we could fix that. We could add struct
>>>>> device ptr to struct device_node and check without searching for
>>>>> example.
>>>>
>>>> That would be great, but I thought there was an issue with a OF node
>>>> being able to be related to more than one struct device (but I haven't
>>>> found this myself yet).
>>>
>>> I think it pretty much should be one to one. I'm not aware of any
>>> examples where that is not the case. This function would already be
>>> broken if you could have more than one struct device.
>>
>> Well, for platform devices we currently know that there can only be
>> one struct device for a given device_node, but that's not so clear for
>> other devices.
>
> Just found this case:
>
> http://lxr.free-electrons.com/source/drivers/spi/spi-tegra114.c#L1124
>
> Looks like SPI master devices point to the same device_node as the
> platform device that registers them.

And finally found the email that warned me against it:

http://lkml.kernel.org/g/20140514140534.897F8C4153D@xxxxxxxxxxxxxxxxxxx

Regards,

Tomeu

> Regards,
>
> Tomeu
>
>>> There is an issue where you could have 2 drivers match the same node,
>>> but on different compatible strings. But that's a different issue.
>>
>> Yup.
>>
>> Thanks,
>>
>> Tomeu
>>
>>> Rob
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/