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

[Mark Brown <broonie@xxxxxxxxxx>]

On Thu, May 16, 2024 at 10:17:58AM +0530, Mohammad Rafi Shaik wrote:

> +static int wcd937x_rx_hph_mode_put(struct snd_kcontrol *kcontrol,
> +				   struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component =
> +				snd_soc_kcontrol_component(kcontrol);
> +	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(component);
> +	u32 mode_val;
> +
> +	mode_val = ucontrol->value.enumerated.item[0];
> +	if (!mode_val) {
> +		dev_warn(component->dev, "Invalid HPH Mode, default to class_AB\n");
> +		mode_val = CLS_AB;

This should be silent (or return an error) otherwise people can DoS the
logs by just spamming in invalid values.

> +	}
> +
> +	wcd937x->hph_mode = mode_val;

I would expect there's more validation needed here, this will blindly
assign any non-zero mode.  Please run the mixer-test selftests on a card
with this device in it and show the results on future submissions, this
will detect this and other issues for you.

Several of the other controls look like they're also missing validation.

> +static int wcd937x_set_swr_port(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct soc_mixer_control *mixer = (struct soc_mixer_control *)kcontrol->private_value;
> +	struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol);
> +	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(comp);
> +	struct wcd937x_sdw_priv *wcd;
> +	int dai_id = mixer->shift;
> +	int ch_idx = mixer->reg;
> +	int portidx;
> +	bool enable;
> +
> +	wcd = wcd937x->sdw_priv[dai_id];
> +
> +	portidx = wcd->ch_info[ch_idx].port_num;
> +
> +	enable = !!ucontrol->value.integer.value[0];
> +
> +	wcd->port_enable[portidx] = enable;
> +	wcd937x_connect_port(wcd, portidx, ch_idx, enable);
> +
> +	return 1;
> +}

This unconditionally reports that the value changed so will generate
spurious events.
> +
> +static const char * const rx_hph_mode_mux_text[] = {
> +	"CLS_H_INVALID", "CLS_H_HIFI", "CLS_H_LP", "CLS_AB", "CLS_H_LOHIFI",
> +	"CLS_H_ULP", "CLS_AB_HIFI",
> +};

It would be more idiomatic to write these in a more human readable form.

> +static const char * const wcd937x_ear_pa_gain_text[] = {
> +	"G_6_DB", "G_4P5_DB", "G_3_DB", "G_1P5_DB", "G_0_DB",
> +	"G_M1P5_DB", "G_M3_DB", "G_M4P5_DB",
> +	"G_M6_DB", "G_7P5_DB", "G_M9_DB",
> +	"G_M10P5_DB", "G_M12_DB", "G_M13P5_DB",
> +	"G_M15_DB", "G_M16P5_DB", "G_M18_DB",
> +};

Why is this an enum and not TLV information?

Attachment: signature.asc
Description: PGP signature