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

[PATCH 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 while loop
to read the samples from RX fifo.

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>
---
 sound/soc/sh/rz-ssi.c | 68 ++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 33 deletions(-)

diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c
index fa0cc08f70ec..37466f65c2b0 100644
--- a/sound/soc/sh/rz-ssi.c
+++ b/sound/soc/sh/rz-ssi.c
@@ -411,54 +411,56 @@ static int rz_ssi_pio_recv(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
 {
 	struct snd_pcm_substream *substream = strm->substream;
 	struct snd_pcm_runtime *runtime;
+	bool done = false;
 	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--;
-	}
+	while (!done) {
+		/* 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--;
+		}
 
-	/* not enough samples yet */
-	if (samples == 0)
-		return 0;
+		/* not enough samples yet */
+		if (!samples)
+			break;
 
-	/* calculate new buffer index */
-	buf = (u16 *)(runtime->dma_area);
-	buf += strm->buffer_pos * runtime->channels;
+		/* calculate new buffer index */
+		buf = (u16 *)(runtime->dma_area);
+		buf += strm->buffer_pos * runtime->channels;
 
-	/* Note, only supports 16-bit samples */
-	for (i = 0; i < samples; i++)
-		*buf++ = (u16)(rz_ssi_reg_readl(ssi, SSIFRDR) >> 16);
+		/* Note, only supports 16-bit samples */
+		for (i = 0; i < samples; i++)
+			*buf++ = (u16)(rz_ssi_reg_readl(ssi, SSIFRDR) >> 16);
 
-	rz_ssi_reg_mask_setl(ssi, SSIFSR, SSIFSR_RDF, 0);
-	rz_ssi_pointer_update(strm, samples / runtime->channels);
+		rz_ssi_reg_mask_setl(ssi, SSIFSR, SSIFSR_RDF, 0);
+		rz_ssi_pointer_update(strm, samples / runtime->channels);
 
-	/*
-	 * 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);
+		/* check if there are no more samples in the RX FIFO */
+		if (!(!frames_left && fifo_samples >= runtime->channels))
+			done = true;
+	}
 
 	return 0;
 }
-- 
2.17.1

[PATCH 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>
---
 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 37466f65c2b0..16de2633a873 100644
--- a/sound/soc/sh/rz-ssi.c
+++ b/sound/soc/sh/rz-ssi.c
@@ -977,6 +977,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)
@@ -1024,8 +1027,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 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>
---
 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 16de2633a873..fa68d3a0bd62 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,
@@ -728,9 +725,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 4/5] ASoC: sh: rz-ssi: Make 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>
---
 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 fa68d3a0bd62..aabd15e9d515 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 5/5] ASoC: sh: rz-ssi: Add functions to get/set substream pointer

[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 and
in rest of the driver no locking was used while assigning substream
pointer.

This patch adds functions to get/set substream pointer with locks acquired
and replaces the instances of access to substream pointer with the
get/set functions.

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>
---
 sound/soc/sh/rz-ssi.c | 55 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 41 insertions(+), 14 deletions(-)

diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c
index aabd15e9d515..057aedacedec 100644
--- a/sound/soc/sh/rz-ssi.c
+++ b/sound/soc/sh/rz-ssi.c
@@ -201,12 +201,36 @@ static bool rz_ssi_stream_is_valid(struct rz_ssi_priv *ssi,
 	return ret;
 }
 
+static struct snd_pcm_substream *rz_ssi_get_substream(struct rz_ssi_stream *strm)
+{
+	struct rz_ssi_priv *ssi = strm->priv;
+	struct snd_pcm_substream *substream;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ssi->lock, flags);
+	substream = strm->substream;
+	spin_unlock_irqrestore(&ssi->lock, flags);
+
+	return substream;
+}
+
+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 void rz_ssi_stream_init(struct rz_ssi_stream *strm,
 			       struct snd_pcm_substream *substream)
 {
 	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;
@@ -223,12 +247,13 @@ static void rz_ssi_stream_init(struct rz_ssi_stream *strm,
 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;
+	struct snd_pcm_substream *substream;
+	struct snd_soc_dai *dai;
 
-	spin_lock_irqsave(&ssi->lock, flags);
-	strm->substream = NULL;
-	spin_unlock_irqrestore(&ssi->lock, flags);
+	substream = rz_ssi_get_substream(strm);
+	rz_ssi_set_substream(strm, NULL);
+
+	dai = rz_ssi_get_dai(substream);
 
 	if (strm->oerr_num > 0)
 		dev_info(dai->dev, "overrun = %d\n", strm->oerr_num);
@@ -301,7 +326,8 @@ static int rz_ssi_clk_setup(struct rz_ssi_priv *ssi, unsigned int rate,
 
 static int rz_ssi_start(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
 {
-	bool is_play = rz_ssi_stream_is_play(ssi, strm->substream);
+	struct snd_pcm_substream *substream = rz_ssi_get_substream(strm);
+	bool is_play = rz_ssi_stream_is_play(ssi, substream);
 	u32 ssicr, ssifcr;
 
 	ssicr = rz_ssi_reg_readl(ssi, SSICR);
@@ -382,7 +408,7 @@ static int rz_ssi_stop(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
 
 static void rz_ssi_pointer_update(struct rz_ssi_stream *strm, int frames)
 {
-	struct snd_pcm_substream *substream = strm->substream;
+	struct snd_pcm_substream *substream = rz_ssi_get_substream(strm);
 	struct snd_pcm_runtime *runtime;
 	int current_period;
 
@@ -399,14 +425,14 @@ static void rz_ssi_pointer_update(struct rz_ssi_stream *strm, int frames)
 
 	current_period = strm->buffer_pos / runtime->period_size;
 	if (strm->period_counter != current_period) {
-		snd_pcm_period_elapsed(strm->substream);
+		snd_pcm_period_elapsed(substream);
 		strm->period_counter = current_period;
 	}
 }
 
 static int rz_ssi_pio_recv(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
 {
-	struct snd_pcm_substream *substream = strm->substream;
+	struct snd_pcm_substream *substream = rz_ssi_get_substream(strm);
 	struct snd_pcm_runtime *runtime;
 	bool done = false;
 	u16 *buf;
@@ -464,7 +490,7 @@ static int rz_ssi_pio_recv(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
 
 static int rz_ssi_pio_send(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
 {
-	struct snd_pcm_substream *substream = strm->substream;
+	struct snd_pcm_substream *substream = rz_ssi_get_substream(strm);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	int sample_space;
 	int samples = 0;
@@ -588,7 +614,7 @@ static int rz_ssi_dma_slave_config(struct rz_ssi_priv *ssi,
 static int rz_ssi_dma_transfer(struct rz_ssi_priv *ssi,
 			       struct rz_ssi_stream *strm)
 {
-	struct snd_pcm_substream *substream = strm->substream;
+	struct snd_pcm_substream *substream = rz_ssi_get_substream(strm);
 	struct dma_async_tx_descriptor *desc;
 	struct snd_pcm_runtime *runtime;
 	enum dma_transfer_direction dir;
@@ -646,12 +672,13 @@ static int rz_ssi_dma_transfer(struct rz_ssi_priv *ssi,
 static void rz_ssi_dma_complete(void *data)
 {
 	struct rz_ssi_stream *strm = (struct rz_ssi_stream *)data;
+	struct snd_pcm_substream *substream = rz_ssi_get_substream(strm);
 
-	if (!strm->running || !strm->substream || !strm->substream->runtime)
+	if (!strm->running || !substream || !substream->runtime)
 		return;
 
 	/* Note that next DMA transaction has probably already started */
-	rz_ssi_pointer_update(strm, strm->substream->runtime->period_size);
+	rz_ssi_pointer_update(strm, substream->runtime->period_size);
 
 	/* Queue up another DMA transaction */
 	rz_ssi_dma_transfer(strm->priv, strm);
-- 
2.17.1

Re: [PATCH 5/5] ASoC: sh: rz-ssi: Add functions to get/set substream pointer

[Mark Brown]

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

On Mon, Jan 10, 2022 at 09:47:11AM +0000, Lad Prabhakar wrote:

> +static struct snd_pcm_substream *rz_ssi_get_substream(struct rz_ssi_stream *strm)
> +{
> +	struct rz_ssi_priv *ssi = strm->priv;
> +	struct snd_pcm_substream *substream;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ssi->lock, flags);
> +	substream = strm->substream;
> +	spin_unlock_irqrestore(&ssi->lock, flags);

This locking doesn't seem useful, we just take a copy of the lock and
then immediately return so the lock isn't protecting anything in
particular - the caller can happily continue using the substream after
the variable has been updated.

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

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

[Cezary Rojewski]

On 2022-01-10 10:47 AM, Lad Prabhakar wrote:
> Instead of recursively calling rz_ssi_pio_recv() use a while loop
> to read the samples from RX fifo.

Recursion and loops are means for doing something repeatedly. Could you 
specify _why_ such change was made i.e. the conversion from one method 
into the other? I bet the code is not being changed for the sake of 
changing it, the reason is simply missing in the commit message.

Please note that refactoring below function into while-loop has a side 
effect: everything had to be indented by additional tab. Generally, 
readability increases if function is shaped 'linearly'.

> 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>
> ---
>   sound/soc/sh/rz-ssi.c | 68 ++++++++++++++++++++++---------------------
>   1 file changed, 35 insertions(+), 33 deletions(-)
> 
> diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c
> index fa0cc08f70ec..37466f65c2b0 100644
> --- a/sound/soc/sh/rz-ssi.c
> +++ b/sound/soc/sh/rz-ssi.c
> @@ -411,54 +411,56 @@ static int rz_ssi_pio_recv(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
>   {
>   	struct snd_pcm_substream *substream = strm->substream;
>   	struct snd_pcm_runtime *runtime;
> +	bool done = false;
>   	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--;
> -	}
> +	while (!done) {

I wonder if converting this into do-while isn't a better option. Maybe 
I'm missing something but 'done' flag seems to be changed only as an 
outcome of the last if-statement (last step) in this entire procedure. 
Perhaps condition from said if-statement could also be moved into 
'while' portion of do-while loop.

> +		/* 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--;
> +		}
>   
> -	/* not enough samples yet */
> -	if (samples == 0)
> -		return 0;
> +		/* not enough samples yet */
> +		if (!samples)
> +			break;
>   
> -	/* calculate new buffer index */
> -	buf = (u16 *)(runtime->dma_area);
> -	buf += strm->buffer_pos * runtime->channels;
> +		/* calculate new buffer index */
> +		buf = (u16 *)(runtime->dma_area);

Is the second pair of brackets needed?

> +		buf += strm->buffer_pos * runtime->channels;
>   
> -	/* Note, only supports 16-bit samples */
> -	for (i = 0; i < samples; i++)
> -		*buf++ = (u16)(rz_ssi_reg_readl(ssi, SSIFRDR) >> 16);
> +		/* Note, only supports 16-bit samples */
> +		for (i = 0; i < samples; i++)
> +			*buf++ = (u16)(rz_ssi_reg_readl(ssi, SSIFRDR) >> 16);
>   
> -	rz_ssi_reg_mask_setl(ssi, SSIFSR, SSIFSR_RDF, 0);
> -	rz_ssi_pointer_update(strm, samples / runtime->channels);
> +		rz_ssi_reg_mask_setl(ssi, SSIFSR, SSIFSR_RDF, 0);
> +		rz_ssi_pointer_update(strm, samples / runtime->channels);
>   
> -	/*
> -	 * 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);
> +		/* check if there are no more samples in the RX FIFO */
> +		if (!(!frames_left && fifo_samples >= runtime->channels))
> +			done = true;
> +	}
>   
>   	return 0;
>   }
> 

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

[Lad, Prabhakar]

Hi Cezary,

Thank you for the review.

On Mon, Jan 10, 2022 at 3:48 PM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
>
> On 2022-01-10 10:47 AM, Lad Prabhakar wrote:
> > Instead of recursively calling rz_ssi_pio_recv() use a while loop
> > to read the samples from RX fifo.
>
> Recursion and loops are means for doing something repeatedly. Could you
> specify _why_ such change was made i.e. the conversion from one method
> into the other? I bet the code is not being changed for the sake of
> changing it, the reason is simply missing in the commit message.
>
I had feedback from Pavel "recursion is unwelcome in kernel due to
limited stack use." which I did agree with as a result I have come up
with this patch. Also to add this driver will later be used on Renesas
RZ/A2 SoC's which runs with limited memory.

> Please note that refactoring below function into while-loop has a side
> effect: everything had to be indented by additional tab. Generally,
> readability increases if function is shaped 'linearly'.
>
I do agree, my initial patch just added a jump back to the start of
the function if there are more samples, but Biju suggested to use a
while loop instead.

> > 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>
> > ---
> >   sound/soc/sh/rz-ssi.c | 68 ++++++++++++++++++++++---------------------
> >   1 file changed, 35 insertions(+), 33 deletions(-)
> >
> > diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c
> > index fa0cc08f70ec..37466f65c2b0 100644
> > --- a/sound/soc/sh/rz-ssi.c
> > +++ b/sound/soc/sh/rz-ssi.c
> > @@ -411,54 +411,56 @@ static int rz_ssi_pio_recv(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
> >   {
> >       struct snd_pcm_substream *substream = strm->substream;
> >       struct snd_pcm_runtime *runtime;
> > +     bool done = false;
> >       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--;
> > -     }
> > +     while (!done) {
>
> I wonder if converting this into do-while isn't a better option. Maybe
> I'm missing something but 'done' flag seems to be changed only as an
> outcome of the last if-statement (last step) in this entire procedure.
> Perhaps condition from said if-statement could also be moved into
> 'while' portion of do-while loop.
>
Agreed.

> > +             /* 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--;
> > +             }
> >
> > -     /* not enough samples yet */
> > -     if (samples == 0)
> > -             return 0;
> > +             /* not enough samples yet */
> > +             if (!samples)
> > +                     break;
> >
> > -     /* calculate new buffer index */
> > -     buf = (u16 *)(runtime->dma_area);
> > -     buf += strm->buffer_pos * runtime->channels;
> > +             /* calculate new buffer index */
> > +             buf = (u16 *)(runtime->dma_area);
>
> Is the second pair of brackets needed?
>
Nope can be dropped.

Cheers,
Prabhakar

Re: [PATCH 5/5] ASoC: sh: rz-ssi: Add functions to get/set substream pointer

[Lad, Prabhakar]

Hi Mark,

Thank you for the review.

On Mon, Jan 10, 2022 at 3:10 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jan 10, 2022 at 09:47:11AM +0000, Lad Prabhakar wrote:
>
> > +static struct snd_pcm_substream *rz_ssi_get_substream(struct rz_ssi_stream *strm)
> > +{
> > +     struct rz_ssi_priv *ssi = strm->priv;
> > +     struct snd_pcm_substream *substream;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&ssi->lock, flags);
> > +     substream = strm->substream;
> > +     spin_unlock_irqrestore(&ssi->lock, flags);
>
> This locking doesn't seem useful, we just take a copy of the lock and
> then immediately return so the lock isn't protecting anything in
> particular - the caller can happily continue using the substream after
> the variable has been updated.
>
Ok will drop the locking from get function.

Cheers,
Prabhakar

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

[Cezary Rojewski]

On 2022-01-10 5:03 PM, Lad, Prabhakar wrote:
> Hi Cezary,
> 
> Thank you for the review.
> 

...

>> Recursion and loops are means for doing something repeatedly. Could you
>> specify _why_ such change was made i.e. the conversion from one method
>> into the other? I bet the code is not being changed for the sake of
>> changing it, the reason is simply missing in the commit message.
>>
> I had feedback from Pavel "recursion is unwelcome in kernel due to
> limited stack use." which I did agree with as a result I have come up
> with this patch. Also to add this driver will later be used on Renesas
> RZ/A2 SoC's which runs with limited memory.

Adding that reasoning to the commits message will prevent questions 
(such as mine) in the future. Thank you for a quick reply and a 
transparent answer.


Regards,
Czarek

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

[Pavel Machek]

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

Hi!

> > On 2022-01-10 10:47 AM, Lad Prabhakar wrote:
> > > Instead of recursively calling rz_ssi_pio_recv() use a while loop
> > > to read the samples from RX fifo.
> >
> > Recursion and loops are means for doing something repeatedly. Could you
> > specify _why_ such change was made i.e. the conversion from one method
> > into the other? I bet the code is not being changed for the sake of
> > changing it, the reason is simply missing in the commit message.
> >
> I had feedback from Pavel "recursion is unwelcome in kernel due to
> limited stack use." which I did agree with as a result I have come up
> with this patch. Also to add this driver will later be used on Renesas
> RZ/A2 SoC's which runs with limited memory.
> 
> > Please note that refactoring below function into while-loop has a side
> > effect: everything had to be indented by additional tab. Generally,
> > readability increases if function is shaped 'linearly'.
> >
> I do agree, my initial patch just added a jump back to the start of
> the function if there are more samples, but Biju suggested to use a
> while loop instead.

Yes, loop is better.

I'd actually do while(true) and avoid using the done variable.

    if (!(!frames_left && fifo_samples >= runtime->channels))
              break;

will do the trick. Better yet, do

    if (frames_left || fifo_samples < runtime->channels)
              break;

because double negation is quite confusing and looks like typo.

Best regards,
								Pavel

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

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

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

[Cezary Rojewski]

On 2022-01-10 7:44 PM, Pavel Machek wrote:
> Hi!
> 
>>> On 2022-01-10 10:47 AM, Lad Prabhakar wrote:
>>>> Instead of recursively calling rz_ssi_pio_recv() use a while loop
>>>> to read the samples from RX fifo.
>>>
>>> Recursion and loops are means for doing something repeatedly. Could you
>>> specify _why_ such change was made i.e. the conversion from one method
>>> into the other? I bet the code is not being changed for the sake of
>>> changing it, the reason is simply missing in the commit message.
>>>
>> I had feedback from Pavel "recursion is unwelcome in kernel due to
>> limited stack use." which I did agree with as a result I have come up
>> with this patch. Also to add this driver will later be used on Renesas
>> RZ/A2 SoC's which runs with limited memory.

...

> 
> Yes, loop is better.
> 
> I'd actually do while(true) and avoid using the done variable.
> 
>      if (!(!frames_left && fifo_samples >= runtime->channels))
>                break;
> 
> will do the trick. Better yet, do
> 
>      if (frames_left || fifo_samples < runtime->channels)
>                break;
> 
> because double negation is quite confusing and looks like typo.

You could achieve similar results by enlisting do-while loop. That's my 
proposal.


Regards,
Czarek

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

[Lad, Prabhakar]

On Mon, Jan 10, 2022 at 5:48 PM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
>
> On 2022-01-10 5:03 PM, Lad, Prabhakar wrote:
> > Hi Cezary,
> >
> > Thank you for the review.
> >
>
> ...
>
> >> Recursion and loops are means for doing something repeatedly. Could you
> >> specify _why_ such change was made i.e. the conversion from one method
> >> into the other? I bet the code is not being changed for the sake of
> >> changing it, the reason is simply missing in the commit message.
> >>
> > I had feedback from Pavel "recursion is unwelcome in kernel due to
> > limited stack use." which I did agree with as a result I have come up
> > with this patch. Also to add this driver will later be used on Renesas
> > RZ/A2 SoC's which runs with limited memory.
>
> Adding that reasoning to the commits message will prevent questions
> (such as mine) in the future. Thank you for a quick reply and a
> transparent answer.
>
My bad! I'll update the commit message.

CHeers,
Prabhakar