Re: [PATCH 1/9] ARM: DaVinci: ASoC: Adds ASoC driver support for TI DM646X EVM platform

[Kevin Hilman <khilman@deeprootsystems.com>]

David Brownell <david-b@pacbell.net> writes:

> On Monday 16 March 2009, Naresh Medisetty wrote:
>> @@ -43,14 +48,14 @@ static int evm_hw_params(struct snd_pcm_substream *substream,
>>         unsigned sysclk;
>>  
>>         /* ASP1 on DM355 EVM is clocked by an external oscillator */
>
> ... and on this DM646x EVM ... ?
>
>
>> -       if (machine_is_davinci_dm355_evm())
>> +       if (cpu_is_davinci_dm355() || cpu_is_davinci_dm646x())
>
> Shouldn't that stay as a machine_is_*() test,
> just adding a machine_is_davinci_dm646x_evm() case?
>
> Code tends to get modified by clone-and-modify,
> and making this code be cpu-specific instead of
> board-specific will thus promote errors.
>
> On top of that, there's currently an effort to
> minimize the amount of cpu_is_*() testing found
> in drivers.  Patches that increase such testing,
> especially needlessly increasing it!, seem to be
> the wrong direction...
>

Agreed.

In an earlier thread, I mentioned that some of this board and chip
specific code does not belong in sound/asoc/* at all, but Mark seemed
to think it does so it can better stay in sync with ALSA API change
etc., which makes sense to m.

That being said, some effort should be made to split out the
chip/board specific settings into platform data which can be managed
by chip/board specific code.

Kevin