Re: [PATCH v13 04/10] usb: dwc3: qcom: Add helper function to request threaded IRQ

From: Johan Hovold <johan@kernel.org>
To: Krishna Kurapati <quic_kriskura@quicinc.com>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Felipe Balbi <balbi@kernel.org>,
	Wesley Cheng <quic_wcheng@quicinc.com>,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
	quic_pkondeti@quicinc.com, quic_ppratap@quicinc.com,
	quic_jackp@quicinc.com, ahalaney@redhat.com,
	quic_shazhuss@quicinc.com,
	Bjorn Andersson <quic_bjorande@quicinc.com>
Subject: Re: [PATCH v13 04/10] usb: dwc3: qcom: Add helper function to request threaded IRQ
Date: Fri, 20 Oct 2023 14:30:27 +0200	[thread overview]
Message-ID: <ZTJy456413VVT8Uv@hovoldconsulting.com> (raw)
In-Reply-To: <20231007154806.605-5-quic_kriskura@quicinc.com>

On Sat, Oct 07, 2023 at 09:18:00PM +0530, Krishna Kurapati wrote:
> Cleanup setup irq call by implementing a new prep_irq helper function
> and using it to request threaded IRQ's.

Please replace this with:

	Refactor interrupt setup by adding a new helper function for
	requesting the wakeup interrupts.

and similarly for Subject ("wakeup interrupts").

> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> Reviewed-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 59 ++++++++++++++++--------------------
>  1 file changed, 26 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 3de43df6bbe8..ef2006db7601 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -535,6 +535,24 @@ static int dwc3_qcom_get_irq(struct platform_device *pdev,
>  	return ret;
>  }
>  
> +static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name,
> +				char *disp_name, int irq)

Please rename this one dwc3_qcom_request_irq() so that is obvious what
it does without having to look at the implementation.

This series eventually makes the driver only call this with irq_name ==
disp_name so just drop the latter and rename the parameter as "name" and
mention that in the commit message.

Also move irq before name and add the missing const. That is:

	static int dwc3_qcom_request_irq(struct dwc3_qcom *qcom, int irq, const char *name);

> +{
> +	int ret;
> +
> +	/* Keep wakeup interrupts disabled until suspend */
> +	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,
> +					disp_name, qcom);
> +

Drop stray newline.

> +	if (ret)
> +		dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret);

Please spell out

	"failed to request irq %s: %d\n"

> +
> +	return ret;
> +}

Johan