From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ramiro Oliveira Subject: Re: [PATCH 2/2] xilinx_dma: Add reset support Date: Thu, 15 Dec 2016 11:26:54 +0000 Message-ID: <380d1b11-814d-0164-3d49-e976f2deb3f9@synopsys.com> References: <4220c66c29d83bec1ded798ee383b5460c162cfc.1481735244.git.roliveir@synopsys.com> <5658430.jXmKZEXWrS@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5658430.jXmKZEXWrS@avalon> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Laurent Pinchart Cc: Ramiro Oliveira , dmaengine-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, vinod.koul-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org, soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org, appana.durga.rao-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org, anuragku-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org, dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w@public.gmane.org List-Id: devicetree@vger.kernel.org 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 >> --- >> 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 >> #include >> #include >> +#include > > 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) > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html