Re: [PATCHv3 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Saturday 16 February 2013, Andy Shevchenko wrote:
> > @@ -1836,6 +1825,12 @@ static int dw_probe(struct platform_device *pdev)
> >
> > dma_async_device_register(>dma);
> >
> > + if (pdev->dev.of_node)
> > + err = of_dma_controller_register(pdev->dev.of_node,
> > +dw_dma_xlate, dw);
> > + if (err && err != -ENODEV)
> > + dev_err(>dev, "could not register
> > of_dma_controller\n");
>
> I believe we may make it as
> if (...of_node) {
> err = ...register();
> if (err...)
> dev_err();
> }
I thing the two are equivalent because we only get to the first if()
when err is 0. However, I agree that your version is a bit clearer,
so I'll change it.
> > --- a/drivers/dma/dw_dmac_regs.h
> > +++ b/drivers/dma/dw_dmac_regs.h
>
> > @@ -211,9 +212,15 @@ struct dw_dma_chan {
> > /* hardware configuration */
> > unsigned intblock_size;
> > boolnollp;
> > + unsigned intrequest_line;
> > + struct dw_dma_slave slave;
> > +
>
> Do we really need an extra empty line here?
No, that was an accident.
> > /* configuration passed via DMA_SLAVE_CONFIG */
> > struct dma_slave_config dma_sconfig;
> > +
> > + /* backlink to dw_dma */
> > + struct dw_dma *dw;
>
> Seems it's not needed and came from rebase?
Probably. It certainly was not intentional.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv3 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Sun, Feb 17, 2013 at 12:21 AM, Arnd Bergmann wrote:
> The original device tree binding for this driver, from Viresh Kumar
> unfortunately conflicted with the generic DMA binding, and did not allow
> to completely seperate slave device configuration from the controller.
>
> This is an attempt to replace it with an implementation of the generic
> binding, but it is currently completely untested, because I do not have
> any hardware with this particular controller.
>
> The patch applies on top of the slave-dma tree, which contains both the base
> support for the generic DMA binding, as well as the earlier attempt from
> Viresh. Both of these are currently not merged upstream however.
>
> This version incorporates feedback from Viresh Kumar, Andy Shevchenko
> and Russell King.
Sorry, few comments below.
After addressing them take my Acked-by: Andy Shevchenko
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -1836,6 +1825,12 @@ static int dw_probe(struct platform_device *pdev)
>
> dma_async_device_register(>dma);
>
> + if (pdev->dev.of_node)
> + err = of_dma_controller_register(pdev->dev.of_node,
> +dw_dma_xlate, dw);
> + if (err && err != -ENODEV)
> + dev_err(>dev, "could not register of_dma_controller\n");
I believe we may make it as
if (...of_node) {
err = ...register();
if (err...)
dev_err();
}
> --- a/drivers/dma/dw_dmac_regs.h
> +++ b/drivers/dma/dw_dmac_regs.h
> @@ -211,9 +212,15 @@ struct dw_dma_chan {
> /* hardware configuration */
> unsigned intblock_size;
> boolnollp;
> + unsigned intrequest_line;
> + struct dw_dma_slave slave;
> +
Do we really need an extra empty line here?
>
> /* configuration passed via DMA_SLAVE_CONFIG */
> struct dma_slave_config dma_sconfig;
> +
> + /* backlink to dw_dma */
> + struct dw_dma *dw;
Seems it's not needed and came from rebase?
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv3 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Sun, Feb 17, 2013 at 12:21 AM, Arnd Bergmann a...@arndb.de wrote:
The original device tree binding for this driver, from Viresh Kumar
unfortunately conflicted with the generic DMA binding, and did not allow
to completely seperate slave device configuration from the controller.
This is an attempt to replace it with an implementation of the generic
binding, but it is currently completely untested, because I do not have
any hardware with this particular controller.
The patch applies on top of the slave-dma tree, which contains both the base
support for the generic DMA binding, as well as the earlier attempt from
Viresh. Both of these are currently not merged upstream however.
This version incorporates feedback from Viresh Kumar, Andy Shevchenko
and Russell King.
Sorry, few comments below.
After addressing them take my Acked-by: Andy Shevchenko
andriy.shevche...@linux.intel.com
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1836,6 +1825,12 @@ static int dw_probe(struct platform_device *pdev)
dma_async_device_register(dw-dma);
+ if (pdev-dev.of_node)
+ err = of_dma_controller_register(pdev-dev.of_node,
+dw_dma_xlate, dw);
+ if (err err != -ENODEV)
+ dev_err(pdev-dev, could not register of_dma_controller\n);
I believe we may make it as
if (...of_node) {
err = ...register();
if (err...)
dev_err();
}
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -211,9 +212,15 @@ struct dw_dma_chan {
/* hardware configuration */
unsigned intblock_size;
boolnollp;
+ unsigned intrequest_line;
+ struct dw_dma_slave slave;
+
Do we really need an extra empty line here?
/* configuration passed via DMA_SLAVE_CONFIG */
struct dma_slave_config dma_sconfig;
+
+ /* backlink to dw_dma */
+ struct dw_dma *dw;
Seems it's not needed and came from rebase?
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv3 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Saturday 16 February 2013, Andy Shevchenko wrote:
@@ -1836,6 +1825,12 @@ static int dw_probe(struct platform_device *pdev)
dma_async_device_register(dw-dma);
+ if (pdev-dev.of_node)
+ err = of_dma_controller_register(pdev-dev.of_node,
+dw_dma_xlate, dw);
+ if (err err != -ENODEV)
+ dev_err(pdev-dev, could not register
of_dma_controller\n);
I believe we may make it as
if (...of_node) {
err = ...register();
if (err...)
dev_err();
}
I thing the two are equivalent because we only get to the first if()
when err is 0. However, I agree that your version is a bit clearer,
so I'll change it.
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -211,9 +212,15 @@ struct dw_dma_chan {
/* hardware configuration */
unsigned intblock_size;
boolnollp;
+ unsigned intrequest_line;
+ struct dw_dma_slave slave;
+
Do we really need an extra empty line here?
No, that was an accident.
/* configuration passed via DMA_SLAVE_CONFIG */
struct dma_slave_config dma_sconfig;
+
+ /* backlink to dw_dma */
+ struct dw_dma *dw;
Seems it's not needed and came from rebase?
Probably. It certainly was not intentional.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
