Re: [PATCH v4 02/15] DMA: shdma: add r8a7740 DMAC data to the deviceID table

[Magnus Damm]

Hi Guennadi,

On Wed, Jul 24, 2013 at 6:19 AM, Guennadi Liakhovetski
<g.liakhovetski@xxxxxx> wrote:
> On Wed, 24 Jul 2013, Magnus Damm wrote:
>
>> Hi Guennadi,
>>
>> Thanks for your efforts on this.
>>
>> On Tue, Jul 23, 2013 at 7:49 PM, Guennadi Liakhovetski
>> <g.liakhovetski@xxxxxx> wrote:
>> > This configuration data will be re-used, when DMAC DT support is added to
>> > r8a7740, DMAC platform data in setup-r8a7740.c will be removed.
>> >
>> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@xxxxxxxxx>
>> > ---
>> >
>> > v4: make struct sh_dmae_pdata r8a7740_dma_pdata "const"
>> >
>>
>> [snip]
>>
>> > --- /dev/null
>> > +++ b/drivers/dma/sh/shdma-r8a7740.c
>> > @@ -0,0 +1,95 @@
>> > +#include <linux/sh_dma.h>
>> > +
>> > +#include <mach/dma-register.h>
>> > +#include <mach/r8a7740.h>
>>
>> Including stuff from <mach/..> isn't really compatible with
>> MULTIPLATFORM,
>
> Hmm, right. I modeled this arch-specific driver code after Laurent's
> pinctrl driver revamp, which also includes <mach/*.h> headers. So, we'll
> have to think how to fix both.

I mentioned this to Laurent when he started converting to PINCTRL, and
I believe the only remaining bits are the static GPIO-to-IRQ tables.

>> so please don't write new code like this. Actually we
>> don't want any code under drivers/ to include stuff from the mach
>> directory.
>
> Sure, understood.

Good.

>> I suggest that you arrange your code in a way so the C version of DMAC
>> support has tables with slave ids as usual under
>> arch/arm/mach-shmobile/, but the DT bits that operate independently of
>> C stay in drivers/... Over time we will get rid of the C version, and
>> until that happens the DT and C version can coexist in parallel.
>
> That's already how it is. Data, that I took to drivers/dma/sh/ is needed
> for both DT and C. DMA stuff, needed only for C are only DMAC devices and
> resources. I think, I might be able to carry those DMA specific headers
> and defines over from mach/ to drivers/dma/sh. Maybe it would be easier to
> do this in several steps:
>
> 1. add my drivers/dma/sh/shdma-<arch>.c files *with* mach/ headers
> 2. switch arches over to those files
> (the above two steps are already done in my patch series)
> 3. move headers to drivers/dma/sh
>
> Ok, alternatively, I might be able to do (1) above without using mach/
> headers at all by directly copying them to drivers/dma/sh/ and then
> removing the original mach/headers in step (2)? I'll look in more detail
> at the code tomorrow.

Thanks. My apologies for reviewing your code late in the cycle, but
I've now looked through this series and the following questions popped
up:

1) How will it look like in DT when a DMA Engine slave device will use DMA?

2) Isn't it possible to leave the SHDMA_SLAVE_xx bits to only be used
by legacy C SoC and board code in arch/arm/mach-shmobile? I don't
understand why you have to move them over to drivers/... I just assume
these SHDMA_SLAVE bits won't be used by 1) above.

3) How difficult would it be to describe the information in "struct
sh_dmae_slave_config" using DT?

4) It seems that some patches in this series are unrelated. Can you
submit 4/15 and 9/15 from v4 independently somehow?

5) Reducing and extending (reducing is optional of course)

At this point you cover r8a7740, r8a73a0 and r8a73a4. I'm not sure why
your picked those 3 SoCs, but perhaps it would be a good idea to
select a single SoC to begin with but extend the support to also
provide DT code that makes use of DMA Engine in slave devices like for
instance MMCIF (this would cover question 1) above). When we have
agreed on the big picture then additional SoCs can easily be added
later.

6) Remove untested bits

And while doing this conversion, perhaps this is a good opportunity to
only move over DMA Engine slaves that can be tested? Having tons of
unused DMA configuration seems overly heavy to me. If it's not tested
then it's broken.

Cheers,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/