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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210312155547.GJ5348@sirena.org.uk>
Date:   Fri, 12 Mar 2021 15:55:47 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     perex@...ex.cz, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org, lgirdwood@...il.com
Subject: 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 :(

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)