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

[Maxime Ripard <mripard@xxxxxxxxxx>]

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@xxxxxxxxx>
> ---
>  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: signature.asc
Description: PGP signature