Re: [PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

[Roger Quadros <rogerq@kernel.org>]

On 04/12/2024 15:37, Siddharth Vadapalli wrote:
> On Wed, Dec 04, 2024 at 02:34:56PM +0200, Roger Quadros wrote:
>>
>>
>> On 04/12/2024 12:05, Siddharth Vadapalli wrote:
> 
> [...]
> 
>>> Why not do what this patch does? What is the issue with the current
>>> patch (apart from the checks, which I could retain as well if others
>>> wish so)? The caller clearly mentions the expected role, so why not use
>>> that?
>>
>> Because the patch is more complicated than it needs to be, now that we know:
>> 1) generic_host_probe() will only be called when dr_mode is "host",
>> 2) generic_peripheral_probe() will only be called when dr_mode is "peripheral"
>> or "otg".
>>
>> The problem case is generic_peripheral_probe() being called with when
>> dr_mode is "otg".
> 
> We can also be sure that generic_periperhal_probe() expects dr_mode to
> be "peripheral" only and not "otg". So why not fix it at the root, which

generic_peripheral_probe() expects nothing from dr_mode.
It just expects the USB controller to work in peripheral mode.

dr_mode is just a reflection of platform/device-tree dr_mode. Let's keep it
that way and not abuse dr_mode to "peripheral" when it is actually "otg".

> is what this patch does? Independent of the issue, is the following a
> valid use-case?
> 
> generic_peripheral_probe() with plat->dr_mode = "otg" and hence
> generic_probe() using "otg" rather than "peripheral"

It is a valid use case. The Platform says that the port is "otg"
i.e. (plat->dr_mode ="otg")
and the user says he wants to use it as peripheral
i.e. (by invoking generic_peripheral_probe())
Don't you see this as valid.

> as is the case now. Is this resulting in something functional for other
> users of the USB Designware Controller? If the user requests "peripheral"
> mode of operation, will "otg" meet the user's requirement? If yes, I have
> no further objections. Irrespective of this, if you believe that the fix
> should be in the core as you indicated below, please feel free to post
> the patch.

Let's look at this from the other way around.
Why do you not agree that the fix should be in core.c and abusing dr_mode
is the correct way to go?

> 
>>
>> Another alternative could be to override dr_mode to "peripheral" if
>> it was "otg" in dwc3_generic_probe before calling dwc3_init() but
>> I don't prefer that as we are loosing platform information here
>> and it will have to be reverted when dual-role support is added in core.c.
>>
>> So better to deal with this in core.c and add a note why we are doing it.
>>
> 
> [...]
> 
> Regards,
> Siddharth.

-- 
cheers,
-roger