RE: [EXTERNAL] Re: [PATCH v1] ASoc: tas2781: Add Calibration Kcontrols and tas2563 digtial gain for Chromebook

["Ding, Shenghao" <shenghao-ding@ti.com>]

Hi Andy
Thanks for your comments. Following are my answers.

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Wednesday, May 22, 2024 8:02 PM
> To: Ding, Shenghao <shenghao-ding@ti.com>
> Cc: broonie@kernel.org; lgirdwood@gmail.com; perex@perex.cz; pierre-
> louis.bossart@linux.intel.com; 13916275206@139.com;
> judyhsiao@google.com; alsa-devel@alsa-project.org; Salazar, Ivan <i-
> salazar@ti.com>; linux-kernel@vger.kernel.org; Chadha, Jasjot Singh <j-
> chadha@ti.com>; liam.r.girdwood@intel.com; bard.liao@intel.com; yung-
> chuan.liao@linux.intel.com; Rao, Dipa <dipa@ti.com>; Lu, Kevin <kevin-
> lu@ti.com>; yuhsuan@google.com; tiwai@suse.de; Xu, Baojun
> <baojun.xu@ti.com>; soyer@irl.hu; Baojun.Xu@fpt.com; Navada Kanyana,
> Mukund <navada@ti.com>; cujomalainey@google.com; Kutty, Aanya
> <aanya@ti.com>; Mahmud, Nayeem <nayeem.mahmud@ti.com>
> Subject: [EXTERNAL] Re: [PATCH v1] ASoc: tas2781: Add Calibration Kcontrols
> and tas2563 digtial gain for Chromebook
> 
> On Wed, May 22, 2024 at 07: 29: 41PM +0800, Shenghao Ding wrote: >
> Calibrated data will be set to default after loading DSP config params, >
> which will cause speaker protection work abnormally. Reload calibrated >
> data after loading ZjQcmQRYFpfptBannerStart This message was sent from
> outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source of this
> email and know the content is safe. If you wish to report this message to IT
> Security, please forward the message as an attachment to
> phishing@list.ti.com
> 
........................
> > +	while (r > 1 + l) {
> > +		mid = (l + r) / 2;
> > +		ar_mid = get_unaligned_be32(tas2563_dvc_table[mid]);
> > +		if (target < ar_mid)
> > +			r = mid;
> > +		else
> > +			l = mid;
> > +	}
> 
> Hmm... I'm wondering if bsearch() can be utilised here.
Bsearch is to find out the value, in this function is to find out the member same as or closer to the input value.

> 
> ...
> 
> > +	ucontrol->value.integer.value[0] =
> > +		abs(target - ar_l) <= abs(target - ar_r) ? l : r;
> 
> I don't understand why do you need 'target' to be in this check.
> 
> ...
> 
> > +	uinfo->value.integer.max = (int)tas_priv->ndev - 1;
> 
> Why casting?
> 
> ...
> 
> > +	scnprintf(active_dev_name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN,
> 
> Why 'c' variant in use? You are ignoring the returned value. Isn't strscpy() you
> want or memtostr() (in both cases 2 parameters variant)?
> 
> > +		"Activate Tasdevice Id");
> 
> Same Q to all scnprintf() calls.
> 
> ...
> 
> > +	cali_data->data = devm_kzalloc(tas_priv->dev, tas_priv->ndev *
> > +		(cali_data->reg_array_sz * 4 + 1), GFP_KERNEL);
> 
> No way. First of all, we have kcalloc(), second, there is an overflow.h that has
> necessary macros to calculate sizes for memory allocations.
Memory allocated with devm_kzalloc is automatically freed on driver detach while kcalloc can’t
> 
> > +	if (!cali_data->data)
> > +		return -ENOMEM;
> 
> ...

BR
Shenghao Ding