lkml.org 
[lkml]   [2011]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/3] sound/soc/lapis: add platform driver
On Mon, Nov 21, 2011 at 01:08:52PM +0900, Tomoya MORINAGA wrote:

> +/* =================== I2S CH0 config =================== */
> +#define I2S_CH0_MCLK (12288000) /* Master Clock Frequency[Hz] */

This looks like it should be board specific?

> + #if IOH_I2S_USE_PARAM
> + #define I2S_CH0_FS_16000 16000
> + #define I2S_CH0_FS_32000 32000
> + #define I2S_CH0_FS_48000 48000
> + #else
> + #define I2S_CH0_FS_8000 8000
> + #define I2S_CH0_FS_11025 11025
> + #define I2S_CH0_FS_22050 22050
> + #define I2S_CH0_FS_44100 44100
> + #endif

What are these ifdefs for? This should be configured at runtime.

> +/* select master or slave. The value is ioh_mssel_t */
> +#define I2S_CH0_MSSEL (ioh_mssel_master)
> +/* select MCLK or MLBCLK into Master Clock. The value is enum ioh_masterclk_t */
> +#define I2S_CH0_MASTERCLKSEL (ioh_masterclksel_mclk)

This and most of the following defines also look like compile time
configuration for things that should be runtime configured. As with
the previous patches in the series the big picture issue here is that
you need to update your code to reflect the practices of modern
mainline Linux code - here the major issue is that you appear to be
doing all your configuration with #defines rather than by using runtime
configuration from the machine driver as is normal for ASoC code.

I got through a bunch more defines but stopped reviewing at some point
as there was so much of this issue.


\
 
 \ /
  Last update: 2011-11-21 12:47    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site