[U-Boot] [PATCH v3 3/9] dm: uclass: Add uclass_foreach_dev_probe

[Patrice CHOTARD]

Hi Simon

On 10/19/18 5:27 AM, Simon Glass wrote:
> Hi Patrick,
> 
> On 15 October 2018 at 03:01, Patrice CHOTARD <patrice.chotard at st.com> wrote:
>> Hi Bin
>>
>> On 10/12/2018 12:13 PM, Bin Meng wrote:
>>> Hi Patrice,
>>>
>>> On Fri, Oct 12, 2018 at 3:51 PM Patrice CHOTARD <patrice.chotard at st.com> wrote:
>>>>
>>>> Hi Bin
>>>>
>>>> On 10/11/2018 11:06 AM, Bin Meng wrote:
>>>>> Hi Patrice,
>>>>>
>>>>> On Tue, Oct 9, 2018 at 9:41 PM Patrice Chotard <patrice.chotard at st.com> wrote:
>>>>>>
>>>>>> Add uclass_foreach_dev_probe() which iterates through
>>>>>> devices of a given uclass. Devices are probed if necessary
>>>>>> and are ready to use.
>>>>>>
>>>>>> Signed-off-by: Patrice Chotard <patrice.chotard at st.com>
>>>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>>>>> ---
>>>>>>
>>>>>> Changes in v3: None
>>>>>> Changes in v2: None
>>>>>>
>>>>>>  include/dm/uclass.h | 16 ++++++++++++++++
>>>>>>  1 file changed, 16 insertions(+)
>>>>>>
>>>>>> diff --git a/include/dm/uclass.h b/include/dm/uclass.h
>>>>>> index 6e7c1cd3e8bc..10ccfdce951e 100644
>>>>>> --- a/include/dm/uclass.h
>>>>>> +++ b/include/dm/uclass.h
>>>>>> @@ -376,4 +376,20 @@ int uclass_resolve_seq(struct udevice *dev);
>>>>>>  #define uclass_foreach_dev_safe(pos, next, uc) \
>>>>>>         list_for_each_entry_safe(pos, next, &uc->dev_head, uclass_node)
>>>>>>
>>>>>> +/**
>>>>>> + * uclass_foreach_dev_probe() - Helper function to iteration through devices
>>>>>> + * of given uclass
>>>>>> + *
>>>>>> + * This creates a for() loop which works through the available devices in
>>>>>> + * a uclass in order from start to end. Devices are probed if necessary,
>>>>>> + * and ready for use.
>>>>>> + *
>>>>>> + * @id: Uclass ID
>>>>>> + * @dev: struct udevice * to hold the current device. Set to NULL when there
>>>>>> + * are no more devices.
>>>>>> + */
>>>>>> +#define uclass_foreach_dev_probe(id, dev)      \
>>>>>> +       for (uclass_first_device(id, &dev); dev; \
>>>>>> +            uclass_next_device(&dev))
>>>>>
>>>>> Shouldn't we check the return value of uclass_first_device() and
>>>>> uclass_next_device()?
>>>>
>>>> It's not necessary to check the return value of uclass_first_device(id,
>>>> &dev) because in any error case, dev is NULL, which is the loop output
>>>> condition. This is only for the first iteration.
>>>>
>>>
>>> Please see the comment of uclass_first_device().
>>>
>>>  * @devp: Returns pointer to the first device in that uclass if no error
>>>  * occurred, or NULL if there is no first device, or an error occurred with
>>>  * that device.
>>>  * @return 0 if OK (found or not found), other -ve on error
>>>
>>> So it can return error with a valid dev.
>>
>> I checked the uclass_first_device() implementation and the comment seems
>> not aligned with what is coded. When uclass_first_device() returns a
>> valid dev, return value is always 0. (see uclass_get_device_tail()
>> return value).
> 
> I actually don't like this function. Let's use
> uclass_first_device_err() when we care about errors. I am wondering if
> we should delete uclass_first_device().

Ok i will use uclass_first_device_err() instead

>>
>> In any case uclass_first_device() returns a valid dev with an error.
>>
>> Simon, Bin do you confirm ?
> 
> I don't see how, no.
>>
>>>
>>>> For the other iteration, dev comes from uclass_next_device(&dev),
>>>> similarly to uclass_first_device(), in any error case dev is NULL.
>>>>
>>>
>>> Similar please see the comment of uclass_next_device().
>>
>> I have the same remark regarding uclass_next_device().

I will add a new uclass_next_device_err()

Thanks

Patrice

> 
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
>