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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
