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
