On Wed, Dec 09, 2009 at 11:53:33AM +0900, Kuninori Morimoto wrote:
+static inline unsigned int da7210_read(struct snd_soc_codec *codec,
unsigned int reg)+{
- /* FIXME !!
** we should read status from I2C.* But not supported now.*/
As Liam noted there are a few issues with the I2C stuff - you should be able to resolve these issues by removing the I2C code in the driver with soc-cache.c usage, which is a good idea anyway.
+static const char *da7210_mic_bias_voltage[] = { "1.5", "1.6", "2.2", "2.3" };
+static const struct soc_enum da7210_enum[] = {
- SOC_ENUM_SINGLE(DA7210_MIC_L, 4, 4, da7210_mic_bias_voltage),
+};
Please use individual variables rather than an array - it doesn't make much difference for small numbers of enums (like one!) but as the number increases it makes it much easier to keep track of which enum is bein referenced to by which control.
A couple of other comments of mine will be replicated from Liam's.
+/* Add non DAPM controls */ +static const struct snd_kcontrol_new da7210_snd_controls[] = {
- /* Mixer Playback controls */
- SOC_DOUBLE_R("DAC Gain", DA7210_DAC_L, DA7210_DAC_R, 0, 0x37, 1),
Should be "DAC Volume" - control names are interpreted by ALSA to let it know how to present the controls to applications.
- SOC_DOUBLE("In PGA Gain", DA7210_IN_GAIN, 0, 4, 0x0F, 0),
Should be "In PGA Volume".
- SOC_SINGLE("Mic Bias", DA7210_MIC_L, 6, 1, 0),
This should be a DAPM widget.
- value = da7210_read_reg_cache(codec, reg);
- value &= mask;
- da7210_write(codec, reg, value);
snd_soc_update_bits().
- da7210_dai.dev = codec->dev;
- da7210_codec = codec;
- ret = snd_soc_register_dai(&da7210_dai);
- if (ret) {
dev_err(codec->dev, "Failed to register DAI: %d\n", ret);return -ENOMEM;- }
Should also register the CODEC.
- /* Enable Left & Right MIC PGA and Mic Bias */
- da7210_write(codec, DA7210_MIC_L, DA7210_MIC_L_EN | DA7210_MICBIAS_EN);
- da7210_write(codec, DA7210_MIC_R, DA7210_MIC_R_EN);
- /* Enable Left and Right input PGA */
- da7210_write(codec, DA7210_INMIX_L, DA7210_IN_L_EN);
- da7210_write(codec, DA7210_INMIX_R, DA7210_IN_R_EN);
- /* Enable ADC Highpass Filter */
- da7210_write(codec, DA7210_ADC_HPF, DA7210_ADC_HPF_EN);
- /* Enable Left and Right ADC */
- da7210_write(codec, DA7210_ADC, DA7210_ADC_L_EN | DA7210_ADC_R_EN);
These all look like they should be controls of some kind - either DAPM or regular - and should be fairly straightforward to convert over. Since you're already using DAPM the power stuff especially should be properly supported since a mix of DAPM and non-DAPM control often leads to confusion and bugs.
For power bits there's no point setting a default since DAPM will overwrite them on startup anyway. In general the ASoC policy is for the CODEC driver to leave things at chip default since the driver has to work on many boards and the chip default is usually chosen as it will have to be at least somewhat sane for most boards.
- /* Enable DAI */
- da7210_write(codec, DA7210_DAI_CFG3, DA7210_DAI_OE | DA7210_DAI_EN);
I suspect you should use an AIF DAPM widget for these, or possibly an AIF widget in conjunction with a supply widget.
- /* Diable PLL and bypass it */
- da7210_write(codec, DA7210_PLL, DA7210_PLL_FS_48000);
- /* Bypass PLL and set MCLK freq rang to 10-20MHz */
- da7210_write(codec, DA7210_PLL_DIV3,
DA7210_MCLK_RANGE_10_20_MHZ | DA7210_PLL_BYP);
Since these aren't made configurable by the driver and are a bit more involved than controls it's OK to set defaults for these, though ideally they'd be supported by the driver.
/* Enable standbymode */da7210_write(codec, DA7210_STARTUP2,DA7210_LOUT1_L_STBY | DA7210_LOUT1_R_STBY |
...
/* Activate all enabled subsystem */da7210_write(codec, DA7210_STARTUP1, DA7210_SC_MST_EN);
This definitely looks like it ought to be being handled in the bias level configuration and/or by DAPM.
+#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE) +static int da7210_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
No need to ifdef where I2C is the only control interface.