[RFC] dm: uclass: add functions to get device by platdata

[Walter Lozano <walter.lozano@collabora.com>]

Hi Simon,

Thanks for taking the time to check for my comments

On 4/3/20 20:11, Simon Glass wrote:

> Hi Walter,
>
> On Wed, 4 Mar 2020 at 12:40, Walter Lozano <walter.lozano@collabora.com> wrote:
>> When OF_PLATDATA is enabled DT information is parsed and platdata
>> structures are populated. In this context the links between DT nodes are
>> represented as pointers to platdata structures, and there is no clear way
>> to access to the device which owns the structure.
>>
>> This patch implements a set of functions:
>>
>> - device_find_by_platdata
>> - uclass_find_device_by_platdata
>>
>> to access to the device.
>>
>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>> ---
>>   drivers/core/device.c        | 19 +++++++++++++++++++
>>   drivers/core/uclass.c        | 34 ++++++++++++++++++++++++++++++++++
>>   include/dm/device.h          |  2 ++
>>   include/dm/uclass-internal.h |  3 +++
>>   include/dm/uclass.h          |  2 ++
>>   5 files changed, 60 insertions(+)
> This is interesting. Could you also add the motivation for this? It's
> not clear to me who would call this function.

I have been reviewing the OF_PLATDATA support as an R&D project, in this context, in order to have
a better understanding on the possibilities and limitations I decided to add its support to iMX6,
more particularly to the MMC drivers. The link issue arises when I tried to setup the GPIO for
Card Detection, which is trivial when DT is available. However, when OF_PLATDATA is enabled
this seems, at least for me, not straightforward.

In order to overcome this limitation I think that having a set of functions to find/get devices
based on platdata could be useful. Of course, there might be a better approach/idea, so that is
the motivation for this RFC.

An example of the usage could be

#if CONFIG_IS_ENABLED(DM_GPIO)

 ??????? struct udevice *gpiodev;

 ??????? ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void *)dtplat->cd_gpios->node, &gpiodev);

 ??????? if (ret)
 ??????????????? return ret;

 ??????? ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios",
                                      dtplat->cd_gpios->arg[0], GPIOD_IS_IN,
 ???????????????????????????????????? dtplat->cd_gpios->arg[1], &priv->cd_gpio);

 ??????? if (ret)
 ??????????????? return ret;

#endif

This is part of my current work, a series of patches to add OF_PLATDATA support as explained.

Does this make sense to you?

> Also it relates to another thing I've been thinking about for a while,
> which is to validate that all the structs pointed to are correct.
>
> E.g. if every struct had a magic number like:
>
> struct tpm_platdata {
>      DM_STRUCT(UCLASS_TPM, DM_STRUCT_PLATDATA, ...)
>      fields here
> };
>
> then we could check the structure pointers are correct.
>
> DM_STRUCT() would define to nothing if we were not building with
> CONFIG_DM_DEBUG or similar.

Interesting, I think it could be useful and save us from headaches while debugging.

Thanks for sharing this idea.

> Anyway, I wonder whether you could expand your definition a bit so you
> have an enum for the different types of struct you can request:
>
> enum dm_struct_t {
>     DM_STRUCT_PLATDATA,
>   ...
>
>     DM_STRUCT_COUNT,
> };
>
> and modify the function so it can request it via the enum?

Let me check if I understand correctly, your suggestion is to do 
something like diff --git a/include/dm/uclass.h b/include/dm/uclass.h 
index 92c07f8426..bf09dadf3f 100644 --- a/include/dm/uclass.h +++ 
b/include/dm/uclass.h

@@ -167,8 +167,8 @@ int uclass_get_device(enum uclass_id id, int index, 
struct udevice **devp);

 ?int uclass_get_device_by_name(enum uclass_id id, const char *name, 
 ????????????????????????????? struct udevice **devp); -int 
uclass_get_device_by_platdata(enum uclass_id id, void *platdata, 
-???????????????????????????? struct udevice **devp);

+int uclass_get_device_by_struct(enum uclass_id id, enum dm_struct_t 
struct_id, +???????????????????????????? void *struct_pointer, struct 
udevice **devp); ?/** ? * uclass_get_device_by_seq() - Get a uclass 
device based on an ID and sequence ? *

If that is the case, I would be happy to help.

Also, if my understanding is correct, could you elaborate which cases 
are you trying to cover with this approach? Regards,

Walter