[PATCH 0/2] Add monaural audio support for fsl_ssi.c

[PATCH 0/2] Add monaural audio support for fsl_ssi.c

[Nicolin Chen]

This series of patches need to be applied into one single tree because
the second patch depends on the first one. Without it, SSI would playback
constant noise to the right channel when playback monaural audio files on
i.MX6 Series board.

We might also need to apply the iomux change to the other i.MX platforms,
just currently I don't have those boards so I drop their changes for now.

Nicolin Chen (2):
  ARM: dts: imx: specify the value of audmux pinctrl instead of
    0x80000000
  ASoC: fsl_ssi: Add monaural audio support for non-ac97 interface

 arch/arm/boot/dts/imx6qdl.dtsi | 22 +++++++++++-----------
 sound/soc/fsl/fsl_ssi.c        | 22 +++++++++++++++++++---
 2 files changed, 30 insertions(+), 14 deletions(-)

-- 
1.8.4

[PATCH 1/2] ARM: dts: imx: specify the value of audmux pinctrl instead of 0x80000000

[Nicolin Chen]

We must specify the value of audmux pinctrl if we want to use pinctrl_pm().
Thus change bypass value 0x80000000 to what we exactly need.

This patch also seperately unset PUE bit for TXD so that IOMUX won't pull
up/down the pin after turning into tristate. When we use SSI normal mode to
playback monaural audio via I2S signal, there'd be a pulled curve occur to
its signal at the second slot if setting PUE bit for TXD. And it will make
the second channel to play a constant noise. So by keeping the signal level
in the second slot, we can get a constant high level signal (-1) or a low
level one (0).

Signed-off-by: Nicolin Chen <b42378@freescale.com>
---
 arch/arm/boot/dts/imx6qdl.dtsi | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 6e096ca..6b76e55 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -601,27 +601,27 @@
 				audmux {
 					pinctrl_audmux_1: audmux-1 {
 						fsl,pins = <
-							MX6QDL_PAD_SD2_DAT0__AUD4_RXD  0x80000000
-							MX6QDL_PAD_SD2_DAT3__AUD4_TXC  0x80000000
-							MX6QDL_PAD_SD2_DAT2__AUD4_TXD  0x80000000
-							MX6QDL_PAD_SD2_DAT1__AUD4_TXFS 0x80000000
+							MX6QDL_PAD_SD2_DAT0__AUD4_RXD  0x130b0
+							MX6QDL_PAD_SD2_DAT3__AUD4_TXC  0x130b0
+							MX6QDL_PAD_SD2_DAT2__AUD4_TXD  0x110b0
+							MX6QDL_PAD_SD2_DAT1__AUD4_TXFS 0x130b0
 						>;
 					};
 
 					pinctrl_audmux_2: audmux-2 {
 						fsl,pins = <
-							MX6QDL_PAD_CSI0_DAT7__AUD3_RXD  0x80000000
-							MX6QDL_PAD_CSI0_DAT4__AUD3_TXC  0x80000000
-							MX6QDL_PAD_CSI0_DAT5__AUD3_TXD  0x80000000
-							MX6QDL_PAD_CSI0_DAT6__AUD3_TXFS 0x80000000
+							MX6QDL_PAD_CSI0_DAT7__AUD3_RXD  0x130b0
+							MX6QDL_PAD_CSI0_DAT4__AUD3_TXC  0x130b0
+							MX6QDL_PAD_CSI0_DAT5__AUD3_TXD  0x110b0
+							MX6QDL_PAD_CSI0_DAT6__AUD3_TXFS 0x130b0
 						>;
 					};
 
 					pinctrl_audmux_3: audmux-3 {
 						fsl,pins = <
-							MX6QDL_PAD_DISP0_DAT16__AUD5_TXC  0x80000000
-							MX6QDL_PAD_DISP0_DAT18__AUD5_TXFS 0x80000000
-							MX6QDL_PAD_DISP0_DAT19__AUD5_RXD  0x80000000
+							MX6QDL_PAD_DISP0_DAT16__AUD5_TXC  0x130b0
+							MX6QDL_PAD_DISP0_DAT18__AUD5_TXFS 0x130b0
+							MX6QDL_PAD_DISP0_DAT19__AUD5_RXD  0x130b0
 						>;
 					};
 				};
-- 
1.8.4

[PATCH 1/2] ARM: dts: imx: specify the value of audmux pinctrl instead of 0x80000000

[Shawn Guo]

On Thu, Nov 14, 2013 at 07:07:09PM +0800, Nicolin Chen wrote:
> We must specify the value of audmux pinctrl if we want to use pinctrl_pm().
> Thus change bypass value 0x80000000 to what we exactly need.
> 
> This patch also seperately unset PUE bit for TXD so that IOMUX won't pull
> up/down the pin after turning into tristate. When we use SSI normal mode to
> playback monaural audio via I2S signal, there'd be a pulled curve occur to
> its signal at the second slot if setting PUE bit for TXD. And it will make
> the second channel to play a constant noise. So by keeping the signal level
> in the second slot, we can get a constant high level signal (-1) or a low
> level one (0).
> 
> Signed-off-by: Nicolin Chen <b42378@freescale.com>
> ---
>  arch/arm/boot/dts/imx6qdl.dtsi | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)

We have moved all pin groups settings into
arch/arm/boot/dts/imx6qdl-pingrp.h.  I just rebased and applied the
patch.  Please check my imx/dt branch and ensure I applied the changes
correctly.

Shawn

> 
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index 6e096ca..6b76e55 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -601,27 +601,27 @@
>  				audmux {
>  					pinctrl_audmux_1: audmux-1 {
>  						fsl,pins = <
> -							MX6QDL_PAD_SD2_DAT0__AUD4_RXD  0x80000000
> -							MX6QDL_PAD_SD2_DAT3__AUD4_TXC  0x80000000
> -							MX6QDL_PAD_SD2_DAT2__AUD4_TXD  0x80000000
> -							MX6QDL_PAD_SD2_DAT1__AUD4_TXFS 0x80000000
> +							MX6QDL_PAD_SD2_DAT0__AUD4_RXD  0x130b0
> +							MX6QDL_PAD_SD2_DAT3__AUD4_TXC  0x130b0
> +							MX6QDL_PAD_SD2_DAT2__AUD4_TXD  0x110b0
> +							MX6QDL_PAD_SD2_DAT1__AUD4_TXFS 0x130b0
>  						>;
>  					};
>  
>  					pinctrl_audmux_2: audmux-2 {
>  						fsl,pins = <
> -							MX6QDL_PAD_CSI0_DAT7__AUD3_RXD  0x80000000
> -							MX6QDL_PAD_CSI0_DAT4__AUD3_TXC  0x80000000
> -							MX6QDL_PAD_CSI0_DAT5__AUD3_TXD  0x80000000
> -							MX6QDL_PAD_CSI0_DAT6__AUD3_TXFS 0x80000000
> +							MX6QDL_PAD_CSI0_DAT7__AUD3_RXD  0x130b0
> +							MX6QDL_PAD_CSI0_DAT4__AUD3_TXC  0x130b0
> +							MX6QDL_PAD_CSI0_DAT5__AUD3_TXD  0x110b0
> +							MX6QDL_PAD_CSI0_DAT6__AUD3_TXFS 0x130b0
>  						>;
>  					};
>  
>  					pinctrl_audmux_3: audmux-3 {
>  						fsl,pins = <
> -							MX6QDL_PAD_DISP0_DAT16__AUD5_TXC  0x80000000
> -							MX6QDL_PAD_DISP0_DAT18__AUD5_TXFS 0x80000000
> -							MX6QDL_PAD_DISP0_DAT19__AUD5_RXD  0x80000000
> +							MX6QDL_PAD_DISP0_DAT16__AUD5_TXC  0x130b0
> +							MX6QDL_PAD_DISP0_DAT18__AUD5_TXFS 0x130b0
> +							MX6QDL_PAD_DISP0_DAT19__AUD5_RXD  0x130b0
>  						>;
>  					};
>  				};
> -- 
> 1.8.4
> 
> 

[PATCH 1/2] ARM: dts: imx: specify the value of audmux pinctrl instead of 0x80000000

[Nicolin Chen]

On Fri, Nov 15, 2013 at 02:42:01PM +0800, Shawn Guo wrote:
> On Thu, Nov 14, 2013 at 07:07:09PM +0800, Nicolin Chen wrote:
> > We must specify the value of audmux pinctrl if we want to use pinctrl_pm().
> > Thus change bypass value 0x80000000 to what we exactly need.
> > 
> > This patch also seperately unset PUE bit for TXD so that IOMUX won't pull
> > up/down the pin after turning into tristate. When we use SSI normal mode to
> > playback monaural audio via I2S signal, there'd be a pulled curve occur to
> > its signal at the second slot if setting PUE bit for TXD. And it will make
> > the second channel to play a constant noise. So by keeping the signal level
> > in the second slot, we can get a constant high level signal (-1) or a low
> > level one (0).
> > 
> > Signed-off-by: Nicolin Chen <b42378@freescale.com>
> > ---
> >  arch/arm/boot/dts/imx6qdl.dtsi | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> We have moved all pin groups settings into
> arch/arm/boot/dts/imx6qdl-pingrp.h.  I just rebased and applied the
> patch.  Please check my imx/dt branch and ensure I applied the changes
> correctly.

Simply perfect. Thank you.
Nicolin

---

[PATCH 2/2] ASoC: fsl_ssi: Add monaural audio support for non-ac97 interface

[Nicolin Chen]

The normal mode of SSI allows it to send/receive data to/from the first
slot of each period. So we can use this normal mode to trick I2S signal
by puting/getting data to/from the first slot only (the left channel)
so as to support monaural audio playback and recording.

Signed-off-by: Nicolin Chen <b42378@freescale.com>
---
 sound/soc/fsl/fsl_ssi.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index f43be6d..ccf1d38 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -517,10 +517,12 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,
 {
 	struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(cpu_dai);
 	struct ccsr_ssi __iomem *ssi = ssi_private->ssi;
+	unsigned int channels = params_channels(hw_params);
 	unsigned int sample_size =
 		snd_pcm_format_width(params_format(hw_params));
 	u32 wl = CCSR_SSI_SxCCR_WL(sample_size);
 	int enabled = read_ssi(&ssi->scr) & CCSR_SSI_SCR_SSIEN;
+	static u8 i2s_mode;
 
 	/*
 	 * If we're in synchronous mode, and the SSI is already enabled,
@@ -546,6 +548,21 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,
 	else
 		write_ssi_mask(&ssi->srccr, CCSR_SSI_SxCCR_WL_MASK, wl);
 
+	if (ssi_private->imx_ac97)
+		return 0;
+
+	/* Save i2s mode configuration so that we can restore it later */
+	switch (read_ssi(&ssi->scr) & CCSR_SSI_SCR_I2S_MODE_MASK) {
+	case CCSR_SSI_SCR_I2S_MODE_SLAVE:
+	case CCSR_SSI_SCR_I2S_MODE_MASTER:
+		i2s_mode = read_ssi(&ssi->scr) & CCSR_SSI_SCR_I2S_MODE_MASK;
+	default:
+		break;
+	}
+
+	write_ssi_mask(&ssi->scr, CCSR_SSI_SCR_NET | CCSR_SSI_SCR_I2S_MODE_MASK,
+			channels == 1 ? 0 : CCSR_SSI_SCR_NET | i2s_mode);
+
 	return 0;
 }
 
@@ -658,14 +675,13 @@ static const struct snd_soc_dai_ops fsl_ssi_dai_ops = {
 static struct snd_soc_dai_driver fsl_ssi_dai_template = {
 	.probe = fsl_ssi_dai_probe,
 	.playback = {
-		/* The SSI does not support monaural audio. */
-		.channels_min = 2,
+		.channels_min = 1,
 		.channels_max = 2,
 		.rates = FSLSSI_I2S_RATES,
 		.formats = FSLSSI_I2S_FORMATS,
 	},
 	.capture = {
-		.channels_min = 2,
+		.channels_min = 1,
 		.channels_max = 2,
 		.rates = FSLSSI_I2S_RATES,
 		.formats = FSLSSI_I2S_FORMATS,
-- 
1.8.4

[PATCH 2/2] ASoC: fsl_ssi: Add monaural audio support for non-ac97 interface

[Russell King - ARM Linux]

On Thu, Nov 14, 2013 at 07:07:10PM +0800, Nicolin Chen wrote:
> The normal mode of SSI allows it to send/receive data to/from the first
> slot of each period. So we can use this normal mode to trick I2S signal
> by puting/getting data to/from the first slot only (the left channel)
> so as to support monaural audio playback and recording.
> 
> Signed-off-by: Nicolin Chen <b42378@freescale.com>
> ---
>  sound/soc/fsl/fsl_ssi.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index f43be6d..ccf1d38 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -517,10 +517,12 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,
>  {
>  	struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(cpu_dai);
>  	struct ccsr_ssi __iomem *ssi = ssi_private->ssi;
> +	unsigned int channels = params_channels(hw_params);
>  	unsigned int sample_size =
>  		snd_pcm_format_width(params_format(hw_params));
>  	u32 wl = CCSR_SSI_SxCCR_WL(sample_size);
>  	int enabled = read_ssi(&ssi->scr) & CCSR_SSI_SCR_SSIEN;
> +	static u8 i2s_mode;

Throwing a static variable into the middle of a driver with none is a
really horrid thing to do and is very bad programming practice.  What
if some freescale device decides to have to of these interfaces?  Both
will try to use this same static variable.

This is extremely bad programming practice.

>  
>  	/*
>  	 * If we're in synchronous mode, and the SSI is already enabled,
> @@ -546,6 +548,21 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,
>  	else
>  		write_ssi_mask(&ssi->srccr, CCSR_SSI_SxCCR_WL_MASK, wl);
>  
> +	if (ssi_private->imx_ac97)
> +		return 0;
> +
> +	/* Save i2s mode configuration so that we can restore it later */
> +	switch (read_ssi(&ssi->scr) & CCSR_SSI_SCR_I2S_MODE_MASK) {
> +	case CCSR_SSI_SCR_I2S_MODE_SLAVE:
> +	case CCSR_SSI_SCR_I2S_MODE_MASTER:
> +		i2s_mode = read_ssi(&ssi->scr) & CCSR_SSI_SCR_I2S_MODE_MASK;
> +	default:
> +		break;
> +	}

So all you're doing is saving the mode only if it specifies master or
slave mode, but not if it's normal mode (== 0).  This just looks like
it's complicated just for the sake of being complicated.

Since we know what mode this is in when we run fsl_ssi_setup(), can we
not save that value into the fsl_ssi_private structure and re-use it
here?

[PATCH 2/2] ASoC: fsl_ssi: Add monaural audio support for non-ac97 interface

[Nicolin Chen]

On Fri, Nov 15, 2013 at 12:21:08PM +0000, Russell King - ARM Linux wrote:
> > @@ -517,10 +517,12 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,
> >  {
> >  	struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(cpu_dai);
> >  	struct ccsr_ssi __iomem *ssi = ssi_private->ssi;
> > +	unsigned int channels = params_channels(hw_params);
> >  	unsigned int sample_size =
> >  		snd_pcm_format_width(params_format(hw_params));
> >  	u32 wl = CCSR_SSI_SxCCR_WL(sample_size);
> >  	int enabled = read_ssi(&ssi->scr) & CCSR_SSI_SCR_SSIEN;
> > +	static u8 i2s_mode;
> 
> Throwing a static variable into the middle of a driver with none is a
> really horrid thing to do and is very bad programming practice.  What
> if some freescale device decides to have to of these interfaces?  Both
> will try to use this same static variable.
> 
> This is extremely bad programming practice.

Sir, I'm glad you're teaching me this lesson. I did hesitate before I
sent this patch, just couldn't tell the reason. And swear to god, I
hadn't used and will never use this practice again.
 

> > +	/* Save i2s mode configuration so that we can restore it later */
> > +	switch (read_ssi(&ssi->scr) & CCSR_SSI_SCR_I2S_MODE_MASK) {
> > +	case CCSR_SSI_SCR_I2S_MODE_SLAVE:
> > +	case CCSR_SSI_SCR_I2S_MODE_MASTER:
> > +		i2s_mode = read_ssi(&ssi->scr) & CCSR_SSI_SCR_I2S_MODE_MASK;
> > +	default:
> > +		break;
> > +	}
> 
> So all you're doing is saving the mode only if it specifies master or
> slave mode, but not if it's normal mode (== 0).  This just looks like
> it's complicated just for the sake of being complicated.
> 
> Since we know what mode this is in when we run fsl_ssi_setup(), can we
> not save that value into the fsl_ssi_private structure and re-use it
> here?

Currently we only have fsl_ssi_setup() to set I2S mode. It's definitely
a good idea to save it into private structure at that point. But there
will be new ASoC function -- set_dai_fmt() appending to this driver.
It will provide ASoC machine driver an interface to set I2S mode again,
and we must put a saving code as well at that time. So I think put the
saving code here would keep the modification within a small range. But
I here might be so obsessed with trying to make the patch as neat as
possible that I picked a demon sword.

But I'm still willing to learn the lesson. And I'll refine this patch.

Much obliged,
Nicolin Chen

[PATCH 0/2] Add monaural audio support for fsl_ssi.c

[Shawn Guo]

On Thu, Nov 14, 2013 at 07:07:08PM +0800, Nicolin Chen wrote:
> This series of patches need to be applied into one single tree because
> the second patch depends on the first one. Without it, SSI would playback
> constant noise to the right channel when playback monaural audio files on
> i.MX6 Series board.

Let me try to understand if the dependency is true.

Saying I apply the DTS patch on IMX tree while Mark apply the fsl_ssi
patch on his tree, will there be any regression on either IMX tree or
Mark's tree?  The monaural playback on imx6qdl never worked, so it's not
a regression.  If there is no regression on either tree, there is no
dependency to maintain.

Shawn

> 
> We might also need to apply the iomux change to the other i.MX platforms,
> just currently I don't have those boards so I drop their changes for now.
> 
> Nicolin Chen (2):
>   ARM: dts: imx: specify the value of audmux pinctrl instead of
>     0x80000000
>   ASoC: fsl_ssi: Add monaural audio support for non-ac97 interface
> 
>  arch/arm/boot/dts/imx6qdl.dtsi | 22 +++++++++++-----------
>  sound/soc/fsl/fsl_ssi.c        | 22 +++++++++++++++++++---
>  2 files changed, 30 insertions(+), 14 deletions(-)
> 
> -- 
> 1.8.4
> 
> 

[PATCH 0/2] Add monaural audio support for fsl_ssi.c

[Nicolin Chen]

Hi Shawn,
   
On Fri, Nov 15, 2013 at 11:02:49AM +0800, Shawn Guo wrote:
> On Thu, Nov 14, 2013 at 07:07:08PM +0800, Nicolin Chen wrote:
> > This series of patches need to be applied into one single tree because
> > the second patch depends on the first one. Without it, SSI would playback
> > constant noise to the right channel when playback monaural audio files on
> > i.MX6 Series board.
> 
> Let me try to understand if the dependency is true.
> 
> Saying I apply the DTS patch on IMX tree while Mark apply the fsl_ssi
> patch on his tree, will there be any regression on either IMX tree or
> Mark's tree?  The monaural playback on imx6qdl never worked, so it's not
> a regression.  If there is no regression on either tree, there is no
> dependency to maintain.

It's fair enough to understand in this way. It looks like I misunderstood
the dependency here.

Do I need to resend them separately?

Thank you.

> 
> Shawn
> 
> > 
> > We might also need to apply the iomux change to the other i.MX platforms,
> > just currently I don't have those boards so I drop their changes for now.
> > 
> > Nicolin Chen (2):
> >   ARM: dts: imx: specify the value of audmux pinctrl instead of
> >     0x80000000
> >   ASoC: fsl_ssi: Add monaural audio support for non-ac97 interface
> > 
> >  arch/arm/boot/dts/imx6qdl.dtsi | 22 +++++++++++-----------
> >  sound/soc/fsl/fsl_ssi.c        | 22 +++++++++++++++++++---
> >  2 files changed, 30 insertions(+), 14 deletions(-)
> > 
> > -- 
> > 1.8.4
> > 
> > 

[PATCH 0/2] Add monaural audio support for fsl_ssi.c

[Shawn Guo]

On Fri, Nov 15, 2013 at 10:59:57AM +0800, Nicolin Chen wrote:
> Hi Shawn,
>    
> On Fri, Nov 15, 2013 at 11:02:49AM +0800, Shawn Guo wrote:
> > On Thu, Nov 14, 2013 at 07:07:08PM +0800, Nicolin Chen wrote:
> > > This series of patches need to be applied into one single tree because
> > > the second patch depends on the first one. Without it, SSI would playback
> > > constant noise to the right channel when playback monaural audio files on
> > > i.MX6 Series board.
> > 
> > Let me try to understand if the dependency is true.
> > 
> > Saying I apply the DTS patch on IMX tree while Mark apply the fsl_ssi
> > patch on his tree, will there be any regression on either IMX tree or
> > Mark's tree?  The monaural playback on imx6qdl never worked, so it's not
> > a regression.  If there is no regression on either tree, there is no
> > dependency to maintain.
> 
> It's fair enough to understand in this way. It looks like I misunderstood
> the dependency here.
> 
> Do I need to resend them separately?

No, I will just pick up the DTS patch with some testing.

Shawn

[PATCH 0/2] Add monaural audio support for fsl_ssi.c

[Nicolin Chen]

On Fri, Nov 15, 2013 at 11:22:52AM +0800, Shawn Guo wrote:
> On Fri, Nov 15, 2013 at 10:59:57AM +0800, Nicolin Chen wrote:
> > Hi Shawn,
> >    
> > On Fri, Nov 15, 2013 at 11:02:49AM +0800, Shawn Guo wrote:
> > > On Thu, Nov 14, 2013 at 07:07:08PM +0800, Nicolin Chen wrote:
> > > > This series of patches need to be applied into one single tree because
> > > > the second patch depends on the first one. Without it, SSI would playback
> > > > constant noise to the right channel when playback monaural audio files on
> > > > i.MX6 Series board.
> > > 
> > > Let me try to understand if the dependency is true.
> > > 
> > > Saying I apply the DTS patch on IMX tree while Mark apply the fsl_ssi
> > > patch on his tree, will there be any regression on either IMX tree or
> > > Mark's tree?  The monaural playback on imx6qdl never worked, so it's not
> > > a regression.  If there is no regression on either tree, there is no
> > > dependency to maintain.
> > 
> > It's fair enough to understand in this way. It looks like I misunderstood
> > the dependency here.
> > 
> > Do I need to resend them separately?
> 
> No, I will just pick up the DTS patch with some testing.
> 
> Shawn

Thank you.
Nicolin Chen