lkml.org 
[lkml]   [2016]   [Dec]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 2/2] xilinx_dma: Add reset support
    From
    Date
    Hi Laurent,

    Thank you for your feedback.

    On 12/14/2016 8:16 PM, Laurent Pinchart wrote:
    > 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 ?
    >

    Sure. Actually I was tempted to reorder it, but I decided not to do it. I'll do
    it now

    >>
    >> #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.
    >

    I missed the comment stating that devm_reset_control_get_optional() was deprecated.

    I could fix it. Your sugestion is modelling these functions like the GPIO API?

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

    I'll do that.

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

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