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 :(
