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    [W:0.019 / U:31.880 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site