Hi Ramiro, (CC'ing Philipp Zabel) On Thursday 15 Dec 2016 11:26:54 Ramiro Oliveira wrote: > 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@xxxxxxxxxxxx> > >> --- > >> > >> 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 Yeah, I'll sleep better tonight :-D > >> #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? I think it would be better for driver if the _get_optional functions would return an ERR_PTR() for errors and NULL when reset control is not available, and then have the rest of the reset controller API accept NULL as a no-op. Your implementation would then be xdev->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, "reset"); if (IS_ERR(xdev->rst)) { err = PTR_ERR(xdev->rst); if (err != -EPROBE_DEFER) dev_err(xdev->dev, "error getting reset %d\n", err); return err; } err = reset_control_deassert(xdev->rst); if (err) { dev_err(xdev->dev, "failed to deassert reset: %d\n", err); return err; } That requires modifying the reset controller API, so it's a bit out-of-scope, but if you could give it a go, it would be great. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html