[PATCH v3 4/4] ALSA: hda/cirrus: Add Headphone and Headset MIC Volume Control

[Gerrit]

Hi Pierre-Louis,

Thank you for your comments. It looks like this patch set has been merged, so we will look to address your comments in a future patch.

-----Original Message-----
From: Alsa-devel <alsa-devel-bounces at alsa-project.org> On Behalf Of Pierre-Louis Bossart
Sent: 08 March 2021 15:40
To: Vitaly Rodionov <vitalyr at opensource.cirrus.com>; Jaroslav Kysela <perex at perex.cz>; Takashi Iwai <tiwai at suse.com>
Cc: patches at opensource.cirrus.com; alsa-devel at alsa-project.org; linux-kernel at vger.kernel.org; Stefan Binding <sbinding at opensource.cirrus.com>
Subject: Re: [PATCH v3 4/4] ALSA: hda/cirrus: Add Headphone and Headset MIC Volume Control



On 3/6/21 5:19 AM, Vitaly Rodionov wrote:
> From: Stefan Binding <sbinding at opensource.cirrus.com>
> 
> CS8409 does not support Volume Control for NIDs 0x24 (the Headphones), 
> or 0x34 (The Headset Mic).
> However, CS42L42 codec does support gain control for both.

Volume Control for both

> We can add support for Volume Controls, by writing the the CS42L42 
> regmap via i2c commands, using custom info, get and put volume 
> functions, saved in the control.
> 
> Tested on DELL Inspiron-3500, DELL Inspiron-3501, DELL Inspiron-3500
> 
> Signed-off-by: Stefan Binding <sbinding at opensource.cirrus.com>
> Signed-off-by: Vitaly Rodionov <vitalyr at opensource.cirrus.com>
> ---
> 
> Changes in v3:
> - Added restore volumes after resume
> - Removed redundant debug logging after testing
> 
> 
>   sound/pci/hda/patch_cirrus.c | 200 +++++++++++++++++++++++++++++++++++
>   1 file changed, 200 insertions(+)
> 
> diff --git a/sound/pci/hda/patch_cirrus.c 
> b/sound/pci/hda/patch_cirrus.c index 1d2f6a1224e6..6a9e5c803977 100644
> --- a/sound/pci/hda/patch_cirrus.c
> +++ b/sound/pci/hda/patch_cirrus.c
> @@ -21,6 +21,9 @@
>   /*
>    */
>   
> +#define CS42L42_HP_CH     (2U)
> +#define CS42L42_HS_MIC_CH (1U)
> +
>   struct cs_spec {
>   	struct hda_gen_spec gen;
>   
> @@ -42,6 +45,9 @@ struct cs_spec {
>   
>   	unsigned int cs42l42_hp_jack_in:1;
>   	unsigned int cs42l42_mic_jack_in:1;
> +	unsigned int cs42l42_volume_init:1;

can you describe what this field is? it looks like it's only tracking a one-time initialization?
Yes, this field is used to track one-time initialization.
Since the CS42L42 codec needs to be reset on init, the volume values need to be restored afterwards.
This flag allows us to check whether we have previously cached the volume values, so they can be restored.

> +	char cs42l42_hp_volume[CS42L42_HP_CH];
> +	char cs42l42_hs_mic_volume[CS42L42_HS_MIC_CH];
>   
>   	struct mutex cs8409_i2c_mux;
>   
> @@ -1260,6 +1266,14 @@ static int patch_cs4213(struct hda_codec *codec)
>   #define CIR_I2C_QWRITE	0x005D
>   #define CIR_I2C_QREAD	0x005E
>   
> +#define CS8409_CS42L42_HP_VOL_REAL_MIN   (-63)
> +#define CS8409_CS42L42_HP_VOL_REAL_MAX   (0)
> +#define CS8409_CS42L42_AMIC_VOL_REAL_MIN (-97) #define 
> +CS8409_CS42L42_AMIC_VOL_REAL_MAX (12) #define 
> +CS8409_CS42L42_REG_HS_VOLUME_CHA (0x2301) #define 
> +CS8409_CS42L42_REG_HS_VOLUME_CHB (0x2303)
> +#define CS8409_CS42L42_REG_AMIC_VOLUME   (0x1D03)
> +
>   struct cs8409_i2c_param {
>   	unsigned int addr;
>   	unsigned int reg;
> @@ -1580,6 +1594,165 @@ static unsigned int cs8409_i2c_write(struct hda_codec *codec,
>   	return retval;
>   }
>   
> +static int cs8409_cs42l42_volume_info(struct snd_kcontrol *kcontrol,
> +				  struct snd_ctl_elem_info *uinfo) {
> +	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
> +	u16 nid = get_amp_nid(kcontrol);
> +	u8 chs = get_amp_channels(kcontrol);
> +
> +	codec_dbg(codec, "%s() nid: %d\n", __func__, nid);
> +	switch (nid) {
> +	case CS8409_CS42L42_HP_PIN_NID:
> +		uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
> +		uinfo->count = chs == 3 ? 2 : 1;
> +		uinfo->value.integer.min = CS8409_CS42L42_HP_VOL_REAL_MIN;
> +		uinfo->value.integer.max = CS8409_CS42L42_HP_VOL_REAL_MAX;
> +		break;
> +	case CS8409_CS42L42_AMIC_PIN_NID:
> +		uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
> +		uinfo->count = chs == 3 ? 2 : 1;
> +		uinfo->value.integer.min = CS8409_CS42L42_AMIC_VOL_REAL_MIN;
> +		uinfo->value.integer.max = CS8409_CS42L42_AMIC_VOL_REAL_MAX;
> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static void cs8409_cs42l42_update_volume(struct hda_codec *codec) {
> +	struct cs_spec *spec = codec->spec;
> +
> +	mutex_lock(&spec->cs8409_i2c_mux);
> +	spec->cs42l42_hp_volume[0] = -(cs8409_i2c_read(codec, CS42L42_I2C_ADDR,
> +				CS8409_CS42L42_REG_HS_VOLUME_CHA, 1));
> +	spec->cs42l42_hp_volume[1] = -(cs8409_i2c_read(codec, CS42L42_I2C_ADDR,
> +				CS8409_CS42L42_REG_HS_VOLUME_CHB, 1));
> +	spec->cs42l42_hs_mic_volume[0] = -(cs8409_i2c_read(codec, CS42L42_I2C_ADDR,
> +				CS8409_CS42L42_REG_AMIC_VOLUME, 1));
> +	mutex_unlock(&spec->cs8409_i2c_mux);
> +	spec->cs42l42_volume_init = 1;
> +}
> +
> +static int cs8409_cs42l42_volume_get(struct snd_kcontrol *kcontrol,
> +				 struct snd_ctl_elem_value *ucontrol) {
> +	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
> +	struct cs_spec *spec = codec->spec;
> +	hda_nid_t nid = get_amp_nid(kcontrol);
> +	int chs = get_amp_channels(kcontrol);
> +	long *valp = ucontrol->value.integer.value;
> +
> +	if (!spec->cs42l42_volume_init) {
> +		snd_hda_power_up(codec);
> +		cs8409_cs42l42_update_volume(codec);
> +		snd_hda_power_down(codec);
> +	}
> +	switch (nid) {
> +	case CS8409_CS42L42_HP_PIN_NID:
> +		if (chs & 1)

BIT(0)?
Will fix that.

> +			*valp++ = spec->cs42l42_hp_volume[0];
> +		if (chs & 2)

BIT(1)?
Will fix that.

> +			*valp++ = spec->cs42l42_hp_volume[1];
> +		break;