RE: [EXTERNAL] Re: [PATCH v3] ASoC: tas2781: Support dsp firmware Alpha and Beta seaies

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

Hi Andy
Thanks for your comments

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Tuesday, February 25, 2025 10:41 PM
> To: Ding, Shenghao <shenghao-ding@ti.com>
> Cc: broonie@kernel.org; tiwai@suse.de; 13916275206@139.com;
> 13564923607@139.com; alsa-devel@alsa-project.org; Xu, Baojun
> <baojun.xu@ti.com>
> Subject: [EXTERNAL] Re: [PATCH v3] ASoC: tas2781: Support dsp firmware
> Alpha and Beta seaies
> 
> On Tue, Feb 25, 2025 at 10:03:16PM +0800, Shenghao Ding wrote:
> > For calibration, basic version does not contain any calibration
> > addresses, it depends on calibration tool to convery the addresses to the
> driver.
> > Since Alpha and Beta firmware, all the calibration addresses are saved
> > into the firmware.
> 
> ...
> 
> > +
> > +	for (i = 0; i < 20; i++) {
> > +		const unsigned char *dat = &p->data[24 * i];
> 
> Okay, you have 24-byte records.
> 
> > +		if (dat[23] != 1)
> > +			break;
> 
> > +		if (strstr(dat, "umg_SsmKEGCye") != NULL) {
> 
> These calls basically suggest that the data may be not at the same offset.
> But at the same time they don't have boundary checks and there is a chance
> that the tail of one of these string comparisons may trigger if the value appears
> to be the same, like "Cye".
> 
> Instead, better to introduce a special data type or even data types, where you
> put string with limited length and respective data, like u24/i8 in your
> terminology. With that this code becomes much more clearer.

Calibration parameters locations and data schema in dsp firmware
The number of items are flexible, but not more than 20. The dsp tool
will reserve 20*24-byte space for fct params. In some cases, the
number of fct param is less than 20, the data will be saved from the
beginning, the rest part will be stuffed with zero.
	fct_param_num (not more than 20)
	for (i = 0; i < fct_param_num; i++) {
		Alias of fct param (20 bytes)
		Book (1 byte)
	Page (1 byte)
	Offset (1 byte)
	CoeffLength (1 byte) = 0x1
	}
	if (20 - fct_param_num)
		24*(20 - fct_param_num) pieces of '0' as stuffing
As follow
umg_SsmKEGCye	 = Book, Page, Offset, CoeffLength
iks_E0 		 = Book, Page, Offset, CoeffLength
yep_LsqM0		 = Book, Page, Offset, CoeffLength
oyz_U0_ujx		 = Book, Page, Offset, CoeffLength
iks_GC_GMgq		 = Book, Page, Offset, CoeffLength
gou_Yao		 = Book, Page, Offset, CoeffLength
kgd_Wsc_Qsbp		 = Book, Page, Offset, CoeffLength
yec_CqseSsqs		 = Book, Page, Offset, CoeffLength
iks_SogkGgog2	 = Book, Page, Offset, CoeffLength
yec_Sae_Y		 = Book, Page, Offset, CoeffLength
Re_Int		 = Book, Page, Offset, CoeffLength
SigFlag		 = Book, Page, Offset, CoeffLength
a1_Int		 = Book, Page, Offset, CoeffLength
a2_Int		 = Book, Page, Offset, CoeffLength

Do you mean to use  strncmp instead of strstr?
Due to the different length of the alias of fct params, 
I have to pass the max len, 20 bytes, as the str len.
> 
> > +			hex_parse_u24(&r->pow_reg, &dat[20]);
> > +			continue;
> > +		}
> > +		if (strstr(dat, "iks_E0") != NULL) {
> > +			hex_parse_u24(&r->r0_reg, &dat[20]);
> > +			continue;
> > +		}
> > +		if (strstr(dat, "yep_LsqM0") != NULL) {
> > +			hex_parse_u24(&r->invr0_reg, &dat[20]);
> > +			continue;
> > +		}
> > +		if (strstr(dat, "oyz_U0_ujx") != NULL) {
> > +			hex_parse_u24(&r->r0_low_reg, &dat[20]);
> > +			continue;
> > +		}
> > +		if (strstr(dat, "iks_GC_GMgq") != NULL) {
> > +			hex_parse_u24(&r->tlimit_reg, &dat[20]);
> > +			continue;
> > +		}
> > +		if (strstr(dat, "gou_Yao") != NULL) {
> > +			hex_parse_u8(p->thr, &dat[20], 3);
> > +			continue;
> > +		}
> > +		if (strstr(dat, "kgd_Wsc_Qsbp") != NULL) {
> > +			hex_parse_u8(p->plt_flg, &dat[20], 3);
> > +			continue;
> > +		}
> > +		if (strstr(dat, "yec_CqseSsqs") != NULL) {
> > +			hex_parse_u8(p->sin_gn, &dat[20], 3);
> > +			continue;
> > +		}
> > +		if (strstr(dat, "iks_SogkGgog2") != NULL) {
> > +			hex_parse_u8(p->sin_gn2, &dat[20], 3);
> > +			continue;
> > +		}
> > +		if (strstr(dat, "yec_Sae_Y") != NULL) {
> > +			hex_parse_u8(p->thr2, &dat[20], 3);
> > +			continue;
> > +		}
> > +		if (strstr(dat, "Re_Int") != NULL) {
> > +			hex_parse_u8(p->r0_reg, &dat[20], 3);
> > +			continue;
> > +		}
> > +		/* Check whether the spk connection is open */
> > +		if (strstr(dat, "SigFlag") != NULL) {
> > +			hex_parse_u8(p->tf_reg, &dat[20], 3);
> > +			continue;
> > +		}
> > +		if (strstr(dat, "a1_Int") != NULL) {
> > +			hex_parse_u8(p->a1_reg, &dat[20], 3);
> > +			continue;
> > +		}
> > +		if (strstr(dat, "a2_Int") != NULL) {
> > +			hex_parse_u8(p->a2_reg, &dat[20], 3);
> > +			continue;
> > +		}
> > +	}
> > +}
> 
> ...
> 
> --
> With Best Regards,
> Andy Shevchenko
>