RE: [EXTERNAL] Re: [PATCH v11] ASoc: tas2783: Add tas2783 codec driver

[Ding, Shenghao]

Hi Pierre-Louis
Thanks for your commets.

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Sent: Wednesday, March 6, 2024 12:03 AM
> To: Ding, Shenghao <shenghao-ding@xxxxxx>; broonie@xxxxxxxxxx
> Cc: andriy.shevchenko@xxxxxxxxxxxxxxx; lgirdwood@xxxxxxxxx;
> perex@xxxxxxxx; 13916275206@xxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; liam.r.girdwood@xxxxxxxxx;
> bard.liao@xxxxxxxxx; mengdong.lin@xxxxxxxxx; yung-
> chuan.liao@xxxxxxxxxxxxxxx; Lu, Kevin <kevin-lu@xxxxxx>; tiwai@xxxxxxx;
> soyer@xxxxxx; Baojun.Xu@xxxxxxx; Navada Kanyana, Mukund
> <navada@xxxxxx>
> Subject: [EXTERNAL] Re: [PATCH v11] ASoc: tas2783: Add tas2783 codec
> driver
> 
...............................
> > +static void tas2783_calibration(struct tasdevice_priv *tas_dev) {
> > +	efi_guid_t efi_guid = EFI_GUID(0x1f52d2a1, 0xbb3a, 0x457d, 0xbc,
> > +			0x09, 0x43, 0xa3, 0xf4, 0x31, 0x0a, 0x92);
> > +	static efi_char16_t efi_name[] = L"CALI_DATA";
> > +	struct calibration_data cali_data;
> > +	unsigned int *tmp_val;
> > +	unsigned int crc;
> > +	efi_status_t status;
> > +
> > +	cali_data.total_sz = 0;
> > +
> > +	status = efi.get_variable(efi_name, &efi_guid, NULL,
> > +		&cali_data.total_sz, NULL);
> > +	if (status == EFI_BUFFER_TOO_SMALL
> > +		&& cali_data.total_sz < TAS2783_MAX_CALIDATA_SIZE) {
> > +		status = efi.get_variable(efi_name, &efi_guid, NULL,
> > +			&cali_data.total_sz,
> > +			cali_data.data);
> > +		dev_dbg(tas_dev->dev, "%s: cali get %lx bytes result:%ld\n",
> > +			__func__, cali_data.total_sz, status);
> > +	}
> > +	if (status != 0) {
> > +		/* Failed got calibration data from EFI. */
> > +		dev_dbg(tas_dev->dev, "No calibration data in UEFI.");
> > +		return;
> > +	}
> > +
> > +	tmp_val = (unsigned int *)cali_data.data;
> > +
> > +	if (tmp_val[0] == 2783) {
> 
> Goodness, what guaranteers that the '2783' value is NOT a calibrated value?
> 
> V2:
> *	ChipID (2783, 4 bytes)
> 
> V1:
> *	Calibrated Data of Dev 0(unique_id offset 0) (20 bytes)
> 
> Comparing binary values is very risky. Usually you need some sort of CRC to
> avoid conflicts...
> 
> > +		/* Calibrated Data V2 */
> > +		unsigned int dev_sum = tmp_val[1];
> > +
> > +		if (dev_sum > TAS2783_MAX_DEV_NUM ||
> 
> I thought dev_sum meant some sort of 'sum' in the CRC sense, but it's really
> number_of_devices, isn't it?
> 
> > +			dev_sum == 0) {
> > +			dev_dbg(tas_dev->dev, "No dev in calibrated data
> V2.");
> > +			return;
> > +		}
> > +		crc = crc32(~0, cali_data.data, 12 + dev_sum * 24) ^ ~0;
> > +		if (crc == tmp_val[3 + dev_sum * 6]) {
> > +			tas2783_apply_calibv2(tas_dev, tmp_val);
> > +			dev_dbg(tas_dev->dev, "V2: %ptTs", &tmp_val[40]);
> > +		} else {
> > +			dev_dbg(tas_dev->dev,
> > +				"V2: CRC 0x%08x not match 0x%08x\n",
> > +				crc, tmp_val[41]);
> > +		}
> > +	} else {
> > +		/* Calibrated Data V1 */
> > +		/* 8 devs * 20 bytes calibrated data/dev + 4 bytes Timestamp
> */
> > +		crc = crc32(~0, cali_data.data, 164) ^ ~0;
> > +		if (crc == tmp_val[41]) {
> > +			/* Date and time of when calibration was done. */
> > +			tas2783_apply_calibv1(tas_dev, tmp_val);
> > +			dev_dbg(tas_dev->dev, "V1: %ptTs", &tmp_val[40]);
> > +		} else {
> > +			dev_dbg(tas_dev->dev,
> > +				"V1: CRC 0x%08x not match 0x%08x\n",
> > +				crc, tmp_val[41]);
> > +		}
> 
> The CRC check should be used to determine if the v1 or v2 should be used.
> This is really broken IMHO, you could detect the wrong layout then flag that
> the CRC is bad without even trying the other layout...

It seemed not easy to add an extra CRC in V1, especially for the old device in the end users.
As you know, the V1 is stored in raw data. In order to take care of the
old devices have been already in the end users, the V1 part has to keep here.

> 
> > +	}
> > +}
> > +
> > +static void tasdevice_dspfw_ready(const struct firmware *fmw,
> > +	void *context)
> > +{
> > +	struct tasdevice_priv *tas_dev =
> > +		(struct tasdevice_priv *) context;
> > +	struct tas2783_firmware_node *p;
> > +	struct regmap *map = tas_dev->regmap;
> > +	unsigned char *buf = NULL;
> > +	int offset = 0, img_sz;
> > +	int ret;
> > +
> > +	if (!fmw || !fmw->data) {
> > +		dev_warn(tas_dev->dev,
> > +			"%s: failed to read %s: work in bypass-dsp mode.\n",
> > +			__func__, tas_dev->dspfw_binaryname);
> > +		return;
> > +	}
> > +	buf = (unsigned char *)fmw->data;
> > +
> > +	img_sz = get_unaligned_le32(&buf[offset]);
> > +	offset  += sizeof(img_sz);
> > +	if (img_sz != fmw->size) {
> > +		dev_warn(tas_dev->dev, "%s: size not matching, %d %u.",
> > +			__func__, (int)fmw->size, img_sz);
> > +		return;
> > +	}
> > +
> > +	while (offset < img_sz) {
> > +		p = (struct tas2783_firmware_node *)(buf + offset);
> > +		if (p->length > 1) {
> > +			ret = regmap_bulk_write(map, p->download_addr,
> > +			buf + offset + sizeof(unsigned int) * 5, p->length);
> > +		} else {
> > +			ret = regmap_write(map, p->download_addr,
> > +				*(buf + offset + sizeof(unsigned int) * 5));
> > +		}
> > +
> > +		if (ret != 0) {
> > +			dev_dbg(tas_dev->dev,
> > +				"%s: load FW fail: %d, work in bypass.\n",
> > +				__func__, ret);
> > +			return;
> > +		}
> > +		offset += sizeof(unsigned int) * 5 + p->length;
> 
> what is '5'? Using a define is best to avoid such magic numbers.
> 
> > +	}
> > +
> > +	tas2783_calibration(tas_dev);
> > +}
> > +
> 
> > +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.
> 
> [1] echo reference data is usually not part of the playback stream but
> provided over a capture stream. Please clarify this comment.
> 
> > +			 */
> > +			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.
> 
> That makes case for [1] above.
> 
> I also don't get the concept of 'active'. Even when the playback is muted, you
> do want to provide a reference stream, don't you?
When playback is muted, it will turn off echo ref.
> 
> Also don't we have a potential race condition, you set the 'pstream'
> status for the playback but use it for capture. What tells you that this cannot
> be executed concurrently?
Capture in tas2783 can be unmute during playback is unmute.
No playback, no capture.
> 
> > +			 */
> > +			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;
> > +}