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

[Ding, Shenghao]

Hi Guy
Thanks for your comments. Feedback inline

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Sent: Tuesday, March 19, 2024 11:35 PM
> 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; Xu,
> Baojun <baojun.xu@xxxxxx>; soyer@xxxxxx; Baojun.Xu@xxxxxxx; Navada
> Kanyana, Mukund <navada@xxxxxx>
> Subject: [EXTERNAL] Re: [PATCH v11] ASoc: tas2783: Add tas2783 codec
> driver
> 
> > +static bool tas2783_readable_register(struct device *dev, > +
> > +unsigned int reg) > +{ > + switch (reg) { > + /* Page 0 */ > + case
> > +0x8000 .. . 0x807F: > + return true; > + default: > + return false;
> > +so only the registers
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source of
> this email and know the content is safe.
> 
> ZjQcmQRYFpfptBannerEnd
> 
> > +static bool tas2783_readable_register(struct device *dev,
................................................
> > +
> > +	/* Check Calibrated Data V2 */
> > +	if (tmp_val[0] == 2783) {
> > +		const struct calibdatav2_info calib_info = {
> > +			.number_of_devices = tmp_val[1],
> > +			.crc32_indx = 3 + tmp_val[1] * 6,
> > +			.byt_sz = 12 + tmp_val[1] * 24,
> > +			.cali_data = &tmp_val[3]
> > +		};
> > +
> > +		if (calib_info.number_of_devices >
> TAS2783_MAX_DEV_NUM ||
> > +			calib_info.number_of_devices == 0) {
> > +			dev_dbg(tas_dev->dev, "No dev in calibrated data
> V2.");
> 
> the log is not aligned with the first condition where you have too many
> devices.
> 
> It's not clear why it's not an error.
playback still work without calibrated data stored in UEFI, for example bypass mode.
Even if in case of bypass mode, algo can still work with default calibrated data.
So, not an error.
> 
> > +			return;
> > +		}
> > +		crc = crc32(~0, cali_data.data, calib_info.byt_sz)
> > +			^ ~0;
> > +		if (crc == tmp_val[calib_info.crc32_indx]) {
> > +			tas2783_apply_calibv2(tas_dev, &calib_info);
> > +			dev_dbg(tas_dev->dev, "V2: %ptTs",
> 
> same, is this needed?
Accepted.
> > +
> 	&tmp_val[TAS2783_CALIDATAV2_TIMESTAMP_INDX]);
> > +		} else {
> > +			dev_dbg(tas_dev->dev,
> > +				"V2: CRC 0x%08x not match 0x%08x\n",
> > +				crc, tmp_val[calib_info.crc32_indx]);
> 
> is this not an error?
> 
> > +		}
> > +	} else {
> > +		dev_err(tas_dev->dev, "Non-2783 chip\n");
> > +	}
> > +}
> 
> the error level seem inconsistent and it's not clear why errors are ignored?
Maybe set them as dev_warn?