Re: [PATCH v1] ASoc: tas2781: Add Calibration Kcontrols and tas2563 digtial gain for Chromebook

[Andy Shevchenko]

On Wed, May 22, 2024 at 07:29:41PM +0800, Shenghao Ding wrote:
> Calibrated data will be set to default after loading DSP config params,
> which will cause speaker protection work abnormally. Reload calibrated
> data after loading DSP config params.

..

> -// tas2781-lib.c -- TAS2781 Common functions for HDA and ASoC Audio drivers
> +// tas2781-comlib.c -- TAS2781 Common functions for HDA and ASoC Audio drivers

Please, drop the filename from the file completely, this change is exactly the
answer to "why having filename in the file is a bad idea in a long-term".

..

> +int tasdevice_chn_switch(struct tasdevice_priv *tas_priv,
> +	unsigned short chn)

Pretty much can be on a single line.

> +{
> +	struct i2c_client *client = (struct i2c_client *)tas_priv->client;
> +	struct tasdevice *tasdev = &tas_priv->tasdevice[chn];
> +	struct regmap *map = tas_priv->regmap;
> +	int ret;

> +	if (client->addr != tasdev->dev_addr) {

With inverted check this entire function becomes neater.

> +		client->addr = tasdev->dev_addr;
> +		/* All devices share the same regmap, clear the page
> +		 * inside regmap once switching to another device.
> +		 * Register 0 at any pages and any books inside tas2781
> +		 * is the same one for page-switching.
> +		 */
> +		ret = regmap_write(map, TASDEVICE_PAGE_SELECT, 0);
> +		if (ret < 0) {
> +			dev_err(tas_priv->dev, "%s, E=%d\n",
> +				__func__, ret);
> +			return ret;
> +		}
> +		return 1;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tasdevice_chn_switch);

Is it namespaced? If not would be good to make it so.
Also I see that other file uses namespaced exports.

..

> +	if (!priv->is_user_space_calidata &&
> +		cal_fmw) {

With all possible restrictions this can be on a single line besides the fact
that the second one currently has a broken indentation.

..

> +				&(data[k + 4 * j]), 4);

How parentheses are helpful here?

> +	}

..

> +#include <asm/unaligned.h>

linux/* followed by asm/* as the latter is not so generic as the former.

>  #include <linux/crc8.h>
>  #include <linux/firmware.h>
>  #include <linux/gpio/consumer.h>

..

> +	{
> +		.reg = TAS2781_PRM_TEST_57_REG,
> +		.val = { 0x14 },
> +		.val_len = 1,
> +		.is_locked = true

Here and everywhere else in cases like this (when it's not a termination line)
leave the trailing comma. It will reduce the churn in case this needs to be
expanded in the future.

> +	},

..

> +	int rc;
> +
> +	mutex_lock(&tas_priv->codec_lock);
> +	rc = tasdevice_digital_getvol(tas_priv, ucontrol, mc);
> +	mutex_unlock(&tas_priv->codec_lock);
>  
> -	return tasdevice_digital_getvol(tas_priv, ucontrol, mc);
> +	return rc;

Why not converting to use cleanup.h and this will become a oneliner update.
Same Q to all these mutex additions.

..

> +{
> +	struct i2c_client *clt = (struct i2c_client *)tas_priv->client;

Hmm... Why explicit casting? Is the client not void * or the same type?

> +	struct tasdevice *tasdev = tas_priv->tasdevice;

> +	int rc = -1;

Use proper error codes.

> +	int i;
> +
> +	if (data_len != 4)
> +		return rc;
> +
> +	for (i = 0; i < tas_priv->ndev; i++) {
> +		if (clt->addr == tasdev[i].dev_addr) {
> +			/* First byte is the device index. */
> +			dst[0] = i;
> +			tasdevice_dev_bulk_read(tas_priv, i, reg, &dst[1],
> +				4);

On one line this will be better to read.

> +			rc = 0;
> +			break;

Why not simply

			return 0;

?

> +		}
> +	}
> +
> +	return rc;
> +}

..

> +	if (tas_priv->chip_id != TAS2781 &&
> +		bytes_ext->max != 8 * tas_priv->ndev) {

Here and seems in many places you have broken indentation.

> +		rc = -1;

error code?

> +		goto out;
> +	}

> +		for (j = 0; j < sum; j++) {

With a temporary variable for p[j]...

> +			if (p[j].val_len == 1) {
> +				if (p[j].is_locked)
> +					tasdevice_dev_write(tas_priv, i,
> +						TAS2781_TEST_UNLOCK_REG,
> +						TAS2781_TEST_PAGE_UNLOCK);
> +				tasdevice_dev_read(tas_priv, i, p[j].reg,
> +					(int *)&p[j].val[0]);
> +			} else
> +				tasdevice_dev_bulk_read(tas_priv, i, p[j].reg,
> +					p[j].val, 4);

..all the above can be made more readable.

> +		}
> +
> +		for (j = 0; j < sum - 2; j++) {

Ditto for tas2781_cali_start_reg[j]

> +		}

..

> +	while (r > 1 + l) {
> +		mid = (l + r) / 2;
> +		ar_mid = get_unaligned_be32(tas2563_dvc_table[mid]);
> +		if (target < ar_mid)
> +			r = mid;
> +		else
> +			l = mid;
> +	}

Hmm... I'm wondering if bsearch() can be utilised here.

..

> +	ucontrol->value.integer.value[0] =
> +		abs(target - ar_l) <= abs(target - ar_r) ? l : r;

I don't understand why do you need 'target' to be in this check.

..

> +	uinfo->value.integer.max = (int)tas_priv->ndev - 1;

Why casting?

..

> +	scnprintf(active_dev_name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN,

Why 'c' variant in use? You are ignoring the returned value. Isn't strscpy()
you want or memtostr() (in both cases 2 parameters variant)?

> +		"Activate Tasdevice Id");

Same Q to all scnprintf() calls.

..

> +	cali_data->data = devm_kzalloc(tas_priv->dev, tas_priv->ndev *
> +		(cali_data->reg_array_sz * 4 + 1), GFP_KERNEL);

No way. First of all, we have kcalloc(), second, there is an overflow.h that
has necessary macros to calculate sizes for memory allocations.

> +	if (!cali_data->data)
> +		return -ENOMEM;

..

> -	int ret = 0;
>  
> -	if (tas_priv->fw_state != TASDEVICE_DSP_FW_ALL_OK) {
> -		dev_err(tas_priv->dev, "DSP bin file not loaded\n");
> -		ret = -EINVAL;
> +	if (!(tas_priv->fw_state == TASDEVICE_DSP_FW_ALL_OK ||
> +		tas_priv->fw_state == TASDEVICE_RCA_FW_OK)) {
> +		dev_err(tas_priv->dev, "Bin file not loaded\n");
> +		return -EINVAL;
>  	}
>  
> -	return ret;
> +	return 0;

This patch is a mess. Try to split out the different logical changes into
different patches.

-- 
With Best Regards,
Andy Shevchenko