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

[David Brownell]

This looks like the wrong approach.

On Thursday 28 May 2009, Chaithrika U S wrote:
> --- a/arch/arm/mach-davinci/dm355.c
> +++ b/arch/arm/mach-davinci/dm355.c

> @@ -278,6 +282,7 @@ static __init void dm355_evm_init(void)
>  
>         dm355_init_spi0(BIT(0), dm355_evm_spi_info,
>                         ARRAY_SIZE(dm355_evm_spi_info));
> +       dm355_init_asp(&dm355_evm_snd_data);

Surely that should be "init_asp1()"?  You are hard-wiring
an assumption that only ASP1 will ever be used on a DM355,
which is unwise.

What will happen when a board uses ASP0 instead?

What will happen when a board uses ASP0 *and* ASP1?

What will happen when a board wants to use ASP1, but
not the same way the EVM uses it?


> --- a/arch/arm/mach-davinci/dm355.c
> +++ b/arch/arm/mach-davinci/dm355.c

> @@ -624,6 +625,32 @@ static struct platform_device dm355_edma_device = {
>         .resource               = edma_resources,
>  };
>  
> +/* DM335 EVM uses ASP1; line-out is a stereo mini-jack */

Why are you putting EVM-specific assumptions into a file
which is intended to be generic for *ALL* boards using
the DM355 chip?


> +static struct resource dm355_asp_resources[] = {
> +       {
> +               .start  = DAVINCI_ASP1_BASE,
> +               .end    = DAVINCI_ASP1_BASE + SZ_8K - 1,
> +               .flags  = IORESOURCE_MEM,
> +       },
> +       {
> +               .start  = DAVINCI_DMA_ASP1_TX,
> +               .end    = DAVINCI_DMA_ASP1_TX,
> +               .flags  = IORESOURCE_DMA,
> +       },
> +       {
> +               .start  = DAVINCI_DMA_ASP1_RX,
> +               .end    = DAVINCI_DMA_ASP1_RX,
> +               .flags  = IORESOURCE_DMA,
> +       },
> +};
> +
> +static struct platform_device dm355_asp_device = {
> +       .name           = "davinci-asp",
> +       .id             = -1,
> +       .num_resources  = ARRAY_SIZE(dm355_asp_resources),
> +       .resource       = dm355_asp_resources,
> +};
> +
>  /*----------------------------------------------------------------------*/
>  
>  static struct map_desc dm355_io_desc[] = {
> @@ -733,6 +760,16 @@ static struct davinci_soc_info davinci_soc_info_dm355
> = { .sram_len               = SZ_32K,
>  };
>  
> +void __init dm355_init_asp(struct evm_snd_platform_data *pdata)
> +{

This would sort of make sense as a "dm355_init_asp1()" routine,
except that you're hard-wiring the platform data to use a data
type that's specific to the EVM.  Also, hard-wiring the use of
both DMA channels ... even though there's no reason a board would
not want to use ASP1 in input-only or output-only mode, and
use the other DMA channel for something other than ASP1_[RT]X.

This is no way to have a generic platform device creation hook;
but that's the only kind of hook that belongs in this file.


> +       /* we don't use ASP1 IRQs, or we'd need to mux them ... */
> +       davinci_cfg_reg(DM355_EVT8_ASP1_TX);
> +       davinci_cfg_reg(DM355_EVT9_ASP1_RX);
> +
> +       dm355_asp_device.dev.platform_data = pdata;
> +       platform_device_register(&dm355_asp_device);
> +}
> +
>  void __init dm355_init(void)
>  {
>         davinci_common_init(&davinci_soc_info_dm355);