[PATCH] usb: cdns3: use VBUS Valid to determine role for dr_mode OTG

[Siddharth Vadapalli]

On 12/02/26 8:12 PM, Marek Vasut wrote:
> On 2/5/26 2:11 AM, Siddharth Vadapalli wrote:
> 
> [...]
> 
>>>> +static enum usb_dr_mode cdns3_get_otg_mode(ofnode node)
>>>> +{
>>>> +    struct cdns3 cdns, *cdnsp;
>>>> +    void __iomem *otg_regs;
>>>> +    fdt_addr_t otg_addr;
>>>> +    int otg_reg_index;
>>>> +    int vbus;
>>>> +
>>>> +    otg_reg_index = ofnode_stringlist_search(node, "reg-names", 
>>>> "otg");
>>>> +    if (otg_reg_index < 0)
>>>> +        return USB_DR_MODE_OTG;
>>> This is wrong, the function should return error and possibly mode via 
>>> function parameter , do not conflate return value and mode please .
>>
>> Regarding returning an error, the patch maintains status quo for the 
>> case where we are unable to read the USB OTG Status register. Please 
>> refer to the following diff in the patch:
>>
>>
>>      @@ -413,6 +462,10 @@ int cdns3_bind(struct udevice *parent)
>>           if (dr_mode == USB_DR_MODE_UNKNOWN)
>>               dr_mode = usb_get_dr_mode(dev_ofnode(parent));
>>
>>      +    /* Use VBUS Valid to determine role */
>>      +    if (dr_mode == USB_DR_MODE_OTG)
>>      +        dr_mode = cdns3_get_otg_mode(node);
>>      +
>>           switch (dr_mode) {
>>       #if defined(CONFIG_SPL_USB_HOST) || \
>>           (!defined(CONFIG_XPL_BUILD) && defined(CONFIG_USB_HOST))
>>
>> Only if the 'dr_mode' is already 'OTG', the newly added function 
>> cdns3_get_otg_mode() is invoked. In case cdns3_get_otg_mode() cannot 
>> determine the mode (otg status register cannot be read), we return otg 
>> mode to maintain existing behavior, following which, the code will use 
>> the CONFIGS to determine the role.
>>
>> Returning an error will alter existing behavior rather than extending 
>> it for enhancing functionality.
>>
>> Regarding passing the mode via a function parameter, I could do so, 
>> but I followed the implementation of other functions such as
>>      usb_get_dr_mode()
>> which 'returns' the mode rather than using a function parameter to 
>> fill in the mode.
>>
>> Please let me know what you think.
> I think this patch is wrong, cdns3_get_otg_mode() should return an error

Ok, I will update it to return an error, but please note that it may 
alter existing behavior since the code is being introduced in the middle 
of cdns3_bind() and not at the end of it.

> code and also return mode via parameter. This:
> 
> static int cdns3_get_otg_mode(ofnode node, enum usb_dr_mode *mode)
> {
>     ...
>     if (error)
>        return -ESOMETHING;
>     ...
>     *mode = some mode
>     return 0;
> }
> 
> Do not conflate mode and error code, this leads to problems.

Ok. I will pass the mode as parameter to the function. Thank you for the 
clarification.

Regards,
Siddharth.