Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <e6848ae656e8e49ced429226d716f2b29cecfcba.camel@pengutronix.de>
Subject: Re: [PATCH v3] dmaengine: qcom: Add ADM driver
From:   Philipp Zabel <p.zabel@pengutronix.de>
To:     Jonathan McDowell <noodles@earth.li>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Dan Williams <dan.j.williams@intel.com>,
        Thomas Pedersen <twp@codeaurora.org>,
        Vinod Koul <vkoul@kernel.org>, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, dmaengine@vger.kernel.org
Date:   Mon, 21 Sep 2020 10:41:51 +0200
In-Reply-To: <20200920181204.GT3411@earth.li>
References: <20200916064326.GA13963@earth.li>
         <20200919185739.GS3411@earth.li> <20200920181204.GT3411@earth.li>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
User-Agent: Evolution 3.30.5-1.1 
MIME-Version: 1.0
Precedence: bulk

Hi Jonathan,

On Sun, 2020-09-20 at 19:12 +0100, Jonathan McDowell wrote:
> Add the DMA engine driver for the QCOM Application Data Mover (ADM) DMA
> controller found in the MSM8x60 and IPQ/APQ8064 platforms.
> 
> The ADM supports both memory to memory transactions and memory
> to/from peripheral device transactions.  The controller also provides
> flow control capabilities for transactions to/from peripheral devices.
> 
> The initial release of this driver supports slave transfers to/from
> peripherals and also incorporates CRCI (client rate control interface)
> flow control.
> 
> Signed-off-by: Andy Gross <agross@codeaurora.org>
> Signed-off-by: Thomas Pedersen <twp@codeaurora.org>
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
[...]

> +static int adm_dma_probe(struct platform_device *pdev)
> {
[...]
> +	adev->core_clk = devm_clk_get(adev->dev, "core");
> +	if (IS_ERR(adev->core_clk))
> +		return PTR_ERR(adev->core_clk);
> +
> +	ret = clk_prepare_enable(adev->core_clk);
> +	if (ret) {
> +		dev_err(adev->dev, "failed to prepare/enable core clock\n");
> +		return ret;
> +	}

It is better to only start enabling the hardware after all resources
have been acquired, see below.

> +	adev->iface_clk = devm_clk_get(adev->dev, "iface");
> +	if (IS_ERR(adev->iface_clk)) {
> +		ret = PTR_ERR(adev->iface_clk);
> +		goto err_disable_core_clk;
> +	}

Move this up before the core_clk enable, and you can directly return the
error.

> +
> +	ret = clk_prepare_enable(adev->iface_clk);
> +	if (ret) {
> +		dev_err(adev->dev, "failed to prepare/enable iface clock\n");
> +		goto err_disable_core_clk;
> +	}
> +
> +	adev->clk_reset = devm_reset_control_get(&pdev->dev, "clk");
> +	if (IS_ERR(adev->clk_reset)) {
> +		dev_err(adev->dev, "failed to get ADM0 reset\n");
> +		ret = PTR_ERR(adev->clk_reset);
> +		goto err_disable_clks;
> +	}
> +
> +	adev->c0_reset = devm_reset_control_get(&pdev->dev, "c0");
> +	if (IS_ERR(adev->c0_reset)) {
> +		dev_err(adev->dev, "failed to get ADM0 C0 reset\n");
> +		ret = PTR_ERR(adev->c0_reset);
> +		goto err_disable_clks;
> +	}
> +
> +	adev->c1_reset = devm_reset_control_get(&pdev->dev, "c1");
> +	if (IS_ERR(adev->c1_reset)) {
> +		dev_err(adev->dev, "failed to get ADM0 C1 reset\n");
> +		ret = PTR_ERR(adev->c1_reset);
> +		goto err_disable_clks;
> +	}
> +
> +	adev->c2_reset = devm_reset_control_get(&pdev->dev, "c2");
> +	if (IS_ERR(adev->c2_reset)) {
> +		dev_err(adev->dev, "failed to get ADM0 C2 reset\n");
> +		ret = PTR_ERR(adev->c2_reset);
> +		goto err_disable_clks;
> +	}

Please use devm_reset_control_get_exclusive(). Move these up before the
core_clk enable, and you can directly return the error.

regards
Philipp