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

[PATCH 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: Make return type of rz_ssi_stream_is_valid() to bool
  ASoC: sh: rz-ssi: Add functions to get/set substream pointer

 sound/soc/sh/rz-ssi.c | 147 ++++++++++++++++++++++++------------------
 1 file changed, 86 insertions(+), 61 deletions(-)

-- 
2.17.1

[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 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

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

[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 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 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

[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

[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

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

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

[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

[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

[PATCH 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>
---
 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 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 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 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

[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 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 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 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: (subset) [PATCH 0/5] ASoC: sh: rz-ssi: Code cleanup and fixes

[Mark Brown]

On Mon, 10 Jan 2022 09:47:06 +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: Make return type of rz_ssi_stream_is_valid() to bool
>   ASoC: sh: rz-ssi: Add functions to get/set substream pointer
> 
> [...]

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: Make return type of rz_ssi_stream_is_valid() to bool
      commit: e42c903e8bf400728c4ae1f922169b4d28b72efa

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