Re: [PATCH 3/3] sound/soc/lapis: add platform driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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.