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