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

[naresh]

Dave,

Thanks for the review.

I will re-submit the patch after fixing the comments.

Regards,
Naresh

>
>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