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

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

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

-- 
With Best Regards,
Andy Shevchenko