Hi Patrick, On 15 October 2018 at 03:01, Patrice CHOTARD <patrice.chot...@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.chot...@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.chot...@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.chot...@st.com> >>>>> Reviewed-by: Simon Glass <s...@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(). > > 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(). Reviewed-by: Simon Glass <s...@chromium.org> _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
