[PATCH 1/2] ASoC: p1022ds: fix incorrect referencing of device tree properties

[PATCH 1/2] ASoC: p1022ds: fix incorrect referencing of device tree properties

[Timur Tabi]

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;
-- 
1.7.3.4

[PATCH 2/2] ASoC: fsl: fix initialization of DMA buffers

[Timur Tabi]

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);
-- 
1.7.3.4

Re: [PATCH 1/2] ASoC: p1022ds: fix incorrect referencing of device tree properties

[Steve Calfee]

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>

Re: [PATCH 1/2] ASoC: p1022ds: fix incorrect referencing of device tree properties

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 1/2] ASoC: p1022ds: fix incorrect referencing of device tree properties

[Liam Girdwood]

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>

Re: [PATCH 2/2] ASoC: fsl: fix initialization of DMA buffers

[Mark Brown]

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?

Re: [PATCH 1/2] ASoC: p1022ds: fix incorrect referencing of device tree properties

[Mark Brown]

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.

Re: [PATCH 2/2] ASoC: fsl: fix initialization of DMA buffers

[Tabi Timur-B04825]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale