[PATCH v2 0/5] ASoC: sh: rz-ssi: Code cleanup and fixes

[PATCH v2 0/5] ASoC: sh: rz-ssi: Code cleanup and fixes

[Lad Prabhakar]

Hi All,

This patch series does code cleanup and fixes to the rz-ssi driver.

Cheers,
Prabhakar

Lad Prabhakar (5):
  ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively
  ASoC: sh: rz-ssi: Make the data structures available before
    registering the handlers
  ASoC: sh: rz-ssi: Drop ssi parameter from rz_ssi_stream_init()
  ASoC: sh: rz-ssi: Change return type of rz_ssi_stream_is_valid() to
    bool
  ASoC: sh: rz-ssi: Add rz_ssi_set_substream() helper function

 sound/soc/sh/rz-ssi.c | 107 +++++++++++++++++++++---------------------
 1 file changed, 54 insertions(+), 53 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively

[Lad Prabhakar]

Instead of recursively calling rz_ssi_pio_recv() use a loop instead
to read the samples from RX fifo.

Inspiration for this patch is to avoid recursion, as recursion is
unwelcome in kernel due to limited stack use. Also to add this driver
will later be used on RZ/A2 SoC's which runs with limited memory.

This also fixes an issue where the return value of rz_ssi_pio_recv()
was ignored when called recursively.

Fixes: 03e786bd4341 ("ASoC: sh: Add RZ/G2L SSIF-2 driver")
Reported-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2
* Used a do while loop
* Fixed comments pointed by Cezary.
---
 sound/soc/sh/rz-ssi.c | 65 +++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 34 deletions(-)

diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c
index fa0cc08f70ec..637802117c6c 100644
--- a/sound/soc/sh/rz-ssi.c
+++ b/sound/soc/sh/rz-ssi.c
@@ -414,51 +414,48 @@ static int rz_ssi_pio_recv(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
 	u16 *buf;
 	int fifo_samples;
 	int frames_left;
-	int samples = 0;
+	int samples;
 	int i;
 
 	if (!rz_ssi_stream_is_valid(ssi, strm))
 		return -EINVAL;
 
 	runtime = substream->runtime;
-	/* frames left in this period */
-	frames_left = runtime->period_size - (strm->buffer_pos %
-					      runtime->period_size);
-	if (frames_left == 0)
-		frames_left = runtime->period_size;
 
-	/* Samples in RX FIFO */
-	fifo_samples = (rz_ssi_reg_readl(ssi, SSIFSR) >>
-			SSIFSR_RDC_SHIFT) & SSIFSR_RDC_MASK;
-
-	/* Only read full frames at a time */
-	while (frames_left && (fifo_samples >= runtime->channels)) {
-		samples += runtime->channels;
-		fifo_samples -= runtime->channels;
-		frames_left--;
-	}
-
-	/* not enough samples yet */
-	if (samples == 0)
-		return 0;
+	do {
+		/* frames left in this period */
+		frames_left = runtime->period_size -
+			      (strm->buffer_pos % runtime->period_size);
+		if (!frames_left)
+			frames_left = runtime->period_size;
+
+		/* Samples in RX FIFO */
+		fifo_samples = (rz_ssi_reg_readl(ssi, SSIFSR) >>
+				SSIFSR_RDC_SHIFT) & SSIFSR_RDC_MASK;
+
+		/* Only read full frames at a time */
+		samples = 0;
+		while (frames_left && (fifo_samples >= runtime->channels)) {
+			samples += runtime->channels;
+			fifo_samples -= runtime->channels;
+			frames_left--;
+		}
 
-	/* calculate new buffer index */
-	buf = (u16 *)(runtime->dma_area);
-	buf += strm->buffer_pos * runtime->channels;
+		/* not enough samples yet */
+		if (!samples)
+			break;
 
-	/* Note, only supports 16-bit samples */
-	for (i = 0; i < samples; i++)
-		*buf++ = (u16)(rz_ssi_reg_readl(ssi, SSIFRDR) >> 16);
+		/* calculate new buffer index */
+		buf = (u16 *)runtime->dma_area;
+		buf += strm->buffer_pos * runtime->channels;
 
-	rz_ssi_reg_mask_setl(ssi, SSIFSR, SSIFSR_RDF, 0);
-	rz_ssi_pointer_update(strm, samples / runtime->channels);
+		/* Note, only supports 16-bit samples */
+		for (i = 0; i < samples; i++)
+			*buf++ = (u16)(rz_ssi_reg_readl(ssi, SSIFRDR) >> 16);
 
-	/*
-	 * If we finished this period, but there are more samples in
-	 * the RX FIFO, call this function again
-	 */
-	if (frames_left == 0 && fifo_samples >= runtime->channels)
-		rz_ssi_pio_recv(ssi, strm);
+		rz_ssi_reg_mask_setl(ssi, SSIFSR, SSIFSR_RDF, 0);
+		rz_ssi_pointer_update(strm, samples / runtime->channels);
+	} while (!frames_left && fifo_samples >= runtime->channels);
 
 	return 0;
 }
-- 
2.17.1

[PATCH v2 1/5] ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively

[Lad Prabhakar]

Instead of recursively calling rz_ssi_pio_recv() use a loop instead
to read the samples from RX fifo.

Inspiration for this patch is to avoid recursion, as recursion is
unwelcome in kernel due to limited stack use. Also to add this driver
will later be used on RZ/A2 SoC's which runs with limited memory.

This also fixes an issue where the return value of rz_ssi_pio_recv()
was ignored when called recursively.

Fixes: 03e786bd4341 ("ASoC: sh: Add RZ/G2L SSIF-2 driver")
Reported-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2
* Used a do while loop
* Fixed comments pointed by Cezary.
---
 sound/soc/sh/rz-ssi.c | 65 +++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 34 deletions(-)

diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c
index fa0cc08f70ec..637802117c6c 100644
--- a/sound/soc/sh/rz-ssi.c
+++ b/sound/soc/sh/rz-ssi.c
@@ -414,51 +414,48 @@ static int rz_ssi_pio_recv(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
 	u16 *buf;
 	int fifo_samples;
 	int frames_left;
-	int samples = 0;
+	int samples;
 	int i;
 
 	if (!rz_ssi_stream_is_valid(ssi, strm))
 		return -EINVAL;
 
 	runtime = substream->runtime;
-	/* frames left in this period */
-	frames_left = runtime->period_size - (strm->buffer_pos %
-					      runtime->period_size);
-	if (frames_left == 0)
-		frames_left = runtime->period_size;
 
-	/* Samples in RX FIFO */
-	fifo_samples = (rz_ssi_reg_readl(ssi, SSIFSR) >>
-			SSIFSR_RDC_SHIFT) & SSIFSR_RDC_MASK;
-
-	/* Only read full frames at a time */
-	while (frames_left && (fifo_samples >= runtime->channels)) {
-		samples += runtime->channels;
-		fifo_samples -= runtime->channels;
-		frames_left--;
-	}
-
-	/* not enough samples yet */
-	if (samples == 0)
-		return 0;
+	do {
+		/* frames left in this period */
+		frames_left = runtime->period_size -
+			      (strm->buffer_pos % runtime->period_size);
+		if (!frames_left)
+			frames_left = runtime->period_size;
+
+		/* Samples in RX FIFO */
+		fifo_samples = (rz_ssi_reg_readl(ssi, SSIFSR) >>
+				SSIFSR_RDC_SHIFT) & SSIFSR_RDC_MASK;
+
+		/* Only read full frames at a time */
+		samples = 0;
+		while (frames_left && (fifo_samples >= runtime->channels)) {
+			samples += runtime->channels;
+			fifo_samples -= runtime->channels;
+			frames_left--;
+		}
 
-	/* calculate new buffer index */
-	buf = (u16 *)(runtime->dma_area);
-	buf += strm->buffer_pos * runtime->channels;
+		/* not enough samples yet */
+		if (!samples)
+			break;
 
-	/* Note, only supports 16-bit samples */
-	for (i = 0; i < samples; i++)
-		*buf++ = (u16)(rz_ssi_reg_readl(ssi, SSIFRDR) >> 16);
+		/* calculate new buffer index */
+		buf = (u16 *)runtime->dma_area;
+		buf += strm->buffer_pos * runtime->channels;
 
-	rz_ssi_reg_mask_setl(ssi, SSIFSR, SSIFSR_RDF, 0);
-	rz_ssi_pointer_update(strm, samples / runtime->channels);
+		/* Note, only supports 16-bit samples */
+		for (i = 0; i < samples; i++)
+			*buf++ = (u16)(rz_ssi_reg_readl(ssi, SSIFRDR) >> 16);
 
-	/*
-	 * If we finished this period, but there are more samples in
-	 * the RX FIFO, call this function again
-	 */
-	if (frames_left == 0 && fifo_samples >= runtime->channels)
-		rz_ssi_pio_recv(ssi, strm);
+		rz_ssi_reg_mask_setl(ssi, SSIFSR, SSIFSR_RDF, 0);
+		rz_ssi_pointer_update(strm, samples / runtime->channels);
+	} while (!frames_left && fifo_samples >= runtime->channels);
 
 	return 0;
 }
-- 
2.17.1

[PATCH v2 2/5] ASoC: sh: rz-ssi: Make the data structures available before registering the handlers

Initialize the spinlock and make the data structures available before
registering the interrupt handlers.

Reported-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2
* No change
---
 sound/soc/sh/rz-ssi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c
index 637802117c6c..89428945d48b 100644
--- a/sound/soc/sh/rz-ssi.c
+++ b/sound/soc/sh/rz-ssi.c
@@ -972,6 +972,9 @@ static int rz_ssi_probe(struct platform_device *pdev)
 	ssi->playback.priv = ssi;
 	ssi->capture.priv = ssi;
 
+	spin_lock_init(&ssi->lock);
+	dev_set_drvdata(&pdev->dev, ssi);
+
 	/* Error Interrupt */
 	ssi->irq_int = platform_get_irq_byname(pdev, "int_req");
 	if (ssi->irq_int < 0)
@@ -1019,8 +1022,6 @@ static int rz_ssi_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_resume_and_get(&pdev->dev);
 
-	spin_lock_init(&ssi->lock);
-	dev_set_drvdata(&pdev->dev, ssi);
 	ret = devm_snd_soc_register_component(&pdev->dev, &rz_ssi_soc_component,
 					      rz_ssi_soc_dai,
 					      ARRAY_SIZE(rz_ssi_soc_dai));
-- 
2.17.1

[PATCH v2 2/5] ASoC: sh: rz-ssi: Make the data structures available before registering the handlers

[Lad Prabhakar]

Initialize the spinlock and make the data structures available before
registering the interrupt handlers.

Reported-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2
* No change
---
 sound/soc/sh/rz-ssi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c
index 637802117c6c..89428945d48b 100644
--- a/sound/soc/sh/rz-ssi.c
+++ b/sound/soc/sh/rz-ssi.c
@@ -972,6 +972,9 @@ static int rz_ssi_probe(struct platform_device *pdev)
 	ssi->playback.priv = ssi;
 	ssi->capture.priv = ssi;
 
+	spin_lock_init(&ssi->lock);
+	dev_set_drvdata(&pdev->dev, ssi);
+
 	/* Error Interrupt */
 	ssi->irq_int = platform_get_irq_byname(pdev, "int_req");
 	if (ssi->irq_int < 0)
@@ -1019,8 +1022,6 @@ static int rz_ssi_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_resume_and_get(&pdev->dev);
 
-	spin_lock_init(&ssi->lock);
-	dev_set_drvdata(&pdev->dev, ssi);
 	ret = devm_snd_soc_register_component(&pdev->dev, &rz_ssi_soc_component,
 					      rz_ssi_soc_dai,
 					      ARRAY_SIZE(rz_ssi_soc_dai));
-- 
2.17.1

[PATCH v2 3/5] ASoC: sh: rz-ssi: Drop ssi parameter from rz_ssi_stream_init()

[Lad Prabhakar]

ssi parameter is unused in rz_ssi_stream_init() so just drop it.

While at it, change the return type of rz_ssi_stream_init() to void
instead of int.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2
* No change
---
 sound/soc/sh/rz-ssi.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c
index 89428945d48b..50699e94772b 100644
--- a/sound/soc/sh/rz-ssi.c
+++ b/sound/soc/sh/rz-ssi.c
@@ -201,9 +201,8 @@ static int rz_ssi_stream_is_valid(struct rz_ssi_priv *ssi,
 	return ret;
 }
 
-static int rz_ssi_stream_init(struct rz_ssi_priv *ssi,
-			      struct rz_ssi_stream *strm,
-			      struct snd_pcm_substream *substream)
+static void rz_ssi_stream_init(struct rz_ssi_stream *strm,
+			       struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
@@ -219,8 +218,6 @@ static int rz_ssi_stream_init(struct rz_ssi_priv *ssi,
 
 	/* fifo init */
 	strm->fifo_sample_size = SSI_FIFO_DEPTH;
-
-	return 0;
 }
 
 static void rz_ssi_stream_quit(struct rz_ssi_priv *ssi,
@@ -723,9 +720,7 @@ static int rz_ssi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 		rz_ssi_reg_mask_setl(ssi, SSIFCR, SSIFCR_SSIRST, 0);
 		udelay(5);
 
-		ret = rz_ssi_stream_init(ssi, strm, substream);
-		if (ret)
-			goto done;
+		rz_ssi_stream_init(strm, substream);
 
 		if (ssi->dma_rt) {
 			bool is_playback;
-- 
2.17.1

[PATCH v2 3/5] ASoC: sh: rz-ssi: Drop ssi parameter from rz_ssi_stream_init()

[Lad Prabhakar]

ssi parameter is unused in rz_ssi_stream_init() so just drop it.

While at it, change the return type of rz_ssi_stream_init() to void
instead of int.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2
* No change
---
 sound/soc/sh/rz-ssi.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c
index 89428945d48b..50699e94772b 100644
--- a/sound/soc/sh/rz-ssi.c
+++ b/sound/soc/sh/rz-ssi.c
@@ -201,9 +201,8 @@ static int rz_ssi_stream_is_valid(struct rz_ssi_priv *ssi,
 	return ret;
 }
 
-static int rz_ssi_stream_init(struct rz_ssi_priv *ssi,
-			      struct rz_ssi_stream *strm,
-			      struct snd_pcm_substream *substream)
+static void rz_ssi_stream_init(struct rz_ssi_stream *strm,
+			       struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
@@ -219,8 +218,6 @@ static int rz_ssi_stream_init(struct rz_ssi_priv *ssi,
 
 	/* fifo init */
 	strm->fifo_sample_size = SSI_FIFO_DEPTH;
-
-	return 0;
 }
 
 static void rz_ssi_stream_quit(struct rz_ssi_priv *ssi,
@@ -723,9 +720,7 @@ static int rz_ssi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 		rz_ssi_reg_mask_setl(ssi, SSIFCR, SSIFCR_SSIRST, 0);
 		udelay(5);
 
-		ret = rz_ssi_stream_init(ssi, strm, substream);
-		if (ret)
-			goto done;
+		rz_ssi_stream_init(strm, substream);
 
 		if (ssi->dma_rt) {
 			bool is_playback;
-- 
2.17.1

[PATCH v2 4/5] ASoC: sh: rz-ssi: Change return type of rz_ssi_stream_is_valid() to bool

[Lad Prabhakar]

rz_ssi_stream_is_valid() never returns an int, it returns the result of
a condition which is either true or false.

While at it, drop "!!" as the expression is boolean.

Reported-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2
* No change
---
 sound/soc/sh/rz-ssi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c
index 50699e94772b..2da43eecfb3e 100644
--- a/sound/soc/sh/rz-ssi.c
+++ b/sound/soc/sh/rz-ssi.c
@@ -188,14 +188,14 @@ static inline bool rz_ssi_is_dma_enabled(struct rz_ssi_priv *ssi)
 	return (ssi->playback.dma_ch && (ssi->dma_rt || ssi->capture.dma_ch));
 }
 
-static int rz_ssi_stream_is_valid(struct rz_ssi_priv *ssi,
-				  struct rz_ssi_stream *strm)
+static bool rz_ssi_stream_is_valid(struct rz_ssi_priv *ssi,
+				   struct rz_ssi_stream *strm)
 {
 	unsigned long flags;
-	int ret;
+	bool ret;
 
 	spin_lock_irqsave(&ssi->lock, flags);
-	ret = !!(strm->substream && strm->substream->runtime);
+	ret = strm->substream && strm->substream->runtime;
 	spin_unlock_irqrestore(&ssi->lock, flags);
 
 	return ret;
-- 
2.17.1

[PATCH v2 4/5] ASoC: sh: rz-ssi: Change return type of rz_ssi_stream_is_valid() to bool

[Lad Prabhakar]

rz_ssi_stream_is_valid() never returns an int, it returns the result of
a condition which is either true or false.

While at it, drop "!!" as the expression is boolean.

Reported-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2
* No change
---
 sound/soc/sh/rz-ssi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c
index 50699e94772b..2da43eecfb3e 100644
--- a/sound/soc/sh/rz-ssi.c
+++ b/sound/soc/sh/rz-ssi.c
@@ -188,14 +188,14 @@ static inline bool rz_ssi_is_dma_enabled(struct rz_ssi_priv *ssi)
 	return (ssi->playback.dma_ch && (ssi->dma_rt || ssi->capture.dma_ch));
 }
 
-static int rz_ssi_stream_is_valid(struct rz_ssi_priv *ssi,
-				  struct rz_ssi_stream *strm)
+static bool rz_ssi_stream_is_valid(struct rz_ssi_priv *ssi,
+				   struct rz_ssi_stream *strm)
 {
 	unsigned long flags;
-	int ret;
+	bool ret;
 
 	spin_lock_irqsave(&ssi->lock, flags);
-	ret = !!(strm->substream && strm->substream->runtime);
+	ret = strm->substream && strm->substream->runtime;
 	spin_unlock_irqrestore(&ssi->lock, flags);
 
 	return ret;
-- 
2.17.1

[PATCH v2 5/5] ASoC: sh: rz-ssi: Add rz_ssi_set_substream() helper function

[Lad Prabhakar]

A copy of substream pointer is stored in priv structure during
rz_ssi_dai_trigger() callback ie in SNDRV_PCM_TRIGGER_START case
and the pointer is assigned to NULL in case of SNDRV_PCM_TRIGGER_STOP.

The driver used the locks only in rz_ssi_stream_is_valid() and assigned
the local substream pointer to NULL in rz_ssi_dai_trigger() callback but
never locked it while making a local copy.

This patch adds the rz_ssi_set_substream() helper function to set the
substream pointer with locks acquired and replaces the instances of
setting the local substream pointer with the rz_ssi_set_substream()
function.

Reported-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2
* Dropped rz_ssi_get_substream() helper.
---
 sound/soc/sh/rz-ssi.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c
index 2da43eecfb3e..07fdbcfa5b63 100644
--- a/sound/soc/sh/rz-ssi.c
+++ b/sound/soc/sh/rz-ssi.c
@@ -188,6 +188,17 @@ static inline bool rz_ssi_is_dma_enabled(struct rz_ssi_priv *ssi)
 	return (ssi->playback.dma_ch && (ssi->dma_rt || ssi->capture.dma_ch));
 }
 
+static void rz_ssi_set_substream(struct rz_ssi_stream *strm,
+				 struct snd_pcm_substream *substream)
+{
+	struct rz_ssi_priv *ssi = strm->priv;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ssi->lock, flags);
+	strm->substream = substream;
+	spin_unlock_irqrestore(&ssi->lock, flags);
+}
+
 static bool rz_ssi_stream_is_valid(struct rz_ssi_priv *ssi,
 				   struct rz_ssi_stream *strm)
 {
@@ -206,7 +217,7 @@ static void rz_ssi_stream_init(struct rz_ssi_stream *strm,
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
-	strm->substream = substream;
+	rz_ssi_set_substream(strm, substream);
 	strm->sample_width = samples_to_bytes(runtime, 1);
 	strm->dma_buffer_pos = 0;
 	strm->period_counter = 0;
@@ -224,11 +235,8 @@ static void rz_ssi_stream_quit(struct rz_ssi_priv *ssi,
 			       struct rz_ssi_stream *strm)
 {
 	struct snd_soc_dai *dai = rz_ssi_get_dai(strm->substream);
-	unsigned long flags;
 
-	spin_lock_irqsave(&ssi->lock, flags);
-	strm->substream = NULL;
-	spin_unlock_irqrestore(&ssi->lock, flags);
+	rz_ssi_set_substream(strm, NULL);
 
 	if (strm->oerr_num > 0)
 		dev_info(dai->dev, "overrun = %d\n", strm->oerr_num);
-- 
2.17.1

[PATCH v2 5/5] ASoC: sh: rz-ssi: Add rz_ssi_set_substream() helper function

[Lad Prabhakar]

A copy of substream pointer is stored in priv structure during
rz_ssi_dai_trigger() callback ie in SNDRV_PCM_TRIGGER_START case
and the pointer is assigned to NULL in case of SNDRV_PCM_TRIGGER_STOP.

The driver used the locks only in rz_ssi_stream_is_valid() and assigned
the local substream pointer to NULL in rz_ssi_dai_trigger() callback but
never locked it while making a local copy.

This patch adds the rz_ssi_set_substream() helper function to set the
substream pointer with locks acquired and replaces the instances of
setting the local substream pointer with the rz_ssi_set_substream()
function.

Reported-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2
* Dropped rz_ssi_get_substream() helper.
---
 sound/soc/sh/rz-ssi.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c
index 2da43eecfb3e..07fdbcfa5b63 100644
--- a/sound/soc/sh/rz-ssi.c
+++ b/sound/soc/sh/rz-ssi.c
@@ -188,6 +188,17 @@ static inline bool rz_ssi_is_dma_enabled(struct rz_ssi_priv *ssi)
 	return (ssi->playback.dma_ch && (ssi->dma_rt || ssi->capture.dma_ch));
 }
 
+static void rz_ssi_set_substream(struct rz_ssi_stream *strm,
+				 struct snd_pcm_substream *substream)
+{
+	struct rz_ssi_priv *ssi = strm->priv;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ssi->lock, flags);
+	strm->substream = substream;
+	spin_unlock_irqrestore(&ssi->lock, flags);
+}
+
 static bool rz_ssi_stream_is_valid(struct rz_ssi_priv *ssi,
 				   struct rz_ssi_stream *strm)
 {
@@ -206,7 +217,7 @@ static void rz_ssi_stream_init(struct rz_ssi_stream *strm,
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
-	strm->substream = substream;
+	rz_ssi_set_substream(strm, substream);
 	strm->sample_width = samples_to_bytes(runtime, 1);
 	strm->dma_buffer_pos = 0;
 	strm->period_counter = 0;
@@ -224,11 +235,8 @@ static void rz_ssi_stream_quit(struct rz_ssi_priv *ssi,
 			       struct rz_ssi_stream *strm)
 {
 	struct snd_soc_dai *dai = rz_ssi_get_dai(strm->substream);
-	unsigned long flags;
 
-	spin_lock_irqsave(&ssi->lock, flags);
-	strm->substream = NULL;
-	spin_unlock_irqrestore(&ssi->lock, flags);
+	rz_ssi_set_substream(strm, NULL);
 
 	if (strm->oerr_num > 0)
 		dev_info(dai->dev, "overrun = %d\n", strm->oerr_num);
-- 
2.17.1

Re: [PATCH v2 1/5] ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively

[Cezary Rojewski]

On 2022-01-15 2:22 AM, Lad Prabhakar wrote:
> Instead of recursively calling rz_ssi_pio_recv() use a loop instead
> to read the samples from RX fifo.
> 
> Inspiration for this patch is to avoid recursion, as recursion is
> unwelcome in kernel due to limited stack use. Also to add this driver
> will later be used on RZ/A2 SoC's which runs with limited memory.
> 
> This also fixes an issue where the return value of rz_ssi_pio_recv()
> was ignored when called recursively.
> 
> Fixes: 03e786bd4341 ("ASoC: sh: Add RZ/G2L SSIF-2 driver")
> Reported-by: Pavel Machek <pavel@denx.de>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v1->v2
> * Used a do while loop
> * Fixed comments pointed by Cezary.

Apart from my previous suggestions which have already been addressed, I 
don't see any issues with the patch, so:

Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>


Regards,
Czarek

Re: [PATCH v2 1/5] ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively

[Cezary Rojewski]

On 2022-01-15 2:22 AM, Lad Prabhakar wrote:
> Instead of recursively calling rz_ssi_pio_recv() use a loop instead
> to read the samples from RX fifo.
> 
> Inspiration for this patch is to avoid recursion, as recursion is
> unwelcome in kernel due to limited stack use. Also to add this driver
> will later be used on RZ/A2 SoC's which runs with limited memory.
> 
> This also fixes an issue where the return value of rz_ssi_pio_recv()
> was ignored when called recursively.
> 
> Fixes: 03e786bd4341 ("ASoC: sh: Add RZ/G2L SSIF-2 driver")
> Reported-by: Pavel Machek <pavel@denx.de>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v1->v2
> * Used a do while loop
> * Fixed comments pointed by Cezary.

Apart from my previous suggestions which have already been addressed, I 
don't see any issues with the patch, so:

Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>


Regards,
Czarek

Re: [PATCH v2 0/5] ASoC: sh: rz-ssi: Code cleanup and fixes

[Cezary Rojewski]

On 2022-01-15 2:22 AM, Lad Prabhakar wrote:
> Hi All,
> 
> This patch series does code cleanup and fixes to the rz-ssi driver.

Hello,

Suggestion for the future contributions below. My input is *not* to be 
treated as a blocker for the series and is not a reason for re-sending it.


Cover letter should provide a high-level view of the series. That 
includes but is not limited to the overall layout of the series. Here, 
it would be good to state why the fixes are made. Also, it is preferred 
to have the fixes leading the series, _before_ any cleanups.

This summary suggests that cleanups are leading the series instead. 
Think of cover-letter as of news-paper. Subsystem maintainers such as 
Mark and Takashi start here (read the first page first) to get the 
general idea of what the contributor is sending them. Being transparent 
only helps. Good descriptions also helps reviewers to know which patches 
to look at first. Fixes are obviously of more importance as they usually 
address issues troubling users of the release-builds. Such patches are 
also often backported down the stream, increasing the importance for 
their review.

There's more to it, but for the scope of this cover letter, this should 
suffice. It's just a suggestion but I hope you find it useful.


Regards,
Czarek

> Cheers,
> Prabhakar
> 
> Lad Prabhakar (5):
>    ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively
>    ASoC: sh: rz-ssi: Make the data structures available before
>      registering the handlers
>    ASoC: sh: rz-ssi: Drop ssi parameter from rz_ssi_stream_init()
>    ASoC: sh: rz-ssi: Change return type of rz_ssi_stream_is_valid() to
>      bool
>    ASoC: sh: rz-ssi: Add rz_ssi_set_substream() helper function
> 
>   sound/soc/sh/rz-ssi.c | 107 +++++++++++++++++++++---------------------
>   1 file changed, 54 insertions(+), 53 deletions(-)
> 

Re: [PATCH v2 0/5] ASoC: sh: rz-ssi: Code cleanup and fixes

[Lad, Prabhakar]

Hi Cezary,

On Wed, Jan 19, 2022 at 2:12 PM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
>
> On 2022-01-15 2:22 AM, Lad Prabhakar wrote:
> > Hi All,
> >
> > This patch series does code cleanup and fixes to the rz-ssi driver.
>
> Hello,
>
> Suggestion for the future contributions below. My input is *not* to be
> treated as a blocker for the series and is not a reason for re-sending it.
>
>
> Cover letter should provide a high-level view of the series. That
> includes but is not limited to the overall layout of the series. Here,
> it would be good to state why the fixes are made. Also, it is preferred
> to have the fixes leading the series, _before_ any cleanups.
>
> This summary suggests that cleanups are leading the series instead.
> Think of cover-letter as of news-paper. Subsystem maintainers such as
> Mark and Takashi start here (read the first page first) to get the
> general idea of what the contributor is sending them. Being transparent
> only helps. Good descriptions also helps reviewers to know which patches
> to look at first. Fixes are obviously of more importance as they usually
> address issues troubling users of the release-builds. Such patches are
> also often backported down the stream, increasing the importance for
> their review.
>
> There's more to it, but for the scope of this cover letter, this should
> suffice. It's just a suggestion but I hope you find it useful.
>
>
Thank you for the valuable input. I'll make a note of it and make sure
to add more information in cover letters too.

Cheers,
Prabhakar

Re: [PATCH v2 0/5] ASoC: sh: rz-ssi: Code cleanup and fixes

[Mark Brown]

On Sat, 15 Jan 2022 01:22:58 +0000, Lad Prabhakar wrote:
> This patch series does code cleanup and fixes to the rz-ssi driver.
> 
> Cheers,
> Prabhakar
> 
> Lad Prabhakar (5):
>   ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively
>   ASoC: sh: rz-ssi: Make the data structures available before
>     registering the handlers
>   ASoC: sh: rz-ssi: Drop ssi parameter from rz_ssi_stream_init()
>   ASoC: sh: rz-ssi: Change return type of rz_ssi_stream_is_valid() to
>     bool
>   ASoC: sh: rz-ssi: Add rz_ssi_set_substream() helper function
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/5] ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively
      commit: 6570f991582e32b7992601d0497c61962a2c5dcc
[2/5] ASoC: sh: rz-ssi: Make the data structures available before registering the handlers
      commit: 0788785c78342d422f93b1c9831c2b2b7f137937
[3/5] ASoC: sh: rz-ssi: Drop ssi parameter from rz_ssi_stream_init()
      commit: 4f78f3c970f131a179fd135806a9b693fa606beb
[4/5] ASoC: sh: rz-ssi: Change return type of rz_ssi_stream_is_valid() to bool
      (no commit info)
[5/5] ASoC: sh: rz-ssi: Add rz_ssi_set_substream() helper function
      (no commit info)

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v2 1/5] ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 236 bytes --]

On Sat, Jan 15, 2022 at 01:22:59AM +0000, Lad Prabhakar wrote:
> Instead of recursively calling rz_ssi_pio_recv() use a loop instead
> to read the samples from RX fifo.

This doesn't apply against current code, please check and resend.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/5] ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 236 bytes --]

On Sat, Jan 15, 2022 at 01:22:59AM +0000, Lad Prabhakar wrote:
> Instead of recursively calling rz_ssi_pio_recv() use a loop instead
> to read the samples from RX fifo.

This doesn't apply against current code, please check and resend.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/5] ASoC: sh: rz-ssi: Code cleanup and fixes

[Lad, Prabhakar]

Hi Mark,

On Tue, Jan 25, 2022 at 10:20 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Sat, 15 Jan 2022 01:22:58 +0000, Lad Prabhakar wrote:
> > This patch series does code cleanup and fixes to the rz-ssi driver.
> >
> > Cheers,
> > Prabhakar
> >
> > Lad Prabhakar (5):
> >   ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively
> >   ASoC: sh: rz-ssi: Make the data structures available before
> >     registering the handlers
> >   ASoC: sh: rz-ssi: Drop ssi parameter from rz_ssi_stream_init()
> >   ASoC: sh: rz-ssi: Change return type of rz_ssi_stream_is_valid() to
> >     bool
> >   ASoC: sh: rz-ssi: Add rz_ssi_set_substream() helper function
> >
> > [...]
>
> Applied to
>
>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
>
> Thanks!
>
> [1/5] ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively
>       commit: 6570f991582e32b7992601d0497c61962a2c5dcc
> [2/5] ASoC: sh: rz-ssi: Make the data structures available before registering the handlers
>       commit: 0788785c78342d422f93b1c9831c2b2b7f137937
> [3/5] ASoC: sh: rz-ssi: Drop ssi parameter from rz_ssi_stream_init()
>       commit: 4f78f3c970f131a179fd135806a9b693fa606beb
> [4/5] ASoC: sh: rz-ssi: Change return type of rz_ssi_stream_is_valid() to bool
>       (no commit info)
> [5/5] ASoC: sh: rz-ssi: Add rz_ssi_set_substream() helper function
>       (no commit info)
>
It looks like you have applied v1 series instead of v2 to your next
[0] (v2 series [1]). And your comment on your patch 1/5 of not being
applying isn't true as you have already applied it ;).
You haven't applied patch 5/5 (this applies cleanly to your -next)

[0] https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/log/
[1] https://patchwork.kernel.org/project/alsa-devel/list/?series=605672

Cheers,
Prabhakar

Re: [PATCH v2 0/5] ASoC: sh: rz-ssi: Code cleanup and fixes

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]

On Tue, Jan 25, 2022 at 12:25:32PM +0000, Lad, Prabhakar wrote:

> It looks like you have applied v1 series instead of v2 to your next
> [0] (v2 series [1]). And your comment on your patch 1/5 of not being
> applying isn't true as you have already applied it ;).
> You haven't applied patch 5/5 (this applies cleanly to your -next)

Like I said, please resend.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/5] ASoC: sh: rz-ssi: Code cleanup and fixes

[Lad, Prabhakar]

On Tue, Jan 25, 2022 at 12:30 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jan 25, 2022 at 12:25:32PM +0000, Lad, Prabhakar wrote:
>
> > It looks like you have applied v1 series instead of v2 to your next
> > [0] (v2 series [1]). And your comment on your patch 1/5 of not being
> > applying isn't true as you have already applied it ;).
> > You haven't applied patch 5/5 (this applies cleanly to your -next)
>
> Like I said, please resend.

Will do.

Cheers,
Prabhakar