Skip to contentSkip to conversation

ALSA: virtuoso: add de-emphasis control

PATCH In review [email protected] 3 participants 8 messages Dec 15, 2018 – Jan 7, 2019

Subsystems (2)
Sound C-media CMI8788 driver
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 ALSA: virtuoso: add de-emphasis control

From: Tom Yan <[email protected]>

Add control for the de-emphasis filter in the PCM179x DACs

Signed-off-by Tom Yan <[email protected]>
sound/pci/oxygen/xonar_pcm179x.c +80 -2
index 24109d37ca09..0014c55ecc3d 100644
@@ -331,7 +331,7 @@ static void pcm1796_init(struct oxygen *chip)
 	struct xonar_pcm179x *data = chip->model_data;
 
 	data->pcm1796_regs[0][18 - PCM1796_REG_BASE] =
-		PCM1796_DMF_DISABLED | PCM1796_FMT_24_I2S | PCM1796_ATLD;
+		PCM1796_FMT_24_I2S | PCM1796_ATLD;
 	if (!data->broken_i2c)
 		data->pcm1796_regs[0][18 - PCM1796_REG_BASE] |= PCM1796_MUTE;
 	data->pcm1796_regs[0][19 - PCM1796_REG_BASE] =

@@ -621,6 +621,33 @@ static void update_pcm1796_oversampling(struct oxygen *chip)
 		pcm1796_write_cached(chip, i, 20, reg);
 }
 
+static void update_pcm1796_deemph(struct oxygen *chip)
+{
+	struct xonar_pcm179x *data = chip->model_data;
+	unsigned int i;
+	u8 reg;
+
+	reg = data->pcm1796_regs[0][18 - PCM1796_REG_BASE] & ~PCM1796_DMF_MASK;
+	switch (data->current_rate) {
+	case 32000:
+	case 64000:
+		reg |= PCM1796_DMF_32;
+		break;
+	case 44100:
+	case 88200:
+	case 176400:
+		reg |= PCM1796_DMF_441;
+		break;
+	case 48000:
+	case 96000:
+	case 192000:
+		reg |= PCM1796_DMF_48;
+		break;
+	}
+	for (i = 0; i < data->dacs; ++i)
+		pcm1796_write_cached(chip, i, 18, reg);
+}
+
 static void set_pcm1796_params(struct oxygen *chip,
 			       struct snd_pcm_hw_params *params)
 {

@@ -629,6 +656,7 @@ static void set_pcm1796_params(struct oxygen *chip,
 	msleep(1);
 	data->current_rate = params_rate(params);
 	update_pcm1796_oversampling(chip);
+	update_pcm1796_deemph(chip);
 }
 
 static void update_pcm1796_volume(struct oxygen *chip)

@@ -653,9 +681,11 @@ static void update_pcm1796_mute(struct oxygen *chip)
 	unsigned int i;
 	u8 value;
 
-	value = PCM1796_DMF_DISABLED | PCM1796_FMT_24_I2S | PCM1796_ATLD;
+	value = data->pcm1796_regs[0][18 - PCM1796_REG_BASE];
 	if (chip->dac_mute)
 		value |= PCM1796_MUTE;
+	else
+		value &= ~PCM1796_MUTE;
 	for (i = 0; i < data->dacs; ++i)
 		pcm1796_write_cached(chip, i, 18, value);
 }

@@ -777,6 +807,50 @@ static const struct snd_kcontrol_new rolloff_control = {
 	.put = rolloff_put,
 };
 
+static int deemph_get(struct snd_kcontrol *ctl,
+		       struct snd_ctl_elem_value *value)
+{
+	struct oxygen *chip = ctl->private_data;
+	struct xonar_pcm179x *data = chip->model_data;
+
+	value->value.integer.value[0] =
+		(data->pcm1796_regs[0][18 - PCM1796_REG_BASE] &
+		 PCM1796_DME) != PCM1796_DMF_DISABLED;
+	return 0;
+}
+
+static int deemph_put(struct snd_kcontrol *ctl,
+		       struct snd_ctl_elem_value *value)
+{
+	struct oxygen *chip = ctl->private_data;
+	struct xonar_pcm179x *data = chip->model_data;
+	unsigned int i;
+	int changed;
+	u8 reg;
+
+	mutex_lock(&chip->mutex);
+	reg = data->pcm1796_regs[0][18 - PCM1796_REG_BASE];
+	if (!value->value.integer.value[0])
+		reg &= ~PCM1796_DME;
+	else
+		reg |= PCM1796_DME;
+	changed = reg != data->pcm1796_regs[0][18 - PCM1796_REG_BASE];
+	if (changed) {
+		for (i = 0; i < data->dacs; ++i)
+			pcm1796_write(chip, i, 18, reg);
+	}
+	mutex_unlock(&chip->mutex);
+	return changed;
+}
+
+static const struct snd_kcontrol_new deemph_control = {
+	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+	.name = "De-emphasis Switch",
+	.info = snd_ctl_boolean_mono_info,
+	.get = deemph_get,
+	.put = deemph_put,
+};
+
 static const struct snd_kcontrol_new hdav_hdmi_control = {
 	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
 	.name = "HDMI Playback Switch",

@@ -1011,6 +1085,10 @@ static int add_pcm1796_controls(struct oxygen *chip)
 				  snd_ctl_new1(&rolloff_control, chip));
 		if (err < 0)
 			return err;
+		err = snd_ctl_add(chip->card,
+				  snd_ctl_new1(&deemph_control, chip));
+		if (err < 0)
+			return err;
 	}
 	return 0;
 }
Signature · 1 line
2.20.0
maintainer
#1 lore raw
Quoted 5 lines · [email protected] wrote:
sound/pci/oxygen/xonar_pcm179x.c +5 -0
+	switch (data->current_rate) {
+	case 32000:
+	case 64000:
+		reg |= PCM1796_DMF_32;
+		break;

sound/pci/oxygen/xonar_pcm179x.c static void update_pcm1796_oversampling(struct oxygen *chip) · line 645

Quoted 10 lines
sound/pci/oxygen/xonar_pcm179x.c +10 -0
+	case 44100:
+	case 88200:
+	case 176400:
+		reg |= PCM1796_DMF_441;
+		break;
+	case 48000:
+	case 96000:
+	case 192000:
+		reg |= PCM1796_DMF_48;
+		break;

The DAC does not have filters for frequencies higher than 48 kHz.

sound/pci/oxygen/xonar_pcm179x.c static const struct snd_kcontrol_new rolloff_control = { · line 848

Quoted 3 lines
sound/pci/oxygen/xonar_pcm179x.c +3 -0
+static const struct snd_kcontrol_new deemph_control = {
+	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+	.name = "De-emphasis Switch",

This should be called "De-emphasis Playback Switch". (But I see that other drivers are quite inconsistent about this.)

Regards,
Clemens

author
#2 lore raw

PATCH 1/1 ALSA: virtuoso: add de-emphasis control

From: Tom Yan <[email protected]>

Add control for the de-emphasis filter in the PCM179x DACs

Signed-off-by Tom Yan <[email protected]>
sound/pci/oxygen/xonar_pcm179x.c +70 -2
index 24109d37ca09..429411d10e08 100644
@@ -331,7 +331,7 @@ static void pcm1796_init(struct oxygen *chip)
 	struct xonar_pcm179x *data = chip->model_data;
 
 	data->pcm1796_regs[0][18 - PCM1796_REG_BASE] =
-		PCM1796_DMF_DISABLED | PCM1796_FMT_24_I2S | PCM1796_ATLD;
+		PCM1796_FMT_24_I2S | PCM1796_ATLD;
 	if (!data->broken_i2c)
 		data->pcm1796_regs[0][18 - PCM1796_REG_BASE] |= PCM1796_MUTE;
 	data->pcm1796_regs[0][19 - PCM1796_REG_BASE] =

@@ -621,6 +621,23 @@ static void update_pcm1796_oversampling(struct oxygen *chip)
 		pcm1796_write_cached(chip, i, 20, reg);
 }
 
+static void update_pcm1796_deemph(struct oxygen *chip)
+{
+	struct xonar_pcm179x *data = chip->model_data;
+	unsigned int i;
+	u8 reg;
+
+	reg = data->pcm1796_regs[0][18 - PCM1796_REG_BASE] & ~PCM1796_DMF_MASK;
+	if (data->current_rate == 48000)
+		reg |= PCM1796_DMF_48;
+	else if (data->current_rate == 44100)
+		reg |= PCM1796_DMF_441;
+	else if (data->current_rate == 32000)
+		reg |= PCM1796_DMF_32;
+	for (i = 0; i < data->dacs; ++i)
+		pcm1796_write_cached(chip, i, 18, reg);
+}
+
 static void set_pcm1796_params(struct oxygen *chip,
 			       struct snd_pcm_hw_params *params)
 {

@@ -629,6 +646,7 @@ static void set_pcm1796_params(struct oxygen *chip,
 	msleep(1);
 	data->current_rate = params_rate(params);
 	update_pcm1796_oversampling(chip);
+	update_pcm1796_deemph(chip);
 }
 
 static void update_pcm1796_volume(struct oxygen *chip)

@@ -653,9 +671,11 @@ static void update_pcm1796_mute(struct oxygen *chip)
 	unsigned int i;
 	u8 value;
 
-	value = PCM1796_DMF_DISABLED | PCM1796_FMT_24_I2S | PCM1796_ATLD;
+	value = data->pcm1796_regs[0][18 - PCM1796_REG_BASE];
 	if (chip->dac_mute)
 		value |= PCM1796_MUTE;
+	else
+		value &= ~PCM1796_MUTE;
 	for (i = 0; i < data->dacs; ++i)
 		pcm1796_write_cached(chip, i, 18, value);
 }

@@ -777,6 +797,50 @@ static const struct snd_kcontrol_new rolloff_control = {
 	.put = rolloff_put,
 };
 
+static int deemph_get(struct snd_kcontrol *ctl,
+		       struct snd_ctl_elem_value *value)
+{
+	struct oxygen *chip = ctl->private_data;
+	struct xonar_pcm179x *data = chip->model_data;
+
+	value->value.integer.value[0] =
+		(data->pcm1796_regs[0][18 - PCM1796_REG_BASE] &
+		 PCM1796_DME) != PCM1796_DMF_DISABLED;
+	return 0;
+}
+
+static int deemph_put(struct snd_kcontrol *ctl,
+		       struct snd_ctl_elem_value *value)
+{
+	struct oxygen *chip = ctl->private_data;
+	struct xonar_pcm179x *data = chip->model_data;
+	unsigned int i;
+	int changed;
+	u8 reg;
+
+	mutex_lock(&chip->mutex);
+	reg = data->pcm1796_regs[0][18 - PCM1796_REG_BASE];
+	if (!value->value.integer.value[0])
+		reg &= ~PCM1796_DME;
+	else
+		reg |= PCM1796_DME;
+	changed = reg != data->pcm1796_regs[0][18 - PCM1796_REG_BASE];
+	if (changed) {
+		for (i = 0; i < data->dacs; ++i)
+			pcm1796_write(chip, i, 18, reg);
+	}
+	mutex_unlock(&chip->mutex);
+	return changed;
+}
+
+static const struct snd_kcontrol_new deemph_control = {
+	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+	.name = "De-emphasis Playback Switch",
+	.info = snd_ctl_boolean_mono_info,
+	.get = deemph_get,
+	.put = deemph_put,
+};
+
 static const struct snd_kcontrol_new hdav_hdmi_control = {
 	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
 	.name = "HDMI Playback Switch",

@@ -1011,6 +1075,10 @@ static int add_pcm1796_controls(struct oxygen *chip)
 				  snd_ctl_new1(&rolloff_control, chip));
 		if (err < 0)
 			return err;
+		err = snd_ctl_add(chip->card,
+				  snd_ctl_new1(&deemph_control, chip));
+		if (err < 0)
+			return err;
 	}
 	return 0;
 }
Signature · 1 line
2.20.0
maintainer Acked-by on v1 1/1
#3 lore raw

sound/pci/oxygen/xonar_pcm179x.c static const struct snd_kcontrol_new rolloff_control = { · line 808

Quoted 2 lines · [email protected] wrote:
sound/pci/oxygen/xonar_pcm179x.c +2 -0
+		(data->pcm1796_regs[0][18 - PCM1796_REG_BASE] &
+		 PCM1796_DME) != PCM1796_DMF_DISABLED;

DME and DMF are different fields. Just use !!.

With this fixed:

Acked-by Clemens Ladisch <[email protected]>

Regards,
Clemens

author
#4 lore raw

Considering with my patch this is the only remaining usage of it, do you think I should just change it into "PCM1796_DME_DISABLED" and keep this manner (so that it is consistent with rolloff_get() / PCM1796_FLT_SHARP)?
Or do you insist that I should use !! anyway?

Quoted 13 lines · Clemens Ladisch

sound/pci/oxygen/xonar_pcm179x.c static const struct snd_kcontrol_new rolloff_control = { · line 808

Quoted 2 lines · [email protected] wrote:
sound/pci/oxygen/xonar_pcm179x.c +2 -0
+             (data->pcm1796_regs[0][18 - PCM1796_REG_BASE] &
+              PCM1796_DME) != PCM1796_DMF_DISABLED;

DME and DMF are different fields. Just use !!.

With this fixed:

Acked-by Clemens Ladisch <[email protected]>

Regards,
Clemens

maintainer
#5 lore raw
Quoted 5 lines · Tom Yan

Considering with my patch this is the only remaining usage of it, do you think I should just change it into "PCM1796_DME_DISABLED" and keep this manner (so that it is consistent with rolloff_get() / PCM1796_FLT_SHARP)?
Or do you insist that I should use !! anyway?

There is no "PCM1796_DME_DISABLED" symbol. IMO this is not necessary for register bits with a boolean meaning.

You could add it if you wanted to, but I do not think that consistency would be sufficient reason for that.

Regards,
Clemens

author
#6 lore raw

PATCH 1/1 ALSA: virtuoso: add de-emphasis control

From: Tom Yan <[email protected]>

Add control for the de-emphasis filter in the PCM179x DACs

Signed-off-by Tom Yan <[email protected]>
sound/pci/oxygen/pcm1796.h +0 -1
index 34d07dd2d22e..d5dcb09e44cd 100644
@@ -10,7 +10,6 @@
 #define PCM1796_MUTE		0x01
 #define PCM1796_DME		0x02
 #define PCM1796_DMF_MASK	0x0c
-#define PCM1796_DMF_DISABLED	0x00
 #define PCM1796_DMF_48		0x04
 #define PCM1796_DMF_441		0x08
 #define PCM1796_DMF_32		0x0c
sound/pci/oxygen/xonar_pcm179x.c +69 -2

109 lines not on this page · raw message

Signature · 1 line
2.20.1