Messages in this thread | | | Date | Wed, 9 Aug 2023 11:36:48 +0530 | Subject | Re: [PATCH v10 06/11] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver | From | Krishna Kurapati PSSNV <> |
| |
On 8/8/2023 5:20 PM, Konrad Dybcio wrote: > On 8.08.2023 10:32, Krishna Kurapati PSSNV wrote: >> + >>>> +enum dwc3_qcom_phy_irq_identifier { >>>> + HS_PHY_IRQ = 0, >>>> + DP_HS_PHY_IRQ, >>>> + DM_HS_PHY_IRQ, >>>> + SS_PHY_IRQ, >>>> }; >>> >>> This enum is unused. >>> >> >> Hi Bjorn, >> >> I didn't use the enum directly, but used its members in the get_port_irq call below. >> >>> [..] >>>> +static int dwc3_get_acpi_index(const struct dwc3_acpi_pdata *pdata, int irq_index) >>>> +{ >>>> + int acpi_index = -1; >>>> + >>>> + if (!pdata) >>>> + return -1; >>>> + >>>> + if (irq_index == DP_HS_PHY_IRQ) >>>> + acpi_index = pdata->dp_hs_phy_irq_index; >>>> + else if (irq_index == DM_HS_PHY_IRQ) >>>> + acpi_index = pdata->dm_hs_phy_irq_index; >>>> + else if (irq_index == SS_PHY_IRQ) >>>> + acpi_index = pdata->ss_phy_irq_index; >>> >>> It looks favourable to put these in an array, instead of having to pull >>> them out of 4 different variables conditionally. >>> >>>> + >>>> + return acpi_index; >>>> +} >>>> + >>>> +static int dwc3_get_port_irq(struct platform_device *pdev, u8 port_index) >>>> +{ >>>> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >>>> + bool is_mp_supported = (qcom->data->num_ports > 1) ? true : false; >>>> + const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata; >>>> + char *disp_name; >>>> + int acpi_index; >>>> + char *dt_name; >>>> + int ret; >>>> + int irq; >>>> + int i; >>>> + >>>> + /* >>>> + * We need to read only DP/DM/SS IRQ's here. >>>> + * So loop over from 1->3 and accordingly modify respective phy_irq[]. >>>> + */ >>>> + for (i = 1; i < MAX_PHY_IRQ; i++) { >>>> + >>>> + if (!is_mp_supported && (port_index == 0)) { >>>> + if (i == DP_HS_PHY_IRQ) { >>>> + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, >>>> + "dp_hs_phy_irq"); >>>> + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, >>>> + "qcom_dwc3 DP_HS"); >>>> + } else if (i == DM_HS_PHY_IRQ) { >>>> + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, >>>> + "dm_hs_phy_irq"); >>>> + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, >>>> + "qcom_dwc3 DM_HS"); >>>> + } else if (i == SS_PHY_IRQ) { >>>> + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, >>>> + "ss_phy_irq"); >>>> + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, >>>> + "qcom_dwc3 SS"); >> Bjorn, Konrad, >> >> If we are to remove this repetitive loops, we might need to make a 2D array for all of Dp/Dm/Ss interrutps and make a global array of names to be used for irq lookup and use them to reduce the if-else-if stuff here. If that is fine, I can make those changes, else I would like to stick to this approach for now because if we don't add the global array of names, prepping them seperately for dp/dm/ss would again lead us to making if-else loops like above. >> >> Please let me know your thoughts on this. > Can we not just reuse the associated interrupt-names from the devicetree > if present? > Hi Konrad,
Thanks for the comments but one more confirmation. We can read the interrupts from DT but I believe the compatible would still need to stay. We need the num_ports information not just for registering interrupts but for modifying the pwr_event_irq registers during suspend/resume. If we rely on the interrupts to find the number of ports, the user is free to remove any IRQ and we might end up in a situation where glue and core are not having same view of how many number of ports present. So I believe its best to keep the compatible and get num_ports info from there and rely on reading interrupt-names to get interrupts cleanly. Can you let me know your view on the same.
Regards, Krishna,
| |