[PATCH 1/5] ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively
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; }
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; }
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
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
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
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
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
participants (4)
-
Cezary Rojewski -
Lad Prabhakar -
Lad, Prabhakar -
Pavel Machek