diff mbox series

[v4,4/7] ASoC: codecs: wcd937x: add basic controls

Message ID 20240516044801.1061838-5-quic_mohs@quicinc.com (mailing list archive)
State Superseded
Headers show
Series ASoC: codecs: wcd937x: add wcd937x audio codec support | expand

Commit Message

Mohammad Rafi Shaik May 16, 2024, 4:47 a.m. UTC
From: Prasad Kumpatla <quic_pkumpatl@quicinc.com>

This patch adds basic controls found in WCD9370/WCD9375 codec.

Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
Co-developed-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
Signed-off-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
---
 sound/soc/codecs/wcd937x.c | 212 +++++++++++++++++++++++++++++++++++++
 1 file changed, 212 insertions(+)

Comments

Mark Brown May 16, 2024, 11:58 a.m. UTC | #1
On Thu, May 16, 2024 at 10:17:58AM +0530, Mohammad Rafi Shaik wrote:

> +static int wcd937x_rx_hph_mode_put(struct snd_kcontrol *kcontrol,
> +				   struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component =
> +				snd_soc_kcontrol_component(kcontrol);
> +	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(component);
> +	u32 mode_val;
> +
> +	mode_val = ucontrol->value.enumerated.item[0];
> +	if (!mode_val) {
> +		dev_warn(component->dev, "Invalid HPH Mode, default to class_AB\n");
> +		mode_val = CLS_AB;

This should be silent (or return an error) otherwise people can DoS the
logs by just spamming in invalid values.

> +	}
> +
> +	wcd937x->hph_mode = mode_val;

I would expect there's more validation needed here, this will blindly
assign any non-zero mode.  Please run the mixer-test selftests on a card
with this device in it and show the results on future submissions, this
will detect this and other issues for you.

Several of the other controls look like they're also missing validation.

> +static int wcd937x_set_swr_port(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct soc_mixer_control *mixer = (struct soc_mixer_control *)kcontrol->private_value;
> +	struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol);
> +	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(comp);
> +	struct wcd937x_sdw_priv *wcd;
> +	int dai_id = mixer->shift;
> +	int ch_idx = mixer->reg;
> +	int portidx;
> +	bool enable;
> +
> +	wcd = wcd937x->sdw_priv[dai_id];
> +
> +	portidx = wcd->ch_info[ch_idx].port_num;
> +
> +	enable = !!ucontrol->value.integer.value[0];
> +
> +	wcd->port_enable[portidx] = enable;
> +	wcd937x_connect_port(wcd, portidx, ch_idx, enable);
> +
> +	return 1;
> +}

This unconditionally reports that the value changed so will generate
spurious events.
> +
> +static const char * const rx_hph_mode_mux_text[] = {
> +	"CLS_H_INVALID", "CLS_H_HIFI", "CLS_H_LP", "CLS_AB", "CLS_H_LOHIFI",
> +	"CLS_H_ULP", "CLS_AB_HIFI",
> +};

It would be more idiomatic to write these in a more human readable form.

> +static const char * const wcd937x_ear_pa_gain_text[] = {
> +	"G_6_DB", "G_4P5_DB", "G_3_DB", "G_1P5_DB", "G_0_DB",
> +	"G_M1P5_DB", "G_M3_DB", "G_M4P5_DB",
> +	"G_M6_DB", "G_7P5_DB", "G_M9_DB",
> +	"G_M10P5_DB", "G_M12_DB", "G_M13P5_DB",
> +	"G_M15_DB", "G_M16P5_DB", "G_M18_DB",
> +};

Why is this an enum and not TLV information?
Mohammad Rafi Shaik May 22, 2024, 11:19 a.m. UTC | #2
On 5/16/2024 5:28 PM, Mark Brown wrote:
> On Thu, May 16, 2024 at 10:17:58AM +0530, Mohammad Rafi Shaik wrote:
> 
>> +static int wcd937x_rx_hph_mode_put(struct snd_kcontrol *kcontrol,
>> +				   struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct snd_soc_component *component =
>> +				snd_soc_kcontrol_component(kcontrol);
>> +	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(component);
>> +	u32 mode_val;
>> +

Thanks for the review.

>> +	mode_val = ucontrol->value.enumerated.item[0];
>> +	if (!mode_val) {
>> +		dev_warn(component->dev, "Invalid HPH Mode, default to class_AB\n");
>> +		mode_val = CLS_AB;
> 
> This should be silent (or return an error) otherwise people can DoS the
> logs by just spamming in invalid values.
> 

okay,

I will remove logs.

>> +	}
>> +
>> +	wcd937x->hph_mode = mode_val;
> 
> I would expect there's more validation needed here, this will blindly
> assign any non-zero mode.  Please run the mixer-test selftests on a card
> with this device in it and show the results on future submissions, this
> will detect this and other issues for you.
> 
> Several of the other controls look like they're also missing validation.
> 

I'll add missing validations for all controls.

>> +static int wcd937x_set_swr_port(struct snd_kcontrol *kcontrol,
>> +				struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct soc_mixer_control *mixer = (struct soc_mixer_control *)kcontrol->private_value;
>> +	struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol);
>> +	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(comp);
>> +	struct wcd937x_sdw_priv *wcd;
>> +	int dai_id = mixer->shift;
>> +	int ch_idx = mixer->reg;
>> +	int portidx;
>> +	bool enable;
>> +
>> +	wcd = wcd937x->sdw_priv[dai_id];
>> +
>> +	portidx = wcd->ch_info[ch_idx].port_num;
>> +
>> +	enable = !!ucontrol->value.integer.value[0];
>> +
>> +	wcd->port_enable[portidx] = enable;
>> +	wcd937x_connect_port(wcd, portidx, ch_idx, enable);
>> +
>> +	return 1;
>> +}
> 
> This unconditionally reports that the value changed so will generate
> spurious events.
>> +
>> +static const char * const rx_hph_mode_mux_text[] = {
>> +	"CLS_H_INVALID", "CLS_H_HIFI", "CLS_H_LP", "CLS_AB", "CLS_H_LOHIFI",
>> +	"CLS_H_ULP", "CLS_AB_HIFI",
>> +};
> 
> It would be more idiomatic to write these in a more human readable form.

i guess this change is fine.

I took the reference from wcd938x and wcd939x.

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/codecs/wcd938x.c#n1693

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/codecs/wcd939x.c#n1635

> 
>> +static const char * const wcd937x_ear_pa_gain_text[] = {
>> +	"G_6_DB", "G_4P5_DB", "G_3_DB", "G_1P5_DB", "G_0_DB",
>> +	"G_M1P5_DB", "G_M3_DB", "G_M4P5_DB",
>> +	"G_M6_DB", "G_7P5_DB", "G_M9_DB",
>> +	"G_M10P5_DB", "G_M12_DB", "G_M13P5_DB",
>> +	"G_M15_DB", "G_M16P5_DB", "G_M18_DB",
>> +};
> 
> Why is this an enum and not TLV information?

I'll check and add TLV instead enum.

Thanks & Regards,
Rafi
diff mbox series

Patch

diff --git a/sound/soc/codecs/wcd937x.c b/sound/soc/codecs/wcd937x.c
index 6038739beb19..32d94620f911 100644
--- a/sound/soc/codecs/wcd937x.c
+++ b/sound/soc/codecs/wcd937x.c
@@ -120,6 +120,9 @@  struct wcd937x_priv {
 	atomic_t ana_clk_count;
 };
 
+static const DECLARE_TLV_DB_SCALE(line_gain, 0, 7, 1);
+static const DECLARE_TLV_DB_SCALE(analog_gain, 0, 25, 1);
+
 struct wcd937x_mbhc_zdet_param {
 	u16 ldo_ctl;
 	u16 noff;
@@ -476,6 +479,169 @@  static int wcd937x_connect_port(struct wcd937x_sdw_priv *wcd, u8 port_idx, u8 ch
 	return 0;
 }
 
+static int wcd937x_rx_hph_mode_get(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(component);
+
+	ucontrol->value.integer.value[0] = wcd937x->hph_mode;
+	return 0;
+}
+
+static int wcd937x_rx_hph_mode_put(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component =
+				snd_soc_kcontrol_component(kcontrol);
+	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(component);
+	u32 mode_val;
+
+	mode_val = ucontrol->value.enumerated.item[0];
+	if (!mode_val) {
+		dev_warn(component->dev, "Invalid HPH Mode, default to class_AB\n");
+		mode_val = CLS_AB;
+	}
+
+	wcd937x->hph_mode = mode_val;
+
+	return 0;
+}
+
+static int wcd937x_ear_pa_gain_get(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	u8 ear_pa_gain;
+
+	ear_pa_gain = snd_soc_component_read(component, WCD937X_ANA_EAR_COMPANDER_CTL);
+
+	ucontrol->value.integer.value[0] = FIELD_GET(WCD937X_EAR_GAIN_MASK, ear_pa_gain);
+
+	return 0;
+}
+
+static int wcd937x_ear_pa_gain_put(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(component);
+	u8 ear_pa_gain = 0;
+
+	ear_pa_gain = ucontrol->value.integer.value[0] << 2;
+
+	if (!wcd937x->comp1_enable) {
+		snd_soc_component_update_bits(component,
+					      WCD937X_ANA_EAR_COMPANDER_CTL,
+					      0x7c, ear_pa_gain);
+	}
+
+	return 0;
+}
+
+static int wcd937x_get_compander(struct snd_kcontrol *kcontrol,
+				 struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(component);
+	struct soc_mixer_control *mc;
+	bool hphr;
+
+	mc = (struct soc_mixer_control *)(kcontrol->private_value);
+	hphr = mc->shift;
+
+	ucontrol->value.integer.value[0] = hphr ? wcd937x->comp2_enable :
+						  wcd937x->comp1_enable;
+	return 0;
+}
+
+static int wcd937x_set_compander(struct snd_kcontrol *kcontrol,
+				 struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(component);
+	struct wcd937x_sdw_priv *wcd = wcd937x->sdw_priv[AIF1_PB];
+	int value = ucontrol->value.integer.value[0];
+	struct soc_mixer_control *mc;
+	int portidx;
+	bool hphr;
+
+	mc = (struct soc_mixer_control *)(kcontrol->private_value);
+	hphr = mc->shift;
+
+	if (hphr)
+		wcd937x->comp2_enable = value;
+	else
+		wcd937x->comp1_enable = value;
+
+	portidx = wcd->ch_info[mc->reg].port_num;
+
+	wcd937x_connect_port(wcd, portidx, mc->reg, !!value);
+
+	return 1;
+}
+
+static int wcd937x_get_swr_port(struct snd_kcontrol *kcontrol,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	struct soc_mixer_control *mixer = (struct soc_mixer_control *)kcontrol->private_value;
+	struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol);
+	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(comp);
+	struct wcd937x_sdw_priv *wcd;
+	int dai_id = mixer->shift;
+	int ch_idx = mixer->reg;
+	int portidx;
+
+	wcd = wcd937x->sdw_priv[dai_id];
+	portidx = wcd->ch_info[ch_idx].port_num;
+
+	ucontrol->value.integer.value[0] = wcd->port_enable[portidx];
+
+	return 0;
+}
+
+static int wcd937x_set_swr_port(struct snd_kcontrol *kcontrol,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	struct soc_mixer_control *mixer = (struct soc_mixer_control *)kcontrol->private_value;
+	struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol);
+	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(comp);
+	struct wcd937x_sdw_priv *wcd;
+	int dai_id = mixer->shift;
+	int ch_idx = mixer->reg;
+	int portidx;
+	bool enable;
+
+	wcd = wcd937x->sdw_priv[dai_id];
+
+	portidx = wcd->ch_info[ch_idx].port_num;
+
+	enable = !!ucontrol->value.integer.value[0];
+
+	wcd->port_enable[portidx] = enable;
+	wcd937x_connect_port(wcd, portidx, ch_idx, enable);
+
+	return 1;
+}
+
+static const char * const rx_hph_mode_mux_text[] = {
+	"CLS_H_INVALID", "CLS_H_HIFI", "CLS_H_LP", "CLS_AB", "CLS_H_LOHIFI",
+	"CLS_H_ULP", "CLS_AB_HIFI",
+};
+
+static const char * const wcd937x_ear_pa_gain_text[] = {
+	"G_6_DB", "G_4P5_DB", "G_3_DB", "G_1P5_DB", "G_0_DB",
+	"G_M1P5_DB", "G_M3_DB", "G_M4P5_DB",
+	"G_M6_DB", "G_7P5_DB", "G_M9_DB",
+	"G_M10P5_DB", "G_M12_DB", "G_M13P5_DB",
+	"G_M15_DB", "G_M16P5_DB", "G_M18_DB",
+};
+
+static const struct soc_enum rx_hph_mode_mux_enum =
+	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(rx_hph_mode_mux_text), rx_hph_mode_mux_text);
+
+static SOC_ENUM_SINGLE_EXT_DECL(wcd937x_ear_pa_gain_enum, wcd937x_ear_pa_gain_text);
+
 /* MBHC related */
 static void wcd937x_mbhc_clk_setup(struct snd_soc_component *component,
 				   bool enable)
@@ -1150,6 +1316,50 @@  static void wcd937x_mbhc_deinit(struct snd_soc_component *component)
 
 /* END MBHC */
 
+static const struct snd_kcontrol_new wcd937x_snd_controls[] = {
+	SOC_ENUM_EXT("EAR PA GAIN", wcd937x_ear_pa_gain_enum,
+		     wcd937x_ear_pa_gain_get, wcd937x_ear_pa_gain_put),
+	SOC_ENUM_EXT("RX HPH Mode", rx_hph_mode_mux_enum,
+		     wcd937x_rx_hph_mode_get, wcd937x_rx_hph_mode_put),
+
+	SOC_SINGLE_EXT("HPHL_COMP Switch", SND_SOC_NOPM, 0, 1, 0,
+		       wcd937x_get_compander, wcd937x_set_compander),
+	SOC_SINGLE_EXT("HPHR_COMP Switch", SND_SOC_NOPM, 1, 1, 0,
+		       wcd937x_get_compander, wcd937x_set_compander),
+
+	SOC_SINGLE_TLV("HPHL Volume", WCD937X_HPH_L_EN, 0, 20, 1, line_gain),
+	SOC_SINGLE_TLV("HPHR Volume", WCD937X_HPH_R_EN, 0, 20, 1, line_gain),
+	SOC_SINGLE_TLV("ADC1 Volume", WCD937X_ANA_TX_CH1, 0, 20, 0, analog_gain),
+	SOC_SINGLE_TLV("ADC2 Volume", WCD937X_ANA_TX_CH2, 0, 20, 0, analog_gain),
+	SOC_SINGLE_TLV("ADC3 Volume", WCD937X_ANA_TX_CH3, 0, 20, 0, analog_gain),
+
+	SOC_SINGLE_EXT("HPHL Switch", WCD937X_HPH_L, 0, 1, 0,
+		       wcd937x_get_swr_port, wcd937x_set_swr_port),
+	SOC_SINGLE_EXT("HPHR Switch", WCD937X_HPH_R, 0, 1, 0,
+		       wcd937x_get_swr_port, wcd937x_set_swr_port),
+
+	SOC_SINGLE_EXT("ADC1 Switch", WCD937X_ADC1, 1, 1, 0,
+		       wcd937x_get_swr_port, wcd937x_set_swr_port),
+	SOC_SINGLE_EXT("ADC2 Switch", WCD937X_ADC2, 1, 1, 0,
+		       wcd937x_get_swr_port, wcd937x_set_swr_port),
+	SOC_SINGLE_EXT("ADC3 Switch", WCD937X_ADC3, 1, 1, 0,
+		       wcd937x_get_swr_port, wcd937x_set_swr_port),
+	SOC_SINGLE_EXT("DMIC0 Switch", WCD937X_DMIC0, 1, 1, 0,
+		       wcd937x_get_swr_port, wcd937x_set_swr_port),
+	SOC_SINGLE_EXT("DMIC1 Switch", WCD937X_DMIC1, 1, 1, 0,
+		       wcd937x_get_swr_port, wcd937x_set_swr_port),
+	SOC_SINGLE_EXT("MBHC Switch", WCD937X_MBHC, 1, 1, 0,
+		       wcd937x_get_swr_port, wcd937x_set_swr_port),
+	SOC_SINGLE_EXT("DMIC2 Switch", WCD937X_DMIC2, 1, 1, 0,
+		       wcd937x_get_swr_port, wcd937x_set_swr_port),
+	SOC_SINGLE_EXT("DMIC3 Switch", WCD937X_DMIC3, 1, 1, 0,
+		       wcd937x_get_swr_port, wcd937x_set_swr_port),
+	SOC_SINGLE_EXT("DMIC4 Switch", WCD937X_DMIC4, 1, 1, 0,
+		       wcd937x_get_swr_port, wcd937x_set_swr_port),
+	SOC_SINGLE_EXT("DMIC5 Switch", WCD937X_DMIC5, 1, 1, 0,
+		       wcd937x_get_swr_port, wcd937x_set_swr_port),
+};
+
 static int wcd937x_set_micbias_data(struct wcd937x_priv *wcd937x)
 {
 	int vout_ctl[3];
@@ -1316,6 +1526,8 @@  static const struct snd_soc_component_driver soc_codec_dev_wcd937x = {
 	.name = "wcd937x_codec",
 	.probe = wcd937x_soc_codec_probe,
 	.remove = wcd937x_soc_codec_remove,
+	.controls = wcd937x_snd_controls,
+	.num_controls = ARRAY_SIZE(wcd937x_snd_controls),
 	.set_jack = wcd937x_codec_set_jack,
 	.endianness = 1,
 };