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

[Sean Anderson]

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.

>>
>> 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.

>>
>>> +		;
>>> +
>>> +	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.

>>
>>> + *
>>> + * @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
>>>
>