Messages in this thread | | | Date | Mon, 23 Oct 2023 00:11:45 +0530 | Subject | Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver | From | Krishna Kurapati PSSNV <> |
| |
On 10/20/2023 6:53 PM, Johan Hovold wrote: > First, drop "QCOM Glue driver" from Subject, you already have the "qcom" > prefix. > > On Sat, Oct 07, 2023 at 09:18:01PM +0530, Krishna Kurapati wrote: >> Refactor setup_irq call to facilitate reading multiport IRQ's along > > "IRQs" or just "interrupts" > >> with non mulitport ones. Read through the interrupt-names property > > "multiport" > > Please spell check all your patches (commit messages and code) before > posting, it's not the reviewers job. > >> to figure out, the type of interrupt (DP/DM/HS/SS) and to which port >> it belongs. Also keep track of port index to calculate port count >> based on interrupts provided as input in DT. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 210 +++++++++++++++++++++++++---------- >> 1 file changed, 154 insertions(+), 56 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> index ef2006db7601..863892284146 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -53,14 +53,25 @@ >> #define APPS_USB_AVG_BW 0 >> #define APPS_USB_PEAK_BW MBps_to_icc(40) >> >> +#define NUM_PHY_IRQ 4 >> + >> +enum dwc3_qcom_ph_index { > > "phy_index" > >> + DP_HS_PHY_IRQ_INDEX = 0, >> + DM_HS_PHY_IRQ_INDEX, >> + SS_PHY_IRQ_INDEX, >> + HS_PHY_IRQ_INDEX, >> +}; >> + >> struct dwc3_acpi_pdata { >> u32 qscratch_base_offset; >> u32 qscratch_base_size; >> u32 dwc3_core_base_size; >> + /* >> + * The phy_irq_index corresponds to ACPI indexes of (in order) DP/DM/SS >> + * IRQ's respectively. >> + */ >> + int phy_irq_index[NUM_PHY_IRQ - 1]; >> int hs_phy_irq_index; >> - int dp_hs_phy_irq_index; >> - int dm_hs_phy_irq_index; >> - int ss_phy_irq_index; >> bool is_urs; >> }; >> >> @@ -73,10 +84,12 @@ struct dwc3_qcom { >> int num_clocks; >> struct reset_control *resets; >> >> + /* >> + * The phy_irq corresponds to IRQ's registered for (in order) DP/DM/SS >> + * respectively. >> + */ >> + int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS]; >> int hs_phy_irq; >> - int dp_hs_phy_irq; >> - int dm_hs_phy_irq; >> - int ss_phy_irq; > > I'm not sure using arrays like this is a good idea (and haven't you > switched the indexes above?). > > Why not add a port structure instead? > > struct dwc3_qcom_port { > int hs_phy_irq; > int dp_hs_phy_irq; > int dm_hs_phy_irq; > int ss_phy_irq; > }; > > and then have > > struct dwc3_qcom_port ports[DWC3_MAX_PORTS]; > > in dwc3_qcom. The port structure can the later also be amended with > whatever other additional per-port data there is need for. > > This should make the implementation cleaner. > > I also don't like the special handling of hs_phy_irq; if this is really > just another name for the pwr_event_irq then this should be cleaned up > before making the code more complicated than it needs to be. > > Make sure to clarify this before posting a new revision. >
hs_phy_irq is different from pwr_event_irq. AFAIK, there is only one of this per controller.
>> -static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name, >> - char *disp_name, int irq) >> +static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, const char *irq_name, >> + const char *disp_name, int irq) > > Ok, here you did drop the second name parameter, but without renaming > the first and hidden in a long diff without any mention anywhere. > I didn't understand the comment. Can you please elaborate. I didn't drop the second parameter. In the usage of this call, I passed same value to both irq_name and disp_name:
"dwc3_qcom_prep_irq(qcom, irq_names[i], irq_names[i], irq);"
I mentioned in cover-letter that I am using same name as obtained from DT to register the interrupts as well. Should've mentioned that in commit text of this patch. Will do it in next version.
>> +static int dwc3_qcom_get_port_index(const char *irq_name, int irq_index) >> +{ >> + int port_index = -1; >> + >> + switch (irq_index) { >> + case DP_HS_PHY_IRQ_INDEX: >> + if (strcmp(irq_name, "dp_hs_phy_irq") == 0) >> + port_index = 1; >> + else >> + sscanf(irq_name, "dp_hs_phy_%d", &port_index); >> + break; >> + > > No need for newlines after break. > >> + case DM_HS_PHY_IRQ_INDEX: >> + if (strcmp(irq_name, "dm_hs_phy_irq") == 0) >> + port_index = 1; >> + else >> + sscanf(irq_name, "dm_hs_phy_%d", &port_index); >> + break; >> + >> + case SS_PHY_IRQ_INDEX: >> + if (strcmp(irq_name, "ss_phy_irq") == 0) >> + port_index = 1; >> + else >> + sscanf(irq_name, "ss_phy_%d", &port_index); >> + break; >> + >> + case HS_PHY_IRQ_INDEX: >> + port_index = 1; >> + break; >> + } >> + >> + if (port_index > DWC3_MAX_PORTS) >> + port_index = -1; >> + >> + return port_index; >> +} > >> static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> { >> struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >> - const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata; >> + struct device_node *np = pdev->dev.of_node; >> + const char **irq_names; >> + int port_index; >> + int acpi_index; >> + int irq_count; >> + int irq_index; >> int irq; >> int ret; >> + int i; >> >> - irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq", >> - pdata ? pdata->hs_phy_irq_index : -1); >> - if (irq > 0) { >> - ret = dwc3_qcom_prep_irq(qcom, "hs_phy_irq", "qcom_dwc3 HS", irq); >> - if (ret) >> - return ret; >> - qcom->hs_phy_irq = irq; >> - } >> + irq_count = of_property_count_strings(np, "interrupt-names"); > > of_property_count_strings() can return negative errnos and you don't > have any sanity checks for the return value... > > Please slow down, and also make sure to get your patches reviewed > internally before posting new revisions. > It did go through internal review but probably went un-noticed. Will add sanity checks here and below.
>> + irq_names = devm_kzalloc(&pdev->dev, sizeof(*irq_names) * irq_count, GFP_KERNEL); > > devm_kcalloc() > >> + if (!irq_names) >> + return -ENOMEM; >> >> - irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq", >> - pdata ? pdata->dp_hs_phy_irq_index : -1); >> - if (irq > 0) { >> - ret = dwc3_qcom_prep_irq(qcom, "dp_hs_phy_irq", "qcom_dwc3 DP_HS", irq); >> - if (ret) >> - return ret; >> - qcom->dp_hs_phy_irq = irq; >> - } >> + ret = of_property_read_string_array(np, "interrupt-names", >> + irq_names, irq_count); > > No sanity check here either? > >> + for (i = 0; i < irq_count; i++) { >> + irq_index = dwc3_qcom_get_irq_index(irq_names[i]); >> + if (irq_index == -1) { >> + dev_dbg(&pdev->dev, "Invalid IRQ not handled"); >> + continue; >> + } > > I'll just stop reviewing here. This is a waste of my time. >
Regards, Krishna,
| |