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 :(
Attachment:
signature.asc
Description: PGP signature