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

[Marek Vasut]

On 2/12/26 4:30 PM, Siddharth Vadapalli wrote:
> 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.
Thank you