Received-SPF: pass (google.com: domain of linux-kernel+bounces-186126-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <c2eb1eb0-2555-6293-999e-ed394e84c946@quicinc.com>
Date: Wed, 22 May 2024 16:49:37 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v4 4/7] ASoC: codecs: wcd937x: add basic controls
Content-Language: en-US
To: Mark Brown <broonie@kernel.org>
CC: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Banajit Goswami
	<bgoswami@quicinc.com>,
        Liam Girdwood <lgirdwood@gmail.com>, Rob Herring
	<robh@kernel.org>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>,
        Conor Dooley
	<conor+dt@kernel.org>, Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai
	<tiwai@suse.com>, <alsa-devel@alsa-project.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-sound@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <quic_rohkumar@quicinc.com>, <quic_pkumpatl@quicinc.com>
References: <20240516044801.1061838-1-quic_mohs@quicinc.com>
 <20240516044801.1061838-5-quic_mohs@quicinc.com>
 <f766e8fc-64e7-4579-ac5a-4afcdae067cc@sirena.org.uk>
From: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
In-Reply-To: <f766e8fc-64e7-4579-ac5a-4afcdae067cc@sirena.org.uk>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

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