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

[Marek Vasut]

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