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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <51a81ac8aace456aae7d07634912367c@ti.com>
Date: Tue, 26 Mar 2024 12:36:05 +0000
From: "Ding, Shenghao" <shenghao-ding@...com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        "broonie@...nel.org" <broonie@...nel.org>
CC: "andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "perex@...ex.cz"
	<perex@...ex.cz>,
        "13916275206@....com" <13916275206@....com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "liam.r.girdwood@...el.com" <liam.r.girdwood@...el.com>,
        "bard.liao@...el.com" <bard.liao@...el.com>,
        "mengdong.lin@...el.com"
	<mengdong.lin@...el.com>,
        "yung-chuan.liao@...ux.intel.com"
	<yung-chuan.liao@...ux.intel.com>,
        "Lu, Kevin" <kevin-lu@...com>, "tiwai@...e.de" <tiwai@...e.de>,
        "Xu, Baojun" <baojun.xu@...com>, "soyer@....hu" <soyer@....hu>,
        "Baojun.Xu@....com" <Baojun.Xu@....com>,
        "Navada Kanyana, Mukund" <navada@...com>
Subject: RE: [EXTERNAL] Re: [PATCH v11] ASoc: tas2783: Add tas2783 codec
 driver

Hi Guy
Thanks for your comments. Feedback inline

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
> Sent: Tuesday, March 19, 2024 11:35 PM
> To: Ding, Shenghao <shenghao-ding@...com>; broonie@...nel.org
> Cc: andriy.shevchenko@...ux.intel.com; lgirdwood@...il.com;
> perex@...ex.cz; 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; Lu, Kevin <kevin-lu@...com>; tiwai@...e.de; Xu,
> Baojun <baojun.xu@...com>; soyer@....hu; Baojun.Xu@....com; Navada
> Kanyana, Mukund <navada@...com>
> 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?