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

[Pierre-Louis Bossart]

>>> +			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.

I was referreing to the existing CRC at the end...

 * V1:
 *	Calibrated Data of Dev 0(unique_id offset 0) (20 bytes)
 *	Calibrated Data of Dev 1(unique_id offset 1) (20 bytes)
 *	Calibrated Data of Dev 2(unique_id offset 2) (20 bytes)
 *	Calibrated Data of Dev 3(unique_id offset 3) (20 bytes)
 *	Calibrated Data of Dev 4(unique_id offset 4) (20 bytes)
 *	Calibrated Data of Dev 5(unique_id offset 5) (20 bytes)
 *	Calibrated Data of Dev 6(unique_id offset 6) (20 bytes)
 *	Calibrated Data of Dev 7(unique_id offset 7) (20 bytes)
 *	TimeStamp of Calibration (4 bytes)
 *	CRC (4 bytes) <<<< HERE
 *	Reserved (88 bytes)

Presumably the CRC covers all the data above, so you could first check
if the format is V1 or V2 by checking the CRC values, then check the
binary value as confirmation. You're doing the opposite comparison.

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

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

What does 'turn off' mean?

This could be
a) the echo reference is made of zero-value samples but is transmitted
on the link
b) the echo reference is no longer transmitted and the stream is
disabled with a bank switch.

>>
>> 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.

I can't follow your answer.

What I was asking is whether we can have a case where tas_dev->pstream
can be tested while it's being set by another thread.

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