Re: [alsa-devel] [PATCH 1/3] ASoC: codecs: Add da7219 codec driver
On Thu, Sep 17, 2015 at 05:01:14PM +0100, Adam Thomson wrote:
- do {
statusa = snd_soc_read(codec, DA7219_ACCDET_STATUS_A);if (statusa & DA7219_MICBIAS_UP_STS_MASK)micbias_up = true;- } while (!micbias_up);
This could go into an inifinite loop.
+static void da7219_aad_hptest_work(struct work_struct *work) +{
- struct da7219_aad_priv *da7219_aad =
container_of(work, struct da7219_aad_priv, hptest_work);- struct snd_soc_codec *codec = da7219_aad->codec;
- struct da7219_priv *da7219 = snd_soc_codec_get_drvdata(codec);
- u8 tonegen_cfg1, tonegen_cfg2, tonegen_onper;
- u16 tonegen_freq1, tonegen_freq_hptest;
- u8 hpl_gain, hpr_gain, dacl_gain, dacr_gain, dac_filters1, dac_filters4;
- u8 dac_filters5, cp_ctrl, routing_dac, dacl_ctrl, dacr_ctrl;
- u8 mixoutl_sel, mixoutr_sel, st_outfilt_1l, st_outfilt_1r;
- u8 mixoutl_ctrl, mixoutr_ctrl, hpl_ctrl, hpr_ctrl, accdet_cfg8;
- int report = 0;
- /* Save current settings */
This is obviously a massive reconfiguration of the device. I'm not seeing anything here which prevents userspace coming in and change the configuration while we're in this function, that would obviously have serious issues.
I'm also wondering if this might be more elegantly implemented by going into cache bypass mode, doing the test and then using a cache resync to restore the initial configuration. That will at least avoid issues with updates adding a new register but not modifying it.
if (statusa & DA7219_JACK_TYPE_STS_MASK) {report |= SND_JACK_HEADSET;mask |= SND_JACK_HEADSET | SND_JACK_LINEOUT;schedule_work(&da7219_aad->btn_det_work);} else {schedule_work(&da7219_aad->hptest_work);}
Why are we scheduling work - we're already in thread context?
/* Un-drive headphones/lineout */snd_soc_update_bits(codec, DA7219_HP_R_CTRL,DA7219_HP_R_AMP_OE_MASK, 0);snd_soc_update_bits(codec, DA7219_HP_L_CTRL,DA7219_HP_L_AMP_OE_MASK, 0);
This looks like DAPM?
+static enum da7219_aad_jack_ins_deb da7219_aad_of_jack_ins_deb(u32 val) +{
- switch (val) {
- case 5:
return DA7219_AAD_JACK_INS_DEB_5MS;- case 10:
return DA7219_AAD_JACK_INS_DEB_10MS;- case 20:
return DA7219_AAD_JACK_INS_DEB_20MS;- case 50:
return DA7219_AAD_JACK_INS_DEB_50MS;- case 100:
return DA7219_AAD_JACK_INS_DEB_100MS;- case 200:
return DA7219_AAD_JACK_INS_DEB_200MS;- case 500:
return DA7219_AAD_JACK_INS_DEB_500MS;- case 1000:
return DA7219_AAD_JACK_INS_DEB_1S;- default:
return DA7219_AAD_JACK_INS_DEB_20MS;
This isn't an error?
+/* Input/Output Enums */ +static const char * const da7219_gain_ramp_rate_txt[] = {
- "nominal rate * 8", "nominal rate", "nominal rate / 8",
- "nominal rate / 16"
+};
The ALSA ABI generally capitalises words.
+/* ToneGen */ +static int da7219_tonegen_freq_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
- struct da7219_priv *da7219 = snd_soc_codec_get_drvdata(codec);
- struct soc_mixer_control *mixer_ctrl =
(struct soc_mixer_control *) kcontrol->private_value;- unsigned int reg = mixer_ctrl->reg;
- u16 val;
- int ret;
- ret = regmap_bulk_read(da7219->regmap, reg, &val, sizeof(val));
- if (ret)
return ret;- ucontrol->value.integer.value[0] = le16_to_cpu(val);
This is *weird*. We do a bulk read for a single register using an API that returns CPU endian data then make it CPU endian (without any annotations on variables...). Why not use regmap_read()? Why swap? Why not use raw I/O?
+static int da7219_hpf_put(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
- struct soc_enum *enum_ctrl = (struct soc_enum *)kcontrol->private_value;
- unsigned int reg = enum_ctrl->reg;
- unsigned int sel = ucontrol->value.integer.value[0];
- unsigned int bits;
- switch (sel) {
- case DA7219_HPF_MODE_DISABLED:
bits = DA7219_HPF_DISABLED;break;- case DA7219_HPF_MODE_AUDIO:
bits = DA7219_HPF_AUDIO_EN;break;- case DA7219_HPF_MODE_VOICE:
bits = DA7219_HPF_VOICE_EN;break;- default:
return -EINVAL;- }
- snd_soc_update_bits(codec, reg, DA7219_HPF_MODE_MASK, bits);
- return 0;
+}
This looks like a standard enumeration with a simple mapping to the register map?
- /* ADCs */
- SOC_SINGLE_TLV("ADC Volume", DA7219_ADC_L_GAIN,
DA7219_ADC_L_DIGITAL_GAIN_SHIFT,DA7219_ADC_L_DIGITAL_GAIN_MAX, DA7219_NO_INVERT,da7219_adc_dig_gain_tlv),- SOC_SINGLE("ADC Switch", DA7219_ADC_L_CTRL,
DA7219_ADC_L_MUTE_EN_SHIFT, DA7219_SWITCH_EN_MAX,DA7219_INVERT),- SOC_SINGLE("ADC Gain Ramp Switch", DA7219_ADC_L_CTRL,
DA7219_ADC_L_RAMP_EN_SHIFT, DA7219_SWITCH_EN_MAX,DA7219_NO_INVERT),
Capture Digital rather than ADC.
- SOC_SINGLE_TLV("ALC Max Attenuation", DA7219_ALC_GAIN_LIMITS,
DA7219_ALC_ATTEN_MAX_SHIFT, DA7219_ALC_ATTEN_GAIN_MAX,DA7219_NO_INVERT, da7219_alc_gain_tlv),- SOC_SINGLE_TLV("ALC Max Gain", DA7219_ALC_GAIN_LIMITS,
DA7219_ALC_GAIN_MAX_SHIFT, DA7219_ALC_ATTEN_GAIN_MAX,DA7219_NO_INVERT, da7219_alc_gain_tlv),- SOC_SINGLE_RANGE_TLV("ALC Min Analog Gain", DA7219_ALC_ANA_GAIN_LIMITS,
DA7219_ALC_ANA_GAIN_MIN_SHIFT,DA7219_ALC_ANA_GAIN_MIN, DA7219_ALC_ANA_GAIN_MAX,DA7219_NO_INVERT, da7219_alc_ana_gain_tlv),- SOC_SINGLE_RANGE_TLV("ALC Max Analog Gain", DA7219_ALC_ANA_GAIN_LIMITS,
DA7219_ALC_ANA_GAIN_MAX_SHIFT,DA7219_ALC_ANA_GAIN_MIN, DA7219_ALC_ANA_GAIN_MAX,DA7219_NO_INVERT, da7219_alc_ana_gain_tlv),
Volume not Gain.
- SOC_SINGLE("ToneGen DTMF Key", DA7219_TONE_GEN_CFG1,
DA7219_DTMF_REG_SHIFT, DA7219_DTMF_REG_MAX,DA7219_NO_INVERT),
Should this be an enumeration with the DTMF digits in it (# and * aren't numbers)?
- /* DACs */
- SOC_DOUBLE_R_TLV("DAC Volume", DA7219_DAC_L_GAIN, DA7219_DAC_R_GAIN,
DA7219_DAC_L_DIGITAL_GAIN_SHIFT,DA7219_DAC_DIGITAL_GAIN_MAX, DA7219_NO_INVERT,da7219_dac_dig_gain_tlv),- SOC_DOUBLE_R("DAC Switch", DA7219_DAC_L_CTRL, DA7219_DAC_R_CTRL,
DA7219_DAC_L_MUTE_EN_SHIFT, DA7219_SWITCH_EN_MAX,DA7219_INVERT),- SOC_DOUBLE_R("DAC Gain Ramp Switch", DA7219_DAC_L_CTRL,
DA7219_DAC_R_CTRL, DA7219_DAC_L_RAMP_EN_SHIFT,DA7219_SWITCH_EN_MAX, DA7219_NO_INVERT),
All the DAC controls should probably be Playback Digital instead.
+static int da7219_set_dai_sysclk(struct snd_soc_dai *codec_dai,
int clk_id, unsigned int freq, int dir)+{
- struct snd_soc_codec *codec = codec_dai->codec;
- struct da7219_priv *da7219 = snd_soc_codec_get_drvdata(codec);
- if (da7219->mclk_rate == freq)
return 0;
Given that we also have source selection is this safe - should we also be checking the source?
- if (da7219->mclk) {
freq = clk_round_rate(da7219->mclk, freq);clk_set_rate(da7219->mclk, freq);- }
Missing error checking.
/* Internal LDO */if (da7219->use_int_ldo)snd_soc_update_bits(codec, DA7219_LDO_CTRL,DA7219_LDO_EN_MASK,DA7219_LDO_EN_MASK);
If there is an option to use an external supply I would expect to see the regulator API used to discover the external LDO (and ideally also to configure the integrated LDO). If the driver works outside the frameworks then it is likely this will lead to integration issues later on.
/* Internal LDO */da7219->use_int_ldo = pdata->use_internal_ldo;
This is likely to lead to surprises for example...
- /* Check if MCLK provided, if not the clock is NULL */
- da7219->mclk = devm_clk_get(codec->dev, "mclk");
- if (IS_ERR(da7219->mclk)) {
if (PTR_ERR(da7219->mclk) == -EPROBE_DEFER)return -EPROBE_DEFER;da7219->mclk = NULL;- }
This should specifically look for the error code that corresponds to a clock not being mapped and only continue silently in that case rather than silently accepting all failures to obtain a clock - if we silently accept all failures that will mean that actual errors will be ignored.
- /*
* There are multiple control bits for the input mixer.* The following can be enabled now as it's not power related.*/- snd_soc_update_bits(codec, DA7219_MIXIN_L_CTRL,
DA7219_MIXIN_L_MIX_EN_MASK,DA7219_MIXIN_L_MIX_EN_MASK);
So the chip designers just put these in for randomness? Fun. It'd be more idiomatic to do something like making these supply widgets so they're controlled via DAPM even if they don't matter much.
- else if (device_may_wakeup(codec->dev))
disable_irq_wake(da7219->aad->irq);
You can use dev_pm_set_wake_irq() and skip having to manage this stuff explicitly in the driver.
On September 19, 2015 18:44, Mark Brown wrote:
- do {
statusa = snd_soc_read(codec, DA7219_ACCDET_STATUS_A);if (statusa & DA7219_MICBIAS_UP_STS_MASK)micbias_up = true;- } while (!micbias_up);
This could go into an inifinite loop.
Only if the HW is unresponsive. However, it's probably better to add a timeout and some debug in the unlikely event that does happen.
+static void da7219_aad_hptest_work(struct work_struct *work) +{
- struct da7219_aad_priv *da7219_aad =
container_of(work, struct da7219_aad_priv, hptest_work);- struct snd_soc_codec *codec = da7219_aad->codec;
- struct da7219_priv *da7219 = snd_soc_codec_get_drvdata(codec);
- u8 tonegen_cfg1, tonegen_cfg2, tonegen_onper;
- u16 tonegen_freq1, tonegen_freq_hptest;
- u8 hpl_gain, hpr_gain, dacl_gain, dacr_gain, dac_filters1, dac_filters4;
- u8 dac_filters5, cp_ctrl, routing_dac, dacl_ctrl, dacr_ctrl;
- u8 mixoutl_sel, mixoutr_sel, st_outfilt_1l, st_outfilt_1r;
- u8 mixoutl_ctrl, mixoutr_ctrl, hpl_ctrl, hpr_ctrl, accdet_cfg8;
- int report = 0;
- /* Save current settings */
This is obviously a massive reconfiguration of the device. I'm not seeing anything here which prevents userspace coming in and change the configuration while we're in this function, that would obviously have serious issues.
I'm also wondering if this might be more elegantly implemented by going into cache bypass mode, doing the test and then using a cache resync to restore the initial configuration. That will at least avoid issues with updates adding a new register but not modifying it.
In a system scenario the likelihood of that happening is small as you cannot use the mic or headphones until they've been inserted. The system is only likely to act after the jack insertion events have occurred. However, it would be better to prevent any chance of concurrent access. The problem is how best to lock the Kcontrols whilst the test procedure in progress. At the moment the only way I can see is to add explicit control set() functions which would use a lock that can also be controlled by the HP test code. Does this make sense to you or do you know of a simpler method? Obviously dapm has function to lock as required.
For the cache resync idea, in terms of code, it will look cleaner, but you are talking about 4 to 5 times the number of I2C accesses to the device, to restore configuration. Does that not seem like a bit too much overhead?
if (statusa & DA7219_JACK_TYPE_STS_MASK) {report |= SND_JACK_HEADSET;mask |= SND_JACK_HEADSET |SND_JACK_LINEOUT;
schedule_work(&da7219_aad->btn_det_work);} else {schedule_work(&da7219_aad->hptest_work);}Why are we scheduling work - we're already in thread context?
hptest will take some time to complete (over 100ms), and in that time it's plausible that a user could remove the jack. If we deal with this in the IRQ thread, we won't be aware of jack removal during the process, and will send a report regardless, which will almost definitely be incorrect, and unnecessary. By spawning off work, it allows the removal to be dealt with if the hptest work procedure is currently in process, and then we can avoid sending incorrect jack reports at the end.
For button detection, for certain mics it's required to pulse the micbias to a higher voltage, for a defined period of time, to enable the mic. Again this period is likely to take maybe 100ms, depending on the mic, so it made far more sense to me to do this outside of the IRQ thread. However, if you have a better proposal for this then am happy to take it on board.
/* Un-drive headphones/lineout */snd_soc_update_bits(codec, DA7219_HP_R_CTRL,DA7219_HP_R_AMP_OE_MASK, 0);snd_soc_update_bits(codec, DA7219_HP_L_CTRL,DA7219_HP_L_AMP_OE_MASK, 0);This looks like DAPM?
The control of driving the headphones or making them high impedance is handled in the AAD code because we cannot have the headphones driven before a jack is inserted as it will affect the pole detection. Adding it to DAPM seemed like it would cause more problems in terms of controlling when it would and wouldn't be enabled.
+static enum da7219_aad_jack_ins_deb da7219_aad_of_jack_ins_deb(u32 val) +{
- switch (val) {
- case 5:
return DA7219_AAD_JACK_INS_DEB_5MS;- case 10:
return DA7219_AAD_JACK_INS_DEB_10MS;- case 20:
return DA7219_AAD_JACK_INS_DEB_20MS;- case 50:
return DA7219_AAD_JACK_INS_DEB_50MS;- case 100:
return DA7219_AAD_JACK_INS_DEB_100MS;- case 200:
return DA7219_AAD_JACK_INS_DEB_200MS;- case 500:
return DA7219_AAD_JACK_INS_DEB_500MS;- case 1000:
return DA7219_AAD_JACK_INS_DEB_1S;- default:
return DA7219_AAD_JACK_INS_DEB_20MS;This isn't an error?
Opted for HW default in case of invalid values provided. Maybe a dev_warn() would be useful though to indicate this is the case?
+/* Input/Output Enums */ +static const char * const da7219_gain_ramp_rate_txt[] = {
- "nominal rate * 8", "nominal rate", "nominal rate / 8",
- "nominal rate / 16"
+};
The ALSA ABI generally capitalises words.
Ok, no problem. Will adjust accordingly.
+/* ToneGen */ +static int da7219_tonegen_freq_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
- struct da7219_priv *da7219 = snd_soc_codec_get_drvdata(codec);
- struct soc_mixer_control *mixer_ctrl =
(struct soc_mixer_control *) kcontrol->private_value;- unsigned int reg = mixer_ctrl->reg;
- u16 val;
- int ret;
- ret = regmap_bulk_read(da7219->regmap, reg, &val, sizeof(val));
- if (ret)
return ret;- ucontrol->value.integer.value[0] = le16_to_cpu(val);
This is *weird*. We do a bulk read for a single register using an API that returns CPU endian data then make it CPU endian (without any annotations on variables...). Why not use regmap_read()? Why swap? Why not use raw I/O?
The device is 8-bit register access only, and the value spans two registers, hence why this is done here. The register defined for the Kcontrol is the first in the sequence of two registers (first lower byte, second upper byte). Thought this was cleaner than having two separate controls to configure upper and lower bytes.
+static int da7219_hpf_put(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
- struct soc_enum *enum_ctrl = (struct soc_enum *)kcontrol->private_value;
- unsigned int reg = enum_ctrl->reg;
- unsigned int sel = ucontrol->value.integer.value[0];
- unsigned int bits;
- switch (sel) {
- case DA7219_HPF_MODE_DISABLED:
bits = DA7219_HPF_DISABLED;break;- case DA7219_HPF_MODE_AUDIO:
bits = DA7219_HPF_AUDIO_EN;break;- case DA7219_HPF_MODE_VOICE:
bits = DA7219_HPF_VOICE_EN;break;- default:
return -EINVAL;- }
- snd_soc_update_bits(codec, reg, DA7219_HPF_MODE_MASK, bits);
- return 0;
+}
This looks like a standard enumeration with a simple mapping to the register map?
The bit values aren't sequential as per a normal enumeration, which is why I added this approach to setting the correct bits. However, I've just spotted the SOC_VALUE_ENUM_SINGLE macro which looks like it will do the job, so I'll use that instead. Thanks.
- /* ADCs */
- SOC_SINGLE_TLV("ADC Volume", DA7219_ADC_L_GAIN,
DA7219_ADC_L_DIGITAL_GAIN_SHIFT,DA7219_ADC_L_DIGITAL_GAIN_MAX, DA7219_NO_INVERT,da7219_adc_dig_gain_tlv),- SOC_SINGLE("ADC Switch", DA7219_ADC_L_CTRL,
DA7219_ADC_L_MUTE_EN_SHIFT, DA7219_SWITCH_EN_MAX,DA7219_INVERT),- SOC_SINGLE("ADC Gain Ramp Switch", DA7219_ADC_L_CTRL,
DA7219_ADC_L_RAMP_EN_SHIFT, DA7219_SWITCH_EN_MAX,DA7219_NO_INVERT),Capture Digital rather than ADC.
Ok, fine. Is this now the common naming to be used for all future codecs?
- SOC_SINGLE_TLV("ALC Max Attenuation", DA7219_ALC_GAIN_LIMITS,
DA7219_ALC_ATTEN_MAX_SHIFT,DA7219_ALC_ATTEN_GAIN_MAX,
DA7219_NO_INVERT, da7219_alc_gain_tlv),- SOC_SINGLE_TLV("ALC Max Gain", DA7219_ALC_GAIN_LIMITS,
DA7219_ALC_GAIN_MAX_SHIFT,DA7219_ALC_ATTEN_GAIN_MAX,
DA7219_NO_INVERT, da7219_alc_gain_tlv),- SOC_SINGLE_RANGE_TLV("ALC Min Analog Gain",
DA7219_ALC_ANA_GAIN_LIMITS,
DA7219_ALC_ANA_GAIN_MIN_SHIFT,DA7219_ALC_ANA_GAIN_MIN,DA7219_ALC_ANA_GAIN_MAX,
DA7219_NO_INVERT, da7219_alc_ana_gain_tlv),- SOC_SINGLE_RANGE_TLV("ALC Max Analog Gain",
DA7219_ALC_ANA_GAIN_LIMITS,
DA7219_ALC_ANA_GAIN_MAX_SHIFT,DA7219_ALC_ANA_GAIN_MIN,DA7219_ALC_ANA_GAIN_MAX,
DA7219_NO_INVERT, da7219_alc_ana_gain_tlv),Volume not Gain.
Fine, will update.
- SOC_SINGLE("ToneGen DTMF Key", DA7219_TONE_GEN_CFG1,
DA7219_DTMF_REG_SHIFT, DA7219_DTMF_REG_MAX,DA7219_NO_INVERT),Should this be an enumeration with the DTMF digits in it (# and * aren't numbers)?
Yes, agreed. Will update.
- /* DACs */
- SOC_DOUBLE_R_TLV("DAC Volume", DA7219_DAC_L_GAIN,
DA7219_DAC_R_GAIN,
DA7219_DAC_L_DIGITAL_GAIN_SHIFT,DA7219_DAC_DIGITAL_GAIN_MAX, DA7219_NO_INVERT,da7219_dac_dig_gain_tlv),- SOC_DOUBLE_R("DAC Switch", DA7219_DAC_L_CTRL,
DA7219_DAC_R_CTRL,
DA7219_DAC_L_MUTE_EN_SHIFT, DA7219_SWITCH_EN_MAX,DA7219_INVERT),- SOC_DOUBLE_R("DAC Gain Ramp Switch", DA7219_DAC_L_CTRL,
DA7219_DAC_R_CTRL, DA7219_DAC_L_RAMP_EN_SHIFT,DA7219_SWITCH_EN_MAX, DA7219_NO_INVERT),All the DAC controls should probably be Playback Digital instead.
Ok, similar naming convention as for Capture Digital. Will update.
+static int da7219_set_dai_sysclk(struct snd_soc_dai *codec_dai,
int clk_id, unsigned int freq, int dir)+{
- struct snd_soc_codec *codec = codec_dai->codec;
- struct da7219_priv *da7219 = snd_soc_codec_get_drvdata(codec);
- if (da7219->mclk_rate == freq)
return 0;Given that we also have source selection is this safe - should we also be checking the source?
Good spot. Will amend.
- if (da7219->mclk) {
freq = clk_round_rate(da7219->mclk, freq);clk_set_rate(da7219->mclk, freq);- }
Missing error checking.
Yes, will add in checking.
/* Internal LDO */if (da7219->use_int_ldo)snd_soc_update_bits(codec, DA7219_LDO_CTRL,DA7219_LDO_EN_MASK,DA7219_LDO_EN_MASK);If there is an option to use an external supply I would expect to see the regulator API used to discover the external LDO (and ideally also to configure the integrated LDO). If the driver works outside the frameworks then it is likely this will lead to integration issues later on.
Given the simplistic nature of the internal LDO, I didn't think it would be necessary to use the framework as it seemed overkill. I assume you mean following something like what is done in the sgtl5000 codec?
/* Internal LDO */da7219->use_int_ldo = pdata->use_internal_ldo;This is likely to lead to surprises for example...
- /* Check if MCLK provided, if not the clock is NULL */
- da7219->mclk = devm_clk_get(codec->dev, "mclk");
- if (IS_ERR(da7219->mclk)) {
if (PTR_ERR(da7219->mclk) == -EPROBE_DEFER)return -EPROBE_DEFER;da7219->mclk = NULL;- }
This should specifically look for the error code that corresponds to a clock not being mapped and only continue silently in that case rather than silently accepting all failures to obtain a clock - if we silently accept all failures that will mean that actual errors will be ignored.
Ok, I will add further error checking to cover this.
- /*
* There are multiple control bits for the input mixer.* The following can be enabled now as it's not power related.*/- snd_soc_update_bits(codec, DA7219_MIXIN_L_CTRL,
DA7219_MIXIN_L_MIX_EN_MASK,DA7219_MIXIN_L_MIX_EN_MASK);So the chip designers just put these in for randomness? Fun. It'd be more idiomatic to do something like making these supply widgets so they're controlled via DAPM even if they don't matter much.
Figured we'd be saving on additional I2C accesses if it's just done the once. Do you really think it needs to be a widget as it seems a little unnecessary enabling and disabling every time that path is powered up and down?
- else if (device_may_wakeup(codec->dev))
disable_irq_wake(da7219->aad->irq);You can use dev_pm_set_wake_irq() and skip having to manage this stuff explicitly in the driver.
Ah, didn't know about that. Will convert over to using that. Thanks.
On Mon, Sep 21, 2015 at 03:08:03PM +0000, Opensource [Adam Thomson] wrote:
On September 19, 2015 18:44, Mark Brown wrote:
This is obviously a massive reconfiguration of the device. I'm not seeing anything here which prevents userspace coming in and change the configuration while we're in this function, that would obviously have serious issues.
In a system scenario the likelihood of that happening is small as you cannot use the mic or headphones until they've been inserted. The system is only likely to act after the jack insertion events have occurred. However, it
This really isn't an OK approach here, you're making a whole bunch of assumptions about how the system is implemented that aren't robust and will lead to loss of audio if things go wrong which is a pretty serious consequence. Are you *sure* there's going to be a quick enough response to cover all jack inserts and remove (especially under load), or that userspace even bothers paying attention given that there's no other input and output devices?
would be better to prevent any chance of concurrent access. The problem is how best to lock the Kcontrols whilst the test procedure in progress. At the moment the only way I can see is to add explicit control set() functions which would use a lock that can also be controlled by the HP test code. Does this make sense to you or do you know of a simpler method? Obviously dapm has function to lock as required.
Yes, you're going to have to do something like that if you want to do this - you'll also need to lock the reads since otherwise userspace will see the intermediate control states.
For the cache resync idea, in terms of code, it will look cleaner, but you are talking about 4 to 5 times the number of I2C accesses to the device, to restore configuration. Does that not seem like a bit too much overhead?
There's regcache_sync_region().
Why are we scheduling work - we're already in thread context?
hptest will take some time to complete (over 100ms), and in that time it's plausible that a user could remove the jack. If we deal with this in the IRQ thread, we won't be aware of jack removal during the process, and will send a report regardless, which will almost definitely be incorrect, and unnecessary. By spawning off work, it allows the removal to be dealt with if the hptest work procedure is currently in process, and then we can avoid sending incorrect jack reports at the end.
OK, please document this then.
/* Un-drive headphones/lineout */snd_soc_update_bits(codec, DA7219_HP_R_CTRL,DA7219_HP_R_AMP_OE_MASK, 0);snd_soc_update_bits(codec, DA7219_HP_L_CTRL,DA7219_HP_L_AMP_OE_MASK, 0);
This looks like DAPM?
The control of driving the headphones or making them high impedance is handled in the AAD code because we cannot have the headphones driven before a jack is inserted as it will affect the pole detection. Adding it to DAPM seemed like it would cause more problems in terms of controlling when it would and wouldn't be enabled.
IIRC you had some DAPM updates in adjacent code?
- default:
return DA7219_AAD_JACK_INS_DEB_20MS;
This isn't an error?
Opted for HW default in case of invalid values provided. Maybe a dev_warn() would be useful though to indicate this is the case?
Yes - the user has explicitly tried to set something and the driver is ignoring it.
- ret = regmap_bulk_read(da7219->regmap, reg, &val, sizeof(val));
- if (ret)
return ret;
- ucontrol->value.integer.value[0] = le16_to_cpu(val);
This is *weird*. We do a bulk read for a single register using an API that returns CPU endian data then make it CPU endian (without any annotations on variables...). Why not use regmap_read()? Why swap? Why not use raw I/O?
The device is 8-bit register access only, and the value spans two registers, hence why this is done here. The register defined for the Kcontrol is the first in the sequence of two registers (first lower byte, second upper byte). Thought this was cleaner than having two separate controls to configure upper and lower bytes.
Again some documentation would help, and also using raw reads rather than bulk reads (which imply that all endianness issues will be taken care of). If you're doing a bulk read and handling endianness that's worrying. You should also have an endianness annotation for val.
This looks like a standard enumeration with a simple mapping to the register map?
The bit values aren't sequential as per a normal enumeration, which is why I added this approach to setting the correct bits. However, I've just spotted the SOC_VALUE_ENUM_SINGLE macro which looks like it will do the job, so I'll use that instead. Thanks.
Yes, you want a VALUE_ENUM.
- /* ADCs */
- SOC_SINGLE_TLV("ADC Volume", DA7219_ADC_L_GAIN,
DA7219_ADC_L_DIGITAL_GAIN_SHIFT,DA7219_ADC_L_DIGITAL_GAIN_MAX, DA7219_NO_INVERT,da7219_adc_dig_gain_tlv),- SOC_SINGLE("ADC Switch", DA7219_ADC_L_CTRL,
DA7219_ADC_L_MUTE_EN_SHIFT, DA7219_SWITCH_EN_MAX,DA7219_INVERT),- SOC_SINGLE("ADC Gain Ramp Switch", DA7219_ADC_L_CTRL,
DA7219_ADC_L_RAMP_EN_SHIFT, DA7219_SWITCH_EN_MAX,DA7219_NO_INVERT),
Capture Digital rather than ADC.
Ok, fine. Is this now the common naming to be used for all future codecs?
It's always been the naming in ControlNames.txt - we don't generally worry about it on devices with more flexible routing which mean that the associated meaning won't always be really true but for this device it seems that the options are sufficiently limited to allow userspace to use the standard name.
/* Internal LDO */if (da7219->use_int_ldo)snd_soc_update_bits(codec, DA7219_LDO_CTRL,DA7219_LDO_EN_MASK,DA7219_LDO_EN_MASK);
If there is an option to use an external supply I would expect to see the regulator API used to discover the external LDO (and ideally also to configure the integrated LDO). If the driver works outside the frameworks then it is likely this will lead to integration issues later on.
Given the simplistic nature of the internal LDO, I didn't think it would be necessary to use the framework as it seemed overkill. I assume you mean following something like what is done in the sgtl5000 codec?
That should work I think. The point here isn't really the control of the LDO itself, it's making sure that the integration with external supplies works well - the key bit is that how we figure out that we don't have an external supply connected should be joined up with how we normally integrate external supplies.
- /*
* There are multiple control bits for the input mixer.* The following can be enabled now as it's not power related.*/- snd_soc_update_bits(codec, DA7219_MIXIN_L_CTRL,
DA7219_MIXIN_L_MIX_EN_MASK,DA7219_MIXIN_L_MIX_EN_MASK);
So the chip designers just put these in for randomness? Fun. It'd be more idiomatic to do something like making these supply widgets so they're controlled via DAPM even if they don't matter much.
Figured we'd be saving on additional I2C accesses if it's just done the once. Do you really think it needs to be a widget as it seems a little unnecessary enabling and disabling every time that path is powered up and down?
It seems likely to be more robust against someone realising that the register bits actually do something useful and need toggling and it raises less eyebrows code wise.
If you're worried about the register writes you should also be able to arrange to map these in as mixer widgets which would mean that the the core will combine the writes with the main power controls.
On September 21, 2015 18:11, Mark Brown wrote:
In a system scenario the likelihood of that happening is small as you cannot use the mic or headphones until they've been inserted. The system is only likely to act after the jack insertion events have occurred. However, it
This really isn't an OK approach here, you're making a whole bunch of assumptions about how the system is implemented that aren't robust and will lead to loss of audio if things go wrong which is a pretty serious consequence. Are you *sure* there's going to be a quick enough response to cover all jack inserts and remove (especially under load), or that userspace even bothers paying attention given that there's no other input and output devices?
You make a fair point, and I'd much rather it was bullet proof.
would be better to prevent any chance of concurrent access. The problem is how best to lock the Kcontrols whilst the test procedure in progress. At the moment the only way I can see is to add explicit control set() functions which would use a lock that can also be controlled by the HP test code. Does this make sense to you or do you know of a simpler method? Obviously dapm has function to lock as required.
Yes, you're going to have to do something like that if you want to do this - you'll also need to lock the reads since otherwise userspace will see the intermediate control states.
Ok, will look at adding set() and get() functions for the controls affected.
For the cache resync idea, in terms of code, it will look cleaner, but you are talking about 4 to 5 times the number of I2C accesses to the device, to restore configuration. Does that not seem like a bit too much overhead?
There's regcache_sync_region().
That's fine if the registers are clumped closely together. Will have a look and see if it works out cleaner. Thanks.
Why are we scheduling work - we're already in thread context?
hptest will take some time to complete (over 100ms), and in that time it's plausible that a user could remove the jack. If we deal with this in the IRQ thread, we won't be aware of jack removal during the process, and will send a report regardless, which will almost definitely be incorrect, and unnecessary. By spawning off work, it allows the removal to be dealt with if the hptest work procedure is currently in process, and then we can avoid sending incorrect jack reports at the end.
OK, please document this then.
Ok, will add comments to clarify.
/* Un-drive headphones/lineout */snd_soc_update_bits(codec, DA7219_HP_R_CTRL,DA7219_HP_R_AMP_OE_MASK, 0);snd_soc_update_bits(codec, DA7219_HP_L_CTRL,DA7219_HP_L_AMP_OE_MASK, 0);This looks like DAPM?
The control of driving the headphones or making them high impedance is handled in the AAD code because we cannot have the headphones driven before a jack is inserted as it will affect the pole detection. Adding it to DAPM seemed like it would cause more problems in terms of controlling when it would and wouldn't be enabled.
IIRC you had some DAPM updates in adjacent code?
Yes, there's a disable_pin() call for micbias. However that will not disable the pin indefinitely which is what I would require for this. I need to know that there's no chance of the pin being enabled prior to jack insertion.
- default:
return DA7219_AAD_JACK_INS_DEB_20MS;This isn't an error?
Opted for HW default in case of invalid values provided. Maybe a dev_warn() would be useful though to indicate this is the case?
Yes - the user has explicitly tried to set something and the driver is ignoring it.
Agreed. Will update accordingly.
- ret = regmap_bulk_read(da7219->regmap, reg, &val, sizeof(val));
- if (ret)
return ret;
- ucontrol->value.integer.value[0] = le16_to_cpu(val);
This is *weird*. We do a bulk read for a single register using an API that returns CPU endian data then make it CPU endian (without any annotations on variables...). Why not use regmap_read()? Why swap? Why not use raw I/O?
The device is 8-bit register access only, and the value spans two registers, hence why this is done here. The register defined for the Kcontrol is the first in the sequence of two registers (first lower byte, second upper byte). Thought this was cleaner than having two separate controls to configure upper and lower bytes.
Again some documentation would help, and also using raw reads rather than bulk reads (which imply that all endianness issues will be taken care of). If you're doing a bulk read and handling endianness that's worrying. You should also have an endianness annotation for val.
If I had used bulk_read() and an array of 2 u8 values instead, and then converted what was read into a u16, would that have been better/clearer? I can do that, but figured this might be a more elegant soluion. Either, way I'll add some comments to clarify what's going on.
Capture Digital rather than ADC.
Ok, fine. Is this now the common naming to be used for all future codecs?
It's always been the naming in ControlNames.txt - we don't generally worry about it on devices with more flexible routing which mean that the associated meaning won't always be really true but for this device it seems that the options are sufficiently limited to allow userspace to use the standard name.
Ok, understood.
/* Internal LDO */if (da7219->use_int_ldo)snd_soc_update_bits(codec, DA7219_LDO_CTRL,DA7219_LDO_EN_MASK,DA7219_LDO_EN_MASK);If there is an option to use an external supply I would expect to see the regulator API used to discover the external LDO (and ideally also to configure the integrated LDO). If the driver works outside the frameworks then it is likely this will lead to integration issues later on.
Given the simplistic nature of the internal LDO, I didn't think it would be necessary to use the framework as it seemed overkill. I assume you mean following something like what is done in the sgtl5000 codec?
That should work I think. The point here isn't really the control of the LDO itself, it's making sure that the integration with external supplies works well - the key bit is that how we figure out that we don't have an external supply connected should be joined up with how we normally integrate external supplies.
Ok, thanks. Will update.
- /*
* There are multiple control bits for the input mixer.* The following can be enabled now as it's not power related.*/- snd_soc_update_bits(codec, DA7219_MIXIN_L_CTRL,
DA7219_MIXIN_L_MIX_EN_MASK,DA7219_MIXIN_L_MIX_EN_MASK);So the chip designers just put these in for randomness? Fun. It'd be more idiomatic to do something like making these supply widgets so they're controlled via DAPM even if they don't matter much.
Figured we'd be saving on additional I2C accesses if it's just done the once. Do you really think it needs to be a widget as it seems a little unnecessary enabling and disabling every time that path is powered up and down?
It seems likely to be more robust against someone realising that the register bits actually do something useful and need toggling and it raises less eyebrows code wise.
If you're worried about the register writes you should also be able to arrange to map these in as mixer widgets which would mean that the the core will combine the writes with the main power controls.
Just didn't seem necessary. However, will change it as requested.
participants (2)
-
Mark Brown -
Opensource [Adam Thomson]