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
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 <redacted>
---
arch/arm/boot/dts/imx6qdl.dtsi | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
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 <redacted>
---
sound/soc/fsl/fsl_ssi.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
@@ -517,10 +517,12 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,{structfsl_ssi_private*ssi_private=snd_soc_dai_get_drvdata(cpu_dai);structccsr_ssi__iomem*ssi=ssi_private->ssi;+unsignedintchannels=params_channels(hw_params);unsignedintsample_size=snd_pcm_format_width(params_format(hw_params));u32wl=CCSR_SSI_SxCCR_WL(sample_size);intenabled=read_ssi(&ssi->scr)&CCSR_SSI_SCR_SSIEN;+staticu8i2s_mode;/**Ifwe'reinsynchronousmode,andtheSSIisalreadyenabled,
@@ -546,6 +548,21 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,elsewrite_ssi_mask(&ssi->srccr,CCSR_SSI_SxCCR_WL_MASK,wl);+if(ssi_private->imx_ac97)+return0;++/* Save i2s mode configuration so that we can restore it later */+switch(read_ssi(&ssi->scr)&CCSR_SSI_SCR_I2S_MODE_MASK){+caseCCSR_SSI_SCR_I2S_MODE_SLAVE:+caseCCSR_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);+return0;}
@@ -658,14 +675,13 @@ static const struct snd_soc_dai_ops fsl_ssi_dai_ops = {staticstructsnd_soc_dai_driverfsl_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,
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
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:
quoted
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
quoted
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
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:
quoted
On Thu, Nov 14, 2013 at 07:07:08PM +0800, Nicolin Chen wrote:
quoted
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
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:
quoted
Hi Shawn,
On Fri, Nov 15, 2013 at 11:02:49AM +0800, Shawn Guo wrote:
quoted
On Thu, Nov 14, 2013 at 07:07:08PM +0800, Nicolin Chen wrote:
quoted
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
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 <redacted>
---
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
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:
quoted
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 <redacted>
---
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.
From: Russell King - ARM Linux <hidden> Date: 2013-11-15 12:22:10
On Thu, Nov 14, 2013 at 07:07:10PM +0800, Nicolin Chen wrote:
quoted hunk
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 <redacted>
---
sound/soc/fsl/fsl_ssi.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
@@ -517,10 +517,12 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,{structfsl_ssi_private*ssi_private=snd_soc_dai_get_drvdata(cpu_dai);structccsr_ssi__iomem*ssi=ssi_private->ssi;+unsignedintchannels=params_channels(hw_params);unsignedintsample_size=snd_pcm_format_width(params_format(hw_params));u32wl=CCSR_SSI_SxCCR_WL(sample_size);intenabled=read_ssi(&ssi->scr)&CCSR_SSI_SCR_SSIEN;+staticu8i2s_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.
quoted hunk
/*
* 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?
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.
quoted
+ /* 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