lkml.org 
[lkml]   [2016]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 2/2] xilinx_dma: Add reset support
Date
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

\
 
 \ /
  Last update: 2016-12-14 21:15    [W:0.152 / U:0.668 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site