On Wed, 2012-09-12 at 10:57 +0800, Mark Brown wrote:
+/* Digital MIC clock rate select */ +static const char * const da9055_dmic_clk_rate_txt[] = {
- "3MHz", "1.5MHz"
+};
+static const struct soc_enum da9055_dmic_clk_rate =
- SOC_ENUM_SINGLE(DA9055_MIC_CONFIG, 2, 2, da9055_dmic_clk_rate_txt);
+/* Digital MIC sample phase select */ +static const char * const da9055_dmic_phase_txt[] = {
- "Sample on DMICCLK edges", "Sample between DMICCLK edges"
+};
+static const struct soc_enum da9055_dmic_phase =
- SOC_ENUM_SINGLE(DA9055_MIC_CONFIG, 1, 2, da9055_dmic_phase_txt);
+/* Digital MIC channel select */ +static const char * const da9055_dmic_channel_select_txt[] = {
- "Rising Left Falling Right", "Falling Left Rising Right"
+};
Why is any of this being exposed to userspace? If this should be configured I'd expect it to be static platform data, not something that gets changed at runtime.
These parameters are exposed considering the fact that DMIC itself is not part of the codec. Codec only provides DMIC interface using which an external DMIC can be attached. These parameters depend on the actual DMIC hardware and hence kept configurable to allow runtime plug in of any DMIC hardware. Doesn't it make sense to keep them runtime configurable?
- /* In slave mode, there is only one set of divisors */
- if (!da9055->master)
fout = 2822400;Should check the user supplied this value
Can you explain which value / user supplied value you are referring to? It is not quite clear to me.
- also, what happens if the
user sets the device to slave mode after setting up the PLL?
Will add required checks in set_fmt().
- /* Enable Mic Bias */
- snd_soc_write(codec, DA9055_MIC_BIAS_CTRL, DA9055_MIC_BIAS_EN);
DAPM for this and most of the rest of this funciton.
I guess here a DAPM_SUPPLY widget should be used and its routing should be part of machine driver instead of this codec driver, right?
For other things like input mixers, Headphone and Lineouts, DAPM is already used to control power specific bits. The confusion is because there are two separate control bits for these blocks. One bit is for "Enabling" that block and other is for "Enabling Amplifier" of that block. e.g for headphone, one bit is for "output enable" while other is for "output amplifier enable".
Chip designers have confirmed that "Amplifier Enable" bits are ones which should be used for power management. This is the reason why "Enable" bits are being set in probe() and "Amplifier Enable" bits are being taken care by DAPM. May be I need to put enough comments in code to clarify this.