On Thu, Mar 07, 2024 at 11:50:50AM +0530, Krishna Kurapati wrote: > On multiport supported controllers, each port has its own DP/DM > and SS (if super speed capable) interrupts. As per the bindings, > their interrupt names differ from standard ones having "_x" added > as suffix (x indicates port number). Identify from the interrupt > names whether the controller is a multiport controller or not. > Refactor dwc3_qcom_setup_irq() call to parse multiport interrupts > along with non-multiport ones accordingly.. > > Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx> > --- > drivers/usb/dwc3/dwc3-qcom.c | 146 +++++++++++++++++++++++++++-------- > 1 file changed, 112 insertions(+), 34 deletions(-) This is much better. Just a couple of nits below. > +static int dwc3_qcom_find_num_ports(struct platform_device *pdev) > +{ > + const char *irq_name; > + int port_index; > + int irq; > + > + irq = platform_get_irq_byname_optional(pdev, "qusb2_phy"); > + if (irq > 0) > + return 1; > + > + irq = platform_get_irq_byname_optional(pdev, "dp_hs_phy_irq"); > + if (irq > 0) > + return 1; > + > + for (port_index = 0; port_index < DWC3_MAX_PORTS; port_index++) { > + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dp_hs_phy_%d", port_index + 1); Please use a stack allocated buffer for these strings as we don't need them any more after this function returns. > + if (!irq_name) > + return -ENOMEM; > + > + irq = platform_get_irq_byname_optional(pdev, irq_name); > + if (irq <= 0) > + return port_index; > + } > + > + return port_index; I think explicitly returning DWC3_MAX_PORTS here would be more readable. > +} > + > +static int dwc3_qcom_setup_irq(struct platform_device *pdev) > +{ > + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > + bool is_multiport; > + int ret; > + int i; > + > + qcom->num_ports = dwc3_qcom_find_num_ports(pdev); > + if (qcom->num_ports < 0) > + return -ENOMEM; Just return 'ret' directly. > + > + is_multiport = (qcom->num_ports > 1) ? true : false; And no need for the ternary operator: is_multiport = (qcom->num_ports > 1); > + > + for (i = 0; i < qcom->num_ports; i++) { > + ret = dwc3_qcom_setup_port_irq(pdev, i, is_multiport); > if (ret) > return ret; > - qcom->ss_phy_irq = irq; > } > > return 0; With that fixed: Reviewed-by: Johan Hovold <johan+linaro@xxxxxxxxxx>