[alsa-devel] [PATCH 1/2] ASoC: p1022ds: fix incorrect referencing of device tree properties
Device tree integer properties are encoded in big-endian format, but some of the Freescale ASoC drivers were assuming that the host is in big-endian format as well. Although this is true, it's better to use endian-safe accessors.
Also add a check for a failed ioremap() call in the SSI driver.
Signed-off-by: Timur Tabi timur@freescale.com --- sound/soc/fsl/fsl_dma.c | 2 +- sound/soc/fsl/fsl_ssi.c | 9 +++++++-- sound/soc/fsl/mpc8610_hpcd.c | 10 +++++----- sound/soc/fsl/p1022_ds.c | 10 +++++----- 4 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/sound/soc/fsl/fsl_dma.c b/sound/soc/fsl/fsl_dma.c index 50b5df8..3872598 100644 --- a/sound/soc/fsl/fsl_dma.c +++ b/sound/soc/fsl/fsl_dma.c @@ -940,7 +940,7 @@ static int __devinit fsl_soc_dma_probe(struct platform_device *pdev)
iprop = of_get_property(ssi_np, "fsl,fifo-depth", NULL); if (iprop) - dma->ssi_fifo_depth = *iprop; + dma->ssi_fifo_depth = be32_to_cpup(iprop); else /* Older 8610 DTs didn't have the fifo-depth property */ dma->ssi_fifo_depth = 8; diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 313e0cc..d48afea 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -678,7 +678,12 @@ static int __devinit fsl_ssi_probe(struct platform_device *pdev) kfree(ssi_private); return ret; } - ssi_private->ssi = ioremap(res.start, 1 + res.end - res.start); + ssi_private->ssi = of_iomap(np, 0); + if (!ssi_private->ssi) { + dev_err(&pdev->dev, "could not map device resources\n"); + kfree(ssi_private); + return -ENOMEM; + } ssi_private->ssi_phys = res.start; ssi_private->irq = irq_of_parse_and_map(np, 0);
@@ -691,7 +696,7 @@ static int __devinit fsl_ssi_probe(struct platform_device *pdev) /* Determine the FIFO depth. */ iprop = of_get_property(np, "fsl,fifo-depth", NULL); if (iprop) - ssi_private->fifo_depth = *iprop; + ssi_private->fifo_depth = be32_to_cpup(iprop); else /* Older 8610 DTs didn't have the fifo-depth property */ ssi_private->fifo_depth = 8; diff --git a/sound/soc/fsl/mpc8610_hpcd.c b/sound/soc/fsl/mpc8610_hpcd.c index c16c6b2..a192979 100644 --- a/sound/soc/fsl/mpc8610_hpcd.c +++ b/sound/soc/fsl/mpc8610_hpcd.c @@ -233,7 +233,7 @@ static int get_parent_cell_index(struct device_node *np) if (!iprop) return -1;
- return *iprop; + return be32_to_cpup(iprop); }
/** @@ -258,7 +258,7 @@ static int codec_node_dev_name(struct device_node *np, char *buf, size_t len) if (!iprop) return -EINVAL;
- addr = *iprop; + addr = be32_to_cpup(iprop);
bus = get_parent_cell_index(np); if (bus < 0) @@ -305,7 +305,7 @@ static int get_dma_channel(struct device_node *ssi_np, return -EINVAL; }
- *dma_channel_id = *iprop; + *dma_channel_id = be32_to_cpup(iprop); *dma_id = get_parent_cell_index(dma_channel_np); of_node_put(dma_channel_np);
@@ -379,7 +379,7 @@ static int mpc8610_hpcd_probe(struct platform_device *pdev) ret = -EINVAL; goto error; } - machine_data->ssi_id = *iprop; + machine_data->ssi_id = be32_to_cpup(iprop);
/* Get the serial format and clock direction. */ sprop = of_get_property(np, "fsl,mode", NULL); @@ -405,7 +405,7 @@ static int mpc8610_hpcd_probe(struct platform_device *pdev) ret = -EINVAL; goto error; } - machine_data->clk_frequency = *iprop; + machine_data->clk_frequency = be32_to_cpup(iprop); } else if (strcasecmp(sprop, "i2s-master") == 0) { machine_data->dai_format = SND_SOC_DAIFMT_I2S; machine_data->codec_clk_direction = SND_SOC_CLOCK_IN; diff --git a/sound/soc/fsl/p1022_ds.c b/sound/soc/fsl/p1022_ds.c index 66e0b68..8fa4d5f 100644 --- a/sound/soc/fsl/p1022_ds.c +++ b/sound/soc/fsl/p1022_ds.c @@ -232,7 +232,7 @@ static int get_parent_cell_index(struct device_node *np)
iprop = of_get_property(parent, "cell-index", NULL); if (iprop) - ret = *iprop; + ret = be32_to_cpup(iprop);
of_node_put(parent);
@@ -261,7 +261,7 @@ static int codec_node_dev_name(struct device_node *np, char *buf, size_t len) if (!iprop) return -EINVAL;
- addr = *iprop; + addr = be32_to_cpup(iprop);
bus = get_parent_cell_index(np); if (bus < 0) @@ -308,7 +308,7 @@ static int get_dma_channel(struct device_node *ssi_np, return -EINVAL; }
- *dma_channel_id = *iprop; + *dma_channel_id = be32_to_cpup(iprop); *dma_id = get_parent_cell_index(dma_channel_np); of_node_put(dma_channel_np);
@@ -379,7 +379,7 @@ static int p1022_ds_probe(struct platform_device *pdev) ret = -EINVAL; goto error; } - mdata->ssi_id = *iprop; + mdata->ssi_id = be32_to_cpup(iprop);
/* Get the serial format and clock direction. */ sprop = of_get_property(np, "fsl,mode", NULL); @@ -405,7 +405,7 @@ static int p1022_ds_probe(struct platform_device *pdev) ret = -EINVAL; goto error; } - mdata->clk_frequency = *iprop; + mdata->clk_frequency = be32_to_cpup(iprop); } else if (strcasecmp(sprop, "i2s-master") == 0) { mdata->dai_format = SND_SOC_DAIFMT_I2S; mdata->codec_clk_direction = SND_SOC_CLOCK_IN;
The DMA (PCM) driver used by some Freescale PowerPC supports separate DAIs for playback and capture, so DMA buffers should be allocated only for the initialized streams. Instead of checking for the number of active channels, which apparently is not reliable, check to see if the actual stream object exists.
Also provide a better name for the DMA interrupt.
Signed-off-by: Timur Tabi timur@freescale.com --- sound/soc/fsl/fsl_dma.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/sound/soc/fsl/fsl_dma.c b/sound/soc/fsl/fsl_dma.c index 3872598..732208c 100644 --- a/sound/soc/fsl/fsl_dma.c +++ b/sound/soc/fsl/fsl_dma.c @@ -312,7 +312,7 @@ static int fsl_dma_new(struct snd_soc_pcm_runtime *rtd) * should allocate a DMA buffer only for the streams that are valid. */
- if (dai->driver->playback.channels_min) { + if (pcm->streams[0].substream) { ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, card->dev, fsl_dma_hardware.buffer_bytes_max, &pcm->streams[0].substream->dma_buffer); @@ -322,13 +322,13 @@ static int fsl_dma_new(struct snd_soc_pcm_runtime *rtd) } }
- if (dai->driver->capture.channels_min) { + if (pcm->streams[1].substream) { ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, card->dev, fsl_dma_hardware.buffer_bytes_max, &pcm->streams[1].substream->dma_buffer); if (ret) { - snd_dma_free_pages(&pcm->streams[0].substream->dma_buffer); dev_err(card->dev, "can't alloc capture dma buffer\n"); + snd_dma_free_pages(&pcm->streams[0].substream->dma_buffer); return ret; } } @@ -451,7 +451,8 @@ static int fsl_dma_open(struct snd_pcm_substream *substream) dma_private->ld_buf_phys = ld_buf_phys; dma_private->dma_buf_phys = substream->dma_buffer.addr;
- ret = request_irq(dma_private->irq, fsl_dma_isr, 0, "DMA", dma_private); + ret = request_irq(dma_private->irq, fsl_dma_isr, 0, "fsldma-audio", + dma_private); if (ret) { dev_err(dev, "can't register ISR for IRQ %u (ret=%i)\n", dma_private->irq, ret);
On Wed, Jun 08, 2011 at 03:02:56PM -0500, Timur Tabi wrote:
The DMA (PCM) driver used by some Freescale PowerPC supports separate DAIs for playback and capture, so DMA buffers should be allocated only for the initialized streams. Instead of checking for the number of active channels, which apparently is not reliable, check to see if the actual stream object exists.
I'll apply this but...
Also provide a better name for the DMA interrupt.
...this doesn't overlap with the bugfix at all. Why have you sent it as part of the same patch?
Mark Brown wrote:
Also provide a better name for the DMA interrupt.
...this doesn't overlap with the bugfix at all. Why have you sent it as part of the same patch?
Because I didn't want to send another tiny patch for this relatively insignificant change.
But I will refrain from doing this again.
On 06/08/11 13:02, Timur Tabi wrote:
Device tree integer properties are encoded in big-endian format, but some of the Freescale ASoC drivers were assuming that the host is in big-endian format as well. Although this is true, it's better to use endian-safe accessors.
Hi Timur,
Can this be true?
I would assume a software constructed data structure would be in host-endian mode. The only reason to be concerned with endianness is if you are transmitting binary over some communications medium, or sending something to hardware (which in rare circumstances may not be host-endian).
Throwing in macros that will always be discarded (if this host is bigendian) seems unnecessary.
Regards, Steve
Also add a check for a failed ioremap() call in the SSI driver.
An urelated change should probably be in a new patch.
Signed-off-by: Timur Tabi timur@freescale.com
Steve Calfee wrote:
Device tree integer properties are encoded in big-endian format, but some of
the Freescale ASoC drivers were assuming that the host is in big-endian format as well. Although this is true, it's better to use endian-safe accessors.
Hi Timur,
Can this be true?
Yes, it's true. When the device tree compiler compiles a property that looks like this:
fsl,ssi-fifo-depth = <15>;
It writes the following bytes into the dtb:
00 00 00 0f
I would assume a software constructed data structure would be in host-endian mode.
I wouldn't assume that at all. The device tree format is a defined binary format. It makes sense that the endianness of multi-byte integers is defined.
On 08/06/11 21:02, Timur Tabi wrote:
Device tree integer properties are encoded in big-endian format, but some of the Freescale ASoC drivers were assuming that the host is in big-endian format as well. Although this is true, it's better to use endian-safe accessors.
Also add a check for a failed ioremap() call in the SSI driver.
Signed-off-by: Timur Tabi timur@freescale.com
Both
Acked-by: Liam Girdwood lrg@ti.com
On Wed, Jun 08, 2011 at 03:02:55PM -0500, Timur Tabi wrote:
Device tree integer properties are encoded in big-endian format, but some of the Freescale ASoC drivers were assuming that the host is in big-endian format as well. Although this is true, it's better to use endian-safe accessors.
Also add a check for a failed ioremap() call in the SSI driver.
Similarly this patch appears to consist of two unrelated changes. I'll apply this time but please don't do this in future.
Please also include non-urgent cleanups like this after bug fixes in your patch serieses - while the bug fixes get applied to the release branch new features and cleanups don't but ordering the cleanups first means the bugfixes may end up depending on them.
participants (5)
-
Liam Girdwood -
Mark Brown -
Steve Calfee -
Tabi Timur-B04825 -
Timur Tabi