Re: [PATCH] usb: dwc3: am62: Use UCLASS_NOP for glue driver

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Siddharth,

Thank you for the the review.

On jeu., févr. 27, 2025 at 13:44, Siddharth Vadapalli <s-vadapalli@ti.com> wrote:

> On Wed, Feb 26, 2025 at 03:47:11PM +0100, Mattijs Korpershoek wrote:
>
> Hello Mattijs,
>
>> The dwc3 driver allows SoC-specific configuration via the dwc3_glue_ops
>> structure.
>> These glue drivers usually just implement the .glue_configure function.
>
> nitpick: Is there a reason behind an abrupt newline above? The sentence:
> "These glue drivers..." could continue on the same line as the previous
> sentence. Also, "These glue drivers..." is referring to glue drivers
> which haven't been mentioned yet and it is the first time that they are
> being referred to. It is not clear to me if there was more content in
> between the first sentence and the second which accidentally got
> removed.

I'll rephrase as:

"""
The dwc3 driver allows SoC-specific configuration via the dwc3_glue_ops
structure. Glue drivers, which implement dwc3_glue_ops, usually just
define the .glue_configure() function.
"""

Does that looks good ?

>
>> 
>> For such glue drivers, we don't need to be a SIMPLE_BUS, since we
>> don't interact with any child devices.
>
> This sounds confusing. What does "we" refer to? The dwc3-am62.c driver
> which is being modified in this patch is *the* glue driver itself.
> "For such glue drivers" and "we don't need to be a SIMPLE_BUS" seem to
> be referring to two different entities when they are probably the same.
>
> "For such glue drivers" => dwc3-am62.c
> "we don't need to be a SIMPLE_BUS" => Again dwc3-am62.c ?

I'll rephrase as:

"""
dwc3-am62 is such glue driver which does not need to be a SIMPLE_BUS
since it does not interact with any child device.
"""

Is that okay ?

>
> Did you mean something like the following?
> "Glue drivers don't need to be a SIMPLE_BUS since they don't interact
> with any child devices"

I can't write that down, because that's not true. Some glue drivers (like
dwc3-meson-g12a) need to be SIMPLE_BUS because they rely on
.child_pre_probe() and .child_post_remove().

>
>> 
>> Use UCLASS_NOP instead as done in the dwc3-generic-wrapper.
>> 
>> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>
> The change made in the driver seems good to me, so with the above
> questions addressed:
>
> Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>
>> ---
>> This is a small cleanup intended for next.
>> 
>> This has been tested on AM62X SK EVM using:
>> => fastboot usb 0
>> 
>> $ fastboot devices
>> ????????????     Android Fastboot
>> ---
>>  drivers/usb/dwc3/dwc3-am62.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/usb/dwc3/dwc3-am62.c b/drivers/usb/dwc3/dwc3-am62.c
>> index 99519602eb2c66299445163fafcdb3e065c89eda..66164d0a80c0264bfc400426782324f8a57b8bd8 100644
>> --- a/drivers/usb/dwc3/dwc3-am62.c
>> +++ b/drivers/usb/dwc3/dwc3-am62.c
>> @@ -116,7 +116,7 @@ static const struct udevice_id dwc3_am62_match[] = {
>>  
>>  U_BOOT_DRIVER(dwc3_am62_wrapper) = {
>>  	.name	= "dwc3-am62",
>> -	.id	= UCLASS_SIMPLE_BUS,
>> +	.id	= UCLASS_NOP,
>>  	.of_match = dwc3_am62_match,
>>  	.bind = dwc3_glue_bind,
>>  	.probe = dwc3_glue_probe,
>
> Regards,
> Siddharth.