[alsa-devel] [PATCH 0/2] Add monaural audio support for fsl_ssi.c
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(-)
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 >; }; };
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 0x80000000MX6QDL_PAD_SD2_DAT3__AUD4_TXC 0x80000000MX6QDL_PAD_SD2_DAT2__AUD4_TXD 0x80000000MX6QDL_PAD_SD2_DAT1__AUD4_TXFS 0x80000000
MX6QDL_PAD_SD2_DAT0__AUD4_RXD 0x130b0MX6QDL_PAD_SD2_DAT3__AUD4_TXC 0x130b0MX6QDL_PAD_SD2_DAT2__AUD4_TXD 0x110b0MX6QDL_PAD_SD2_DAT1__AUD4_TXFS 0x130b0 >; }; pinctrl_audmux_2: audmux-2 { fsl,pins = <
MX6QDL_PAD_CSI0_DAT7__AUD3_RXD 0x80000000MX6QDL_PAD_CSI0_DAT4__AUD3_TXC 0x80000000MX6QDL_PAD_CSI0_DAT5__AUD3_TXD 0x80000000MX6QDL_PAD_CSI0_DAT6__AUD3_TXFS 0x80000000
MX6QDL_PAD_CSI0_DAT7__AUD3_RXD 0x130b0MX6QDL_PAD_CSI0_DAT4__AUD3_TXC 0x130b0MX6QDL_PAD_CSI0_DAT5__AUD3_TXD 0x110b0MX6QDL_PAD_CSI0_DAT6__AUD3_TXFS 0x130b0 >; }; pinctrl_audmux_3: audmux-3 { fsl,pins = <
MX6QDL_PAD_DISP0_DAT16__AUD5_TXC 0x80000000MX6QDL_PAD_DISP0_DAT18__AUD5_TXFS 0x80000000MX6QDL_PAD_DISP0_DAT19__AUD5_RXD 0x80000000
MX6QDL_PAD_DISP0_DAT16__AUD5_TXC 0x130b0MX6QDL_PAD_DISP0_DAT18__AUD5_TXFS 0x130b0MX6QDL_PAD_DISP0_DAT19__AUD5_RXD 0x130b0 >; }; };-- 1.8.4
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
---
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,
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?
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
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:
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
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
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
participants (3)
-
Nicolin Chen -
Russell King - ARM Linux -
Shawn Guo