[PATCH 0/1] ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

[PATCH 0/1] ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

[Matteo Martelli]

I found an issue on the sunxi i2s controller driver while doing some
tests with a Pine64 A64 host board and an external codec (ES8311).
The A64 i2s controller is compatible with the sun8i-h3-i2s driver.
The LRCLK was being inverted when the bus was operated in i2s mode:
normally should be left channel on low LRCLK and right channel on high
LRCLK, but it was the opposite instead.
I noticed this issue due to the playback being addressed on the wrong
codec channel, then confirmed by analyzing the clock signal with a logic
analyzer.

Note that this fix applies for all the i2s controllers compatible with
sun8i-h3-i2s and sun50i-h6-i2s, but I could test it only for the A64.

The issue had likely been introduced in commit dd657eae8164 ("ASoC:
sun4i-i2s: Fix the LRCK polarity") due to a misinterpreted bit in the H3
or H6 User Manual. I suppose that the i2s mode had not been tested at
that time. Can this be confirmed? Otherwise there is something else
going on and this patch should not be applied as is.

Matteo Martelli (1):
  ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

 sound/soc/sunxi/sun4i-i2s.c | 143 ++++++++++++++++++------------------
 1 file changed, 73 insertions(+), 70 deletions(-)


base-commit: 47d09270d7776e46858a161f94b735640b2fb056
-- 
2.45.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/1] ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

[Matteo Martelli]

This fixes the LRCLK polarity for sun8i-h3 and sun50i-h6 in i2s mode
which was wrongly inverted.

The LRCLK was being set in reversed logic compared to the DAI format:
inverted LRCLK for SND_SOC_DAIFMT_IB_NF and SND_SOC_DAIFMT_NB_NF; normal
LRCLK for SND_SOC_DAIFMT_IB_IF and SND_SOC_DAIFMT_NB_IF. Such reversed
logic applies properly for DSP_A, DSP_B, LEFT_J and RIGHT_J modes but
not for I2S mode, for which the LRCLK signal results reversed to what
expected on the bus. The issue is due to a misinterpretation of the
LRCLK polarity bit of the H3 and H6 i2s controllers. Such bit in this
case does not mean "0 => normal" or "1 => inverted" according to the
expected bus operation, but it means "0 => frame starts on low edge" and
"1 => frame starts on high edge" (from the User Manuals).

This commit fixes the LRCLK polarity by setting the LRCLK polarity bit
according to the selected bus mode and renames the LRCLK polarity bit
definition to avoid further confusion.

Fixes: dd657eae8164 ("ASoC: sun4i-i2s: Fix the LRCK polarity")
Fixes: 73adf87b7a58 ("ASoC: sun4i-i2s: Add support for H6 I2S")
Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
 sound/soc/sunxi/sun4i-i2s.c | 143 ++++++++++++++++++------------------
 1 file changed, 73 insertions(+), 70 deletions(-)

diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index 5f8d979585b6..a200ba41679a 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -100,8 +100,8 @@
 #define SUN8I_I2S_CTRL_MODE_PCM			(0 << 4)
 
 #define SUN8I_I2S_FMT0_LRCLK_POLARITY_MASK	BIT(19)
-#define SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED		(1 << 19)
-#define SUN8I_I2S_FMT0_LRCLK_POLARITY_NORMAL		(0 << 19)
+#define SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH	(1 << 19)
+#define SUN8I_I2S_FMT0_LRCLK_POLARITY_START_LOW		(0 << 19)
 #define SUN8I_I2S_FMT0_LRCK_PERIOD_MASK		GENMASK(17, 8)
 #define SUN8I_I2S_FMT0_LRCK_PERIOD(period)	((period - 1) << 8)
 #define SUN8I_I2S_FMT0_BCLK_POLARITY_MASK	BIT(7)
@@ -729,65 +729,37 @@ static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
 static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
 				 unsigned int fmt)
 {
-	u32 mode, val;
+	u32 mode, lrclk_pol, bclk_pol, val;
 	u8 offset;
 
-	/*
-	 * DAI clock polarity
-	 *
-	 * The setup for LRCK contradicts the datasheet, but under a
-	 * scope it's clear that the LRCK polarity is reversed
-	 * compared to the expected polarity on the bus.
-	 */
-	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
-	case SND_SOC_DAIFMT_IB_IF:
-		/* Invert both clocks */
-		val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED;
-		break;
-	case SND_SOC_DAIFMT_IB_NF:
-		/* Invert bit clock */
-		val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED |
-		      SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED;
-		break;
-	case SND_SOC_DAIFMT_NB_IF:
-		/* Invert frame clock */
-		val = 0;
-		break;
-	case SND_SOC_DAIFMT_NB_NF:
-		val = SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG,
-			   SUN8I_I2S_FMT0_LRCLK_POLARITY_MASK |
-			   SUN8I_I2S_FMT0_BCLK_POLARITY_MASK,
-			   val);
-
 	/* DAI Mode */
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 	case SND_SOC_DAIFMT_DSP_A:
+		lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH;
 		mode = SUN8I_I2S_CTRL_MODE_PCM;
 		offset = 1;
 		break;
 
 	case SND_SOC_DAIFMT_DSP_B:
+		lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH;
 		mode = SUN8I_I2S_CTRL_MODE_PCM;
 		offset = 0;
 		break;
 
 	case SND_SOC_DAIFMT_I2S:
+		lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_LOW;
 		mode = SUN8I_I2S_CTRL_MODE_LEFT;
 		offset = 1;
 		break;
 
 	case SND_SOC_DAIFMT_LEFT_J:
+		lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH;
 		mode = SUN8I_I2S_CTRL_MODE_LEFT;
 		offset = 0;
 		break;
 
 	case SND_SOC_DAIFMT_RIGHT_J:
+		lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH;
 		mode = SUN8I_I2S_CTRL_MODE_RIGHT;
 		offset = 0;
 		break;
@@ -805,6 +777,35 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
 			   SUN8I_I2S_TX_CHAN_OFFSET_MASK,
 			   SUN8I_I2S_TX_CHAN_OFFSET(offset));
 
+	/* DAI polarity */
+	bclk_pol = SUN8I_I2S_FMT0_BCLK_POLARITY_NORMAL;
+
+	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+	case SND_SOC_DAIFMT_IB_IF:
+		/* Invert both clocks */
+		lrclk_pol ^= SUN8I_I2S_FMT0_BCLK_POLARITY_MASK;
+		bclk_pol = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED;
+		break;
+	case SND_SOC_DAIFMT_IB_NF:
+		/* Invert bit clock */
+		bclk_pol = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED;
+		break;
+	case SND_SOC_DAIFMT_NB_IF:
+		/* Invert frame clock */
+		lrclk_pol ^= SUN8I_I2S_FMT0_BCLK_POLARITY_MASK;
+		break;
+	case SND_SOC_DAIFMT_NB_NF:
+		/* No inversion */
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG,
+			   SUN8I_I2S_FMT0_LRCLK_POLARITY_MASK |
+			   SUN8I_I2S_FMT0_BCLK_POLARITY_MASK,
+			   lrclk_pol | bclk_pol);
+
 	/* DAI clock master masks */
 	switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
 	case SND_SOC_DAIFMT_BP_FP:
@@ -836,65 +837,37 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
 static int sun50i_h6_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
 				     unsigned int fmt)
 {
-	u32 mode, val;
+	u32 mode, lrclk_pol, bclk_pol, val;
 	u8 offset;
 
-	/*
-	 * DAI clock polarity
-	 *
-	 * The setup for LRCK contradicts the datasheet, but under a
-	 * scope it's clear that the LRCK polarity is reversed
-	 * compared to the expected polarity on the bus.
-	 */
-	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
-	case SND_SOC_DAIFMT_IB_IF:
-		/* Invert both clocks */
-		val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED;
-		break;
-	case SND_SOC_DAIFMT_IB_NF:
-		/* Invert bit clock */
-		val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED |
-		      SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED;
-		break;
-	case SND_SOC_DAIFMT_NB_IF:
-		/* Invert frame clock */
-		val = 0;
-		break;
-	case SND_SOC_DAIFMT_NB_NF:
-		val = SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG,
-			   SUN8I_I2S_FMT0_LRCLK_POLARITY_MASK |
-			   SUN8I_I2S_FMT0_BCLK_POLARITY_MASK,
-			   val);
-
 	/* DAI Mode */
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 	case SND_SOC_DAIFMT_DSP_A:
+		lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH;
 		mode = SUN8I_I2S_CTRL_MODE_PCM;
 		offset = 1;
 		break;
 
 	case SND_SOC_DAIFMT_DSP_B:
+		lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH;
 		mode = SUN8I_I2S_CTRL_MODE_PCM;
 		offset = 0;
 		break;
 
 	case SND_SOC_DAIFMT_I2S:
+		lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_LOW;
 		mode = SUN8I_I2S_CTRL_MODE_LEFT;
 		offset = 1;
 		break;
 
 	case SND_SOC_DAIFMT_LEFT_J:
+		lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH;
 		mode = SUN8I_I2S_CTRL_MODE_LEFT;
 		offset = 0;
 		break;
 
 	case SND_SOC_DAIFMT_RIGHT_J:
+		lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH;
 		mode = SUN8I_I2S_CTRL_MODE_RIGHT;
 		offset = 0;
 		break;
@@ -912,6 +885,36 @@ static int sun50i_h6_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
 			   SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK,
 			   SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset));
 
+	/* DAI clock polarity */
+	bclk_pol = SUN8I_I2S_FMT0_BCLK_POLARITY_NORMAL;
+
+	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+	case SND_SOC_DAIFMT_IB_IF:
+		/* Invert both clocks */
+		lrclk_pol ^= SUN8I_I2S_FMT0_BCLK_POLARITY_MASK;
+		bclk_pol = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED;
+		break;
+	case SND_SOC_DAIFMT_IB_NF:
+		/* Invert bit clock */
+		bclk_pol = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED;
+		break;
+	case SND_SOC_DAIFMT_NB_IF:
+		/* Invert frame clock */
+		lrclk_pol ^= SUN8I_I2S_FMT0_BCLK_POLARITY_MASK;
+		break;
+	case SND_SOC_DAIFMT_NB_NF:
+		/* No inversion */
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG,
+			   SUN8I_I2S_FMT0_LRCLK_POLARITY_MASK |
+			   SUN8I_I2S_FMT0_BCLK_POLARITY_MASK,
+			   lrclk_pol | bclk_pol);
+
+
 	/* DAI clock master masks */
 	switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
 	case SND_SOC_DAIFMT_BP_FP:
-- 
2.45.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/1] ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 774 bytes --]

On Wed, May 29, 2024 at 04:00:14PM +0200, Matteo Martelli wrote:

> I found an issue on the sunxi i2s controller driver while doing some
> tests with a Pine64 A64 host board and an external codec (ES8311).
> The A64 i2s controller is compatible with the sun8i-h3-i2s driver.
> The LRCLK was being inverted when the bus was operated in i2s mode:
> normally should be left channel on low LRCLK and right channel on high
> LRCLK, but it was the opposite instead.

Please don't send cover letters for single patches, if there is anything
that needs saying put it in the changelog of the patch or after the ---
if it's administrative stuff.  This reduces mail volume and ensures that 
any important information is recorded in the changelog rather than being
lost. 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/1] ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

[Matteo Martelli]

Mark Brown wrote:
> On Wed, May 29, 2024 at 04:00:14PM +0200, Matteo Martelli wrote:
> 
> > I found an issue on the sunxi i2s controller driver while doing some
> > tests with a Pine64 A64 host board and an external codec (ES8311).
> > The A64 i2s controller is compatible with the sun8i-h3-i2s driver.
> > The LRCLK was being inverted when the bus was operated in i2s mode:
> > normally should be left channel on low LRCLK and right channel on high
> > LRCLK, but it was the opposite instead.
> 
> Please don't send cover letters for single patches, if there is anything
> that needs saying put it in the changelog of the patch or after the ---
> if it's administrative stuff.  This reduces mail volume and ensures that 
> any important information is recorded in the changelog rather than being
> lost. 

All right, sorry for that. Should I resend this patch without cover letter?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/1] ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 149 bytes --]

On Wed, May 29, 2024 at 04:19:53PM +0200, Matteo Martelli wrote:

> All right, sorry for that. Should I resend this patch without cover letter?

No.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/1] ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2741 bytes --]

Hi,

On Wed, May 29, 2024 at 04:00:15PM GMT, Matteo Martelli wrote:
> This fixes the LRCLK polarity for sun8i-h3 and sun50i-h6 in i2s mode
> which was wrongly inverted.
> 
> The LRCLK was being set in reversed logic compared to the DAI format:
> inverted LRCLK for SND_SOC_DAIFMT_IB_NF and SND_SOC_DAIFMT_NB_NF; normal
> LRCLK for SND_SOC_DAIFMT_IB_IF and SND_SOC_DAIFMT_NB_IF. Such reversed
> logic applies properly for DSP_A, DSP_B, LEFT_J and RIGHT_J modes but
> not for I2S mode, for which the LRCLK signal results reversed to what
> expected on the bus. The issue is due to a misinterpretation of the
> LRCLK polarity bit of the H3 and H6 i2s controllers. Such bit in this
> case does not mean "0 => normal" or "1 => inverted" according to the
> expected bus operation, but it means "0 => frame starts on low edge" and
> "1 => frame starts on high edge" (from the User Manuals).
> 
> This commit fixes the LRCLK polarity by setting the LRCLK polarity bit
> according to the selected bus mode and renames the LRCLK polarity bit
> definition to avoid further confusion.
> 
> Fixes: dd657eae8164 ("ASoC: sun4i-i2s: Fix the LRCK polarity")
> Fixes: 73adf87b7a58 ("ASoC: sun4i-i2s: Add support for H6 I2S")
> Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
> ---
>  sound/soc/sunxi/sun4i-i2s.c | 143 ++++++++++++++++++------------------
>  1 file changed, 73 insertions(+), 70 deletions(-)
> 
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index 5f8d979585b6..a200ba41679a 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -100,8 +100,8 @@
>  #define SUN8I_I2S_CTRL_MODE_PCM			(0 << 4)
>  
>  #define SUN8I_I2S_FMT0_LRCLK_POLARITY_MASK	BIT(19)
> -#define SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED		(1 << 19)
> -#define SUN8I_I2S_FMT0_LRCLK_POLARITY_NORMAL		(0 << 19)
> +#define SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH	(1 << 19)
> +#define SUN8I_I2S_FMT0_LRCLK_POLARITY_START_LOW		(0 << 19)
>  #define SUN8I_I2S_FMT0_LRCK_PERIOD_MASK		GENMASK(17, 8)
>  #define SUN8I_I2S_FMT0_LRCK_PERIOD(period)	((period - 1) << 8)
>  #define SUN8I_I2S_FMT0_BCLK_POLARITY_MASK	BIT(7)
> @@ -729,65 +729,37 @@ static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
>  static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
>  				 unsigned int fmt)
>  {
> -	u32 mode, val;
> +	u32 mode, lrclk_pol, bclk_pol, val;
>  	u8 offset;
>  
> -	/*
> -	 * DAI clock polarity
> -	 *
> -	 * The setup for LRCK contradicts the datasheet, but under a
> -	 * scope it's clear that the LRCK polarity is reversed
> -	 * compared to the expected polarity on the bus.
> -	 */

I think we should keep that comment somewhere.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/1] ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

[Matteo Martelli]

Maxime Ripard wrote:
> > -	/*
> > -	 * DAI clock polarity
> > -	 *
> > -	 * The setup for LRCK contradicts the datasheet, but under a
> > -	 * scope it's clear that the LRCK polarity is reversed
> > -	 * compared to the expected polarity on the bus.
> > -	 */
> 
> I think we should keep that comment somewhere.

I think that keeping that comment would be very misleading since the LRCLK
setup would not contradict the datasheet anymore [1][2].

Also, do you recall any details about the mentioned scope test setup? Was i2s
mode tested in that occasion? It would help clarify the situation.

Could anyone verify this patch against H3/H6 SoCs?

[1]: https://linux-sunxi.org/images/4/4b/Allwinner_H3_Datasheet_V1.2.pdf
section 8.6.7.2
[2]: https://linux-sunxi.org/images/4/46/Allwinner_H6_V200_User_Manual_V1.1.pdf
section 7.2.5.2

Thanks,
Matteo Martelli

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/1] ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]

On Fri, Jun 07, 2024 at 10:04:43AM +0200, Matteo Martelli wrote:
> Maxime Ripard wrote:
> > > -	/*
> > > -	 * DAI clock polarity
> > > -	 *
> > > -	 * The setup for LRCK contradicts the datasheet, but under a
> > > -	 * scope it's clear that the LRCK polarity is reversed
> > > -	 * compared to the expected polarity on the bus.
> > > -	 */
> > 
> > I think we should keep that comment somewhere.
> 
> I think that keeping that comment would be very misleading since the LRCLK
> setup would not contradict the datasheet anymore [1][2].
> 
> Also, do you recall any details about the mentioned scope test setup? Was i2s
> mode tested in that occasion? It would help clarify the situation.
> 
> Could anyone verify this patch against H3/H6 SoCs?

Any news on any of this?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/1] ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

[Matteo Martelli]

Mark Brown wrote:
> On Fri, Jun 07, 2024 at 10:04:43AM +0200, Matteo Martelli wrote:
> > Maxime Ripard wrote:
> > > > -	/*
> > > > -	 * DAI clock polarity
> > > > -	 *
> > > > -	 * The setup for LRCK contradicts the datasheet, but under a
> > > > -	 * scope it's clear that the LRCK polarity is reversed
> > > > -	 * compared to the expected polarity on the bus.
> > > > -	 */
> > > 
> > > I think we should keep that comment somewhere.
> > 
> > I think that keeping that comment would be very misleading since the LRCLK
> > setup would not contradict the datasheet anymore [1][2].
> > 
> > Also, do you recall any details about the mentioned scope test setup? Was i2s
> > mode tested in that occasion? It would help clarify the situation.
> > 
> > Could anyone verify this patch against H3/H6 SoCs?
> 
> Any news on any of this?

Not on my side, unfortunately I cannot test the fix against H3/H6 SoCs.

Matteo Martelli

Re: [PATCH 1/1] ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

Hi,

On Fri, Jun 07, 2024 at 10:04:43AM GMT, Matteo Martelli wrote:
> Maxime Ripard wrote:
> > > -	/*
> > > -	 * DAI clock polarity
> > > -	 *
> > > -	 * The setup for LRCK contradicts the datasheet, but under a
> > > -	 * scope it's clear that the LRCK polarity is reversed
> > > -	 * compared to the expected polarity on the bus.
> > > -	 */
> > 
> > I think we should keep that comment somewhere.
> 
> I think that keeping that comment would be very misleading since the LRCLK
> setup would not contradict the datasheet anymore [1][2].
>
> Also, do you recall any details about the mentioned scope test setup? Was i2s
> mode tested in that occasion? It would help clarify the situation.

I can't remember if I tested i2s, I think I did though. But most of the
work was done on either TDM or DSP modes, and I remember very clearly
that the LRCK polarity was inverted compared to what Allwinner documents.

So the doc was, at best, misleading for these formats and we should keep
the comments.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/1] ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

[Matteo Martelli]

Maxime Ripard wrote:
> Hi,
> 
> On Fri, Jun 07, 2024 at 10:04:43AM GMT, Matteo Martelli wrote:
> > Maxime Ripard wrote:
> > > > -	/*
> > > > -	 * DAI clock polarity
> > > > -	 *
> > > > -	 * The setup for LRCK contradicts the datasheet, but under a
> > > > -	 * scope it's clear that the LRCK polarity is reversed
> > > > -	 * compared to the expected polarity on the bus.
> > > > -	 */
> > > 
> > > I think we should keep that comment somewhere.
> > 
> > I think that keeping that comment would be very misleading since the LRCLK
> > setup would not contradict the datasheet anymore [1][2].
> >
> > Also, do you recall any details about the mentioned scope test setup? Was i2s
> > mode tested in that occasion? It would help clarify the situation.
> 
> I can't remember if I tested i2s, I think I did though. But most of the
> work was done on either TDM or DSP modes, and I remember very clearly
> that the LRCK polarity was inverted compared to what Allwinner documents.
> 
> So the doc was, at best, misleading for these formats and we should keep
> the comments.
> 
> Maxime

Thanks for the reply Maxime, would you be able to point out the Allwinner
document part that is (or was) misleading? The current datasheets (see links
[1][2]) look correct, the current driver setup for TDM and DSP modes respects
those datasheets and it's not "reversed compared to the expected polarity on
the bus" as the comment states. Also I didn't find any related errata in their
changelog. Could it be possible that during those mentioned tests you were
still referring to the datasheets of other SoCs like A10 for instance? Or maybe
that the misleading information was in another document rather than the main
datasheets? If that's the case, would you still think that the comment should be
kept as it is?

[1]: https://linux-sunxi.org/images/4/4b/Allwinner_H3_Datasheet_V1.2.pdf
section 8.6.7.2
[2]: https://linux-sunxi.org/images/4/46/Allwinner_H6_V200_User_Manual_V1.1.pdf
section 7.2.5.2

Thanks,
Matteo

Re: [PATCH 1/1] ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2318 bytes --]

On Tue, Jul 02, 2024 at 05:17:15PM GMT, Matteo Martelli wrote:
> Maxime Ripard wrote:
> > On Fri, Jun 07, 2024 at 10:04:43AM GMT, Matteo Martelli wrote:
> > > Maxime Ripard wrote:
> > > > > -	/*
> > > > > -	 * DAI clock polarity
> > > > > -	 *
> > > > > -	 * The setup for LRCK contradicts the datasheet, but under a
> > > > > -	 * scope it's clear that the LRCK polarity is reversed
> > > > > -	 * compared to the expected polarity on the bus.
> > > > > -	 */
> > > > 
> > > > I think we should keep that comment somewhere.
> > > 
> > > I think that keeping that comment would be very misleading since the LRCLK
> > > setup would not contradict the datasheet anymore [1][2].
> > >
> > > Also, do you recall any details about the mentioned scope test setup? Was i2s
> > > mode tested in that occasion? It would help clarify the situation.
> > 
> > I can't remember if I tested i2s, I think I did though. But most of the
> > work was done on either TDM or DSP modes, and I remember very clearly
> > that the LRCK polarity was inverted compared to what Allwinner documents.
> > 
> > So the doc was, at best, misleading for these formats and we should keep
> > the comments.
> 
> Thanks for the reply Maxime, would you be able to point out the Allwinner
> document part that is (or was) misleading? The current datasheets (see links
> [1][2]) look correct, the current driver setup for TDM and DSP modes respects
> those datasheets and it's not "reversed compared to the expected polarity on
> the bus" as the comment states. 

I clearly remember having to debug something there, but I don't remember
much more, sorry.

I guess if you have tested on the H3 I2S, TDM and DSP and it all works
as expected with your changes, go ahead and ignore my comment then.

> Also I didn't find any related errata in their changelog. 

Yeah... Allwinner doesn't do errata.

> Could it be possible that during those mentioned tests you
> were still referring to the datasheets of other SoCs like A10 for
> instance? Or maybe that the misleading information was in another
> document rather than the main datasheets? If that's the case, would
> you still think that the comment should be kept as it is?

Possibly, or an older version of the datasheet, I really can't remember.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 1/1] ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode

[Matteo Martelli]

Maxime Ripard wrote:
> On Tue, Jul 02, 2024 at 05:17:15PM GMT, Matteo Martelli wrote:
> > Maxime Ripard wrote:
> > > On Fri, Jun 07, 2024 at 10:04:43AM GMT, Matteo Martelli wrote:
> > > > Maxime Ripard wrote:
> > > > > > -	/*
> > > > > > -	 * DAI clock polarity
> > > > > > -	 *
> > > > > > -	 * The setup for LRCK contradicts the datasheet, but under a
> > > > > > -	 * scope it's clear that the LRCK polarity is reversed
> > > > > > -	 * compared to the expected polarity on the bus.
> > > > > > -	 */
> > > > > 
> > > > > I think we should keep that comment somewhere.
> > > > 
> > > > I think that keeping that comment would be very misleading since the LRCLK
> > > > setup would not contradict the datasheet anymore [1][2].
> > > >
> > > > Also, do you recall any details about the mentioned scope test setup? Was i2s
> > > > mode tested in that occasion? It would help clarify the situation.
> > > 
> > > I can't remember if I tested i2s, I think I did though. But most of the
> > > work was done on either TDM or DSP modes, and I remember very clearly
> > > that the LRCK polarity was inverted compared to what Allwinner documents.
> > > 
> > > So the doc was, at best, misleading for these formats and we should keep
> > > the comments.
> > 
> > Thanks for the reply Maxime, would you be able to point out the Allwinner
> > document part that is (or was) misleading? The current datasheets (see links
> > [1][2]) look correct, the current driver setup for TDM and DSP modes respects
> > those datasheets and it's not "reversed compared to the expected polarity on
> > the bus" as the comment states. 
> 
> I clearly remember having to debug something there, but I don't remember
> much more, sorry.
> 
> I guess if you have tested on the H3 I2S, TDM and DSP and it all works
> as expected with your changes, go ahead and ignore my comment then.

I did test it on the A64 SoC only (all modes I2S, TDM and DSP working).

Best reguards,
Matteo