Re: [PATCH 2/2] xilinx_dma: Add reset support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Ramiro,

Thank you for the patch.

On Wednesday 14 Dec 2016 17:18:24 Ramiro Oliveira wrote:
> Add a DT property to control an optional external reset line
> 
> Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>
> ---
>  drivers/dma/xilinx/xilinx_dma.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c
> b/drivers/dma/xilinx/xilinx_dma.c index 5c9f11b..b845224 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -46,6 +46,7 @@
>  #include <linux/slab.h>
>  #include <linux/clk.h>
>  #include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/reset.h>

I had neatly sorted the header alphabetically until someone added clk.h and 
io-64-nonatomic-lo-hi.h :-( Could you please move reset.h just before slab.h ?

> 
>  #include "../dmaengine.h"
> 
> @@ -409,6 +410,7 @@ struct xilinx_dma_device {
>  	struct clk *rxs_clk;
>  	u32 nr_channels;
>  	u32 chan_id;
> +	struct reset_control *rst;
>  };
> 
>  /* Macros */
> @@ -2543,6 +2545,27 @@ static int xilinx_dma_probe(struct platform_device
> *pdev) if (IS_ERR(xdev->regs))
>  		return PTR_ERR(xdev->regs);
> 
> +	xdev->rst = devm_reset_control_get_optional(&pdev->dev, "reset");

devm_reset_control_get_optional() is deprecated as explained in linux/reset.h, 
you should use devm_reset_control_get_optional_exclusive() or 
devm_reset_control_get_optional_shared() instead, as applicable.

This being said, I'm wondering why the optional versions of those functions 
exist, as they do exactly the same as the non-optional versions. The API feels 
wrong, it should have been modelled like the GPIO API. Feel free to fix it if 
you want :-) But that's out of scope for this patch.

> +	if (IS_ERR(xdev->rst)) {
> +		err = PTR_ERR(xdev->rst);
> +		switch (err) {
> +		case -ENOENT:

If you drop the name as proposed in the review of patch 1/2 you don't have to 
check for -ENOENT.

> +		case -ENOTSUPP:
> +			xdev->rst = NULL;
> +		break;

Wrong indentation.

You need to handle -EPROBE_DEFER and defer probing of the xilinx_dma device.

> +		default:
> +			dev_err(xdev->dev, "error getting reset %d\n", err);
> +		return err;

Wrong indentation.

> +		}
> +	} else {
> +		err = reset_control_deassert(xdev->rst);
> +		if (err) {
> +			dev_err(xdev->dev, "failed to deassert reset: %d\n",
> +					err);

Wrong indentation.

> +			return err;
> +		}
> +	}
> +
>  	/* Retrieve the DMA engine properties from the device tree */
>  	xdev->has_sg = of_property_read_bool(node, "xlnx,include-sg");
>  	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA)

-- 
Regards,

Laurent Pinchart