[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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[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 <b42...@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
>;
};
};

[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 <b42...@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,

[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

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

[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

[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

[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 <b42...@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

[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 <b42...@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

---

[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 <b42...@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?

[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

[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 <b42...@freescale.com>
---

Changelog
v2:
* Moved i2s_mode to ssi_private so that we can save and retore it as needed
* And dropped the horrible static thing.

sound/soc/fsl/fsl_ssi.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index f43be6d..cba5c4d 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -144,6 +144,7 @@ struct fsl_ssi_private {
bool imx_ac97;
bool use_dma;
bool use_dual_fifo;
+ u8 i2s_mode;
struct clk *clk;
struct snd_dmaengine_dai_dma_data dma_params_tx;
struct snd_dmaengine_dai_dma_data dma_params_rx;
@@ -325,14 +326,13 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id)
static int fsl_ssi_setup(struct fsl_ssi_private *ssi_private)
{

struct ccsr_ssi __iomem *ssi = ssi_private->ssi;

- u8 i2s_mode;
u8 wm;
int synchronous = ssi_private->cpu_dai_drv.symmetric_rates;

if (ssi_private->imx_ac97)
- i2s_mode = CCSR_SSI_SCR_I2S_MODE_NORMAL | CCSR_SSI_SCR_NET;
+ ssi_private->i2s_mode = CCSR_SSI_SCR_I2S_MODE_NORMAL | CCSR_SSI_SCR_NET;
else
- i2s_mode = CCSR_SSI_SCR_I2S_MODE_SLAVE;
+ ssi_private->i2s_mode = CCSR_SSI_SCR_I2S_MODE_SLAVE;

/*
* Section 16.5 of the MPC8610 reference manual says that the SSI needs
@@ -349,7 +349,7 @@ static int fsl_ssi_setup(struct fsl_ssi_private *ssi_private)
write_ssi_mask(&ssi->scr,
CCSR_SSI_SCR_I2S_MODE_MASK | CCSR_SSI_SCR_SYN,
CCSR_SSI_SCR_TFR_CLK_DIS |
- i2s_mode |
+ ssi_private->i2s_mode |
(synchronous ? CCSR_SSI_SCR_SYN : 0));

write_ssi(CCSR_SSI_STCR_TXBIT0 | CCSR_SSI_STCR_TFEN0 |
@@ -517,6 +517,7 @@ 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);

@@ -546,6 +547,11 @@ 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)
+ write_ssi_mask(&ssi->scr,
+ CCSR_SSI_SCR_NET | CCSR_SSI_SCR_I2S_MODE_MASK,
+ channels == 1 ? 0 : ssi_private->i2s_mode);
+
return 0;
}

@@ -658,14 +664,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

[Mark Brown]

On Sat, Nov 16, 2013 at 12:25:29AM +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.

This looks OK but doesn't apply against current code, can you please
check and resend?

[Nicolin Chen]

Ah, I rebased it on the dual-fifo set. Sorry I didn't make it clean.
I will resend it. Thank you.