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

[Mohammad Rafi Shaik]

On 5/16/2024 5:28 PM, Mark Brown wrote:
> 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;
> > +

Thanks for the review.

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

okay,

I will remove logs.

> > +	}
> > +
> > +	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.

I'll add missing validations for all controls.

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

i guess this change is fine.

I took the reference from wcd938x and wcd939x.

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/codecs/wcd938x.c#n1693

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/codecs/wcd939x.c#n1635

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

I'll check and add TLV instead enum.

Thanks & Regards,
Rafi