lkml.org 
[lkml]   [2022]   [Jan]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/5] ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively
From
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;
> }
>

\
 
 \ /
  Last update: 2022-01-10 16:49    [W:0.090 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site