[PATCH 0/3] ASoC: sun8i-codec: A33 codec fixes

[PATCH 0/3] ASoC: sun8i-codec: A33 codec fixes

[Maxime Ripard]

Hi,

The commit 043b8daa578f ("ASoC: sun4i-i2s: Update global enable with
bitmask") has broken the A33 codec since it's been merged as part as
4.14-rc1.

The reason has been that while that commit was obviously right, one of
its side effect was to clear the bit that would set the I2S controller
in slave mode, effectively forcing it to act as master.

This was actually working by accident since the codec also had its
condition on whether to act as a master or a slave backward, meaning
that even though we were setting the i2s controller as slave and the
codec as master, we were ending up with the exact opposite.

The commit mentionned above (rightfully) broke that combination, and
we ended up with the two devices in slave mode, which obviously didn't
work.

The way to fix that is in two parts. The first one is obviously to
make the codec act as its proper role. However, that's not sufficient
because some logic was missing in the initial driver that was merged
to act as such. Indeed the BCLK divider was never programmed, meaning
that we would generate a BCLK running at the PLL rate, which is way
too fast. The second patch addresses that.

The final fix is here to address an issue that has been there for
quite some time too and would invert the two channels. It appears that
the codec and I2S drivers don't share the same polarity for the LRCK
signal. The fix is quite obvious and is to invert whatever value has
been programmed in ALSA.

All these commits should go as fix in 4.14 if possible, as without
them the audio is completely broken.

Let me know what you think,
Maxime

Maxime Ripard (3):
  ASoC: sun8i-codec: Invert Master / Slave condition
  ASoC: sun8i-codec: Set the BCLK divider
  ASoC: sun8i-codec: Fix left and right channels inversion

 sound/soc/sunxi/sun8i-codec.c | 63 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 5 deletions(-)

-- 
2.14.3

[PATCH 1/3] ASoC: sun8i-codec: Invert Master / Slave condition

[Maxime Ripard]

The current code had the condition backward when checking if the codec
should be running in slave or master mode.

Fix it, and make the comment a bit more readable.

Fixes: 36c684936fae ("ASoC: Add sun8i digital audio codec")
Cc: <stable@vger.kernel.org>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 sound/soc/sunxi/sun8i-codec.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
index abfb710df7cb..038107baf414 100644
--- a/sound/soc/sunxi/sun8i-codec.c
+++ b/sound/soc/sunxi/sun8i-codec.c
@@ -170,11 +170,11 @@ static int sun8i_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 
 	/* clock masters */
 	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
-	case SND_SOC_DAIFMT_CBS_CFS: /* DAI Slave */
-		value = 0x0; /* Codec Master */
+	case SND_SOC_DAIFMT_CBS_CFS: /* Codec slave, DAI master */
+		value = 0x1;
 		break;
-	case SND_SOC_DAIFMT_CBM_CFM: /* DAI Master */
-		value = 0x1; /* Codec Slave */
+	case SND_SOC_DAIFMT_CBM_CFM: /* Codec Master, DAI slave */
+		value = 0x0;
 		break;
 	default:
 		return -EINVAL;
-- 
2.14.3

[PATCH 2/3] ASoC: sun8i-codec: Set the BCLK divider

[Maxime Ripard]

While the current code was reporting to be able to work in master mode, it
failed to do so because the BCLK divider wasn't programmed, meaning that
the BCLK would run at the PLL's frequency no matter the sample rate.

It was obviously a bit too fast.

Add support to retrieve the divider to use, and set it. Since our PLL is
not always able to generate a perfect multiple of the sample rate, we'll
have to choose the closest divider that matches our setup.

Fixes: 36c684936fae ("ASoC: Add sun8i digital audio codec")
Cc: <stable@vger.kernel.org>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 sound/soc/sunxi/sun8i-codec.c | 53 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
index 038107baf414..522546e6b153 100644
--- a/sound/soc/sunxi/sun8i-codec.c
+++ b/sound/soc/sunxi/sun8i-codec.c
@@ -73,6 +73,7 @@
 #define SUN8I_SYS_SR_CTRL_AIF2_FS_MASK		GENMASK(11, 8)
 #define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_MASK	GENMASK(5, 4)
 #define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK	GENMASK(8, 6)
+#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK	GENMASK(12, 9)
 
 struct sun8i_codec {
 	struct device	*dev;
@@ -226,12 +227,57 @@ static int sun8i_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 	return 0;
 }
 
+struct sun8i_codec_clk_div {
+	u8	div;
+	u8	val;
+};
+
+static const struct sun8i_codec_clk_div sun8i_codec_bclk_div[] = {
+	{ .div = 1,	.val = 0 },
+	{ .div = 2,	.val = 1 },
+	{ .div = 4,	.val = 2 },
+	{ .div = 6,	.val = 3 },
+	{ .div = 8,	.val = 4 },
+	{ .div = 12,	.val = 5 },
+	{ .div = 16,	.val = 6 },
+	{ .div = 24,	.val = 7 },
+	{ .div = 32,	.val = 8 },
+	{ .div = 48,	.val = 9 },
+	{ .div = 64,	.val = 10 },
+	{ .div = 96,	.val = 11 },
+	{ .div = 128,	.val = 12 },
+	{ .div = 192,	.val = 13 },
+};
+
+static u8 sun8i_codec_get_bclk_div(struct sun8i_codec *scodec,
+				   unsigned int rate,
+				   unsigned int word_size)
+{
+	unsigned long clk_rate = clk_get_rate(scodec->clk_module);
+	unsigned int div = clk_rate / rate / word_size / 2;
+	unsigned int best_val = 0, best_diff = ~0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(sun8i_codec_bclk_div); i++) {
+		const struct sun8i_codec_clk_div *bdiv = &sun8i_codec_bclk_div[i];
+		unsigned int diff = abs(bdiv->div - div);
+
+		if (diff < best_diff) {
+			best_diff = diff;
+			best_val = bdiv->val;
+		}
+	}
+
+	return best_val;
+}
+
 static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
 				 struct snd_pcm_hw_params *params,
 				 struct snd_soc_dai *dai)
 {
 	struct sun8i_codec *scodec = snd_soc_codec_get_drvdata(dai->codec);
 	int sample_rate;
+	u8 bclk_div;
 
 	/*
 	 * The CPU DAI handles only a sample of 16 bits. Configure the
@@ -241,6 +287,13 @@ static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
 			   SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_MASK,
 			   SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_16);
 
+	bclk_div = sun8i_codec_get_bclk_div(scodec, params_rate(params),
+					    params_width(params));
+
+	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
+			   SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK,
+			   bclk_div << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV);
+
 	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
 			   SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK,
 			   SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_16);
-- 
2.14.3

[PATCH 3/3] ASoC: sun8i-codec: Fix left and right channels inversion

[Maxime Ripard]

Since its introduction, the codec had an inversion of the left and right
channels. It turned out to be pretty simple as it appears that the codec
doesn't have the same polarity on the LRCK signal than the I2S block.

Fix this by inverting our bit value for the LRCK inversion.

Fixes: 36c684936fae ("ASoC: Add sun8i digital audio codec")
Cc: <stable@vger.kernel.org>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 sound/soc/sunxi/sun8i-codec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
index 522546e6b153..c8dcb1502d74 100644
--- a/sound/soc/sunxi/sun8i-codec.c
+++ b/sound/soc/sunxi/sun8i-codec.c
@@ -200,7 +200,7 @@ static int sun8i_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 			   value << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_INV);
 	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
 			   BIT(SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV),
-			   value << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV);
+			   !value << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV);
 
 	/* DAI format */
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
-- 
2.14.3

[PATCH 2/3] ASoC: sun8i-codec: Set the BCLK divider

[Chen-Yu Tsai]

On Wed, Nov 8, 2017 at 11:47 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> While the current code was reporting to be able to work in master mode, it
> failed to do so because the BCLK divider wasn't programmed, meaning that
> the BCLK would run at the PLL's frequency no matter the sample rate.
>
> It was obviously a bit too fast.
>
> Add support to retrieve the divider to use, and set it. Since our PLL is
> not always able to generate a perfect multiple of the sample rate, we'll
> have to choose the closest divider that matches our setup.
>
> Fixes: 36c684936fae ("ASoC: Add sun8i digital audio codec")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  sound/soc/sunxi/sun8i-codec.c | 53 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
> index 038107baf414..522546e6b153 100644
> --- a/sound/soc/sunxi/sun8i-codec.c
> +++ b/sound/soc/sunxi/sun8i-codec.c
> @@ -73,6 +73,7 @@
>  #define SUN8I_SYS_SR_CTRL_AIF2_FS_MASK         GENMASK(11, 8)
>  #define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_MASK  GENMASK(5, 4)
>  #define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK  GENMASK(8, 6)
> +#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK  GENMASK(12, 9)
>
>  struct sun8i_codec {
>         struct device   *dev;
> @@ -226,12 +227,57 @@ static int sun8i_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>         return 0;
>  }
>
> +struct sun8i_codec_clk_div {
> +       u8      div;
> +       u8      val;
> +};
> +
> +static const struct sun8i_codec_clk_div sun8i_codec_bclk_div[] = {
> +       { .div = 1,     .val = 0 },
> +       { .div = 2,     .val = 1 },
> +       { .div = 4,     .val = 2 },
> +       { .div = 6,     .val = 3 },
> +       { .div = 8,     .val = 4 },
> +       { .div = 12,    .val = 5 },
> +       { .div = 16,    .val = 6 },
> +       { .div = 24,    .val = 7 },
> +       { .div = 32,    .val = 8 },
> +       { .div = 48,    .val = 9 },
> +       { .div = 64,    .val = 10 },
> +       { .div = 96,    .val = 11 },
> +       { .div = 128,   .val = 12 },
> +       { .div = 192,   .val = 13 },
> +};
> +
> +static u8 sun8i_codec_get_bclk_div(struct sun8i_codec *scodec,
> +                                  unsigned int rate,
> +                                  unsigned int word_size)
> +{
> +       unsigned long clk_rate = clk_get_rate(scodec->clk_module);
> +       unsigned int div = clk_rate / rate / word_size / 2;
> +       unsigned int best_val = 0, best_diff = ~0;
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(sun8i_codec_bclk_div); i++) {
> +               const struct sun8i_codec_clk_div *bdiv = &sun8i_codec_bclk_div[i];
> +               unsigned int diff = abs(bdiv->div - div);
> +
> +               if (diff < best_diff) {
> +                       best_diff = diff;
> +                       best_val = bdiv->val;
> +               }
> +       }
> +
> +       return best_val;
> +}
> +
>  static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
>                                  struct snd_pcm_hw_params *params,
>                                  struct snd_soc_dai *dai)
>  {
>         struct sun8i_codec *scodec = snd_soc_codec_get_drvdata(dai->codec);
>         int sample_rate;
> +       u8 bclk_div;
>
>         /*
>          * The CPU DAI handles only a sample of 16 bits. Configure the
> @@ -241,6 +287,13 @@ static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
>                            SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_MASK,
>                            SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_16);

This looks like it's using 16 bit words regardless of the settings in params.

>
> +       bclk_div = sun8i_codec_get_bclk_div(scodec, params_rate(params),
> +                                           params_width(params));

But here you pass params_width(params). Seems a bit error prone.

Otherwise,

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

> +
> +       regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
> +                          SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK,
> +                          bclk_div << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV);
> +
>         regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
>                            SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK,
>                            SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_16);
> --
> 2.14.3
>

[PATCH 1/3] ASoC: sun8i-codec: Invert Master / Slave condition

[Chen-Yu Tsai]

On Wed, Nov 8, 2017 at 11:47 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> The current code had the condition backward when checking if the codec
> should be running in slave or master mode.
>
> Fix it, and make the comment a bit more readable.
>
> Fixes: 36c684936fae ("ASoC: Add sun8i digital audio codec")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

[PATCH 3/3] ASoC: sun8i-codec: Fix left and right channels inversion

[Chen-Yu Tsai]

On Wed, Nov 8, 2017 at 11:47 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Since its introduction, the codec had an inversion of the left and right
> channels. It turned out to be pretty simple as it appears that the codec
> doesn't have the same polarity on the LRCK signal than the I2S block.
>
> Fix this by inverting our bit value for the LRCK inversion.
>
> Fixes: 36c684936fae ("ASoC: Add sun8i digital audio codec")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

I suggest adding a comment in case anyone stumbles across it.

Otherwise,

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

> ---
>  sound/soc/sunxi/sun8i-codec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
> index 522546e6b153..c8dcb1502d74 100644
> --- a/sound/soc/sunxi/sun8i-codec.c
> +++ b/sound/soc/sunxi/sun8i-codec.c
> @@ -200,7 +200,7 @@ static int sun8i_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>                            value << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_INV);
>         regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
>                            BIT(SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV),
> -                          value << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV);
> +                          !value << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV);
>
>         /* DAI format */
>         switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> --
> 2.14.3
>

Applied "ASoC: sun8i-codec: Invert Master / Slave condition" to the asoc tree

[Mark Brown]

The patch

   ASoC: sun8i-codec: Invert Master / Slave condition

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 560bfe774f058e97596f30ff71cffdac52b72914 Mon Sep 17 00:00:00 2001
From: Maxime Ripard <maxime.ripard@free-electrons.com>
Date: Wed, 8 Nov 2017 16:47:08 +0100
Subject: [PATCH] ASoC: sun8i-codec: Invert Master / Slave condition

The current code had the condition backward when checking if the codec
should be running in slave or master mode.

Fix it, and make the comment a bit more readable.

Fixes: 36c684936fae ("ASoC: Add sun8i digital audio codec")
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Reviewed-by: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: <stable@vger.kernel.org>
---
 sound/soc/sunxi/sun8i-codec.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
index abfb710df7cb..038107baf414 100644
--- a/sound/soc/sunxi/sun8i-codec.c
+++ b/sound/soc/sunxi/sun8i-codec.c
@@ -170,11 +170,11 @@ static int sun8i_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 
 	/* clock masters */
 	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
-	case SND_SOC_DAIFMT_CBS_CFS: /* DAI Slave */
-		value = 0x0; /* Codec Master */
+	case SND_SOC_DAIFMT_CBS_CFS: /* Codec slave, DAI master */
+		value = 0x1;
 		break;
-	case SND_SOC_DAIFMT_CBM_CFM: /* DAI Master */
-		value = 0x1; /* Codec Slave */
+	case SND_SOC_DAIFMT_CBM_CFM: /* Codec Master, DAI slave */
+		value = 0x0;
 		break;
 	default:
 		return -EINVAL;
-- 
2.15.0

Applied "ASoC: sun8i-codec: Fix left and right channels inversion" to the asoc tree

[Mark Brown]

The patch

   ASoC: sun8i-codec: Fix left and right channels inversion

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 18c1bf35c1c09bca05cf70bc984a4764e0b0372b Mon Sep 17 00:00:00 2001
From: Maxime Ripard <maxime.ripard@free-electrons.com>
Date: Wed, 8 Nov 2017 16:47:10 +0100
Subject: [PATCH] ASoC: sun8i-codec: Fix left and right channels inversion

Since its introduction, the codec had an inversion of the left and right
channels. It turned out to be pretty simple as it appears that the codec
doesn't have the same polarity on the LRCK signal than the I2S block.

Fix this by inverting our bit value for the LRCK inversion.

Fixes: 36c684936fae ("ASoC: Add sun8i digital audio codec")
Cc: <stable@vger.kernel.org>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Reviewed-by: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/sunxi/sun8i-codec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
index 038107baf414..c2ceca485d6a 100644
--- a/sound/soc/sunxi/sun8i-codec.c
+++ b/sound/soc/sunxi/sun8i-codec.c
@@ -199,7 +199,7 @@ static int sun8i_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 			   value << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_INV);
 	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
 			   BIT(SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV),
-			   value << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV);
+			   !value << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV);
 
 	/* DAI format */
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
-- 
2.15.0