| Message ID | 4220c66c29d83bec1ded798ee383b5460c162cfc.1481735244.git.roliveir@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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)
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) >
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@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 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.
Am Donnerstag, den 15.12.2016, 15:56 +0200 schrieb Laurent Pinchart: > 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@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 > > 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. Seeing that the clk and gpiod APIs behave in the same way, I think it would be good to align the reset API with the common behaviour. regards Philipp
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> #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"); + if (IS_ERR(xdev->rst)) { + err = PTR_ERR(xdev->rst); + switch (err) { + case -ENOENT: + case -ENOTSUPP: + xdev->rst = NULL; + break; + default: + dev_err(xdev->dev, "error getting reset %d\n", err); + return err; + } + } else { + err = reset_control_deassert(xdev->rst); + if (err) { + dev_err(xdev->dev, "failed to deassert reset: %d\n", + err); + 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)
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(+)