Re: [alsa-devel] [PATCH 1/3] ASoC: codecs: Add da7219 codec 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.