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 in 0x8000..807F are readable. That's the usual non-SDCA vendor-specific areas ...

+static const struct regmap_config tasdevice_regmap = {

• .reg_bits = 32,
• .val_bits = 8,
• .readable_reg = tas2783_readable_register,
• .volatile_reg = tas2783_volatile_register,
• .max_register = 0x44ffffff,

.... but here you show support for a much larger register set in SDCA space.

I am having a hard-time believing that none of these SDCA registers are readable?

+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;
• /* Check Calibrated Data V1 */
• crc = crc32(~0, cali_data.data, TAS2783_CALIDATAV1_BYTE_SIZE) ^ ~0;
• if (crc == tmp_val[TAS2783_CALIDATAV1_CRC32_INDX]) {

• /* Date and time of when calibration was done. */
  

• tas2783_apply_calibv1(tas_dev, tmp_val);
  

• dev_dbg(tas_dev->dev, "V1: %ptTs",
  

Is this really needed/helpful?

• 	&tmp_val[TAS2783_CALIDATAV1_TIMESTAMP_INDX]);
  

• return;
  

• }
• /* 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.

• 	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?

• 		&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?