RE: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
The udelay just doesn't make sense to what you are talking about.
Does SAI really need 10us delay between two register-updating?
No, this is not must be.
We basically use udelay only if the IP hardware actually needs it: some
IP needs time to boot itself up after doing software reset for example.
But it doesn't look reasonable to me by using udelay to make sure the
last enabled.
And from the 'Synchronous mode' you just provided, there're another issue:
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_RESUME:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ tcsr |= FSL_SAI_CSR_TERE;
+ rcsr |= FSL_SAI_CSR_TERE;
+ writel(rcsr, sai-base + FSL_SAI_RCSR);
+ udelay(10);
+ writel(tcsr, sai-base + FSL_SAI_TCSR);
+ break;
+
+ case SNDRV_PCM_TRIGGER_STOP:
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ if (!(dai-playback_active || dai-capture_active)) {
+ tcsr = ~FSL_SAI_CSR_TERE;
+ rcsr = ~FSL_SAI_CSR_TERE;
+ }
+ writel(rcsr, sai-base + FSL_SAI_RCSR);
+ udelay(10);
+ writel(tcsr, sai-base + FSL_SAI_TCSR);
+ break;
ISSUE 1: You might make sure transmitter is the last enabled.
However, it's not the first disabled. Is this okay?
Yes, this is just programming mistake. I'll revise it.
In this case the transmitter should be the last enabled and the first disabled.
ISSUE 2: There are two cases listed in 'Synchronous mode'.
However, your driver doesn't take care of them.
The SAI's synchronous mode looks like more flexible
than SSI's. The driver needs to be more sophisticated
so that it can handle multiple cases when TX/RX clocks
are controlled by either TX or RX, and surely, the
asynchronous mode as well.
Because in Vybrid the transmitter bit clock and frame sync are to be used by
both the transmitter and receiver, and only this case can be used here, so now
I only handle this case.
And there's another personal tip: I think you can first try to focus on
this SAI driver and pend the others. There might be two many things you
need to refine if you are doing them at the same time.
I'll implement them later if needed.
--
Best Regards,
Xiubo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
On Wed, Nov 20, 2013 at 11:37:45AM +0800, Xiubo Li-B47053 wrote:
The udelay just doesn't make sense to what you are talking about.
Does SAI really need 10us delay between two register-updating?
No, this is not must be.
Then you should explain in your comments why you really put it here or just
drop it if it's just a mistake.
We basically use udelay only if the IP hardware actually needs it: some
IP needs time to boot itself up after doing software reset for example.
But it doesn't look reasonable to me by using udelay to make sure the
last enabled.
And from the 'Synchronous mode' you just provided, there're another issue:
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_RESUME:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ tcsr |= FSL_SAI_CSR_TERE;
+ rcsr |= FSL_SAI_CSR_TERE;
+ writel(rcsr, sai-base + FSL_SAI_RCSR);
+ udelay(10);
+ writel(tcsr, sai-base + FSL_SAI_TCSR);
+ break;
+
+ case SNDRV_PCM_TRIGGER_STOP:
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ if (!(dai-playback_active || dai-capture_active)) {
+ tcsr = ~FSL_SAI_CSR_TERE;
+ rcsr = ~FSL_SAI_CSR_TERE;
+ }
+ writel(rcsr, sai-base + FSL_SAI_RCSR);
+ udelay(10);
+ writel(tcsr, sai-base + FSL_SAI_TCSR);
+ break;
ISSUE 1: You might make sure transmitter is the last enabled.
However, it's not the first disabled. Is this okay?
Yes, this is just programming mistake. I'll revise it.
In this case the transmitter should be the last enabled and the first
disabled.
ISSUE 2: There are two cases listed in 'Synchronous mode'.
However, your driver doesn't take care of them.
The SAI's synchronous mode looks like more flexible
than SSI's. The driver needs to be more sophisticated
so that it can handle multiple cases when TX/RX clocks
are controlled by either TX or RX, and surely, the
asynchronous mode as well.
Because in Vybrid the transmitter bit clock and frame sync are to be used by
both the transmitter and receiver, and only this case can be used here, so
now I only handle this case.
It's fairly okay if adding explicit comments to indicate that currently the
driver only supports its Synchronous mode with clocks controlled by TX only.
Best,
Nicolin Chen
And there's another personal tip: I think you can first try to focus on
this SAI driver and pend the others. There might be two many things you
need to refine if you are doing them at the same time.
I'll implement them later if needed.
--
Best Regards,
Xiubo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
The udelay just doesn't make sense to what you are talking about. Does SAI really need 10us delay between two register-updating? No, this is not must be. Then you should explain in your comments why you really put it here or just drop it if it's just a mistake. The udelay will be removed then. ISSUE 2: There are two cases listed in 'Synchronous mode'. However, your driver doesn't take care of them. The SAI's synchronous mode looks like more flexible than SSI's. The driver needs to be more sophisticated so that it can handle multiple cases when TX/RX clocks are controlled by either TX or RX, and surely, the asynchronous mode as well. Because in Vybrid the transmitter bit clock and frame sync are to be used by both the transmitter and receiver, and only this case can be used here, so now I only handle this case. It's fairly okay if adding explicit comments to indicate that currently the driver only supports its Synchronous mode with clocks controlled by TX only. Just think, on other platforms maybe only the Rx's clock is available. Thus I think there should be one DT property to control this, and then the SAI driver can be more flexible. Or could you give me some more practical ideas ? -- Best Regards, Xiubo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
On Wed, Nov 06, 2013 at 03:53:24AM +, Li Xiubo wrote: If there are any comments that say PPC but are not PPC-specific, that should be fixed. Yes, find it. The comments is in sound/soc/fsl/Makefile : +++ # Freescale PowerPC SSI/DMA Platform Support --- But fsl-spdif.o is also under it. And this is also support ARM and PowerPC platforms at the same time ? If so, the comments should be modified to : +++ # Freescale PowerPC and ARM SSI/DMA Platform Support --- Yes, this should be changed. How about : + # Freescale PowerPC and ARM SSI/DMA/SAI/SPDIF Platform Support - ? Or we can just drop 'PowerPC and ARM'? Shawn ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
If there are any comments that say PPC but are not PPC-specific, that should be fixed. Yes, find it. The comments is in sound/soc/fsl/Makefile : +++ # Freescale PowerPC SSI/DMA Platform Support --- But fsl-spdif.o is also under it. And this is also support ARM and PowerPC platforms at the same time ? If so, the comments should be modified to : +++ # Freescale PowerPC and ARM SSI/DMA Platform Support --- Yes, this should be changed. How about : + # Freescale PowerPC and ARM SSI/DMA/SAI/SPDIF Platform Support - ? Or we can just drop 'PowerPC and ARM'? Yes, this will be better. Best Regards, Xiubo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
On Tue, Nov 05, 2013 at 03:21:49AM +, Li Xiubo wrote: As your opinions, should I move the four register writing operations to .set_sysclk/set_clkdiv/... functions too ? Or just add a clk_disable_unprepare() after them here, and then add clk_prepare_enable in one of .set_sysclk/set_clkdiv/...? The latter. signature.asc Description: Digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
Li Xiubo wrote: But fsl-ssi.o and fsl-spdif.o is based PowrePC platform? Which we can see from the comments. fsl_ssi was originally PPC-only, but it now supports PPC and ARM. You can see that from the git history. If there are any comments that say PPC but are not PPC-specific, that should be fixed. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
But fsl-ssi.o and fsl-spdif.o is based PowrePC platform? Which we can see from the comments. fsl_ssi was originally PPC-only, but it now supports PPC and ARM. You can see that from the git history. If there are any comments that say PPC but are not PPC-specific, that should be fixed. Yes, find it. The comments is in sound/soc/fsl/Makefile : +++ # Freescale PowerPC SSI/DMA Platform Support --- But fsl-spdif.o is also under it. And this is also support ARM and PowerPC platforms at the same time ? If so, the comments should be modified to : +++ # Freescale PowerPC and ARM SSI/DMA Platform Support --- Best Regards, Xiubo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
Li Xiubo wrote: If there are any comments that say PPC but are not PPC-specific, that should be fixed. Yes, find it. The comments is in sound/soc/fsl/Makefile : +++ # Freescale PowerPC SSI/DMA Platform Support --- But fsl-spdif.o is also under it. And this is also support ARM and PowerPC platforms at the same time ? If so, the comments should be modified to : +++ # Freescale PowerPC and ARM SSI/DMA Platform Support --- Yes, this should be changed. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
If there are any comments that say PPC but are not PPC-specific, that should be fixed. Yes, find it. The comments is in sound/soc/fsl/Makefile : +++ # Freescale PowerPC SSI/DMA Platform Support --- But fsl-spdif.o is also under it. And this is also support ARM and PowerPC platforms at the same time ? If so, the comments should be modified to : +++ # Freescale PowerPC and ARM SSI/DMA Platform Support --- Yes, this should be changed. How about : + # Freescale PowerPC and ARM SSI/DMA/SAI/SPDIF Platform Support - ? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
On Mon, Nov 04, 2013 at 07:35:12AM +, Li Xiubo wrote:
From the ASoC subsystem comments we can see that:
++
Configures the clock dividers. This is used to derive the best DAI bit and
frame clocks from the system or master clock. It's best to set the DAI bit
and frame clocks as low as possible to save system power.
--
You should never use this unless you have to, there is no point in every
single machine driver using your driver having to duplicate the same
calculations.
+static int fsl_sai_dai_probe(struct snd_soc_dai *dai) {
+ int ret;
+ struct fsl_sai *sai = dev_get_drvdata(dai-dev);
+
+ ret = clk_prepare_enable(sai-clk);
+ if (ret)
+ return ret;
It'd be nicer to only enable the clock while the device is in active use.
While if the module clock is not enabled here, the followed registers cannot
read/write in the same function.
And this _probe function is the _dai_probe not the driver's module _probe.
So you can enable the clock when you explicitly need to write to the
registers...
If the clk_prepare_enable(sai-clk) is not here, where should it be will be
nicer ?
One of the following functions ?
.set_sysclk = fsl_sai_set_dai_sysclk,
.set_clkdiv = fsl_sai_set_dai_clkdiv,
.set_fmt= fsl_sai_set_dai_fmt,
.set_tdm_slot = fsl_sai_set_dai_tdm_slot,
.hw_params = fsl_sai_hw_params,
.trigger= fsl_sai_trigger,
It could be in any or all of them except trigger (where the core should
hold a runtime PM reference anyway). You can always take a reference
for the duration of the function if you're concerned it may be called
when the referent isn't otherwise held.
+ ret = snd_dmaengine_pcm_register(pdev-dev, NULL,
+ SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);
+ if (ret)
+ return ret;
We should have a devm_ version of this.
Sorry, is there one patch for adding the devm_ version of
snd_dmaengine_pcm_register() already ?
In the -next and other topics branches I could not find it.
No, there isn't one but there should be one.
signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
From the ASoC subsystem comments we can see that:
++
Configures the clock dividers. This is used to derive the best DAI bit
and frame clocks from the system or master clock. It's best to set the
DAI bit and frame clocks as low as possible to save system power.
--
You should never use this unless you have to, there is no point in every
single machine driver using your driver having to duplicate the same
calculations.
Okey, I'll think it over, if not have to, I will revise this.
+static int fsl_sai_dai_probe(struct snd_soc_dai *dai) {
+ int ret;
+ struct fsl_sai *sai = dev_get_drvdata(dai-dev);
+
+ ret = clk_prepare_enable(sai-clk);
+ if (ret)
+ return ret;
It'd be nicer to only enable the clock while the device is in active
use.
While if the module clock is not enabled here, the followed registers
cannot read/write in the same function.
And this _probe function is the _dai_probe not the driver's module
_probe.
So you can enable the clock when you explicitly need to write to the
registers...
If the clk_prepare_enable(sai-clk) is not here, where should it be
will be nicer ?
One of the following functions ?
.set_sysclk = fsl_sai_set_dai_sysclk,
.set_clkdiv = fsl_sai_set_dai_clkdiv,
.set_fmt= fsl_sai_set_dai_fmt,
.set_tdm_slot = fsl_sai_set_dai_tdm_slot,
.hw_params = fsl_sai_hw_params,
.trigger= fsl_sai_trigger,
It could be in any or all of them except trigger (where the core should
hold a runtime PM reference anyway). You can always take a reference for
the duration of the function if you're concerned it may be called when
the referent isn't otherwise held.
While in this _sai_dai_probe function just followed the clock enable sentence,
there are some register writing operations:
The PATCH:
+++
+static int fsl_sai_dai_probe(struct snd_soc_dai *dai) {
+ int ret;
+ struct fsl_sai *sai = dev_get_drvdata(dai-dev);
+
+ ret = clk_prepare_enable(sai-clk); = clock
enable here
+ if (ret)
+ return ret;
+
+ writel(0x0, sai-base + FSL_SAI_RCSR);
==registers writing here.
+ writel(0x0, sai-base + FSL_SAI_TCSR);== and
here
+ writel(sai-dma_params_tx.maxburst, sai-base + FSL_SAI_TCR1);
===and here
+ writel(sai-dma_params_rx.maxburst, sai-base + FSL_SAI_RCR1);
===and here
+
+ dai-playback_dma_data = sai-dma_params_tx;
+ dai-capture_dma_data = sai-dma_params_rx;
+
+ snd_soc_dai_set_drvdata(dai, sai);
+
+ return 0;
+}
-
As your opinions, should I move the four register writing operations to
.set_sysclk/set_clkdiv/... functions too ?
Or just add a clk_disable_unprepare() after them here, and then add
clk_prepare_enable in one of .set_sysclk/set_clkdiv/...?
And the first two of this four registers must be initialize as early as
possible, and if move them to one of the .set_sysclk/set_clkdiv/... functions,
How can I very ensure which one is the first to be called ?
Won't the calling sequence be changed in the feature ?
From the debug logs, we can see that:
1, _sai_probe
This is called when the machine brings up and has one SAI device.
2, _sai_dai_probe
3, .set_sysclk
4, .set_fmt
Are called in order when the machine has Audio driver and is enabled, and also
while the machine brings up.
The above four steps only be called one time in order.
When aplay/arecord is runs the following will be called in order:
5, .set_tdm_slot
6, .hw_param
7, .trigger --begain
8, .trigger -- end
The 2,3,4 are always called almost the same time, and they are all have
register read/write operations.
Now the clk_prepare_enable() is in step 2, and it won't be any different moving
to step 3 or 4.
So, only could move it to step 5 or 6, if so every time the aplay/arecord runs,
clk_prepare_enable() will be
called, and there has no chance to call clk_disable_unprepare().
Now from the code we can see that I have add clk_prepare_enable() in
_sai_dai_probe() and clk_disable_unprepare() in _sai_dai_remove().
Isn't this okey ?
+ ret = snd_dmaengine_pcm_register(pdev-dev, NULL,
+ SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);
+ if (ret)
+ return ret;
We should have a devm_ version of this.
Sorry, is there one patch for adding the devm_ version of
snd_dmaengine_pcm_register() already ?
In the -next and other topics branches I could not find it.
No, there isn't one but there should be one.
And if it has existed then I will use it.
BRs,
Xiubo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
This adds Freescale SAI ASoC Audio support.
This implementation is only compatible with device tree definition.
Features:
o Supports playback/capture
o Supports 16/20/24 bit PCM
o Supports 8k - 96k sample rates
o Supports slave mode only.
Just for curiosity, I found there're quite a bit code actually support
I2S master mode such as set_sysclk() and register configuration to FMT
SND_SOC_DAIFMT_CBS_CFS.
Is that really supports slave mode only? Or just you haven't make it
properly work?
Sorry, this comments is not very consistent with the driver implementation.
On VF610 there only supports slave mode.
So, I will revise this.
diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig index
b7ab71f..9a8851e 100644
--- a/sound/soc/fsl/Kconfig
+++ b/sound/soc/fsl/Kconfig
@@ -213,3 +213,19 @@ config SND_SOC_IMX_MC13783
select SND_SOC_IMX_PCM_DMA
endif # SND_IMX_SOC
+
+menuconfig SND_FSL_SOC
+ tristate SoC Audio for Freescale FSL CPUs
+ help
+ Say Y or M if you want to add support for codecs attached to
+ the FSL CPUs.
+
+ This will enable Freeacale SAI, SGT15000 codec.
+
+if SND_FSL_SOC
Why check this here? SAI should be a regular IP module which can be used
by other SoC as well. We would never know if next i.MX SoC won't contain
SAI.
+
+config SND_SOC_FSL_SAI
+ tristate
+ select SND_SOC_GENERIC_DMAENGINE_PCM
I think you should keep SAI beside SSI/SPDIF directly.
That's right.
+
+endif # SND_FSL_SOC
diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile index
8db705b..e5acc03 100644
--- a/sound/soc/fsl/Makefile
+++ b/sound/soc/fsl/Makefile
@@ -56,3 +56,8 @@ obj-$(CONFIG_SND_SOC_IMX_SGTL5000) +=
snd-soc-imx-sgtl5000.o
obj-$(CONFIG_SND_SOC_IMX_WM8962) += snd-soc-imx-wm8962.o
obj-$(CONFIG_SND_SOC_IMX_SPDIF) += snd-soc-imx-spdif.o
obj-$(CONFIG_SND_SOC_IMX_MC13783) += snd-soc-imx-mc13783.o
+
+# FSL ARM SAI/SGT15000 Platform Support
Should be SGTL5000, not SGT1. And SAI should not be used with SGTL5000
only, it's a bit confusing to mention SGTL5000 here.
Yes, this will be revised then.
+snd-soc-fsl-sai-objs := fsl-sai.o
And I think it should be better to put it along with fsl-ssi.o and fsl-
spdif.o
But fsl-ssi.o and fsl-spdif.o is based PowrePC platform? Which we can see from
the comments.
While fsl-sai.o is base ARM platform.
Despite whether there is any different between ARM and PPC or not, the comments
won't be correct.
+
+obj-$(CONFIG_SND_SOC_FSL_SAI) += snd-soc-fsl-sai.o
Ditto
diff --git a/sound/soc/fsl/fsl-sai.c b/sound/soc/fsl/fsl-sai.c new
file mode 100644 index 000..bb57e67
--- /dev/null
+++ b/sound/soc/fsl/fsl-sai.c
@@ -0,0 +1,472 @@
+/*
+ * Freescale SAI ALSA SoC Digital Audio Interface driver.
+ *
+ * Copyright 2012-2013 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+modify it
+ * under the terms of the GNU General Public License as published
+by the
+ * Free Software Foundation; either version 2 of the License, or
+(at your
+ * option) any later version.
There're too many double space inside. Could you pls revise it?
Yes, see the next version.
+ *
+ */
+
+#include linux/clk.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/of_address.h
+#include sound/core.h
+#include sound/pcm_params.h
+#include linux/delay.h
+#include linux/dmaengine.h
+#include sound/dmaengine_pcm.h
I think it's better to keep sound/xxx.h together. And basically we can
keep header files ordered by alphabet.
Okey.
+
+#include fsl-sai.h
+
+static int fsl_sai_set_dai_sysclk_tr(struct snd_soc_dai *cpu_dai,
+ int clk_id, unsigned int freq, int fsl_dir) {
+ u32 val_cr2, reg_cr2;
+ struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
+
+ if (fsl_dir == FSL_FMT_TRANSMITTER)
+ reg_cr2 = FSL_SAI_TCR2;
+ else
+ reg_cr2 = FSL_SAI_RCR2;
+
+ val_cr2 = readl(sai-base + reg_cr2);
+ switch (clk_id) {
+ case FSL_SAI_CLK_BUS:
+ val_cr2 = ~FSL_SAI_CR2_MSEL_MASK;
+ val_cr2 |= FSL_SAI_CR2_MSEL_BUS;
+ break;
+ case FSL_SAI_CLK_MAST1:
+ val_cr2 = ~FSL_SAI_CR2_MSEL_MASK;
+ val_cr2 |= FSL_SAI_CR2_MSEL_MCLK1;
+ break;
+ case FSL_SAI_CLK_MAST2:
+ val_cr2 = ~FSL_SAI_CR2_MSEL_MASK;
+ val_cr2 |= FSL_SAI_CR2_MSEL_MCLK2;
+ break;
+ case FSL_SAI_CLK_MAST3:
+ val_cr2 = ~FSL_SAI_CR2_MSEL_MASK;
+ val_cr2 |= FSL_SAI_CR2_MSEL_MCLK3;
+ break;
+ default:
+ return -EINVAL;
+ }
+ writel(val_cr2, sai-base + reg_cr2);
+
+ return 0;
+}
+
+static int fsl_sai_set_dai_sysclk(struct snd_soc_dai *cpu_dai,
+ int clk_id, unsigned int freq, int dir) {
+ int ret;
+
+ if (dir ==
Re: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
On Mon, Nov 04, 2013 at 11:45:10AM +0800, Xiubo Li-B47053 wrote:
+snd-soc-fsl-sai-objs := fsl-sai.o
And I think it should be better to put it along with fsl-ssi.o and fsl-
spdif.o
But fsl-ssi.o and fsl-spdif.o is based PowrePC platform? Which we can see
from the comments.
No. fsl-ssi.c is compatible to both ARM and PPC. And fsl-spdif.c is currently
used on ARM only. So please just put along with them.
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_RESUME:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ tcsr |= FSL_SAI_CSR_TERE;
+ rcsr |= FSL_SAI_CSR_TERE;
+ writel(rcsr, sai-base + FSL_SAI_RCSR);
+ udelay(10);
Does SAI really needs this udelay() here? Required by IP's operation flow?
If so, I think it's better to add comments here to explain it.
+
Synchronous mode
The SAI transmitter and receiver can be configured to operate with
synchronous bit clock
and frame sync.
1,
If the transmitter bit clock and frame sync are to be used by both the
transmitter and
receiver:
...
* It is recommended that the transmitter is the last enabled and the first
disabled.
2,
If the receiver bit clock and frame sync are to be used by both the
transmitter and
receiver:
...
* It is recommended that the receiver is the last enabled and the first
disabled.
--
So I think the delay is needed, and I still tunning it.
The udelay just doesn't make sense to what you are talking about.
Does SAI really need 10us delay between two register-updating?
We basically use udelay only if the IP hardware actually needs it: some
IP needs time to boot itself up after doing software reset for example.
But it doesn't look reasonable to me by using udelay to make sure the
last enabled.
And from the 'Synchronous mode' you just provided, there're another issue:
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_RESUME:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ tcsr |= FSL_SAI_CSR_TERE;
+ rcsr |= FSL_SAI_CSR_TERE;
+ writel(rcsr, sai-base + FSL_SAI_RCSR);
+ udelay(10);
+ writel(tcsr, sai-base + FSL_SAI_TCSR);
+ break;
+
+ case SNDRV_PCM_TRIGGER_STOP:
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ if (!(dai-playback_active || dai-capture_active)) {
+ tcsr = ~FSL_SAI_CSR_TERE;
+ rcsr = ~FSL_SAI_CSR_TERE;
+ }
+ writel(rcsr, sai-base + FSL_SAI_RCSR);
+ udelay(10);
+ writel(tcsr, sai-base + FSL_SAI_TCSR);
+ break;
ISSUE 1: You might make sure transmitter is the last enabled.
However, it's not the first disabled. Is this okay?
ISSUE 2: There are two cases listed in 'Synchronous mode'.
However, your driver doesn't take care of them.
The SAI's synchronous mode looks like more flexible
than SSI's. The driver needs to be more sophisticated
so that it can handle multiple cases when TX/RX clocks
are controlled by either TX or RX, and surely, the
asynchronous mode as well.
And there's another personal tip: I think you can first try to focus on
this SAI driver and pend the others. There might be two many things you
need to refine if you are doing them at the same time.
Best regards,
Nicolin Chen
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
+static int fsl_sai_set_dai_clkdiv(struct snd_soc_dai *cpu_dai,
+ int div_id, int div)
+{
+ struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
+ u32 tcr2, rcr2;
+
+ if (div_id == FSL_SAI_TX_DIV) {
+ tcr2 = readl(sai-base + FSL_SAI_TCR2);
+ tcr2 = ~FSL_SAI_CR2_DIV_MASK;
+ tcr2 |= FSL_SAI_CR2_DIV(div);
+ writel(tcr2, sai-base + FSL_SAI_TCR2);
What is this divider and why does the user have to set it manually?
This is the bit clock divider. I'll add some comments or rename them to be more
readable.
From the IP spec:
++
Bit Clock Divide
Divides down the audio master clock to generate the bit clock when configured
for an internal bit clock.
--
From the ASoC subsystem comments we can see that:
++
Configures the clock dividers. This is used to derive the best DAI bit and
frame clocks from the system or master clock. It's best to set the DAI bit
and frame clocks as low as possible to save system power.
--
+static int fsl_sai_dai_probe(struct snd_soc_dai *dai) {
+ int ret;
+ struct fsl_sai *sai = dev_get_drvdata(dai-dev);
+
+ ret = clk_prepare_enable(sai-clk);
+ if (ret)
+ return ret;
It'd be nicer to only enable the clock while the device is in active use.
While if the module clock is not enabled here, the followed registers cannot
read/write in the same function.
And this _probe function is the _dai_probe not the driver's module _probe.
If the clk_prepare_enable(sai-clk) is not here, where should it be will be
nicer ?
One of the following functions ?
.set_sysclk = fsl_sai_set_dai_sysclk,
.set_clkdiv = fsl_sai_set_dai_clkdiv,
.set_fmt= fsl_sai_set_dai_fmt,
.set_tdm_slot = fsl_sai_set_dai_tdm_slot,
.hw_params = fsl_sai_hw_params,
.trigger= fsl_sai_trigger,
+ ret = snd_dmaengine_pcm_register(pdev-dev, NULL,
+ SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);
+ if (ret)
+ return ret;
We should have a devm_ version of this.
Sorry, is there one patch for adding the devm_ version of
snd_dmaengine_pcm_register() already ?
In the -next and other topics branches I could not find it.
Best Regards,
Xiubo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
Hi Xiubo,
On Fri, Nov 01, 2013 at 03:04:48PM +0800, Xiubo Li wrote:
This adds Freescale SAI ASoC Audio support.
This implementation is only compatible with device tree definition.
Features:
o Supports playback/capture
o Supports 16/20/24 bit PCM
o Supports 8k - 96k sample rates
o Supports slave mode only.
Just for curiosity, I found there're quite a bit code actually support
I2S master mode such as set_sysclk() and register configuration to FMT
SND_SOC_DAIFMT_CBS_CFS.
Is that really supports slave mode only? Or just you haven't make
it properly work?
diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
index b7ab71f..9a8851e 100644
--- a/sound/soc/fsl/Kconfig
+++ b/sound/soc/fsl/Kconfig
@@ -213,3 +213,19 @@ config SND_SOC_IMX_MC13783
select SND_SOC_IMX_PCM_DMA
endif # SND_IMX_SOC
+
+menuconfig SND_FSL_SOC
+ tristate SoC Audio for Freescale FSL CPUs
+ help
+ Say Y or M if you want to add support for codecs attached to
+ the FSL CPUs.
+
+ This will enable Freeacale SAI, SGT15000 codec.
+
+if SND_FSL_SOC
Why check this here? SAI should be a regular IP module which can be used by
other SoC as well. We would never know if next i.MX SoC won't contain SAI.
+
+config SND_SOC_FSL_SAI
+ tristate
+ select SND_SOC_GENERIC_DMAENGINE_PCM
I think you should keep SAI beside SSI/SPDIF directly.
+
+endif # SND_FSL_SOC
diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile
index 8db705b..e5acc03 100644
--- a/sound/soc/fsl/Makefile
+++ b/sound/soc/fsl/Makefile
@@ -56,3 +56,8 @@ obj-$(CONFIG_SND_SOC_IMX_SGTL5000) += snd-soc-imx-sgtl5000.o
obj-$(CONFIG_SND_SOC_IMX_WM8962) += snd-soc-imx-wm8962.o
obj-$(CONFIG_SND_SOC_IMX_SPDIF) += snd-soc-imx-spdif.o
obj-$(CONFIG_SND_SOC_IMX_MC13783) += snd-soc-imx-mc13783.o
+
+# FSL ARM SAI/SGT15000 Platform Support
Should be SGTL5000, not SGT1. And SAI should not be used with SGTL5000 only,
it's a bit confusing to mention SGTL5000 here.
+snd-soc-fsl-sai-objs := fsl-sai.o
And I think it should be better to put it along with fsl-ssi.o and fsl-spdif.o
+
+obj-$(CONFIG_SND_SOC_FSL_SAI) += snd-soc-fsl-sai.o
Ditto
diff --git a/sound/soc/fsl/fsl-sai.c b/sound/soc/fsl/fsl-sai.c
new file mode 100644
index 000..bb57e67
--- /dev/null
+++ b/sound/soc/fsl/fsl-sai.c
@@ -0,0 +1,472 @@
+/*
+ * Freescale SAI ALSA SoC Digital Audio Interface driver.
+ *
+ * Copyright 2012-2013 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
There're too many double space inside. Could you pls revise it?
+ *
+ */
+
+#include linux/clk.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/of_address.h
+#include sound/core.h
+#include sound/pcm_params.h
+#include linux/delay.h
+#include linux/dmaengine.h
+#include sound/dmaengine_pcm.h
I think it's better to keep sound/xxx.h together. And basically
we can keep header files ordered by alphabet.
+
+#include fsl-sai.h
+
+static int fsl_sai_set_dai_sysclk_tr(struct snd_soc_dai *cpu_dai,
+ int clk_id, unsigned int freq, int fsl_dir)
+{
+ u32 val_cr2, reg_cr2;
+ struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
+
+ if (fsl_dir == FSL_FMT_TRANSMITTER)
+ reg_cr2 = FSL_SAI_TCR2;
+ else
+ reg_cr2 = FSL_SAI_RCR2;
+
+ val_cr2 = readl(sai-base + reg_cr2);
+ switch (clk_id) {
+ case FSL_SAI_CLK_BUS:
+ val_cr2 = ~FSL_SAI_CR2_MSEL_MASK;
+ val_cr2 |= FSL_SAI_CR2_MSEL_BUS;
+ break;
+ case FSL_SAI_CLK_MAST1:
+ val_cr2 = ~FSL_SAI_CR2_MSEL_MASK;
+ val_cr2 |= FSL_SAI_CR2_MSEL_MCLK1;
+ break;
+ case FSL_SAI_CLK_MAST2:
+ val_cr2 = ~FSL_SAI_CR2_MSEL_MASK;
+ val_cr2 |= FSL_SAI_CR2_MSEL_MCLK2;
+ break;
+ case FSL_SAI_CLK_MAST3:
+ val_cr2 = ~FSL_SAI_CR2_MSEL_MASK;
+ val_cr2 |= FSL_SAI_CR2_MSEL_MCLK3;
+ break;
+ default:
+ return -EINVAL;
+ }
+ writel(val_cr2, sai-base + reg_cr2);
+
+ return 0;
+}
+
+static int fsl_sai_set_dai_sysclk(struct snd_soc_dai *cpu_dai,
+ int clk_id, unsigned int freq, int dir)
+{
+ int ret;
+
+ if (dir == SND_SOC_CLOCK_IN)
+ return 0;
+
+ ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq,
+ FSL_FMT_TRANSMITTER);
+ if (ret) {
+ dev_err(cpu_dai-dev,
+ Cannot set sai's transmitter sysclk: %d\n,
+ ret);
+ return ret;
+ }
+
+ ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq,
+
Re: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
On Fri, Nov 01, 2013 at 03:04:48PM +0800, Xiubo Li wrote:
+static int fsl_sai_set_dai_clkdiv(struct snd_soc_dai *cpu_dai,
+ int div_id, int div)
+{
+ struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
+ u32 tcr2, rcr2;
+
+ if (div_id == FSL_SAI_TX_DIV) {
+ tcr2 = readl(sai-base + FSL_SAI_TCR2);
+ tcr2 = ~FSL_SAI_CR2_DIV_MASK;
+ tcr2 |= FSL_SAI_CR2_DIV(div);
+ writel(tcr2, sai-base + FSL_SAI_TCR2);
What is this divider and why does the user have to set it manually?
+ } else
+ return -EINVAL;
+
Coding style?
+static int fsl_sai_dai_probe(struct snd_soc_dai *dai)
+{
+ int ret;
+ struct fsl_sai *sai = dev_get_drvdata(dai-dev);
+
+ ret = clk_prepare_enable(sai-clk);
+ if (ret)
+ return ret;
It'd be nicer to only enable the clock while the device is in active
use.
+ ret = snd_dmaengine_pcm_register(pdev-dev, NULL,
+ SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);
+ if (ret)
+ return ret;
We should have a devm_ version of this.
signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
