Re: [PATCH v2 2/6] ARM: DaVinci: ASoC: Add the platform devices for ASP

[David Brownell <david-b@pacbell.net>]

On Thursday 16 April 2009, Medisetty, Naresh wrote:
>  arch/arm/mach-davinci/dm355.c            |   29 ++++++++++++-
>  arch/arm/mach-davinci/dm644x.c           |   27 +++++++++++-
>  arch/arm/mach-davinci/dm646x.c           |   67 ++++++++++++++++++++++++++
>  arch/arm/mach-davinci/include/mach/asp.h |   39 ++++++++++++++++-

Note the merge dependency:  most of those aren't yet in mainline.
I think Kevin sent the dm644x.c changes to the ARM list for their
first review cycle recently.

My comments below are for the dm355.c changes, but they apply just
as much to the other two SoC-specific setup files.


> +static struct resource dm355_evm_snd_resources[] = {
> +       {
> +               .start  = DAVINCI_ASP1_BASE,
> +               .end    = DAVINCI_ASP1_BASE + SZ_8K - 1,
> +               .flags  = IORESOURCE_MEM,
> +       },
> +};
> +
> +static struct evm_snd_platform_data dm355_evm_snd_data = {
> +       .clk_name       = "asp1",
> +       .tx_dma_ch      = DAVINCI_DMA_ASP1_TX,
> +       .rx_dma_ch      = DAVINCI_DMA_ASP1_RX,
> +};

If you're going to move to proper driver model support
(good!) ... the DMA channels should be IORESOURCE_DMA
resources, and the clock should be associated directly
with that platform device.

That might seem to leave that current platform data
struct as useless ... not so!  It should tell about how
the audio is configured:  whether RX and TX are both
wired, if they're stereo or what, how *audio* clocks
are set up, and so forth.  All that is specific to the
specific *board* in use, not to that SoC, so it should
be provided by board setup code.  A lot of it is now
encoded in the sound/soc/davinci/davinci-evm.c file.

Example:  dm6446 evm supports six different audio clocks,
but right now one choice is fixed at board setup time.

ASoC seems to have ways to let drivers make that choice.
That could be handled by board-specific callbacks and data
tables, and would be good to package in platform data.
A different dm6446 board may use a different audio clock
setup, with different choices.


> +
> +static struct platform_device dm355_asoc_device = {
> +       .name                           = "davinci-asoc",
> +       .id                                     = -1,
> +       .dev.platform_data      = &dm355_evm_snd_data,
> +       .num_resources          = ARRAY_SIZE(dm355_evm_snd_resources),
> +       .resource                       = dm355_evm_snd_resources,

Looks like the alignment goofage there (for "=") is more than
what can be explained by patch mangling from email...


> +};


> @@ -555,6 +579,7 @@ static int __init dm355_init_devices(void)
>
>         davinci_cfg_reg(DM355_INT_EDMA_CC);
>         platform_device_register(&dm355_edma_device);
> +       platform_device_register(&dm355_asoc_device);

This does *NOT* belong in the generic dm355 setup code like that.
Not every DM355 board will even have audio!  Or have it wired
exactly like Spectrum's EVM boards.

It'd be appropriate to add dm355_init_asoc(asp_num, platform_data)
or something similar, so that different boards wouldn't need to
repeat the won't-change parts.  That routine might need to set up
some pin, IRQ, and EDMA muxing.  Boards with no audio support would
not call that routine.  Ones with two audio interfaces might need
to call it once for each interface.


>         return 0;
>  }
>  postcore_initcall(dm355_init_devices);


Similar comments for the two other SoC chips touched by this patch;
although obviously the dm6446 wouldn't need an asp_num, etc.

- dave