Re: [alsa-devel] [PATCH] ASoC: add RT5640 CODEC driver
On Tue, Mar 26, 2013 at 05:35:38PM -0600, Stephen Warren wrote:
Mark, a couple questions:
- Is the custom index_reg device attr file OK, or should I remove that?
- Do new CODEC drivers have to use regmap directly, or is using the ASoC IO system still OK?
Convert to regmap please - it looks like this ought to be using the paging support from a quick glance at what the register I/O stuff is doing. The device file appears to duplicate the register dump stuff and isn't suitable for sysfs anyway as it's not one value per file, it ought to be in debugfs.
+/* IN1/IN2 Input Type */ +static const char * const rt5640_input_mode[] = {
- "Single ended", "Differential"};
This looks like platform data.
+static const char * const rt5640_data_select[] = {
- "Normal", "left copy to right", "right copy to left", "Swap"};
DAPM?
+/* DMIC */ +static const char * const rt5640_dmic_mode[] = {"Disable", "DMIC1", "DMIC2"};
+static const SOC_ENUM_SINGLE_DECL(rt5640_dmic_enum, 0, 0, rt5640_dmic_mode);
DAPM?
+static int rt5640_vol_rescale_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct soc_mixer_control *mc =
(struct soc_mixer_control *)kcontrol->private_value;- struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
- unsigned int val = snd_soc_read(codec, mc->reg);
- ucontrol->value.integer.value[0] = VOL_RESCALE_MAX_VOL -
((val & RT5640_L_VOL_MASK) >> mc->shift);- ucontrol->value.integer.value[1] = VOL_RESCALE_MAX_VOL -
(val & RT5640_R_VOL_MASK);
This looks like a range control.
+static int hp_event(struct snd_soc_dapm_widget *w,
- struct snd_kcontrol *kcontrol, int event)
+{
- switch (event) {
- case SND_SOC_DAPM_POST_PMU:
break;- case SND_SOC_DAPM_PRE_PMD:
break;- default:
return 0;- }
- return 0;
+}
This does nothing, it should be removed.
+static int rt5640_set_dmic1_event(struct snd_soc_dapm_widget *w,
- struct snd_kcontrol *kcontrol, int event)
+{
- struct snd_soc_codec *codec = w->codec;
- switch (event) {
- case SND_SOC_DAPM_PRE_PMU:
snd_soc_update_bits(codec, RT5640_GPIO_CTRL1,RT5640_GP2_PIN_MASK | RT5640_GP3_PIN_MASK,RT5640_GP2_PIN_DMIC1_SCL | RT5640_GP3_PIN_DMIC1_SDA);snd_soc_update_bits(codec, RT5640_DMIC,RT5640_DMIC_1L_LH_MASK | RT5640_DMIC_1R_LH_MASK |RT5640_DMIC_1_DP_MASK,RT5640_DMIC_1L_LH_FALLING | RT5640_DMIC_1R_LH_RISING |RT5640_DMIC_1_DP_IN1P);snd_soc_update_bits(codec, RT5640_DMIC,RT5640_DMIC_1_EN_MASK, RT5640_DMIC_1_EN);- default:
return 0;
The last write there looks awfully like an enable but there's not any corresponding disable...
+static int rt5640_set_dmic2_event(struct snd_soc_dapm_widget *w,
- struct snd_kcontrol *kcontrol, int event)
+{
- struct snd_soc_codec *codec = w->codec;
- switch (event) {
- case SND_SOC_DAPM_PRE_PMU:
snd_soc_update_bits(codec, RT5640_GPIO_CTRL1,RT5640_GP2_PIN_MASK | RT5640_GP4_PIN_MASK,RT5640_GP2_PIN_DMIC1_SCL | RT5640_GP4_PIN_DMIC2_SDA);
There's a degree of overlap between these two functions...
+static const struct snd_soc_dapm_route rt5640_dapm_routes[] = {
- {"IN1P", NULL, "LDO2"},
- {"IN2P", NULL, "LDO2"},
- {"IN1P", NULL, "MIC1"},
- {"IN1N", NULL, "MIC1"},
- {"IN2P", NULL, "MIC2"},
- {"IN2N", NULL, "MIC2"},
Are MIC1 and MIC2 pins on the device? It's a bit odd to have a single physical input feeding both sides of a differential path internally.
- {"DMIC L1", NULL, "DMIC CLK"},
- {"DMIC L2", NULL, "DMIC CLK"},
Shouldn't the DMIC CLK widget be handling the clock enables/muxes from the DMIC event above?
+static int get_sdp_info(struct snd_soc_codec *codec, int dai_id) +{
- int ret = 0, val = snd_soc_read(codec, RT5640_I2S1_SDP);
- if (codec == NULL)
return -EINVAL;
No point in the NULL check immediately after a dereference.
- val = (val & RT5640_I2S_IF_MASK) >> RT5640_I2S_IF_SFT;
- switch (dai_id) {
- case RT5640_AIF1:
if (val == RT5640_IF_123 || val == RT5640_IF_132 ||val == RT5640_IF_113)ret |= RT5640_U_IF1;if (val == RT5640_IF_312 || val == RT5640_IF_213 ||val == RT5640_IF_113)ret |= RT5640_U_IF2;if (val == RT5640_IF_321 || val == RT5640_IF_231)ret |= RT5640_U_IF3;
The tree of if ( || || ) looks like a switch statement and should probably be written as such.
- frame_size = snd_soc_params_to_frame_size(params);
- if (frame_size < 0) {
dev_err(codec->dev, "Unsupported frame size: %d\n", frame_size);return -EINVAL;- }
The error message here is confusing as a negative code is an errno not a frame size and it probably ought to be passed to the caller.
- bclk_ms = frame_size > 32 ? 1 : 0;
Grumble.
+static int rt5640_prepare(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct snd_soc_codec *codec = rtd->codec;
- struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec);
- rt5640->aif_pu = dai->id;
- return 0;
+}
This looks like only one DAI can be active at once, shouldn't there be some sort of checking for busy here?
- dai_sel = get_sdp_info(codec, dai->id);
- if (dai_sel < 0) {
dev_err(codec->dev, "Failed to get sdp info: %d\n", dai_sel);return -EINVAL;- }
I suspect using dai->base (or just picking a suitable value for dai->id even) would be simpler.
- if (dai_sel & RT5640_U_IF1) {
snd_soc_update_bits(codec, RT5640_I2S1_SDP,RT5640_I2S_MS_MASK | RT5640_I2S_BP_MASK |RT5640_I2S_DF_MASK, reg_val);- }
- if (dai_sel & RT5640_U_IF2) {
snd_soc_update_bits(codec, RT5640_I2S2_SDP,RT5640_I2S_MS_MASK | RT5640_I2S_BP_MASK |RT5640_I2S_DF_MASK, reg_val);- }
This looks like switch statements again, though (not having checked) I'd expect that there's just blocks of registers for each DAI and we could just work in terms of a base?
+/**
- rt5640_pll_calc - Calcualte PLL M/N/K code.
Typo.
- snd_soc_add_codec_controls(codec, rt5640_snd_controls,
ARRAY_SIZE(rt5640_snd_controls));
Set in the driver structure.
+static int rt5640_resume(struct snd_soc_codec *codec) +{
- int ret = 0 ;
- codec->cache_sync = 1;
- ret = snd_soc_cache_sync(codec);
- if (ret) {
dev_err(codec->dev, "Failed to sync cache: %d\n", ret);return ret;- }
- rt5640_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
We also sync the cache in the bias management code.
+static int rt5640_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)+{
- struct rt5640_priv *rt5640;
- int ret;
- rt5640 = kzalloc(sizeof(struct rt5640_priv), GFP_KERNEL);
- if (NULL == rt5640)
return -ENOMEM;
devm_kzalloc()
+struct rt5640_pll_code {
- bool m_bp; /* Indicates bypass m code or not. */
- int m_code;
- int n_code;
- int k_code;
+};
+struct rt5640_priv {
- struct snd_soc_codec *codec;
Move these into the body of the driver.
On 03/26/2013 07:15 PM, Mark Brown wrote:
On Tue, Mar 26, 2013 at 05:35:38PM -0600, Stephen Warren wrote:
Mark, a couple questions: * Is the custom index_reg device attr file OK, or should I remove that? * Do new CODEC drivers have to use regmap directly, or is using the ASoC IO system still OK?
Convert to regmap please - it looks like this ought to be using the paging support from a quick glance at what the register I/O stuff is doing. The device file appears to duplicate the register dump stuff and isn't suitable for sysfs anyway as it's not one value per file, it ought to be in debugfs.
Ah, I hadn't seen the ranges stuff before. I think that should work.
+/* IN1/IN2 Input Type */ +static const char * const rt5640_input_mode[] = { + "Single ended", "Differential"};
This looks like platform data.
I thought the idea was to expose all the CODEC's configuration/routing through DAPM, and then let everything get set up through controls at run-time? Looking at the WM8903 driver, it seems like it has the exact same kind of option.
+static const char * const rt5640_data_select[] = { + "Normal", "left copy to right", "right copy to left", "Swap"};
DAPM?
+/* DMIC */ +static const char * const rt5640_dmic_mode[] = {"Disable", "DMIC1", "DMIC2"}; + +static const SOC_ENUM_SINGLE_DECL(rt5640_dmic_enum, 0, 0, rt5640_dmic_mode);
DAPM?
I'm not sure what change you're asking for here; isn't this defining a control that influences the DAPM routing?
Perhaps the issue is that the enum above feeds into a snd_kcontrol, rather than being an snd_soc_dapm_route that's conditional upon one of the values in that list?
If so, that's going to be a lot of stuff to change in the driver considering that I can't actually test any of the DMIC (or even analog mic) support yet since I can't get it working. Perhaps I should just rip out all the widgets/controls I can't test?
+static const struct snd_soc_dapm_route rt5640_dapm_routes[] = {
- {"IN1P", NULL, "LDO2"}, + {"IN2P", NULL, "LDO2"}, + + {"IN1P",
NULL, "MIC1"}, + {"IN1N", NULL, "MIC1"}, + {"IN2P", NULL, "MIC2"}, + {"IN2N", NULL, "MIC2"},
Are MIC1 and MIC2 pins on the device?
Hmmm. I see SND_SOC_DAPM_INPUT()s for MIC1 and MIC2, but they don't appear to be pins on the device. Rather, IN1P/N and IN2P/N are the pins, and also are declared as SND_SOC_DAPM_INPUT().
I'm not really sure what MIC1/MIC2 are meant to represent...
- {"DMIC L1", NULL, "DMIC CLK"}, + {"DMIC L2", NULL, "DMIC
CLK"},
Shouldn't the DMIC CLK widget be handling the clock enables/muxes from the DMIC event above?
Oh right, so you mean:
* Add a SND_SOC_DAPM_OUTPUT() for the "DMIC CLK".
* Change the "DMIC L*" from SND_SOC_DAPM_PGA_E() to SND_SOC_DAPM_PGA(), and move the event to the "DMIC CLK" widget.
* Add routing table entries where "DMIC1 *" are each fed from "DMIC CLK", with "conditions" based on which DMIC is "on":
{"DMIC L1" "DMIC1" "DMIC CLK"}, {"DMIC R1" "DMIC1" "DMIC CLK"}, {"DMIC L2" "DMIC2" "DMIC CLK"}, {"DMIC R2" "DMIC2" "DMIC CLK"},
or something like that?
+static int get_sdp_info(struct snd_soc_codec *codec, int dai_id)
...
- bclk_ms = frame_size > 32 ? 1 : 0;
Grumble.
Sorry, I don't understand the issue here. (through lack of familiarity with the issue; I'm not saying there isn't one).
+static int rt5640_prepare(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_codec *codec = rtd->codec; + struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec); + + rt5640->aif_pu = dai->id; + return 0; +}
This looks like only one DAI can be active at once, shouldn't there be some sort of checking for busy here?
I haven't read the spec through in explicit detail, but I didn't see anything to indicate that only one DAI could be active at once. In fact the diagrams in the documentation make it look at least somewhat cross-bar-like.
It looks like rt5640->aif_pu is used solely by set_dmic_clk() to somehow derive the DMIC CLK from the DAI LRCLK based on which DAI is actually active. I assume "which DAI is actually active" should be implemented as "which DAI is routed from the DMICs". I guess this should be implemented in a very different way then?
- dai_sel = get_sdp_info(codec, dai->id); + if (dai_sel < 0) { +
dev_err(codec->dev, "Failed to get sdp info: %d\n", dai_sel); + return -EINVAL; + }
I suspect using dai->base (or just picking a suitable value for dai->id even) would be simpler.
So there is something dynamic here; there are 2 physical DAIs on the chip package, but somehow I think there are 3 digital channels within the device that can be routed to/from the physical DAIs in various (sometimes broadcast?) configurations based on the rt5640_dai_iis_map. Hence, the implementation of get_sdp_info() looks up the value of the register that configures that routing, and returns data that isn't static...
I wonder if this all shouldn't be explicitly represented by DAPM widgets/routes, but the diagrams and text in the documentation aren't particularly clear on what this third thing is that the code seems to support:-(
+static int rt5640_resume(struct snd_soc_codec *codec) +{ + int ret = 0 ; + + codec->cache_sync = 1; + ret = snd_soc_cache_sync(codec); + if (ret) { + dev_err(codec->dev, "Failed to sync cache: %d\n", ret); + return ret; + } + rt5640_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
We also sync the cache in the bias management code.
In other words, I should just remove the call to snd_soc_cache_sync(codec) here, since it's redundant?
On Wed, Mar 27, 2013 at 04:50:21PM -0600, Stephen Warren wrote:
On 03/26/2013 07:15 PM, Mark Brown wrote:
On Tue, Mar 26, 2013 at 05:35:38PM -0600, Stephen Warren wrote:
+/* IN1/IN2 Input Type */ +static const char * const rt5640_input_mode[] = { + "Single ended", "Differential"};
This looks like platform data.
I thought the idea was to expose all the CODEC's configuration/routing through DAPM, and then let everything get set up through controls at
Given that changing between single ended and differential generally involves a modification of the circuit this is something we know at build time in the same way we know how the microphone biases are wired up. Changing it would generally need to be coordinated with some external circuit (eg, flipping an analouge switch to reroute the signals) and hence done from the machine driver.
run-time? Looking at the WM8903 driver, it seems like it has the exact same kind of option.
Older drivers aren't always good examples of this stuff.
+/* DMIC */ +static const char * const rt5640_dmic_mode[] = {"Disable", "DMIC1", "DMIC2"}; + +static const SOC_ENUM_SINGLE_DECL(rt5640_dmic_enum, 0, 0, rt5640_dmic_mode);
DAPM?
I'm not sure what change you're asking for here; isn't this defining a control that influences the DAPM routing?
It looks like just a plain old enum to me.
Perhaps the issue is that the enum above feeds into a snd_kcontrol, rather than being an snd_soc_dapm_route that's conditional upon one of the values in that list?
Probably, or a mux or something.
I'm not really sure what MIC1/MIC2 are meant to represent...
I think they're supposed to represent the microphones someone might connect to the inputs but that should be done in the machine driver.
Shouldn't the DMIC CLK widget be handling the clock enables/muxes from the DMIC event above?
Oh right, so you mean:
- Add a SND_SOC_DAPM_OUTPUT() for the "DMIC CLK".
No need for that, it can just be handled as a supply within the device since it's an integral part of the DMIC interface.
- Add routing table entries where "DMIC1 *" are each fed from "DMIC
CLK", with "conditions" based on which DMIC is "on":
{"DMIC L1" "DMIC1" "DMIC CLK"}, {"DMIC R1" "DMIC1" "DMIC CLK"}, {"DMIC L2" "DMIC2" "DMIC CLK"}, {"DMIC R2" "DMIC2" "DMIC CLK"},
or something like that?
Pretty much.
+static int get_sdp_info(struct snd_soc_codec *codec, int dai_id)
- bclk_ms = frame_size > 32 ? 1 : 0;
Grumble.
Sorry, I don't understand the issue here. (through lack of familiarity with the issue; I'm not saying there isn't one).
I don't like the ternery operator where it's not especially idiomatic, if I wanted to read line noise I'd be working with perl.
+static int rt5640_prepare(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_codec *codec = rtd->codec; + struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec); + + rt5640->aif_pu = dai->id; + return 0; +}
This looks like only one DAI can be active at once, shouldn't there be some sort of checking for busy here?
I haven't read the spec through in explicit detail, but I didn't see anything to indicate that only one DAI could be active at once. In fact the diagrams in the documentation make it look at least somewhat cross-bar-like.
It looks like rt5640->aif_pu is used solely by set_dmic_clk() to somehow derive the DMIC CLK from the DAI LRCLK based on which DAI is actually active. I assume "which DAI is actually active" should be implemented as "which DAI is routed from the DMICs". I guess this should be implemented in a very different way then?
Probably, yes. This code really doesn't look like it'll work properly if the two interfaces are running at once, especially if they are started simultaneously.
I wonder if this all shouldn't be explicitly represented by DAPM widgets/routes, but the diagrams and text in the documentation aren't particularly clear on what this third thing is that the code seems to support:-(
Yes, putting it all into DAPM seems like the way forward.
+static int rt5640_resume(struct snd_soc_codec *codec) +{ + int ret = 0 ; + + codec->cache_sync = 1; + ret = snd_soc_cache_sync(codec); + if (ret) { + dev_err(codec->dev, "Failed to sync cache: %d\n", ret); + return ret; + } + rt5640_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
We also sync the cache in the bias management code.
In other words, I should just remove the call to snd_soc_cache_sync(codec) here, since it's redundant?
Or remove it from the bias management since there's no regulator or other code I can see which cuts the power or otherwise resets the device so restoring the cache outside of suspend is probably a waste of time at the minute.
participants (2)
-
Mark Brown -
Stephen Warren