| Message ID | 20220110094711.8574-2-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | ASoC: sh: rz-ssi: Code cleanup and fixes | expand |
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
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
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
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; }
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(-)