linux-kernel - Re: [PATCHv3 2/4] dmaengine: dw_dmac: move to generic DMA binding

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 17 Feb 2013 00:54:57 +0200
From:	Andy Shevchenko <andy.shevchenko@...il.com>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	Vinod Koul <vinod.koul@...el.com>, Dan Williams <djbw@...com>,
	linux-arm-kernel@...ts.infradead.org,
	devicetree-discuss@...ts.ozlabs.org,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Olof Johansson <olof@...om.net>, linux-kernel@...r.kernel.org,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Vinod Koul <vinod.koul@...ux.intel.com>
Subject: Re: [PATCHv3 2/4] dmaengine: dw_dmac: move to generic DMA binding

On Sun, Feb 17, 2013 at 12:21 AM, Arnd Bergmann <arnd@...db.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.shevchenko@...ux.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 int            block_size;
>         bool                    nollp;
> +       unsigned int            request_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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/