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.

...

+static void hex_parse_u8(unsigned char *item, const unsigned char *val,

• int len)

+{

• for (int i = 0; i < len; i++)

• item[i] = val[i];
  

memcpy() ?

+}

...

+static void hex_parse_u24(unsigned int *item, const unsigned char *val) +{

• *item = TASDEVICE_REG(val[0], val[1], val[2]);

+}

Also useless function, just use this macro directly.

...

+static void fct_param_address_parser(struct cali_reg *r,

• struct tasdevice_fw *tas_fmw)

+{

• struct fct_param_address *p = &(tas_fmw->fct_par_addr);

Unneeded parentheses.

• int i;

unsigned int ?

• 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.

• 	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;
  

• }
  

• }

+}

...

+static int fw_parse_fct_param_address(struct tasdevice_priv *tas_priv,

• struct tasdevice_fw *tas_fmw, const struct firmware *fmw, int offset)

+{

• struct fct_param_address *p = &(tas_fmw->fct_par_addr);
• struct calidata *cali_data = &tas_priv->cali_data;
• struct cali_reg *r = &cali_data->cali_reg_array;
• const unsigned char *data = fmw->data;
• if (offset + 520 > fmw->size) {

• dev_err(tas_priv->dev, "%s: File Size error\n", __func__);
  

• offset = -1;
  

• goto out;
  

Just return directly and use proper error codes.

• }
• /* skip reserved part */
• offset += 40;
• p->size = 480;
• p->data = kmemdup(&data[offset], 480, GFP_KERNEL);
• if (!p->data) {

• offset = -1;
  

• goto out;
  

Ditto.

• }
• offset += 480;
• fct_param_address_parser(r, tas_fmw);

+out:

Useless label.

• return offset;

+}

...

offset = tas_priv->fw_parse_configuration_data(tas_priv, tas_fmw, fmw, offset);

• if (offset < 0)

• if (offset < 0) { ret = offset;

This is the error code from offset, right? what you use is -1 in both cases which maps currently to EPERM.

• goto out;
  

• }

...

• if (tas_priv->fw_parse_fct_param_address) {

• offset = tas_priv->fw_parse_fct_param_address(tas_priv,
  

• 	tas_fmw, fmw, offset);
  

• if (offset < 0)
  

• 	ret = offset;
  

Ditto.

• }

...

out:

Yet another useless label.

return ret;

...

• if (tas_priv->dspbin_typ == TASDEV_ALPHA)
  

• 	tasdevice_dev_bulk_write(tas_priv, i, t->reg,
  

• 		t->val, 4);
  

Follow the logical splits

tasdevice_dev_bulk_write(tas_priv, i, t->reg, t->val, 4);

or do it on a single line (it's only 81 characters).

tasdevice_dev_bulk_write(tas_priv, i, t->reg, t->val, 4);

...

• if (tas_priv->dspbin_typ)
  

• 	reg = TASDEVICE_REG(p->tf_reg[0], p->tf_reg[1],
  

• 		p->tf_reg[2]);
  

One line will be not so big (86 characters).

...

+static void cali_reg_update(struct bulk_reg_val *p,

• struct fct_param_address *t)

+{

• const int sum = ARRAY_SIZE(tas2781_cali_start_reg);
• int reg = 0;
• int j;
• for (j = 0; j < sum; j++) {

• switch (tas2781_cali_start_reg[j].reg) {
  

• case 0:
  

• reg = TASDEVICE_REG(t->thr[0], t->thr[1], t->thr[2]);
  

Indentation issues with all these reg = ...

• 	break;
  

• case TAS2781_PRM_PLT_FLAG_REG:
  

• reg = TASDEVICE_REG(t->plt_flg[0], t->plt_flg[1],
  

• 	t->plt_flg[2]);
  

You have room on the previous line.

• 	break;
  

• case TAS2781_PRM_SINEGAIN_REG:
  

• reg = TASDEVICE_REG(t->sin_gn[0], t->sin_gn[1], t->sin_gn[2]);
  

• 	break;
  

• case TAS2781_PRM_SINEGAIN2_REG:
  

• reg = TASDEVICE_REG(t->sin_gn[0], t->sin_gn[1], t->sin_gn[2]);
  

• 	break;
  

• default:
  

• reg = 0;
  

• 	break;
  

• }
  

• if (reg)
  

• 	p[j].reg = reg;
  

• }

+}