[PATCH v4 1/2] dm: core: add function uclass_probe_all() to probe all devices

[Vabhav Sharma (OSS)]

> -----Original Message-----
> From: Sean Anderson <seanga2 at gmail.com>
> Sent: Monday, November 23, 2020 6:54 PM
> To: Vabhav Sharma (OSS) <vabhav.sharma at oss.nxp.com>;
> sjg at chromium.org; sr at denx.de
> Cc: u-boot at lists.denx.de; Varun Sethi <V.Sethi at nxp.com>;
> andre.przywara at arm.com
> Subject: Re: [PATCH v4 1/2] dm: core: add function uclass_probe_all() to
> probe all devices
> 
> On 11/23/20 3:06 AM, Vabhav Sharma (OSS) wrote:
> >
> >
> >> -----Original Message-----
> >> From: Sean Anderson <seanga2 at gmail.com>
> >> Sent: Thursday, November 19, 2020 9:05 AM
> >> To: Vabhav Sharma (OSS) <vabhav.sharma at oss.nxp.com>;
> >> sjg at chromium.org; sr at denx.de
> >> Cc: u-boot at lists.denx.de; Varun Sethi <V.Sethi at nxp.com>;
> >> andre.przywara at arm.com; Vabhav Sharma <vabhav.sharma at nxp.com>
> >> Subject: Re: [PATCH v4 1/2] dm: core: add function uclass_probe_all()
> >> to probe all devices
> >>
> >> On 11/17/20 10:00 AM, Vabhav Sharma wrote:
> >>> From: Vabhav Sharma <vabhav.sharma at nxp.com>
> >>>
> >>> Support a common method to probe all devices associated with uclass.
> >>>
> >>> This includes data structures and code for finding the first device
> >>> and looping for remaining devices associated with uclasses (groups
> >>> of devices with the same purpose, e.g. all SERIAL ports will be in
> >>> the same
> >> uclass).
> >>>
> >>> An example is SBSA compliant PL011 UART IP, where firmware does the
> >>> serial port initialization and prepare uart device to let the kernel
> >>> use it for sending and reveiving the characters.SERIAL uclass will
> >>> use this function to initialize PL011 UART ports.
> >>>
> >>> The feature is enabled with CONFIG_DM.
> >>>
> >>> Signed-off-by: Vabhav Sharma <vabhav.sharma at nxp.com>
> >>> Reviewed-by: Stefan Roese <sr at denx.de>
> >>> Reviewed-by: Simon Glass <sjg at chromium.org>
> >>> --
> >>>    v4:
> >>>    Incorporated review comments of Simon
> >>>    Removed if (dev)..  conditional check
> >>>
> >>>    v3:
> >>>    Incorporated review comments of Stephan,Simon
> >>>    Related discussion
> >>> https://patchwork.ozlabs.org/project/uboot/patch/1601400
> >>> 385-11854-1-git-send-email-vabhav.sharma at oss.nxp.com/
> >>> ---
> >>>   drivers/core/uclass.c | 16 ++++++++++++++++
> >>>   include/dm/uclass.h   | 12 ++++++++++++
> >>>   2 files changed, 28 insertions(+)
> >>>
> >>> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index
> >>> c3f1b73..a1dc8bb 100644
> >>> --- a/drivers/core/uclass.c
> >>> +++ b/drivers/core/uclass.c
> >>> @@ -792,6 +792,22 @@ int uclass_pre_remove_device(struct udevice
> >> *dev)
> >>> }  #endif
> >>>
> >>> +int uclass_probe_all(enum uclass_id id) {
> >>> +	struct udevice *dev;
> >>> +	int ret;
> >>> +
> >>> +	ret = uclass_first_device(id, &dev);
> >>> +	if (ret || !dev)
> >>> +		return ret;
> >>> +
> >>> +	/* Scanning uclass to probe all devices */
> >>> +	for (; dev; uclass_next_device(&dev))
> >>
> >> You must check the return value of this function.
> > Error check is done for first device before passing the device to
> > uclass_next_device(), I think of other implementation is to combine
> > the first device check and iterating through device list of u-class as for (ret =
> uclass_first_device(id, &dev); dev && !ret;  ret = uclass_next_device(&dev))
> > 	;
> > But iteration is not required if first device is not found and current
> > changes seems to be ok Please share valuable feedback
> 
> 
> If the second (or any device after the first) fails to init, then the error will be
> silently ignored.
I see
> 
> >>
> >> Also, I would suggest using a while loop instead of an empty for loop.
> > Please elaborate, Found for loop best suitable to use here
> 
> In terms of original functionality,
> 
> while (dev)
> 	uclass_next_device(&dev)
> 
> However, I suggest
> 
> while (dev) {
> 	ret = uclass_next_device(&dev)
> 	if (ret)
> 		return ret;
> }
> 
> So that errors are handled properly.
> 
Sure, I will verify the changes to push next version 
> >>
> >>> +		;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>>   UCLASS_DRIVER(nop) = {
> >>>   	.id		= UCLASS_NOP,
> >>>   	.name		= "nop",
> >>> diff --git a/include/dm/uclass.h b/include/dm/uclass.h index
> >>> 7188304..7ac0aaa 100644
> >>> --- a/include/dm/uclass.h
> >>> +++ b/include/dm/uclass.h
> >>> @@ -381,6 +381,18 @@ int uclass_first_device_drvdata(enum uclass_id
> >>> id, ulong driver_data,  int uclass_resolve_seq(struct udevice *dev);
> >>>
> >>>   /**
> >>> + * uclass_probe_all() - Probe all devices based on an uclass ID
> >>> + *
> >>> + * Every uclass is identified by an ID, a number from 0 to n-1
> >>> + where n is
> >>> + * the number of uclasses. This function probe all devices
> >>> + asocciated with
> >>
> >> nit: probes associated
> > Ok
> >>
> >>> + * a uclass by looking its ID.
> >>
> >> nit: for its
> > Sure
> >>
> >> AFAICT uclass_find_next_device walks the linked-list of devices in a
> >> uclass, and does not care about the ID. So this documentation is incorrect.
> > This documentation is for new function uclass_probe_all() and understand
> each Uclass is identified by enum ID e.g. UCLASS_SERIAL for serial devices.
> > According used the statement  "Every uclass is identified by an ID"
> >
> > Please suggest.
> 
> Ok, this documentation is confusing because the idea of uclasses being
> identified by their UCLASS_XXX id is a very common idea in U-Boot already.
> On my first reading, I thought you were instead referring to udevices being
> identified by id, when instead you were using udevice_get_next to get the
> device. I suggest documenting the method used to initialize device, and
> refrain from (re)documenting the method used to identify the uclass. If
> someone is confused about that, they need only refer to the definition of
> enum uclass_id.
Thank you, Will rephrase it
> 
> >>
> >>> + *
> >>> + * @id: uclass ID to look up
> >>> + * @return 0 if OK, other -ve on error  */ int
> >>> +uclass_probe_all(enum uclass_id id);
> >>> +
> >>> +/**
> >>>    * uclass_id_foreach_dev() - Helper function to iteration through
> devices
> >>>    *
> >>>    * This creates a for() loop which works through the available
> >>> devices in
> >>>
> >