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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Feb 27, 2025 at 11:58:24AM +0000, Ding, Shenghao wrote:
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Tuesday, February 25, 2025 10:41 PM
> > On Tue, Feb 25, 2025 at 10:03:16PM +0800, Shenghao Ding wrote:

...

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

Yes,

> Due to the different length of the alias of fct params, 
> I have to pass the max len, 20 bytes, as the str len.

Yes, for each alias you need just to define a data type:

struct fct_param_alias_u24 {
	const char name[20];
	u8 book;
	u8 page;
	u8 offset;
	u8 coeff_len;
};

struct fct_param_alias_u8 {
	...
};

Then in the loop you do the following (just an example, can be done
differently):

	union {
		const char name[20];

		struct fct_param_alias_u8 *u8;
		struct fct_param_alias_u24 *u24;
		...
		const void *data;
	} a;

	a.data = data;

	if (!strncmp(a.name, ..., sizeof(a.name)))
		foo = ...(a.u24->book, a.u24->page, a.u24->offset);
	else if (...)
		...

Also parameters can be split to a table which will have the need to use method
as a callback

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