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.
