[RFC 2/6] core: extend struct driver_info to point to device

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

Hi Simon,

On 20/5/20 00:07, Simon Glass wrote:
> Hi Walter,
>
> On Wed, 13 May 2020 at 14:14, Walter Lozano<walter.lozano@collabora.com>  wrote:
>> Currently when creating an U_BOOT_DEVICE entry a struct driver_info
>> is declared, which contains the data needed to instantiate the device.
>> However, the actual device is created at runtime and there is no proper
>> way to get the device based on its struct driver_info.
>>
>> This patch extends struct driver_info adding a pointer to udevice which
>> is populated during the bind process, allowing to generate a set of
>> functions to get the device based on its struct driver_info.
>>
>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com>
>> ---
>>   drivers/core/device.c        | 25 +++++++++++++++++++++++--
>>   drivers/core/root.c          |  6 +++++-
>>   include/dm/device-internal.h |  2 +-
>>   include/dm/device.h          | 13 +++++++++++++
>>   include/dm/platdata.h        |  6 ++++++
>>   5 files changed, 48 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>> index 0157bb1fe0..2476f170a5 100644
>> --- a/drivers/core/device.c
>> +++ b/drivers/core/device.c
>> @@ -246,10 +246,11 @@ int device_bind_ofnode(struct udevice *parent, const struct driver *drv,
>>   }
>>
>>   int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
>> -                       const struct driver_info *info, struct udevice **devp)
>> +                       struct driver_info *info, struct udevice **devp)
>>   {
>>          struct driver *drv;
>>          uint platdata_size = 0;
>> +       int ret = 0;
>>
>>          drv = lists_driver_lookup_name(info->name);
>>          if (!drv)
>> @@ -260,9 +261,19 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
>>   #if CONFIG_IS_ENABLED(OF_PLATDATA)
>>          platdata_size = info->platdata_size;
>>   #endif
>> -       return device_bind_common(parent, drv, info->name,
>> +       ret = device_bind_common(parent, drv, info->name,
>>                          (void *)info->platdata, 0, ofnode_null(), platdata_size,
>>                          devp);
>> +
> drop blank line
Sure.
>> +       if (ret)
>> +               return ret;
>> +
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +       info->dev = *devp;
>> +#endif
>> +
>> +       return ret;
>> +
> and here
Sure.
>>   }
>>
>>   static void *alloc_priv(int size, uint flags)
>> @@ -727,6 +738,16 @@ int device_get_global_by_ofnode(ofnode ofnode, struct udevice **devp)
>>          return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
>>   }
>>
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +int device_get_by_driver_info(struct driver_info * info, struct udevice **devp)
>> +{
>> +       struct udevice *dev;
>> +
>> +       dev = info->dev;
> blank line before return:-)
Sure, thanks!
>> +       return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
>> +}
>> +#endif
>> +
>>   int device_find_first_child(const struct udevice *parent, struct udevice **devp)
>>   {
>>          if (list_empty(&parent->child_head)) {
>> diff --git a/drivers/core/root.c b/drivers/core/root.c
>> index 14df16c280..8f47a6b356 100644
>> --- a/drivers/core/root.c
>> +++ b/drivers/core/root.c
>> @@ -25,7 +25,7 @@
>>
>>   DECLARE_GLOBAL_DATA_PTR;
>>
>> -static const struct driver_info root_info = {
>> +static struct driver_info root_info = {
>>          .name           = "root_driver",
>>   };
>>
>> @@ -346,6 +346,10 @@ int dm_init_and_scan(bool pre_reloc_only)
>>   {
>>          int ret;
>>
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> This one can use if() I think

I think that as populate_phandle_data() is only defined when OF_PLATDATA 
is enabled (as it is created by dtoc), this will trigger and link error 
on those board which don't use this config. Is my understanding correct?

>> +       populate_phandle_data();
>> +#endif
>> +
>>          ret = dm_init(IS_ENABLED(CONFIG_OF_LIVE));
>>          if (ret) {
>>                  debug("dm_init() failed: %d\n", ret);
>> diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h
>> index 294d6c1810..5145fb4e14 100644
>> --- a/include/dm/device-internal.h
>> +++ b/include/dm/device-internal.h
>> @@ -81,7 +81,7 @@ int device_bind_with_driver_data(struct udevice *parent,
>>    * @return 0 if OK, -ve on error
>>    */
>>   int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
>> -                       const struct driver_info *info, struct udevice **devp);
>> +                       struct driver_info *info, struct udevice **devp);
> That change should really be a separate patch I think.

If we move this change to a separate patch, this will fail to compile as 
struct driver_info needs to be updated by device_bind_by_name to fill 
info->dev. I can move both things to a different patch, in that case.

Please advice.

>>   /**
>>    * device_ofdata_to_platdata() - Read platform data for a device
>> diff --git a/include/dm/device.h b/include/dm/device.h
>> index d5b3e732c3..590c1f99ce 100644
>> --- a/include/dm/device.h
>> +++ b/include/dm/device.h
>> @@ -537,6 +537,19 @@ int device_find_global_by_ofnode(ofnode node, struct udevice **devp);
>>    */
>>   int device_get_global_by_ofnode(ofnode node, struct udevice **devp);
>>
>> +/**
>> + * device_get_by_driver_info() - Get a device based on driver_info
>> + *
>> + * Locates a device by its struct driver_info.
>> + *
>> + * The device is probed to activate it ready for use.
>> + *
>> + * @info: Struct driver_info
>> + * @devp: Returns pointer to device if found, otherwise this is set to NULL
>> + * @return 0 if OK, -ve on error
>> + */
>> +int device_get_by_driver_info(struct driver_info * info, struct udevice **devp);
>> +
>>   /**
>>    * device_find_first_child() - Find the first child of a device
>>    *
>> diff --git a/include/dm/platdata.h b/include/dm/platdata.h
>> index c972fa6936..17eef7c7a3 100644
>> --- a/include/dm/platdata.h
>> +++ b/include/dm/platdata.h
>> @@ -28,6 +28,7 @@ struct driver_info {
>>          const void *platdata;
>>   #if CONFIG_IS_ENABLED(OF_PLATDATA)
>>          uint platdata_size;
>> +       struct udevice *dev;
> Please also update comment for this struct
Sure
>>   #endif
>>   };
>>
>> @@ -43,4 +44,9 @@ struct driver_info {
>>   #define U_BOOT_DEVICES(__name)                                         \
>>          ll_entry_declare_list(struct driver_info, __name, driver_info)
>>
>> +/* Get a pointer to a given driver */
>> +#define DM_GET_DEVICE(__name)                                          \
>> +       ll_entry_get(struct driver_info, __name, driver_info)
>> +
>> +void populate_phandle_data(void);
> Function comment. Also can this ever fail?

It shouldn't. It basically does something like this

void populate_phandle_data(void) {
     dtv_dmc_at_20020000.clocks[0].node = DM_GET_DEVICE(clock_controller_at_20000000);
     dtv_dmc_at_20020000.clocks[1].node = DM_GET_DEVICE(clock_controller_at_20000000);
     dtv_serial_at_20064000.clocks[0].node = DM_GET_DEVICE(clock_controller_at_20000000);
     dtv_serial_at_20064000.clocks[1].node = DM_GET_DEVICE(clock_controller_at_20000000);
}

as we discuss previously, it is all about to be able to use DM_GET_DEVICE.

>>   #endif
>> --
>> 2.20.1
>>
Regards,

Walter