linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Add monaural audio support for fsl_ssi.c
@ 2013-11-14 11:07 Nicolin Chen
  2013-11-14 11:07 ` [PATCH 1/2] ARM: dts: imx: specify the value of audmux pinctrl instead of 0x80000000 Nicolin Chen
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Nicolin Chen @ 2013-11-14 11:07 UTC (permalink / raw)
  To: timur, shawn.guo, broonie
  Cc: linux-kernel, linux-arm-kernel, devicetree, linuxppc-dev,
	alsa-devel, linux, ijc+devicetree, swarren, mark.rutland,
	pawel.moll, rob.herring, lgirdwood

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] ARM: dts: imx: specify the value of audmux pinctrl instead of 0x80000000
  2013-11-14 11:07 [PATCH 0/2] Add monaural audio support for fsl_ssi.c Nicolin Chen
@ 2013-11-14 11:07 ` Nicolin Chen
  2013-11-15  6:42   ` Shawn Guo
  2013-11-14 11:07 ` [PATCH 2/2] ASoC: fsl_ssi: Add monaural audio support for non-ac97 interface Nicolin Chen
  2013-11-15  3:02 ` [PATCH 0/2] Add monaural audio support for fsl_ssi.c Shawn Guo
  2 siblings, 1 reply; 11+ messages in thread
From: Nicolin Chen @ 2013-11-14 11:07 UTC (permalink / raw)
  To: timur, shawn.guo, broonie
  Cc: linux-kernel, linux-arm-kernel, devicetree, linuxppc-dev,
	alsa-devel, linux, ijc+devicetree, swarren, mark.rutland,
	pawel.moll, rob.herring, lgirdwood

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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] ASoC: fsl_ssi: Add monaural audio support for non-ac97 interface
  2013-11-14 11:07 [PATCH 0/2] Add monaural audio support for fsl_ssi.c Nicolin Chen
  2013-11-14 11:07 ` [PATCH 1/2] ARM: dts: imx: specify the value of audmux pinctrl instead of 0x80000000 Nicolin Chen
@ 2013-11-14 11:07 ` Nicolin Chen
  2013-11-15 12:21   ` Russell King - ARM Linux
  2013-11-15  3:02 ` [PATCH 0/2] Add monaural audio support for fsl_ssi.c Shawn Guo
  2 siblings, 1 reply; 11+ messages in thread
From: Nicolin Chen @ 2013-11-14 11:07 UTC (permalink / raw)
  To: timur, shawn.guo, broonie
  Cc: linux-kernel, linux-arm-kernel, devicetree, linuxppc-dev,
	alsa-devel, linux, ijc+devicetree, swarren, mark.rutland,
	pawel.moll, rob.herring, lgirdwood

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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] Add monaural audio support for fsl_ssi.c
  2013-11-15  3:02 ` [PATCH 0/2] Add monaural audio support for fsl_ssi.c Shawn Guo
@ 2013-11-15  2:59   ` Nicolin Chen
  2013-11-15  3:22     ` Shawn Guo
  0 siblings, 1 reply; 11+ messages in thread
From: Nicolin Chen @ 2013-11-15  2:59 UTC (permalink / raw)
  To: Shawn Guo
  Cc: timur, broonie, linux-kernel, linux-arm-kernel, devicetree,
	linuxppc-dev, alsa-devel, linux, ijc+devicetree, swarren,
	mark.rutland, pawel.moll, rob.herring, lgirdwood

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
> > 
> > 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] Add monaural audio support for fsl_ssi.c
  2013-11-14 11:07 [PATCH 0/2] Add monaural audio support for fsl_ssi.c Nicolin Chen
  2013-11-14 11:07 ` [PATCH 1/2] ARM: dts: imx: specify the value of audmux pinctrl instead of 0x80000000 Nicolin Chen
  2013-11-14 11:07 ` [PATCH 2/2] ASoC: fsl_ssi: Add monaural audio support for non-ac97 interface Nicolin Chen
@ 2013-11-15  3:02 ` Shawn Guo
  2013-11-15  2:59   ` Nicolin Chen
  2 siblings, 1 reply; 11+ messages in thread
From: Shawn Guo @ 2013-11-15  3:02 UTC (permalink / raw)
  To: Nicolin Chen
  Cc: timur, broonie, linux-kernel, linux-arm-kernel, devicetree,
	linuxppc-dev, alsa-devel, linux, ijc+devicetree, swarren,
	mark.rutland, pawel.moll, rob.herring, lgirdwood

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
> 
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] Add monaural audio support for fsl_ssi.c
  2013-11-15  3:22     ` Shawn Guo
@ 2013-11-15  3:15       ` Nicolin Chen
  0 siblings, 0 replies; 11+ messages in thread
From: Nicolin Chen @ 2013-11-15  3:15 UTC (permalink / raw)
  To: Shawn Guo
  Cc: timur, broonie, linux-kernel, linux-arm-kernel, devicetree,
	linuxppc-dev, alsa-devel, linux, ijc+devicetree, swarren,
	mark.rutland, pawel.moll, rob.herring, lgirdwood

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] Add monaural audio support for fsl_ssi.c
  2013-11-15  2:59   ` Nicolin Chen
@ 2013-11-15  3:22     ` Shawn Guo
  2013-11-15  3:15       ` Nicolin Chen
  0 siblings, 1 reply; 11+ messages in thread
From: Shawn Guo @ 2013-11-15  3:22 UTC (permalink / raw)
  To: Nicolin Chen
  Cc: timur, broonie, linux-kernel, linux-arm-kernel, devicetree,
	linuxppc-dev, alsa-devel, linux, ijc+devicetree, swarren,
	mark.rutland, pawel.moll, rob.herring, lgirdwood

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ARM: dts: imx: specify the value of audmux pinctrl instead of 0x80000000
  2013-11-15  6:42   ` Shawn Guo
@ 2013-11-15  6:40     ` Nicolin Chen
  0 siblings, 0 replies; 11+ messages in thread
From: Nicolin Chen @ 2013-11-15  6:40 UTC (permalink / raw)
  To: Shawn Guo
  Cc: timur, broonie, linux-kernel, linux-arm-kernel, devicetree,
	linuxppc-dev, alsa-devel, linux, ijc+devicetree, swarren,
	mark.rutland, pawel.moll, rob.herring, lgirdwood

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

---



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ARM: dts: imx: specify the value of audmux pinctrl instead of 0x80000000
  2013-11-14 11:07 ` [PATCH 1/2] ARM: dts: imx: specify the value of audmux pinctrl instead of 0x80000000 Nicolin Chen
@ 2013-11-15  6:42   ` Shawn Guo
  2013-11-15  6:40     ` Nicolin Chen
  0 siblings, 1 reply; 11+ messages in thread
From: Shawn Guo @ 2013-11-15  6:42 UTC (permalink / raw)
  To: Nicolin Chen
  Cc: timur, broonie, linux-kernel, linux-arm-kernel, devicetree,
	linuxppc-dev, alsa-devel, linux, ijc+devicetree, swarren,
	mark.rutland, pawel.moll, rob.herring, lgirdwood

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
> 
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ASoC: fsl_ssi: Add monaural audio support for non-ac97 interface
  2013-11-14 11:07 ` [PATCH 2/2] ASoC: fsl_ssi: Add monaural audio support for non-ac97 interface Nicolin Chen
@ 2013-11-15 12:21   ` Russell King - ARM Linux
  2013-11-15 15:17     ` Nicolin Chen
  0 siblings, 1 reply; 11+ messages in thread
From: Russell King - ARM Linux @ 2013-11-15 12:21 UTC (permalink / raw)
  To: Nicolin Chen
  Cc: timur, shawn.guo, broonie, linux-kernel, linux-arm-kernel,
	devicetree, linuxppc-dev, alsa-devel, ijc+devicetree, swarren,
	mark.rutland, pawel.moll, rob.herring, lgirdwood

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?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ASoC: fsl_ssi: Add monaural audio support for non-ac97 interface
  2013-11-15 12:21   ` Russell King - ARM Linux
@ 2013-11-15 15:17     ` Nicolin Chen
  0 siblings, 0 replies; 11+ messages in thread
From: Nicolin Chen @ 2013-11-15 15:17 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: timur, shawn.guo, broonie, linux-kernel, linux-arm-kernel,
	devicetree, linuxppc-dev, alsa-devel, ijc+devicetree, swarren,
	mark.rutland, pawel.moll, rob.herring, lgirdwood

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-11-15 15:27 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-14 11:07 [PATCH 0/2] Add monaural audio support for fsl_ssi.c Nicolin Chen
2013-11-14 11:07 ` [PATCH 1/2] ARM: dts: imx: specify the value of audmux pinctrl instead of 0x80000000 Nicolin Chen
2013-11-15  6:42   ` Shawn Guo
2013-11-15  6:40     ` Nicolin Chen
2013-11-14 11:07 ` [PATCH 2/2] ASoC: fsl_ssi: Add monaural audio support for non-ac97 interface Nicolin Chen
2013-11-15 12:21   ` Russell King - ARM Linux
2013-11-15 15:17     ` Nicolin Chen
2013-11-15  3:02 ` [PATCH 0/2] Add monaural audio support for fsl_ssi.c Shawn Guo
2013-11-15  2:59   ` Nicolin Chen
2013-11-15  3:22     ` Shawn Guo
2013-11-15  3:15       ` Nicolin Chen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).