Received-SPF: pass (google.com: domain of linux-kernel+bounces-91766-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 5 Mar 2024 08:09:30 +0100
From: Johan Hovold <johan@kernel.org>
To: Krishna Kurapati <quic_kriskura@quicinc.com>
Cc: Bjorn Andersson <andersson@kernel.org>,
	Wesley Cheng <quic_wcheng@quicinc.com>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org, quic_ppratap@quicinc.com,
	quic_jackp@quicinc.com
Subject: Re: [PATCH] usb: dwc3: qcom: Remove ACPI support from glue driver
Message-ID: <ZebFKlae0a-deBKl@hovoldconsulting.com>
References: <20240305042143.3455101-1-quic_kriskura@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240305042143.3455101-1-quic_kriskura@quicinc.com>

On Tue, Mar 05, 2024 at 09:51:43AM +0530, Krishna Kurapati wrote:
> Minimal ACPI support was added to the Qualcomm DWC3 glue driver in order to
> enable USB on SDM850 and SC8180X compute platforms. The support is still
> functional, but unnoticed regressions in other drivers indicates that no
> one actually booting any of platforms dependent on this implementation.
> 
> The functionality provides is the bare minimum and is not expected to aid
> in the effort of bringing full ACPI support to the driver in the future.
> 
> Remove the ACPI code from the Qualcomm DWC3 glue driver to aid in the
> implementation of improvements that are actually used like multiport and
> flattening device tree.

With a simple lookup function that returns the ACPI index based on name
this shouldn't be required to add multiport support even if it may
simplify it slightly. But IIRC it would help more with the devicetree
binding rework.
 
> Commit message by Bjorn Andersson.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 273 ++---------------------------------
>  1 file changed, 11 insertions(+), 262 deletions(-)

You should update the Kconfig entry for USB_DWC3_QCOM as well and drop
the ACPI dependency.

>  static int dwc3_qcom_probe(struct platform_device *pdev)
>  {
>  	struct device_node	*np = pdev->dev.of_node;
>  	struct device		*dev = &pdev->dev;
>  	struct dwc3_qcom	*qcom;
>  	struct resource		*res, *parent_res = NULL;

You should drop parent_res as well.

> -	struct resource		local_res;
>  	int			ret, i;
>  	bool			ignore_pipe_clk;
>  	bool			wakeup_source;
> @@ -825,14 +659,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, qcom);
>  	qcom->dev = &pdev->dev;
>  
> -	if (has_acpi_companion(dev)) {
> -		qcom->acpi_pdata = acpi_device_get_match_data(dev);
> -		if (!qcom->acpi_pdata) {
> -			dev_err(&pdev->dev, "no supporting ACPI device data\n");
> -			return -EINVAL;
> -		}
> -	}
> -
>  	qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
>  	if (IS_ERR(qcom->resets)) {
>  		return dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
> @@ -860,41 +686,18 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	}
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -
> -	if (np) {
> -		parent_res = res;
> -	} else {
> -		memcpy(&local_res, res, sizeof(struct resource));
> -		parent_res = &local_res;
> -
> -		parent_res->start = res->start +
> -			qcom->acpi_pdata->qscratch_base_offset;
> -		parent_res->end = parent_res->start +
> -			qcom->acpi_pdata->qscratch_base_size;
> -
> -		if (qcom->acpi_pdata->is_urs) {
> -			qcom->urs_usb = dwc3_qcom_create_urs_usb_platdev(dev);
> -			if (IS_ERR_OR_NULL(qcom->urs_usb)) {
> -				dev_err(dev, "failed to create URS USB platdev\n");
> -				if (!qcom->urs_usb)
> -					ret = -ENODEV;
> -				else
> -					ret = PTR_ERR(qcom->urs_usb);
> -				goto clk_disable;
> -			}
> -		}
> -	}
> +	parent_res = res;
>  
>  	qcom->qscratch_base = devm_ioremap_resource(dev, parent_res);

And just use res here.

>  	if (IS_ERR(qcom->qscratch_base)) {
>  		ret = PTR_ERR(qcom->qscratch_base);
> -		goto free_urs;
> +		goto clk_disable;
> }

Looks good to me otherwise.

Johan