[PATCH v2] ALSA: hda/tas2781: Fix the order of TAS2781 calibrated-data

[PATCH v2] ALSA: hda/tas2781: Fix the order of TAS2781 calibrated-data

[Shenghao Ding]

A bug reported by one of my customers that the order of TAS2781
calibrated-data is incorrect, the correct way is to move R0_Low
and insert it between R0 and InvR0.

Fixes: 4fe238513407 ("ALSA: hda/tas2781: Move and unified the calibrated-data getting function for SPI and I2C into the tas2781_hda lib")
Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>

---
v2:
 - Submit to sound branch maintianed by Tiwai instead of linux-next branch
 - drop other fix
---
 sound/hda/codecs/side-codecs/tas2781_hda.c | 38 ++++++++++++++++++----
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/sound/hda/codecs/side-codecs/tas2781_hda.c b/sound/hda/codecs/side-codecs/tas2781_hda.c
index f46d2e06c64f..cd9990869e18 100644
--- a/sound/hda/codecs/side-codecs/tas2781_hda.c
+++ b/sound/hda/codecs/side-codecs/tas2781_hda.c
@@ -33,6 +33,32 @@ const efi_guid_t tasdev_fct_efi_guid[] = {
 };
 EXPORT_SYMBOL_NS_GPL(tasdev_fct_efi_guid, "SND_HDA_SCODEC_TAS2781");
 
+/*
+ * The order of calibrated-data writing is a bit different from the order
+ * in UEFI. Here is the conversion to match the order of calibrated-data
+ * writing.
+ */
+static void cali_cnv(unsigned char *data, unsigned int base, int offset)
+{
+	__be32 bedata[TASDEV_CALIB_N];
+	int i;
+
+	/* r0_reg */
+	bedata[0] = cpu_to_be32(*(uint32_t *)&data[base]);
+	/* r0_low_reg */
+	bedata[1] = cpu_to_be32(*(uint32_t *)&data[base + 8]);
+	/* invr0_reg */
+	bedata[2] = cpu_to_be32(*(uint32_t *)&data[base + 4]);
+	/* pow_reg */
+	bedata[3] = cpu_to_be32(*(uint32_t *)&data[base + 12]);
+	/* tlimit_reg */
+	bedata[4] = cpu_to_be32(*(uint32_t *)&data[base + 16]);
+
+	for (i = 0; i < TASDEV_CALIB_N; i++)
+		memcpy(&data[offset + i * 4 + 1], &bedata[i],
+			sizeof(bedata[i]));
+}
+
 static void tas2781_apply_calib(struct tasdevice_priv *p)
 {
 	struct calidata *cali_data = &p->cali_data;
@@ -86,6 +112,7 @@ static void tas2781_apply_calib(struct tasdevice_priv *p)
 
 		for (j = 0, k = 0; j < node_num; j++) {
 			oft = j * 6 + 3;
+			/* Calibration registers address */
 			if (tmp_val[oft] == TASDEV_UEFI_CALI_REG_ADDR_FLG) {
 				for (i = 0; i < TASDEV_CALIB_N; i++) {
 					buf = &data[(oft + i + 1) * 4];
@@ -93,6 +120,7 @@ static void tas2781_apply_calib(struct tasdevice_priv *p)
 						buf[2], buf[3]);
 				}
 			} else {
+				/* Calibrated data */
 				l = j * (cali_data->cali_dat_sz_per_dev + 1);
 				if (k >= p->ndev || l > oft * 4) {
 					dev_err(p->dev, "%s: dev sum error\n",
@@ -103,8 +131,7 @@ static void tas2781_apply_calib(struct tasdevice_priv *p)
 
 				data[l] = k;
 				oft++;
-				for (i = 0; i < TASDEV_CALIB_N * 4; i++)
-					data[l + i + 1] = data[4 * oft + i];
+				cali_cnv(data, 4 * oft, l);
 				k++;
 			}
 		}
@@ -127,12 +154,11 @@ static void tas2781_apply_calib(struct tasdevice_priv *p)
 			dev_err(p->dev, "%s: V1 CRC error\n", __func__);
 			return;
 		}
-
+		/* reverse rearrangement in case of overlap */
 		for (j = p->ndev - 1; j >= 0; j--) {
 			l = j * (cali_data->cali_dat_sz_per_dev + 1);
-			for (i = TASDEV_CALIB_N * 4; i > 0 ; i--)
-				data[l + i] = data[p->index * 5 + i];
-			data[l+i] = j;
+			cali_cnv(data, cali_data->cali_dat_sz_per_dev * j, l);
+			data[l] = j;
 		}
 	}
 
-- 
2.43.0

Re: [PATCH v2] ALSA: hda/tas2781: Fix the order of TAS2781 calibrated-data

[Takashi Iwai]

On Wed, 03 Sep 2025 06:13:51 +0200,
Shenghao Ding wrote:
> 
> A bug reported by one of my customers that the order of TAS2781
> calibrated-data is incorrect, the correct way is to move R0_Low
> and insert it between R0 and InvR0.
> 
> Fixes: 4fe238513407 ("ALSA: hda/tas2781: Move and unified the calibrated-data getting function for SPI and I2C into the tas2781_hda lib")
> Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>
> 
> ---
> v2:
>  - Submit to sound branch maintianed by Tiwai instead of linux-next branch
>  - drop other fix

You haven't answered to my previous question at all.

Is the issue you're trying to fix something different from what Gergo
already fixed in commit d5f8458e34a331e5b228de142145e62ac5bfda34
(which was already merged to Linus tree).


Takashi

RE: [EXTERNAL] Re: [PATCH v2] ALSA: hda/tas2781: Fix the order of TAS2781 calibrated-data

[Ding, Shenghao]

Glad to answer your question

> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de>
> Sent: Wednesday, September 3, 2025 3:21 PM
> To: Ding, Shenghao <shenghao-ding@ti.com>
> Cc: broonie@kernel.org; andriy.shevchenko@linux.intel.com;
> 13564923607@139.com; 13916275206@139.com; alsa-devel@alsa-
> project.org; linux-kernel@vger.kernel.org; Xu, Baojun <baojun.xu@ti.com>;
> Baojun.Xu@fpt.com
> Subject: [EXTERNAL] Re: [PATCH v2] ALSA: hda/tas2781: Fix the order of
> TAS2781 calibrated-data
> 
.........................
> ZjQcmQRYFpfptBannerEnd
> On Wed, 03 Sep 2025 06:13:51 +0200,
> Shenghao Ding wrote:
> >
> > A bug reported by one of my customers that the order of TAS2781
> > calibrated-data is incorrect, the correct way is to move R0_Low and
> > insert it between R0 and InvR0.
> >
> > Fixes: 4fe238513407 ("ALSA: hda/tas2781: Move and unified the
> > calibrated-data getting function for SPI and I2C into the tas2781_hda
> > lib")
> > Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>
> >
> > ---
> > v2:
> >  - Submit to sound branch maintianed by Tiwai instead of linux-next
> > branch
> >  - drop other fix
> 
> You haven't answered to my previous question at all.
> 
> Is the issue you're trying to fix something different from what Gergo already
> fixed in commit d5f8458e34a331e5b228de142145e62ac5bfda34
> (which was already merged to Linus tree).
Yes, this patch is to fix TAS2781. Gergo fixed TAS2563
> 
> 
> Takashi

Re: [PATCH v2] ALSA: hda/tas2781: Fix the order of TAS2781 calibrated-data

[Takashi Iwai]

On Wed, 03 Sep 2025 06:13:51 +0200,
Shenghao Ding wrote:
> 
> A bug reported by one of my customers that the order of TAS2781
> calibrated-data is incorrect, the correct way is to move R0_Low
> and insert it between R0 and InvR0.
> 
> Fixes: 4fe238513407 ("ALSA: hda/tas2781: Move and unified the calibrated-data getting function for SPI and I2C into the tas2781_hda lib")
> Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>
> 
> ---
> v2:
>  - Submit to sound branch maintianed by Tiwai instead of linux-next branch
>  - drop other fix
> ---
>  sound/hda/codecs/side-codecs/tas2781_hda.c | 38 ++++++++++++++++++----
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/hda/codecs/side-codecs/tas2781_hda.c b/sound/hda/codecs/side-codecs/tas2781_hda.c
> index f46d2e06c64f..cd9990869e18 100644
> --- a/sound/hda/codecs/side-codecs/tas2781_hda.c
> +++ b/sound/hda/codecs/side-codecs/tas2781_hda.c
> @@ -33,6 +33,32 @@ const efi_guid_t tasdev_fct_efi_guid[] = {
>  };
>  EXPORT_SYMBOL_NS_GPL(tasdev_fct_efi_guid, "SND_HDA_SCODEC_TAS2781");
>  
> +/*
> + * The order of calibrated-data writing is a bit different from the order
> + * in UEFI. Here is the conversion to match the order of calibrated-data
> + * writing.
> + */
> +static void cali_cnv(unsigned char *data, unsigned int base, int offset)
> +{
> +	__be32 bedata[TASDEV_CALIB_N];
> +	int i;
> +
> +	/* r0_reg */
> +	bedata[0] = cpu_to_be32(*(uint32_t *)&data[base]);
> +	/* r0_low_reg */
> +	bedata[1] = cpu_to_be32(*(uint32_t *)&data[base + 8]);
> +	/* invr0_reg */
> +	bedata[2] = cpu_to_be32(*(uint32_t *)&data[base + 4]);
> +	/* pow_reg */
> +	bedata[3] = cpu_to_be32(*(uint32_t *)&data[base + 12]);
> +	/* tlimit_reg */
> +	bedata[4] = cpu_to_be32(*(uint32_t *)&data[base + 16]);
> +
> +	for (i = 0; i < TASDEV_CALIB_N; i++)
> +		memcpy(&data[offset + i * 4 + 1], &bedata[i],
> +			sizeof(bedata[i]));
> +}

IMO, this can be more readable when you use struct calidata, e.g.

static void cali_cnv(unsigned char *data, unsigned int base, int offset)
{
	struct calidata reg;

	reg.r0_reg = *(u32 *)&data[base]
	reg.r0_low_reg = *(u32 *)&data[base + 8]
	reg.invr0_reg = *(u32 *)&data[base + 4]
	reg.pow_reg = *(u32 *)&data[base + 12];
	reg.tlimit_reg = *(u32 *)&data[base + 16]);

	cpu_to_be32_array((__force __be32 *)(data + offset + 1), &reg,
			  TASDEV_CALIB_N);
}

... or even simpler like:

static void cali_cnv(unsigned char *data, unsigned int base, int offset)
{
	struct calidata reg;

	memcpy(&reg, data, sizeof(reg));
	/* the data order has to be swapped between r0_low_reg and inv0_reg */
	swap(reg.r0_low_reg, reg.invr0_reg);

	cpu_to_be32_array((__force __be32 *)(data + offset + 1), &reg,
			  TASDEV_CALIB_N);
}

>  static void tas2781_apply_calib(struct tasdevice_priv *p)
>  {
>  	struct calidata *cali_data = &p->cali_data;
> @@ -86,6 +112,7 @@ static void tas2781_apply_calib(struct tasdevice_priv *p)
>  
>  		for (j = 0, k = 0; j < node_num; j++) {
>  			oft = j * 6 + 3;
> +			/* Calibration registers address */

Don't try to add unrelated changes.  This comment won't fix or explain
what your patch does.  If any, make another patch to update / add more
comments.
Putting unrelated changes disturbs the patch readability *a lot*

>  			if (tmp_val[oft] == TASDEV_UEFI_CALI_REG_ADDR_FLG) {
>  				for (i = 0; i < TASDEV_CALIB_N; i++) {
>  					buf = &data[(oft + i + 1) * 4];
> @@ -93,6 +120,7 @@ static void tas2781_apply_calib(struct tasdevice_priv *p)
>  						buf[2], buf[3]);
>  				}
>  			} else {
> +				/* Calibrated data */

Ditto.

> @@ -127,12 +154,11 @@ static void tas2781_apply_calib(struct tasdevice_priv *p)
>  			dev_err(p->dev, "%s: V1 CRC error\n", __func__);
>  			return;
>  		}
> -
> +		/* reverse rearrangement in case of overlap */

Ditto.


thanks,

Takashi

RE: [EXTERNAL] Re: [PATCH v2] ALSA: hda/tas2781: Fix the order of TAS2781 calibrated-data

[Ding, Shenghao]

Thanks for your ref code
> > +/*
> > + * The order of calibrated-data writing is a bit different from the
> > +order
> > + * in UEFI. Here is the conversion to match the order of
> > +calibrated-data
> > + * writing.
> > + */
> > +static void cali_cnv(unsigned char *data, unsigned int base, int
> > +offset) {
> > +	__be32 bedata[TASDEV_CALIB_N];
> > +	int i;
> > +
> > +	/* r0_reg */
> > +	bedata[0] = cpu_to_be32(*(uint32_t *)&data[base]);
> > +	/* r0_low_reg */
> > +	bedata[1] = cpu_to_be32(*(uint32_t *)&data[base + 8]);
> > +	/* invr0_reg */
> > +	bedata[2] = cpu_to_be32(*(uint32_t *)&data[base + 4]);
> > +	/* pow_reg */
> > +	bedata[3] = cpu_to_be32(*(uint32_t *)&data[base + 12]);
> > +	/* tlimit_reg */
> > +	bedata[4] = cpu_to_be32(*(uint32_t *)&data[base + 16]);
> > +
> > +	for (i = 0; i < TASDEV_CALIB_N; i++)
> > +		memcpy(&data[offset + i * 4 + 1], &bedata[i],
> > +			sizeof(bedata[i]));
> > +}
> 
> IMO, this can be more readable when you use struct calidata, e.g.
> 
> static void cali_cnv(unsigned char *data, unsigned int base, int offset) {
> 	struct calidata reg;
> 
> 	reg.r0_reg = *(u32 *)&data[base]
> 	reg.r0_low_reg = *(u32 *)&data[base + 8]
> 	reg.invr0_reg = *(u32 *)&data[base + 4]
> 	reg.pow_reg = *(u32 *)&data[base + 12];
> 	reg.tlimit_reg = *(u32 *)&data[base + 16]);
> 
> 	cpu_to_be32_array((__force __be32 *)(data + offset + 1), &reg,
> 			  TASDEV_CALIB_N);
> }
> 
> ... or even simpler like:
> 
> static void cali_cnv(unsigned char *data, unsigned int base, int offset) {
> 	struct calidata reg;
> 
> 	memcpy(&reg, data, sizeof(reg));
> 	/* the data order has to be swapped between r0_low_reg and inv0_reg
> */
> 	swap(reg.r0_low_reg, reg.invr0_reg);
> 
> 	cpu_to_be32_array((__force __be32 *)(data + offset + 1), &reg,
> 			  TASDEV_CALIB_N);
> }
I like this code so much. It's elegant simplicity.

Thanks,
Shenghao Ding