Re: [PATCH v3] dmaengine: qcom: Add ADM driver

[Philipp Zabel]

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@xxxxxxxxxxxxxx>
> Signed-off-by: Thomas Pedersen <twp@xxxxxxxxxxxxxx>
> Signed-off-by: Jonathan McDowell <noodles@xxxxxxxx>
[...]

> +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