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

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.

+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?

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