Messages in this thread | | | Date | Mon, 10 Jan 2022 16:48:30 +0100 | Subject | Re: [PATCH 1/5] ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv() recursively | From | 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; > } >
| |