Messages in this thread | | | Date | Mon, 3 Jul 2023 00:29:41 +0530 | Subject | Re: [PATCH v9 06/10] usb: dwc3: qcom: Add support to read IRQ's related to multiport | From | Krishna Kurapati PSSNV <> |
| |
On 6/27/2023 8:01 PM, Johan Hovold wrote: > On Wed, Jun 21, 2023 at 10:06:24AM +0530, Krishna Kurapati wrote: >> Add support to read Multiport IRQ's related to quad port controller >> of SA8295 Device. > > Please use a more descriptive summary and commit message; "read" is to > vague. You're looking up interrupts from the devicetree. Also this > should not just be about SA8295. > >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 108 +++++++++++++++++++++++++++++------ >> 1 file changed, 91 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> index 3de43df6bbe8..3ab48a6925fe 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -74,9 +74,9 @@ struct dwc3_qcom { >> struct reset_control *resets; >> >> int hs_phy_irq; >> - int dp_hs_phy_irq; >> - int dm_hs_phy_irq; >> - int ss_phy_irq; >> + int dp_hs_phy_irq[4]; >> + int dm_hs_phy_irq[4]; >> + int ss_phy_irq[2]; > > As has already been pointed out, you should use a define for these. And > you already have DWC3_MAX_PORTS. > > The driver should not be hardcoding the fact that there are only two SS > ports on this particular SoC that you're interested in. > >> enum usb_device_speed usb2_speed; >> >> struct extcon_dev *edev; > >> @@ -535,6 +535,80 @@ static int dwc3_qcom_get_irq(struct platform_device *pdev, >> return ret; >> } >> >> +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev) >> +{ >> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >> + char irq_name[15]; >> + int irq; >> + int ret; >> + int i; >> + >> + for (i = 0; i < 4; i++) { > > DWC3_MAX_PORTS here too and similar below. > >> + if (qcom->dp_hs_phy_irq[i]) >> + continue; > > This is not very nice. You should try to integrate the current lookup > code as I told you to do with the PHY lookups. That is, use a single > loop for all HS/SS IRQs, and pick the legacy name if the number of ports > is 1. > > Of course, you added the xhci capability parsing to the core driver so > that information is not yet available, but you need it in the glue > driver also... > > As I mentioned earlier, you can infer the number of ports from the > interrupt names. Alternatively, you can infer it from the compatible > string. In any case, you should not need to ways to determine the same > information in the glue driver, then in the core part, and then yet > again in the xhci driver... > Hi Johan,
The reason why I didn't integrate this with the original function was the ACPI stuff. The MP devices have no ACPI variant. And I think for clarity sake its best to keep these two functions separated.
Regards, Krishna,
>> + >> + sprintf(irq_name, "dp%d_hs_phy_irq", i+1); > > Spaces around binary operators. Does not checkpatch warn about that? > >> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); >> + if (irq > 0) { >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, >> + qcom_dwc3_resume_irq, >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >> + irq_name, qcom); >> + if (ret) { >> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); >> + return ret; >> + } >> + } >> + >> + qcom->dp_hs_phy_irq[i] = irq; >> + } >> + >> + for (i = 0; i < 4; i++) { >> + if (qcom->dm_hs_phy_irq[i]) >> + continue; >> + >> + sprintf(irq_name, "dm%d_hs_phy_irq", i+1); >> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); >> + if (irq > 0) { >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, >> + qcom_dwc3_resume_irq, >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >> + irq_name, qcom); >> + if (ret) { >> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); >> + return ret; >> + } >> + } >> + >> + qcom->dm_hs_phy_irq[i] = irq; >> + } >> + >> + for (i = 0; i < 2; i++) { >> + if (qcom->ss_phy_irq[i]) >> + continue; >> + >> + sprintf(irq_name, "ss%d_phy_irq", i+1); >> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); >> + if (irq > 0) { >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, >> + qcom_dwc3_resume_irq, >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >> + irq_name, qcom); >> + if (ret) { >> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); >> + return ret; >> + } >> + } >> + >> + qcom->ss_phy_irq[i] = irq; >> + } > > So the above should all be merged in either a single helper looking up > all the interrupts for a port and resused for the non-MP case. > I agree, Will merge all under some common helper function.
Thanks, Krishna, >> + >> + return 0; >> +} >> + >> static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> { >> struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >> @@ -570,7 +644,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret); >> return ret; >> } >> - qcom->dp_hs_phy_irq = irq; >> + qcom->dp_hs_phy_irq[0] = irq; >> } >> >> irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq", >> @@ -585,7 +659,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret); >> return ret; >> } >> - qcom->dm_hs_phy_irq = irq; >> + qcom->dm_hs_phy_irq[0] = irq; >> } >> >> irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq", >> @@ -600,10 +674,10 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret); >> return ret; >> } >> - qcom->ss_phy_irq = irq; >> + qcom->ss_phy_irq[0] = irq; >> } >> >> - return 0; >> + return dwc3_qcom_setup_mp_irq(pdev);; > > Stray ; > >> } >> >> static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count) > > Johan
| |