From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933152AbcLNUPg (ORCPT ); Wed, 14 Dec 2016 15:15:36 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:44872 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751845AbcLNUPd (ORCPT ); Wed, 14 Dec 2016 15:15:33 -0500 From: Laurent Pinchart To: Ramiro Oliveira Cc: dmaengine@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, vinod.koul@intel.com, robh+dt@kernel.org, mark.rutland@arm.com, michal.simek@xilinx.com, soren.brinkmann@xilinx.com, appana.durga.rao@xilinx.com, anuragku@xilinx.com, dan.j.williams@intel.com, CARLOS.PALMINHA@synopsys.com Subject: Re: [PATCH 2/2] xilinx_dma: Add reset support Date: Wed, 14 Dec 2016 22:16:07 +0200 Message-ID: <5658430.jXmKZEXWrS@avalon> User-Agent: KMail/4.14.10 (Linux/4.8.6-gentoo; KDE/4.14.24; x86_64; ; ) In-Reply-To: <4220c66c29d83bec1ded798ee383b5460c162cfc.1481735244.git.roliveir@synopsys.com> References: <4220c66c29d83bec1ded798ee383b5460c162cfc.1481735244.git.roliveir@synopsys.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 ? > > #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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Wed, 14 Dec 2016 22:16:07 +0200 Subject: [PATCH 2/2] xilinx_dma: Add reset support In-Reply-To: <4220c66c29d83bec1ded798ee383b5460c162cfc.1481735244.git.roliveir@synopsys.com> References: <4220c66c29d83bec1ded798ee383b5460c162cfc.1481735244.git.roliveir@synopsys.com> Message-ID: <5658430.jXmKZEXWrS@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 ? > > #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