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

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

[Mattijs Korpershoek]

The dwc3 driver allows SoC-specific configuration via the dwc3_glue_ops
structure.
These glue drivers usually just implement the .glue_configure function.

For such glue drivers, we don't need to be a SIMPLE_BUS, since we
don't interact with any child devices.

Use UCLASS_NOP instead as done in the dwc3-generic-wrapper.

Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.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,

---
base-commit: 8dd7186ca7821446c6f46b6cccefab502912f2e0
change-id: 20250226-dwc3-am62-gadget-nop-4b33c79d8d5e

Best regards,
-- 
Mattijs Korpershoek <mkorpershoek@baylibre.com>

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

[Siddharth Vadapalli]

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.

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

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"

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

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

[Mattijs Korpershoek]

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.

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

[Siddharth Vadapalli]

On Thu, Feb 27, 2025 at 11:19:37AM +0100, Mattijs Korpershoek wrote:
> 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 ?

Yes.

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

Yes.

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

Understood. Thank you for clarifying and for rephrasing the commit
message above. Please feel free to add my Reviewed-by tag when posting
the v2 patch.


Regards,
Siddharth.

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

[Mattijs Korpershoek]

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

[...]

>> 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().
>
> Understood. Thank you for clarifying and for rephrasing the commit
> message above. Please feel free to add my Reviewed-by tag when posting
> the v2 patch.

Great, will do. Thanks again for the timely and useful review!

>
>
> Regards,
> Siddharth.