Message-ID: <14fc724c-bc99-4b5d-9893-3e5eff8895f7@quicinc.com>
Date:   Mon, 23 Oct 2023 00:11:45 +0530
From:   Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To:     Johan Hovold <johan@...nel.org>
CC:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        "Andy Gross" <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        "Konrad Dybcio" <konrad.dybcio@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Felipe Balbi <balbi@...nel.org>,
        Wesley Cheng <quic_wcheng@...cinc.com>,
        <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <quic_pkondeti@...cinc.com>, <quic_ppratap@...cinc.com>,
        <quic_jackp@...cinc.com>, <ahalaney@...hat.com>,
        <quic_shazhuss@...cinc.com>
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM
 Glue driver



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@...cinc.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,

Powered by blists - more mailing lists