Date: Tue, 5 Mar 2024 17:04:57 +0100
From: Amadeusz Sławiński
 <amadeuszx.slawinski@...ux.intel.com>
To: Shenghao Ding <shenghao-ding@...com>, broonie@...nel.org
Cc: andriy.shevchenko@...ux.intel.com, lgirdwood@...il.com, perex@...ex.cz,
 pierre-louis.bossart@...ux.intel.com, 13916275206@....com,
 alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
 liam.r.girdwood@...el.com, bard.liao@...el.com, mengdong.lin@...el.com,
 yung-chuan.liao@...ux.intel.com, kevin-lu@...com, tiwai@...e.de,
 soyer@....hu, Baojun.Xu@....com, navada@...com
Subject: Re: [PATCH v11] ASoc: tas2783: Add tas2783 codec driver

On 3/5/2024 2:26 PM, Shenghao Ding wrote:
> The tas2783 is a smart audio amplifier with integrated MIPI SoundWire
> interface (Version 1.2.1 compliant), I2C, and I2S/TDM interfaces designed
> for portable applications. An on-chip DSP supports Texas Instruments
> SmartAmp speaker protection algorithm. The integrated speaker voltage and
> current sense provides for real-time monitoring of loudspeakers.
> 
> The ASoC component provides the majority of the functionality of the
> device, all the audio functions.
> 
> Signed-off-by: Shenghao Ding <shenghao-ding@...com>
> 
> ---

..

> +
> +static void tas2783_apply_calibv2(struct tasdevice_priv *tas_dev,
> +	unsigned int *cali_data)
> +{
> +	const unsigned int arr_size = ARRAY_SIZE(tas2783_cali_reg);
> +	struct regmap *map = tas_dev->regmap;
> +	unsigned int dev_sum = cali_data[1], i, j, k;
> +	u8 *cali_start;
> +	u16 dev_info;
> +	int ret;
> +
> +	if (!tas_dev->sdw_peripheral) {
> +		dev_err(tas_dev->dev, "%s: peripheral doesn't exist.\n",
> +			__func__);
> +		return;
> +	}
> +
> +	dev_info = tas_dev->sdw_peripheral->bus->link_id |
> +		tas_dev->sdw_peripheral->id.unique_id << 16;
> +
> +	/*
> +	 * The area saving tas2783 calibrated data is specified by its
> +	 * unique_id offset. cali_start is the first address of current
> +	 * tas2783's calibrated data.
> +	 */
> +	cali_start = (u8 *)&cali_data[3];
> +	for (i = 0; i < dev_sum; i++) {
> +		k = i * (arr_size + 1) + 3;
> +		if (dev_info != cali_data[k]) {
> +			for (j = 0; j < arr_size; j++) {
> +				k = 4 * (k + 1 + j);
> +				ret = regmap_bulk_write(map,
> +					tas2783_cali_reg[j],
> +					&cali_start[k], 4);
> +				if (ret) {
> +					dev_err(tas_dev->dev,
> +						"Cali failed %x:%d\n",
> +						tas2783_cali_reg[j], ret);
> +					break;
> +				}
> +			}
> +			break;
> +		}
> +	}

This seems a bit hard to read, any chance to do some reordering to make 
it more readable?

> +}
> +

..

> +
> +static int tasdevice_mute(struct snd_soc_dai *dai, int mute,
> +	int direction)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct tasdevice_priv *tas_dev =
> +		snd_soc_component_get_drvdata(component);
> +	struct regmap *map = tas_dev->regmap;
> +	int ret;
> +
> +	dev_dbg(tas_dev->dev, "%s: %d.\n", __func__, mute);
> +
> +	if (mute) {
> +		if (direction == SNDRV_PCM_STREAM_CAPTURE) {
> +			ret = regmap_update_bits(map, TAS2873_REG_PWR_CTRL,
> +				TAS2783_REG_AEF_MASK,
> +				TAS2783_REG_AEF_INACTIVE);
> +			if (ret)
> +				dev_err(tas_dev->dev,
> +					"%s: Disable AEF failed.\n", __func__);
> +		} else {
> +			/* FU23 mute (0x40400108) */
> +			ret = regmap_write(map,
> +				SDW_SDCA_CTL(TAS2783_FUNC_TYPE_SMART_AMP,
> +				TAS2783_SDCA_ENT_FU23,
> +				TAS2783_SDCA_CTL_FU_MUTE, 0), 1);
> +			if (ret) {
> +				dev_err(tas_dev->dev,
> +					"%s: FU23 mute failed.\n", __func__);
> +				goto out;
> +			}
> +			/*
> +			 * Both playback and echo data will be shutdown in
> +			 * playback stream.
> +			 */
> +			ret = regmap_update_bits(map, TAS2873_REG_PWR_CTRL,
> +				TAS2783_REG_PWR_MODE_MASK |
> +				TAS2783_REG_AEF_MASK,
> +				TAS2783_REG_PWR_MODE_ACTIVE |
> +				TAS2783_REG_PWR_MODE_SW_PWD);
> +			if (ret) {
> +				dev_err(tas_dev->dev,
> +					"%s: PWR&AEF shutdown failed.\n",
> +					__func__);
> +				goto out;
> +			}
> +			tas_dev->pstream = false;
> +		}
> +	} else {
> +		/* FU23 Unmute, 0x40400108. */
> +		if (direction == SNDRV_PCM_STREAM_PLAYBACK) {
> +			ret = regmap_write(map,
> +				SDW_SDCA_CTL(TAS2783_FUNC_TYPE_SMART_AMP,
> +				TAS2783_SDCA_ENT_FU23,
> +				TAS2783_SDCA_CTL_FU_MUTE, 0), 0);
> +			if (ret) {
> +				dev_err(tas_dev->dev,
> +					"%s: FU23 Unmute failed.\n", __func__);
> +				goto out;
> +			}
> +			ret = regmap_update_bits(map, TAS2873_REG_PWR_CTRL,
> +				TAS2783_REG_PWR_MODE_MASK,
> +				TAS2783_REG_PWR_MODE_ACTIVE);
> +			if (ret) {
> +				dev_err(tas_dev->dev,
> +					"%s: PWR Unmute failed.\n", __func__);
> +				goto out;
> +			}
> +			tas_dev->pstream = true;
> +		} else {
> +			/* Capture stream is the echo ref data for voice.
> +			 * Without playback, it can't be active.
> +			 */
> +			if (tas_dev->pstream == true) {
> +				ret = regmap_update_bits(map,
> +					TAS2873_REG_PWR_CTRL,
> +					TAS2783_REG_AEF_MASK,
> +					TAS2783_REG_AEF_ACTIVE);
> +				if (ret) {
> +					dev_err(tas_dev->dev,
> +						"%s: AEF enable failed.\n",
> +						__func__);
> +					goto out;
> +				}
> +			} else {
> +				dev_err(tas_dev->dev,
> +					"%s: No playback, no AEF!", __func__);
> +				ret = -EINVAL;
> +			}
> +		}
> +	}
> +out:
> +	if (ret)
> +		dev_err(tas_dev->dev, "Mute or unmute %d failed %d.\n",
> +			mute, ret);
> +
> +	return ret;
> +}

Above function seem to be bit long, which also causes a lot of 
indentation, perhaps split it into mute and unmute helpers?

..

> +
> +static int tasdevice_read_prop(struct sdw_slave *slave)
> +{
> +	struct sdw_slave_prop *prop = &slave->prop;
> +	struct sdw_dpn_prop *dpn;
> +	unsigned long addr;
> +	int nval, i, j;
> +	u32 bit;
> +
> +	prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
> +	prop->quirks = SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY;
> +
> +	prop->paging_support = true;
> +
> +	/* first we need to allocate memory for set bits in port lists */
> +	prop->source_ports = BIT(2); /* BITMAP: 00000100 */
> +	prop->sink_ports = BIT(1); /* BITMAP:  00000010 */
> +
> +	nval = hweight32(prop->source_ports);
> +	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
> +		sizeof(*prop->src_dpn_prop), GFP_KERNEL);
> +	if (!prop->src_dpn_prop)
> +		return -ENOMEM;
> +
> +	i = 0;
> +	dpn = prop->src_dpn_prop;
> +	addr = prop->source_ports;
> +	for_each_set_bit(bit, &addr, 32) {
> +		dpn[i].num = bit;
> +		dpn[i].type = SDW_DPN_FULL;
> +		dpn[i].simple_ch_prep_sm = true;
> +		dpn[i].ch_prep_timeout = 10;
> +		i++;
> +	}
> +
> +	/* do this again for sink now */
> +	nval = hweight32(prop->sink_ports);
> +	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
> +		sizeof(*prop->sink_dpn_prop), GFP_KERNEL);
> +	if (!prop->sink_dpn_prop)
> +		return -ENOMEM;
> +
> +	j = 0;

No need for separate j variable, you can reuse i here.

> +	dpn = prop->sink_dpn_prop;
> +	addr = prop->sink_ports;
> +	for_each_set_bit(bit, &addr, 32) {
> +		dpn[j].num = bit;
> +		dpn[j].type = SDW_DPN_FULL;
> +		dpn[j].simple_ch_prep_sm = true;
> +		dpn[j].ch_prep_timeout = 10;
> +		j++;
> +	}
> +
> +	/* set the timeout values */
> +	prop->clk_stop_timeout = 20;
> +
> +	return 0;
> +}
> +

..

> +
> +struct tasdevice_priv {
> +	struct snd_soc_component *component;

Apart from being assigned this field seems to be unused.

> +	struct sdw_slave *sdw_peripheral;
> +	enum sdw_slave_status status;

This one seems to be only used in tasdevice_update_status()? Does it 
really need to be kept in struct?

> +	struct sdw_bus_params params;

Unused?

> +	struct regmap *regmap;
> +	struct device *dev;
> +	unsigned char dspfw_binaryname[TAS2783_DSPFW_FILENAME_LEN];

This one also seems weird, it is mainly needed when loading FW and could 
be local to tasdevice_comp_probe(), although there is one dev_warn which 
uses it outside of it, but pretty sure it could be dropped.

> +	unsigned char dev_name[32];

Another unused field.

> +	unsigned int chip_id;

Another one that only seems to be assigned.

> +	bool pstream;
> +	bool hw_init;
> +	bool first_hw_init;
> +};
> +
> +#endif /*__TAS2783_H__ */

Powered by blists - more mailing lists