linux-kernel - Re: [PATCH v1] ASoc: tas2781: Add Calibration Kcontrols for Chromebook

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] [day] [month] [year] [list]

Message-ID: <97275835-fff5-49ac-bd15-c6b6c6e89fe0@sirena.org.uk>
Date: Wed, 14 Aug 2024 17:49:23 +0100
From: Mark Brown <broonie@...nel.org>
To: Shenghao Ding <shenghao-ding@...com>
Cc: andriy.shevchenko@...ux.intel.com, lgirdwood@...il.com, perex@...ex.cz,
	pierre-louis.bossart@...ux.intel.com, 13916275206@....com,
	zhourui@...qin.com, alsa-devel@...a-project.org, i-salazar@...com,
	linux-kernel@...r.kernel.org, j-chadha@...com,
	liam.r.girdwood@...el.com, jaden-yue@...com,
	yung-chuan.liao@...ux.intel.com, dipa@...com, yuhsuan@...gle.com,
	henry.lo@...com, tiwai@...e.de, baojun.xu@...com, soyer@....hu,
	Baojun.Xu@....com, judyhsiao@...gle.com, navada@...com,
	cujomalainey@...gle.com, aanya@...com, nayeem.mahmud@...com,
	savyasanchi.shukla@...radyne.com, flaviopr@...rosoft.com,
	jesse-ji@...com, darren.ye@...iatek.com, antheas.dk@...il.com,
	Jerry2.Huang@...uturecenter.com
Subject: Re: [PATCH v1] ASoc: tas2781: Add Calibration Kcontrols for
 Chromebook

On Fri, Jul 26, 2024 at 04:47:55PM +0800, Shenghao Ding wrote:
> Add calibration related kcontrol for speaker impedance calibration and
> speaker leakage check for Chromebook

This is pretty hard to understand, it feels like there's a bunch of
cleanup work in here along with the actual control addition and there's
nothing really saying anything concrete about the actual controls to
check that the controls do the right thing.  It's hard to do anything
but the most superficial review here since I don't really understand
what the changes are trying to accomplish.

> -/* pow(10, db/20) * pow(2,30) */
> -static const unsigned char tas2563_dvc_table[][4] = {
> -	{ 0X00, 0X00, 0X00, 0X00 }, /* -121.5db */

For example moving this to the header could be done separately (though
perhaps it should just be exported rather than placed in the header)?

> @@ -64,8 +64,8 @@ static int tasdevice_change_chn_book(struct tasdevice_priv *tas_priv,
>  			 */
>  			ret = regmap_write(map, TASDEVICE_PAGE_SELECT, 0);
>  			if (ret < 0) {
> -				dev_err(tas_priv->dev, "%s, E=%d\n",
> -					__func__, ret);
> +				dev_err(tas_priv->dev, "%s, E=%d channel:%d\n",
> +					__func__, ret, chn);
>  				goto out;
>  			}
>  		}

This is another example of a random cleanup.

>  static void tasdev_load_calibrated_data(struct tasdevice_priv *priv, int i)
>  {
> +	struct tasdevice_fw *cal_fmw = priv->tasdevice[i].cali_data_fmw;
> +	struct calidata *cali_data = &priv->cali_data;
> +	unsigned char *data = &cali_data->data[1];
>  	struct tasdevice_calibration *cal;
> -	struct tasdevice_fw *cal_fmw;
> +	int k = i * (cali_data->cali_dat_sz + 1);
> +	int j, rc;
>  
> -	cal_fmw = priv->tasdevice[i].cali_data_fmw;
> +	/* Load the calibrated data from cal bin file */
> +	if (!priv->is_user_space_calidata && cal_fmw) {
> +		cal = cal_fmw->calibrations;
>  
> -	/* No calibrated data for current devices, playback will go ahead. */
> -	if (!cal_fmw)
> +		if (cal)
> +			load_calib_data(priv, &cal->dev_data);
>  		return;

It feels like there's an abstraction problem with the different ways to
get calibration data.  Perhaps each way of loading calibration data
should parse the data into a standard format on load and then the rest
of the code doesn't need to worry about where it came from?

> @@ -67,8 +215,13 @@ static int tas2781_digital_getvol(struct snd_kcontrol *kcontrol,
>  	struct tasdevice_priv *tas_priv = snd_soc_component_get_drvdata(codec);
>  	struct soc_mixer_control *mc =
>  		(struct soc_mixer_control *)kcontrol->private_value;
> +	int rc;
> +
> +	mutex_lock(&tas_priv->codec_lock);
> +	rc = tasdevice_digital_getvol(tas_priv, ucontrol, mc);
> +	mutex_unlock(&tas_priv->codec_lock);
>  
> -	return tasdevice_digital_getvol(tas_priv, ucontrol, mc);
> +	return rc;
>  }
>  

Why all these locking changes?  Could they be split out into a
praparatory change?

> +static int tasdevice_get_chip_id(struct snd_kcontrol *kcontrol,
> +			struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *codec = snd_soc_kcontrol_component(kcontrol);
> +	struct tasdevice_priv *tas_priv = snd_soc_component_get_drvdata(codec);
> +
> +	ucontrol->value.integer.value[0] = tas_priv->chip_id;
> +
> +	return 0;
> +}

Should these chip ID controls be done separately?  They don't seem super
related.

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