[alsa-devel] [PATCH 00/10] ASoC: ARM: ux500: Better prepare for full Device Tree
At this moment in time the Ux500 ASoC driver obtains its DMA information via AUXDATA platform data passing. To make the driver more independent we have to start extracting it from the Device Tree. This patch-set goes some way to ensure the ux500 Audio driver is ready for the switch over.
arch/arm/mach-ux500/board-mop500-audio.c | 8 +++---- include/linux/platform_data/asoc-ux500-msp.h | 9 +------- sound/soc/codecs/ab8500-codec.c | 4 ++-- sound/soc/ux500/mop500.c | 2 ++ sound/soc/ux500/ux500_msp_dai.c | 146 +++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------------------- sound/soc/ux500/ux500_msp_i2s.c | 56 ++++++++++++++++++++++++++++++++------------ sound/soc/ux500/ux500_msp_i2s.h | 2 +- sound/soc/ux500/ux500_pcm.c | 50 ++++++++++++++++++++++++++-------------- 8 files changed, 144 insertions(+), 133 deletions(-)
Signed-off-by: Lee Jones lee.jones@linaro.org --- arch/arm/mach-ux500/board-mop500-audio.c | 8 ++++---- include/linux/platform_data/asoc-ux500-msp.h | 9 +-------- sound/soc/ux500/ux500_msp_i2s.h | 2 +- 3 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/arch/arm/mach-ux500/board-mop500-audio.c b/arch/arm/mach-ux500/board-mop500-audio.c index 154e15f..43d6cb8 100644 --- a/arch/arm/mach-ux500/board-mop500-audio.c +++ b/arch/arm/mach-ux500/board-mop500-audio.c @@ -31,7 +31,7 @@ static struct stedma40_chan_cfg msp0_dma_tx = { };
struct msp_i2s_platform_data msp0_platform_data = { - .id = MSP_I2S_0, + .id = 0, .msp_i2s_dma_rx = &msp0_dma_rx, .msp_i2s_dma_tx = &msp0_dma_tx, }; @@ -49,7 +49,7 @@ static struct stedma40_chan_cfg msp1_dma_tx = { };
struct msp_i2s_platform_data msp1_platform_data = { - .id = MSP_I2S_1, + .id = 1, .msp_i2s_dma_rx = NULL, .msp_i2s_dma_tx = &msp1_dma_tx, }; @@ -69,13 +69,13 @@ static struct stedma40_chan_cfg msp2_dma_tx = { };
struct msp_i2s_platform_data msp2_platform_data = { - .id = MSP_I2S_2, + .id = 2, .msp_i2s_dma_rx = &msp2_dma_rx, .msp_i2s_dma_tx = &msp2_dma_tx, };
struct msp_i2s_platform_data msp3_platform_data = { - .id = MSP_I2S_3, + .id = 3, .msp_i2s_dma_rx = &msp1_dma_rx, .msp_i2s_dma_tx = NULL, }; diff --git a/include/linux/platform_data/asoc-ux500-msp.h b/include/linux/platform_data/asoc-ux500-msp.h index 9991aea..2f34bb9 100644 --- a/include/linux/platform_data/asoc-ux500-msp.h +++ b/include/linux/platform_data/asoc-ux500-msp.h @@ -10,16 +10,9 @@
#include <linux/platform_data/dma-ste-dma40.h>
-enum msp_i2s_id { - MSP_I2S_0 = 0, - MSP_I2S_1, - MSP_I2S_2, - MSP_I2S_3, -}; - /* Platform data structure for a MSP I2S-device */ struct msp_i2s_platform_data { - enum msp_i2s_id id; + int id; struct stedma40_chan_cfg *msp_i2s_dma_rx; struct stedma40_chan_cfg *msp_i2s_dma_tx; }; diff --git a/sound/soc/ux500/ux500_msp_i2s.h b/sound/soc/ux500/ux500_msp_i2s.h index 258d0bce..875de0f 100644 --- a/sound/soc/ux500/ux500_msp_i2s.h +++ b/sound/soc/ux500/ux500_msp_i2s.h @@ -475,7 +475,7 @@ struct ux500_msp_dma_params { };
struct ux500_msp { - enum msp_i2s_id id; + int id; void __iomem *registers; struct device *dev; struct ux500_msp_dma_params playback_dma_data;
On Thu, Dec 19, 2013 at 4:54 PM, Lee Jones lee.jones@linaro.org wrote:
Signed-off-by: Lee Jones lee.jones@linaro.org
I guess it's best to merge this with the other ASoC patches through Mark's tree? Acked-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
On Tue, 07 Jan 2014, Linus Walleij wrote:
On Thu, Dec 19, 2013 at 4:54 PM, Lee Jones lee.jones@linaro.org wrote:
Signed-off-by: Lee Jones lee.jones@linaro.org
I guess it's best to merge this with the other ASoC patches through Mark's tree? Acked-by: Linus Walleij linus.walleij@linaro.org
I'm happy with that, although I haven't heard from Mark for ages.
Mark, are you still tracking this?
On Tue, Jan 07, 2014 at 02:11:54PM +0000, Lee Jones wrote:
Acked-by: Linus Walleij linus.walleij@linaro.org
I'm happy with that, although I haven't heard from Mark for ages.
Mark, are you still tracking this?
Yes, I've been waiting for both Linus and your testing of Takashi's fix.
On Tue, 07 Jan 2014, Mark Brown wrote:
On Tue, Jan 07, 2014 at 02:11:54PM +0000, Lee Jones wrote:
Acked-by: Linus Walleij linus.walleij@linaro.org
I'm happy with that, although I haven't heard from Mark for ages.
Mark, are you still tracking this?
Yes, I've been waiting for both Linus and your testing of Takashi's fix.
Hmm... yes I need to get round to that, but it's not dependant on the other patches in the set.
On Thu, Dec 19, 2013 at 03:54:58PM +0000, Lee Jones wrote:
Signed-off-by: Lee Jones lee.jones@linaro.org
Applied, thanks.
Commit '166a34d ASoC: ab8500: Fix invalid cast to long pointer' rather carelessly converts find_next_bit() to fls() (find last bit set), which are not the same.
Although this implementation may not be portable, the alternative breaks audio on anything using the AB8500 CODEC, which is a much greater evil. Besides, this driver will never run on anything else.
Cc: Takashi Iwai tiwai@suse.de Signed-off-by: Lee Jones lee.jones@linaro.org --- sound/soc/codecs/ab8500-codec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c index 1ad92cb..51ae3e9 100644 --- a/sound/soc/codecs/ab8500-codec.c +++ b/sound/soc/codecs/ab8500-codec.c @@ -2322,7 +2322,7 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai, slot = ffs(tx_mask); snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, slot); - slot = fls(tx_mask); + slot = find_next_bit((unsigned long *)&tx_mask, 32, slot + 1); snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, slot); break; @@ -2364,7 +2364,7 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai, AB8500_ADSLOTSEL(slot), AB8500_MASK_SLOT(slot), AB8500_ADSLOTSELX_AD_OUT_TO_SLOT(AB8500_AD_OUT3, slot)); - slot = fls(rx_mask); + slot = find_next_bit((unsigned long *)&rx_mask, 32, slot + 1); snd_soc_update_bits(codec, AB8500_ADSLOTSEL(slot), AB8500_MASK_SLOT(slot),
At Thu, 19 Dec 2013 15:54:59 +0000, Lee Jones wrote:
Commit '166a34d ASoC: ab8500: Fix invalid cast to long pointer' rather carelessly converts find_next_bit() to fls() (find last bit set), which are not the same.
Does it break on the real machines?
fls() behaves differently from find_next_bit(), of course, but in this case, it should work same in the end, since there are at most two bits.
Takashi
Although this implementation may not be portable, the alternative breaks audio on anything using the AB8500 CODEC, which is a much greater evil. Besides, this driver will never run on anything else.
Cc: Takashi Iwai tiwai@suse.de Signed-off-by: Lee Jones lee.jones@linaro.org
sound/soc/codecs/ab8500-codec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c index 1ad92cb..51ae3e9 100644 --- a/sound/soc/codecs/ab8500-codec.c +++ b/sound/soc/codecs/ab8500-codec.c @@ -2322,7 +2322,7 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai, slot = ffs(tx_mask); snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, slot);
slot = fls(tx_mask);
slot = find_next_bit((unsigned long *)&tx_mask, 32, slot + 1);@@ -2364,7 +2364,7 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai, AB8500_ADSLOTSEL(slot), AB8500_MASK_SLOT(slot), AB8500_ADSLOTSELX_AD_OUT_TO_SLOT(AB8500_AD_OUT3, slot));
slot = fls(rx_mask);
slot = find_next_bit((unsigned long *)&rx_mask, 32, slot + 1);-- 1.8.3.2
Commit '166a34d ASoC: ab8500: Fix invalid cast to long pointer' rather carelessly converts find_next_bit() to fls() (find last bit set), which are not the same.
Does it break on the real machines?
It does, that's how I found the bug.
fls() behaves differently from find_next_bit(), of course, but in this case, it should work same in the end, since there are at most two bits.
Unfortunately it doesn't work at all. This patch brings audio back to a working state. It took me the best part of a day to track down the issue. :(
Although this implementation may not be portable, the alternative breaks audio on anything using the AB8500 CODEC, which is a much greater evil. Besides, this driver will never run on anything else.
Cc: Takashi Iwai tiwai@suse.de Signed-off-by: Lee Jones lee.jones@linaro.org
sound/soc/codecs/ab8500-codec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c index 1ad92cb..51ae3e9 100644 --- a/sound/soc/codecs/ab8500-codec.c +++ b/sound/soc/codecs/ab8500-codec.c @@ -2322,7 +2322,7 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai, slot = ffs(tx_mask); snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, slot);
slot = fls(tx_mask);
slot = find_next_bit((unsigned long *)&tx_mask, 32, slot + 1);@@ -2364,7 +2364,7 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai, AB8500_ADSLOTSEL(slot), AB8500_MASK_SLOT(slot), AB8500_ADSLOTSELX_AD_OUT_TO_SLOT(AB8500_AD_OUT3, slot));
slot = fls(rx_mask);
slot = find_next_bit((unsigned long *)&rx_mask, 32, slot + 1);
At Thu, 19 Dec 2013 16:48:24 +0000, Lee Jones wrote:
Commit '166a34d ASoC: ab8500: Fix invalid cast to long pointer' rather carelessly converts find_next_bit() to fls() (find last bit set), which are not the same.
Does it break on the real machines?
It does, that's how I found the bug.
fls() behaves differently from find_next_bit(), of course, but in this case, it should work same in the end, since there are at most two bits.
Unfortunately it doesn't work at all. This patch brings audio back to a working state. It took me the best part of a day to track down the issue. :(
Hmm, then isn't the original code rather buggy?
Check the values set there by ffs(), fls() and the original find_next_bit(), especially whether find_next_bit() gives a valid value fitting with the mask bits.
Takashi
Commit '166a34d ASoC: ab8500: Fix invalid cast to long pointer' rather carelessly converts find_next_bit() to fls() (find last bit set), which are not the same.
Does it break on the real machines?
It does, that's how I found the bug.
fls() behaves differently from find_next_bit(), of course, but in this case, it should work same in the end, since there are at most two bits.
Unfortunately it doesn't work at all. This patch brings audio back to a working state. It took me the best part of a day to track down the issue. :(
Hmm, then isn't the original code rather buggy?
Check the values set there by ffs(), fls() and the original find_next_bit(), especially whether find_next_bit() gives a valid value fitting with the mask bits.
I wish I had the time to delve into exactly what's happening. All I can tell you is your patch broke audio and this one fixes it again. If you have time for a deep dive and produce a permanent solution I'd be much obliged and will of course test anything you send me. In the mean time, this patch should definitely be applied, as sound is crippled without it.
At Fri, 20 Dec 2013 14:02:11 +0000, Lee Jones wrote:
Commit '166a34d ASoC: ab8500: Fix invalid cast to long pointer' rather carelessly converts find_next_bit() to fls() (find last bit set), which are not the same.
Does it break on the real machines?
It does, that's how I found the bug.
fls() behaves differently from find_next_bit(), of course, but in this case, it should work same in the end, since there are at most two bits.
Unfortunately it doesn't work at all. This patch brings audio back to a working state. It took me the best part of a day to track down the issue. :(
Hmm, then isn't the original code rather buggy?
Check the values set there by ffs(), fls() and the original find_next_bit(), especially whether find_next_bit() gives a valid value fitting with the mask bits.
I wish I had the time to delve into exactly what's happening. All I can tell you is your patch broke audio and this one fixes it again. If you have time for a deep dive and produce a permanent solution I'd be much obliged and will of course test anything you send me. In the mean time, this patch should definitely be applied, as sound is crippled without it.
The driver code was buggy from the beginning, but it looked as if casually working in stereo mode. May patch corrected things to what it should be :)
I already have a fix patch, but need to check the information above at first to see whether it can be used as is. So, check the value set there, and ffs(), fls() and find_next_bit() values. Also, check whether the stereo left/right channels are really output as expected with the original driver code.
Takashi
At Thu, 19 Dec 2013 18:29:09 +0100, Takashi Iwai wrote:
At Thu, 19 Dec 2013 16:48:24 +0000, Lee Jones wrote:
Commit '166a34d ASoC: ab8500: Fix invalid cast to long pointer' rather carelessly converts find_next_bit() to fls() (find last bit set), which are not the same.
Does it break on the real machines?
It does, that's how I found the bug.
fls() behaves differently from find_next_bit(), of course, but in this case, it should work same in the end, since there are at most two bits.
Unfortunately it doesn't work at all. This patch brings audio back to a working state. It took me the best part of a day to track down the issue. :(
Hmm, then isn't the original code rather buggy?
Check the values set there by ffs(), fls() and the original find_next_bit(), especially whether find_next_bit() gives a valid value fitting with the mask bits.
Meanwhile, it's not good to keep the broken code in the upstream, and the bug is obvious. Below is the fixed patch, applicable with git am scissors option. Lee, let me know if this still doesn't fix.
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ASoC: ab8500: Back to first_find_bit() & co
ffs() and fls() give the off-by-one value in comparison with find_*_bit() helpers, and find_first_bit() and find_next_bit() are more intuitive in the case of two bits. So, back to find_*_bit() but call them properly without invalid cast. This fixes the slot assignment regression introduced by commit 166a34d27fca.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/soc/codecs/ab8500-codec.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c index 1ad92cbf0b24..cf53c1e2fabc 100644 --- a/sound/soc/codecs/ab8500-codec.c +++ b/sound/soc/codecs/ab8500-codec.c @@ -2242,6 +2242,7 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai, { struct snd_soc_codec *codec = dai->codec; unsigned int val, mask, slot, slots_active; + unsigned long tmp;
mask = BIT(AB8500_DIGIFCONF2_IF0WL0) | BIT(AB8500_DIGIFCONF2_IF0WL1); @@ -2308,21 +2309,22 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai, dev_dbg(dai->codec->dev, "%s: Slots, active, TX: %d\n", __func__, slots_active);
+ tmp = tx_mask; switch (slots_active) { case 0: break; case 1: - slot = ffs(tx_mask); + slot = find_first_bit(&tmp, 32); snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, slot); break; case 2: - slot = ffs(tx_mask); + slot = find_first_bit(&tmp, 32); snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, slot); - slot = fls(tx_mask); + slot = find_next_bit(&tmp, 32, slot + 1); snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, slot); break; @@ -2349,22 +2351,23 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai, dev_dbg(dai->codec->dev, "%s: Slots, active, RX: %d\n", __func__, slots_active);
+ tmp = rx_mask; switch (slots_active) { case 0: break; case 1: - slot = ffs(rx_mask); + slot = find_first_bit(&tmp, 32); snd_soc_update_bits(codec, AB8500_ADSLOTSEL(slot), AB8500_MASK_SLOT(slot), AB8500_ADSLOTSELX_AD_OUT_TO_SLOT(AB8500_AD_OUT3, slot)); break; case 2: - slot = ffs(rx_mask); + slot = find_first_bit(&tmp, 32); snd_soc_update_bits(codec, AB8500_ADSLOTSEL(slot), AB8500_MASK_SLOT(slot), AB8500_ADSLOTSELX_AD_OUT_TO_SLOT(AB8500_AD_OUT3, slot)); - slot = fls(rx_mask); + slot = find_next_bit(&tmp, 32, slot + 1); snd_soc_update_bits(codec, AB8500_ADSLOTSEL(slot), AB8500_MASK_SLOT(slot),
These drivers will not work without platform specific data, which is passed in via Device Tree or Platform Data. To avoid the chance of NULL pointer dereferencing and alike, let's ensure we have at least one of the methods in play before continuing.
Signed-off-by: Lee Jones lee.jones@linaro.org --- sound/soc/ux500/ux500_msp_dai.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/sound/soc/ux500/ux500_msp_dai.c b/sound/soc/ux500/ux500_msp_dai.c index c6fb5cc..bc042cc 100644 --- a/sound/soc/ux500/ux500_msp_dai.c +++ b/sound/soc/ux500/ux500_msp_dai.c @@ -771,10 +771,14 @@ static const struct snd_soc_component_driver ux500_msp_component = { static int ux500_msp_drv_probe(struct platform_device *pdev) { struct ux500_msp_i2s_drvdata *drvdata; + struct msp_i2s_platform_data *pdata = pdev->dev.platform_data; + struct device_node *np = pdev->dev.of_node; int ret = 0;
- dev_dbg(&pdev->dev, "%s: Enter (pdev->name = %s).\n", __func__, - pdev->name); + if (!pdata && !np) { + dev_err(&pdev->dev, "No platform data or Device Tree found\n"); + return -ENODEV; + }
drvdata = devm_kzalloc(&pdev->dev, sizeof(struct ux500_msp_i2s_drvdata),
On Thu, Dec 19, 2013 at 03:55:00PM +0000, Lee Jones wrote:
These drivers will not work without platform specific data, which is passed in via Device Tree or Platform Data. To avoid the chance of NULL pointer dereferencing and alike, let's ensure we have at least one of the methods in play before continuing.
Applied, thanks.
We're getting closer to fully enabling the Ux500 ASoC driver for Device Tree. When we switch over from using AUXDATA we'll need to match platform by only Device Tree nodes. In this patch we NULL out the platform_name, and supply nodes for each platform device.
Acked-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Lee Jones lee.jones@linaro.org --- sound/soc/ux500/mop500.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/ux500/mop500.c b/sound/soc/ux500/mop500.c index 178d1ba..b3b66aa 100644 --- a/sound/soc/ux500/mop500.c +++ b/sound/soc/ux500/mop500.c @@ -91,6 +91,8 @@ static int mop500_of_probe(struct platform_device *pdev, for (i = 0; i < 2; i++) { mop500_dai_links[i].cpu_of_node = msp_np[i]; mop500_dai_links[i].cpu_dai_name = NULL; + mop500_dai_links[i].platform_of_node = msp_np[i]; + mop500_dai_links[i].platform_name = NULL; mop500_dai_links[i].codec_of_node = codec_np; mop500_dai_links[i].codec_name = NULL; }
On Thu, Dec 19, 2013 at 03:55:01PM +0000, Lee Jones wrote:
We're getting closer to fully enabling the Ux500 ASoC driver for Device Tree. When we switch over from using AUXDATA we'll need to match platform by only Device Tree nodes. In this patch we NULL out the platform_name, and supply nodes for each platform device.
Applied, thanks.
The Slave Config's addr_width attribute is populated by data_width of dma_cfg, which in turn is derived from dma_params' data_size attribute and that comes from the slot_width which is always 16 bits (2 Bytes). We're cutting out the middle man here and just setting the DMA Slave Config directly.
Signed-off-by: Lee Jones lee.jones@linaro.org --- sound/soc/ux500/ux500_pcm.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/sound/soc/ux500/ux500_pcm.c b/sound/soc/ux500/ux500_pcm.c index ce554de..32d4572 100644 --- a/sound/soc/ux500/ux500_pcm.c +++ b/sound/soc/ux500/ux500_pcm.c @@ -109,20 +109,19 @@ static int ux500_pcm_prepare_slave_config(struct snd_pcm_substream *substream, { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct ux500_msp_dma_params *dma_params; - struct stedma40_chan_cfg *dma_cfg; int ret;
dma_params = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream); - dma_cfg = dma_params->dma_cfg;
ret = snd_hwparams_to_dma_slave_config(substream, params, slave_config); if (ret) return ret;
slave_config->dst_maxburst = 4; - slave_config->dst_addr_width = dma_cfg->dst_info.data_width; slave_config->src_maxburst = 4; - slave_config->src_addr_width = dma_cfg->src_info.data_width; + + slave_config->src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES; + slave_config->dst_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) slave_config->dst_addr = dma_params->tx_rx_addr;
On Thu, Dec 19, 2013 at 03:55:02PM +0000, Lee Jones wrote:
The Slave Config's addr_width attribute is populated by data_width of dma_cfg, which in turn is derived from dma_params' data_size attribute and that comes from the slot_width which is always 16 bits (2 Bytes). We're cutting out the middle man here and just setting the DMA Slave Config directly.
Applied, thanks.
In preparation for full Device Tree enablement we must differentiate between the two varying ways DMA data can be held in the DAI store. If we're booting with Device Tree the provided 'snd_dmaengine_dai_dma_data' data structure shall be used, whereas in order to avoid breaking legacy platform data we also need to be able to translate DMA data stored using the UX500 specific 'ux500_msp_dma_params' method.
Once we move over to solely use Device Tree, we can enforce the use of 'snd_dmaengine_dai_dma_data' and this code can be removed altogether.
Signed-off-by: Lee Jones lee.jones@linaro.org --- sound/soc/ux500/ux500_pcm.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/sound/soc/ux500/ux500_pcm.c b/sound/soc/ux500/ux500_pcm.c index 32d4572..8b53f22 100644 --- a/sound/soc/ux500/ux500_pcm.c +++ b/sound/soc/ux500/ux500_pcm.c @@ -108,10 +108,21 @@ static int ux500_pcm_prepare_slave_config(struct snd_pcm_substream *substream, struct dma_slave_config *slave_config) { struct snd_soc_pcm_runtime *rtd = substream->private_data; - struct ux500_msp_dma_params *dma_params; + struct msp_i2s_platform_data *pdata = rtd->cpu_dai->dev->platform_data; + struct snd_dmaengine_dai_dma_data *snd_dma_params; + struct ux500_msp_dma_params *ste_dma_params; + dma_addr_t dma_addr; int ret;
- dma_params = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream); + if (pdata) { + ste_dma_params = + snd_soc_dai_get_dma_data(rtd->cpu_dai, substream); + dma_addr = ste_dma_params->tx_rx_addr; + } else { + snd_dma_params = + snd_soc_dai_get_dma_data(rtd->cpu_dai, substream); + dma_addr = snd_dma_params->addr; + }
ret = snd_hwparams_to_dma_slave_config(substream, params, slave_config); if (ret) @@ -124,9 +135,9 @@ static int ux500_pcm_prepare_slave_config(struct snd_pcm_substream *substream, slave_config->dst_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - slave_config->dst_addr = dma_params->tx_rx_addr; + slave_config->dst_addr = dma_addr; else - slave_config->src_addr = dma_params->tx_rx_addr; + slave_config->src_addr = dma_addr;
return 0; }
On Thu, Dec 19, 2013 at 03:55:03PM +0000, Lee Jones wrote:
In preparation for full Device Tree enablement we must differentiate between the two varying ways DMA data can be held in the DAI store. If we're booting with Device Tree the provided 'snd_dmaengine_dai_dma_data' data structure shall be used, whereas in order to avoid breaking legacy platform data we also need to be able to translate DMA data stored using the UX500 specific 'ux500_msp_dma_params' method.
Applied, thanks.
Soon we will strip out pdata support from the Ux500 set of ASoC drivers. When this happens it will have to supply a DMA slave_config to the dmaengine. At the moment a great deal of this comes from pdata via AUXDATA. We need to become independent of this soon. This patch starts the process by allocating memory for the associated data structures and fetches the MSP id used for const struct indexing.
Acked-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Lee Jones lee.jones@linaro.org --- sound/soc/ux500/ux500_msp_i2s.c | 56 ++++++++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 15 deletions(-)
diff --git a/sound/soc/ux500/ux500_msp_i2s.c b/sound/soc/ux500/ux500_msp_i2s.c index 1ca8b08..7f2a4ac 100644 --- a/sound/soc/ux500/ux500_msp_i2s.c +++ b/sound/soc/ux500/ux500_msp_i2s.c @@ -646,6 +646,34 @@ int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
}
+int ux500_msp_i2s_of_init_msp(struct platform_device *pdev, + struct ux500_msp *msp, + struct msp_i2s_platform_data **platform_data) +{ + struct msp_i2s_platform_data *pdata; + + *platform_data = devm_kzalloc(&pdev->dev, + sizeof(struct msp_i2s_platform_data), + GFP_KERNEL); + pdata = *platform_data; + if (!pdata) + return -ENOMEM; + + msp->playback_dma_data.dma_cfg = devm_kzalloc(&pdev->dev, + sizeof(struct stedma40_chan_cfg), + GFP_KERNEL); + if (!msp->playback_dma_data.dma_cfg) + return -ENOMEM; + + msp->capture_dma_data.dma_cfg = devm_kzalloc(&pdev->dev, + sizeof(struct stedma40_chan_cfg), + GFP_KERNEL); + if (!msp->capture_dma_data.dma_cfg) + return -ENOMEM; + + return 0; +} + int ux500_msp_i2s_init_msp(struct platform_device *pdev, struct ux500_msp **msp_p, struct msp_i2s_platform_data *platform_data) @@ -653,30 +681,28 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev, struct resource *res = NULL; struct device_node *np = pdev->dev.of_node; struct ux500_msp *msp; + int ret;
*msp_p = devm_kzalloc(&pdev->dev, sizeof(struct ux500_msp), GFP_KERNEL); msp = *msp_p; if (!msp) return -ENOMEM;
- if (np) { - if (!platform_data) { - platform_data = devm_kzalloc(&pdev->dev, - sizeof(struct msp_i2s_platform_data), GFP_KERNEL); - if (!platform_data) - return -ENOMEM; - } - } else - if (!platform_data) + if (!platform_data) { + if (np) { + ret = ux500_msp_i2s_of_init_msp(pdev, msp, + &platform_data); + if (ret) + return ret; + } else return -EINVAL; + } else { + msp->playback_dma_data.dma_cfg = platform_data->msp_i2s_dma_tx; + msp->capture_dma_data.dma_cfg = platform_data->msp_i2s_dma_rx; + msp->id = platform_data->id; + }
- dev_dbg(&pdev->dev, "%s: Enter (name: %s, id: %d).\n", __func__, - pdev->name, platform_data->id); - - msp->id = platform_data->id; msp->dev = &pdev->dev; - msp->playback_dma_data.dma_cfg = platform_data->msp_i2s_dma_tx; - msp->capture_dma_data.dma_cfg = platform_data->msp_i2s_dma_rx;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (res == NULL) {
On Thu, Dec 19, 2013 at 03:55:04PM +0000, Lee Jones wrote:
Soon we will strip out pdata support from the Ux500 set of ASoC drivers. When this happens it will have to supply a DMA slave_config to the dmaengine. At the moment a great deal of this comes from pdata via AUXDATA. We need to become independent of this soon. This patch starts the process by allocating memory for the associated data structures and fetches the MSP id used for const struct indexing.
Applied, thanks.
In this patch we do two things. Firstly, instead of open coding the store of DMA data in to the DAI for later use, we use the API provided. Secondly we create and store similar DMA data for the DT case, only this time we use 'struct snd_dmaengine_dai_dma_data' which is provided by the core for this very reason.
Acked-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Lee Jones lee.jones@linaro.org --- sound/soc/ux500/ux500_msp_dai.c | 42 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)
diff --git a/sound/soc/ux500/ux500_msp_dai.c b/sound/soc/ux500/ux500_msp_dai.c index bc042cc..f4d607a 100644 --- a/sound/soc/ux500/ux500_msp_dai.c +++ b/sound/soc/ux500/ux500_msp_dai.c @@ -17,12 +17,14 @@ #include <linux/bitops.h> #include <linux/platform_device.h> #include <linux/clk.h> +#include <linux/of.h> #include <linux/regulator/consumer.h> #include <linux/mfd/dbx500-prcmu.h> #include <linux/platform_data/asoc-ux500-msp.h>
#include <sound/soc.h> #include <sound/soc-dai.h> +#include <sound/dmaengine_pcm.h>
#include "ux500_msp_i2s.h" #include "ux500_msp_dai.h" @@ -654,16 +656,52 @@ static int ux500_msp_dai_trigger(struct snd_pcm_substream *substream, return ret; }
+static int ux500_msp_dai_of_probe(struct snd_soc_dai *dai) +{ + struct ux500_msp_i2s_drvdata *drvdata = dev_get_drvdata(dai->dev); + struct snd_dmaengine_dai_dma_data *playback_dma_data; + struct snd_dmaengine_dai_dma_data *capture_dma_data; + + playback_dma_data = devm_kzalloc(dai->dev, + sizeof(*playback_dma_data), + GFP_KERNEL); + if (!playback_dma_data) + return -ENOMEM; + + capture_dma_data = devm_kzalloc(dai->dev, + sizeof(*capture_dma_data), + GFP_KERNEL); + if (!capture_dma_data) + return -ENOMEM; + + playback_dma_data->addr = drvdata->msp->playback_dma_data.tx_rx_addr; + capture_dma_data->addr = drvdata->msp->capture_dma_data.tx_rx_addr; + + playback_dma_data->maxburst = 4; + capture_dma_data->maxburst = 4; + + snd_soc_dai_init_dma_data(dai, playback_dma_data, capture_dma_data); + + return 0; +} + static int ux500_msp_dai_probe(struct snd_soc_dai *dai) { struct ux500_msp_i2s_drvdata *drvdata = dev_get_drvdata(dai->dev); + struct msp_i2s_platform_data *pdata = dai->dev->platform_data; + int ret;
- dai->playback_dma_data = &drvdata->msp->playback_dma_data; - dai->capture_dma_data = &drvdata->msp->capture_dma_data; + if (!pdata) { + ret = ux500_msp_dai_of_probe(dai); + return ret; + }
drvdata->msp->playback_dma_data.data_size = drvdata->slot_width; drvdata->msp->capture_dma_data.data_size = drvdata->slot_width;
+ snd_soc_dai_init_dma_data(dai, + &drvdata->msp->playback_dma_data, + &drvdata->msp->capture_dma_data); return 0; }
On Thu, Dec 19, 2013 at 03:55:05PM +0000, Lee Jones wrote:
In this patch we do two things. Firstly, instead of open coding the store of DMA data in to the DAI for later use, we use the API provided. Secondly we create and store similar DMA data for the DT case, only this time we use 'struct snd_dmaengine_dai_dma_data' which is provided by the core for this very reason.
Applied, thanks.
Acked-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Lee Jones lee.jones@linaro.org --- sound/soc/ux500/ux500_pcm.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/sound/soc/ux500/ux500_pcm.c b/sound/soc/ux500/ux500_pcm.c index 8b53f22..3d1c342 100644 --- a/sound/soc/ux500/ux500_pcm.c +++ b/sound/soc/ux500/ux500_pcm.c @@ -65,14 +65,10 @@ static struct dma_chan *ux500_pcm_request_chan(struct snd_soc_pcm_runtime *rtd, struct snd_pcm_substream *substream) { struct snd_soc_dai *dai = rtd->cpu_dai; - struct device *dev = dai->dev; u16 per_data_width, mem_data_width; struct stedma40_chan_cfg *dma_cfg; struct ux500_msp_dma_params *dma_params;
- dev_dbg(dev, "%s: MSP %d (%s): Enter.\n", __func__, dai->id, - snd_pcm_stream_str(substream)); - dma_params = snd_soc_dai_get_dma_data(dai, substream); dma_cfg = dma_params->dma_cfg;
On Thu, Dec 19, 2013 at 03:55:06PM +0000, Lee Jones wrote:
Acked-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Lee Jones lee.jones@linaro.org
Applied, thanks.
If booting with full DT support (i.e. DMA too, the last piece of the puzzle), then we don't need to use the compatible_request_channel call back or require some of the historical bumph which probably isn't required by a platform data start-up now either. This will also be ripped out in upcoming commits.
Acked-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Lee Jones lee.jones@linaro.org --- sound/soc/ux500/ux500_pcm.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/sound/soc/ux500/ux500_pcm.c b/sound/soc/ux500/ux500_pcm.c index 3d1c342..55a8634 100644 --- a/sound/soc/ux500/ux500_pcm.c +++ b/sound/soc/ux500/ux500_pcm.c @@ -145,15 +145,25 @@ static const struct snd_dmaengine_pcm_config ux500_dmaengine_pcm_config = { .prepare_slave_config = ux500_pcm_prepare_slave_config, };
+static const struct snd_dmaengine_pcm_config ux500_dmaengine_of_pcm_config = { + .compat_request_channel = ux500_pcm_request_chan, + .prepare_slave_config = ux500_pcm_prepare_slave_config, +}; + int ux500_pcm_register_platform(struct platform_device *pdev) { + const struct snd_dmaengine_pcm_config *pcm_config; + struct device_node *np = pdev->dev.of_node; int ret;
- ret = snd_dmaengine_pcm_register(&pdev->dev, - &ux500_dmaengine_pcm_config, - SND_DMAENGINE_PCM_FLAG_NO_RESIDUE | - SND_DMAENGINE_PCM_FLAG_COMPAT | - SND_DMAENGINE_PCM_FLAG_NO_DT); + if (np) + pcm_config = &ux500_dmaengine_of_pcm_config; + else + pcm_config = &ux500_dmaengine_pcm_config; + + ret = snd_dmaengine_pcm_register(&pdev->dev, pcm_config, + SND_DMAENGINE_PCM_FLAG_NO_RESIDUE | + SND_DMAENGINE_PCM_FLAG_COMPAT); if (ret < 0) { dev_err(&pdev->dev, "%s: ERROR: Failed to register platform '%s' (%d)!\n",
On Thu, Dec 19, 2013 at 03:55:07PM +0000, Lee Jones wrote:
If booting with full DT support (i.e. DMA too, the last piece of the puzzle), then we don't need to use the compatible_request_channel call back or require some of the historical bumph which probably isn't required by a platform data start-up now either. This will also be ripped out in upcoming commits.
Applied, thanks.
We no longer have a means to differentiate between MSP devices at probe time, mainly because we don't really have to. So rather than have an over- sized static data structure in place, where the only difference between devices is the ID and name (which are unused), we'll just create one succinct, statically assigned and shared one instead.
Signed-off-by: Lee Jones lee.jones@linaro.org --- sound/soc/ux500/ux500_msp_dai.c | 96 ++++++----------------------------------- 1 file changed, 14 insertions(+), 82 deletions(-)
diff --git a/sound/soc/ux500/ux500_msp_dai.c b/sound/soc/ux500/ux500_msp_dai.c index f4d607a..5f4807b 100644 --- a/sound/soc/ux500/ux500_msp_dai.c +++ b/sound/soc/ux500/ux500_msp_dai.c @@ -718,87 +718,19 @@ static struct snd_soc_dai_ops ux500_msp_dai_ops[] = { } };
-static struct snd_soc_dai_driver ux500_msp_dai_drv[UX500_NBR_OF_DAI] = { - { - .name = "ux500-msp-i2s.0", - .probe = ux500_msp_dai_probe, - .id = 0, - .suspend = NULL, - .resume = NULL, - .playback = { - .channels_min = UX500_MSP_MIN_CHANNELS, - .channels_max = UX500_MSP_MAX_CHANNELS, - .rates = UX500_I2S_RATES, - .formats = UX500_I2S_FORMATS, - }, - .capture = { - .channels_min = UX500_MSP_MIN_CHANNELS, - .channels_max = UX500_MSP_MAX_CHANNELS, - .rates = UX500_I2S_RATES, - .formats = UX500_I2S_FORMATS, - }, - .ops = ux500_msp_dai_ops, - }, - { - .name = "ux500-msp-i2s.1", - .probe = ux500_msp_dai_probe, - .id = 1, - .suspend = NULL, - .resume = NULL, - .playback = { - .channels_min = UX500_MSP_MIN_CHANNELS, - .channels_max = UX500_MSP_MAX_CHANNELS, - .rates = UX500_I2S_RATES, - .formats = UX500_I2S_FORMATS, - }, - .capture = { - .channels_min = UX500_MSP_MIN_CHANNELS, - .channels_max = UX500_MSP_MAX_CHANNELS, - .rates = UX500_I2S_RATES, - .formats = UX500_I2S_FORMATS, - }, - .ops = ux500_msp_dai_ops, - }, - { - .name = "ux500-msp-i2s.2", - .id = 2, - .probe = ux500_msp_dai_probe, - .suspend = NULL, - .resume = NULL, - .playback = { - .channels_min = UX500_MSP_MIN_CHANNELS, - .channels_max = UX500_MSP_MAX_CHANNELS, - .rates = UX500_I2S_RATES, - .formats = UX500_I2S_FORMATS, - }, - .capture = { - .channels_min = UX500_MSP_MIN_CHANNELS, - .channels_max = UX500_MSP_MAX_CHANNELS, - .rates = UX500_I2S_RATES, - .formats = UX500_I2S_FORMATS, - }, - .ops = ux500_msp_dai_ops, - }, - { - .name = "ux500-msp-i2s.3", - .probe = ux500_msp_dai_probe, - .id = 3, - .suspend = NULL, - .resume = NULL, - .playback = { - .channels_min = UX500_MSP_MIN_CHANNELS, - .channels_max = UX500_MSP_MAX_CHANNELS, - .rates = UX500_I2S_RATES, - .formats = UX500_I2S_FORMATS, - }, - .capture = { - .channels_min = UX500_MSP_MIN_CHANNELS, - .channels_max = UX500_MSP_MAX_CHANNELS, - .rates = UX500_I2S_RATES, - .formats = UX500_I2S_FORMATS, - }, - .ops = ux500_msp_dai_ops, - }, +static struct snd_soc_dai_driver ux500_msp_dai_drv = { + .probe = ux500_msp_dai_probe, + .suspend = NULL, + .resume = NULL, + .playback.channels_min = UX500_MSP_MIN_CHANNELS, + .playback.channels_max = UX500_MSP_MAX_CHANNELS, + .playback.rates = UX500_I2S_RATES, + .playback.formats = UX500_I2S_FORMATS, + .capture.channels_min = UX500_MSP_MIN_CHANNELS, + .capture.channels_max = UX500_MSP_MAX_CHANNELS, + .capture.rates = UX500_I2S_RATES, + .capture.formats = UX500_I2S_FORMATS, + .ops = ux500_msp_dai_ops, };
static const struct snd_soc_component_driver ux500_msp_component = { @@ -868,7 +800,7 @@ static int ux500_msp_drv_probe(struct platform_device *pdev) dev_set_drvdata(&pdev->dev, drvdata);
ret = snd_soc_register_component(&pdev->dev, &ux500_msp_component, - &ux500_msp_dai_drv[drvdata->msp->id], 1); + &ux500_msp_dai_drv, 1); if (ret < 0) { dev_err(&pdev->dev, "Error: %s: Failed to register MSP%d!\n", __func__, drvdata->msp->id);
On Thu, Dec 19, 2013 at 03:55:08PM +0000, Lee Jones wrote:
We no longer have a means to differentiate between MSP devices at probe time, mainly because we don't really have to. So rather than have an over- sized static data structure in place, where the only difference between devices is the ID and name (which are unused), we'll just create one succinct, statically assigned and shared one instead.
Applied, thanks.
On Thu, Dec 19, 2013 at 03:54:57PM +0000, Lee Jones wrote:
At this moment in time the Ux500 ASoC driver obtains its DMA information via AUXDATA platform data passing. To make the driver more independent we have to start extracting it from the Device Tree. This patch-set goes some way to ensure the ux500 Audio driver is ready for the switch over.
This all looks good to me, I guess it's OK if I merge the ARM patch?
At this moment in time the Ux500 ASoC driver obtains its DMA information via AUXDATA platform data passing. To make the driver more independent we have to start extracting it from the Device Tree. This patch-set goes some way to ensure the ux500 Audio driver is ready for the switch over.
This all looks good to me, I guess it's OK if I merge the ARM patch?
That would be up to Linus.
On Sat, Dec 21, 2013 at 3:29 PM, Mark Brown broonie@kernel.org wrote:
On Thu, Dec 19, 2013 at 03:54:57PM +0000, Lee Jones wrote:
At this moment in time the Ux500 ASoC driver obtains its DMA information via AUXDATA platform data passing. To make the driver more independent we have to start extracting it from the Device Tree. This patch-set goes some way to ensure the ux500 Audio driver is ready for the switch over.
This all looks good to me, I guess it's OK if I merge the ARM patch?
Yes I ACKed it now.
Sorry for slow response & thanks Mark.
Yours, Linus Walleij
participants (4)
-
Lee Jones -
Linus Walleij -
Mark Brown -
Takashi Iwai