Re: [PATCH 4/7] ASoC: codecs: wcd938x: add basic controls

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks Mark for review,

On 12/03/2021 15:55, Mark Brown wrote:
On Thu, Mar 11, 2021 at 05:34:13PM +0000, Srinivas Kandagatla wrote:

+	if (wcd938x->variant == WCD9380) {

switch statements please.

+		if (mode_val == CLS_H_HIFI || mode_val == CLS_AB_HIFI) {
+			dev_info(component->dev,
+				"%s:Invalid HPH Mode, default to CLS_H_ULP\n",
+				__func__);
+			mode_val = CLS_H_ULP;
+		}

If the value can't be set an error should be returned rather than the
input ignored.

I agree with all the comments related to switch statements and ignoring return values, will address them in next version.


+static int wcd938x_ear_pa_gain_get(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+
+	ucontrol->value.integer.value[0] = snd_soc_component_read_field(component,
+						WCD938X_ANA_EAR_COMPANDER_CTL,
+						WCD938X_EAR_GAIN_MASK);
+
+	return 0;
+}

This just looks like a normal operation?

Yes, this can probably go into a TLV control, will fix this in next version.


--srini
+static int wcd938x_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 wcd938x_sdw_priv *wcd = snd_soc_component_get_drvdata(component);
+	struct wcd938x_priv *wcd938x = wcd->wcd938x;
+
+	if (!wcd938x->comp1_enable) {
+		snd_soc_component_write_field(component,
+				WCD938X_ANA_EAR_COMPANDER_CTL,
+				WCD938X_EAR_GAIN_MASK,
+				ucontrol->value.integer.value[0]);
+	}

Again, values should not be ignored on error.

+	if (wcd938x->variant == WCD9380) {
+		ret = snd_soc_add_component_controls(component, wcd9380_snd_controls,
+					ARRAY_SIZE(wcd9380_snd_controls));
+		if (ret < 0) {
+			dev_err(component->dev,
+				"%s: Failed to add snd ctrls for variant: %d\n",
+				__func__, wcd938x->variant);
+			goto err;
+		}
+	}
+	if (wcd938x->variant == WCD9385) {

Again, switch statements - I'll never understand why people seem so
intent on writing if trees :(




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux