Message-ID: <2f338a37-f2ca-33e4-284e-5d263f7b93da@intel.com>
Date:   Mon, 10 Jan 2022 16:48:30 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.3.0
Subject: Re: [PATCH 1/5] ASoC: sh: rz-ssi: Drop calling rz_ssi_pio_recv()
 recursively
Content-Language: en-US
To:     Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Biju Das <biju.das.jz@bp.renesas.com>
CC:     Pavel Machek <pavel@denx.de>, <linux-renesas-soc@vger.kernel.org>,
        Prabhakar <prabhakar.csengg@gmail.com>,
        <alsa-devel@alsa-project.org>, <linux-kernel@vger.kernel.org>
References: <20220110094711.8574-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <20220110094711.8574-2-prabhakar.mahadev-lad.rj@bp.renesas.com>
From:   Cezary Rojewski <cezary.rojewski@intel.com>
In-Reply-To: <20220110094711.8574-2-prabhakar.mahadev-lad.rj@bp.renesas.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZGV6OGZVam1VVVZ2ZnZnSE5TcXl3dVMyNDFuZUt3alE3cTJ4RTJObnFwdXFt?=
 =?utf-8?B?RDYraDBORE9HQkQ0VGpQTE84Q1Ixa3d5cXhFbkk3TUoySERsMDlOelJ0UVd0?=
 =?utf-8?B?YVh4MysrUHJOOTRhUlRiSnV1YmN5ZHFqWk9tM2FXaUtPYmZGOXpYNXYxeXh3?=
 =?utf-8?B?U25KOGNWRzNUazNVcDl1QmRwajJmREFEelMxa3hOK0J0cVkwYnIvU2FUR3FY?=
 =?utf-8?B?ZUlPVERjZzVPTXVLYlJoWjVINHIrYjE0cTlHUjVjazU3alZHdit3VnA5c2I0?=
 =?utf-8?B?Q3lVd3lhWmlKcDc0QjN1a1lNbHpUbUlxOCtuVmxXL05WaXVRYmhUS0NWL0JH?=
 =?utf-8?B?T09wQjFVb1lyZjBrVGc0V2tSZDdoajNCN3ZYVThVRk9GZm9COGxuK0RXWUhx?=
 =?utf-8?B?dmZlNDFVNDRGSjRQelRjL1FMYTE2QUdqcWhwdW5acGN6dlh2NGszamhkZ29X?=
 =?utf-8?B?WGxUdUhIWlBIK3ovMGFrYm11VkNrc3RzRG5ROFQyYmhGa3A2aVVyNk5BY0s3?=
 =?utf-8?B?VlBrbmZaRnR4QXpTbm1YYjAzSXJMaWNCaGtMc3F0SHM5V015c09HNDB2dk5Z?=
 =?utf-8?B?Zy9CNFpyMWlyNFhBaldBQklKSzlmWEFJdm5sWVNNSWFhRzM0TlVFZjRPZkU3?=
 =?utf-8?B?SEV4eXpTcVlUQ3p2WkNidFpsbTVUdDExTTRZaG1zZ252M29JY1VRS1Z0QkpO?=
 =?utf-8?B?U3FlK3JvdjVVcStObnV3Rk10WXE5UWUyZTFucFpKT0l4Z3luYTV0dVJmY3g5?=
 =?utf-8?B?T3FUK3pEcnNCNVZhdUd1bEJPRzhhclRkOE9Ob3VySDU3bk05QmFCdktxNXFm?=
 =?utf-8?B?Qm0zZGZyWHd2ZFM4QlY3MUczQm1MV3lSWlJDZGpDZVVWSU01K2ZocGl0ZFV4?=
 =?utf-8?B?SjZmb253Zko5czRkbU1HSUZsSUw5bjN5ZFo1NGN4eWVkbmg2Y3hFYnJvM2tU?=
 =?utf-8?B?ZG56RmlMT0lKRkw4NVhPMjdIeTBSazAzdWVpR1FzOE5xTlE2MzBJNDBaVHpx?=
 =?utf-8?B?dzlLUld3WFlLTHpFRHcySTJ1ZFpXMFl1bHNSY0V0VUo0bzZ1aEFkTCtoU0c3?=
 =?utf-8?B?WmlwanhQVkJUZitmRVZaYjZFY1FFazB5dTNkZGVXNWNaMVh2eEZYTEMyMUR2?=
 =?utf-8?B?Q1Y1eTI3aHRtdUdFZ1ovcmpxelBKRlJJckNzTkpuZmhqWnRKSWQ3cWZUSXVB?=
 =?utf-8?B?WHJLU1czREE4d2Z0RWFJUDdzaUpQZDMvZE4xLzl6SzhlcUkrekpPSHRBQUsy?=
 =?utf-8?B?ZTd6aGcyOFpQcURBbDlWTi9iUlV5bk5VSTlIRXFtVEhNNXRoR2o1VVRiVHpE?=
 =?utf-8?B?UUVvN2FRalpWa01xd0MyRUdtRUs4N0VLT1h1OWVGeG12UXB3WHlYOGxYQjFC?=
 =?utf-8?B?d0hKU3I3N2VDWXBMN3U4dHlva1NNTWFZLzNOZXcyaGxBSTlBSVY2Yzl3T0pV?=
 =?utf-8?B?UFRBNjJneUhmbUtFL1NIZ0RWTzRtV3NudDd0ajA5ckJGeG1Ea1F2V0JOYlRM?=
 =?utf-8?B?blE3SE0zbC8zYnNNNnVtR0swR09IZVJIdzdNbi9EWW5qQ3R3eHdrMncvZmxu?=
 =?utf-8?B?dzBBczVRb1FDWm9PampkbGtCeXV6cUtML25jZkJUK2crY0oya3NLeGxQeEJE?=
 =?utf-8?B?YVVDZEtLL1VyUnJaY3FKSlNJTE9uRk1sdjZ3U3JFMy9tRjZ4QTFKdVNLeitS?=
 =?utf-8?B?YitRb2V4U0dvQWxDZFZKWTFEeWpob291dmxPa3BLZWtLYVFudmMrYlJxWm5v?=
 =?utf-8?B?TmhnK1Faby9BK1o5NE9NS3BpU2xDcWgzWllzVEt2aEFXcS81NGxwVEMzbDc2?=
 =?utf-8?B?S3NJWmhiWmVPZ28ydldhWGpOcTROb2VEZEtSYXgwa1YzaGJaUmozZW5rU0x0?=
 =?utf-8?B?U0pQUDYrd3oyREtxaUlzcnZjMU5qMjFFS1I0Q1JRMHB6ZENJTjZCUG44VUhS?=
 =?utf-8?B?bmdneFRMRDRQdW5DTGcvelY1NjJ3OW42a1o3TGZFdm9HS1VPc2NOSks2b2lG?=
 =?utf-8?B?alA0WUo4VjlkVFBxalE3dHZ4SW8wMzFjdk1rd1pnT1JEcFNHZ1NCeGRNV0U4?=
 =?utf-8?B?akhxZ0F5SUVPWUU3clliV1orYkhyck9nckpwOVpSYjJvYjlRQmNvNDJMQkVp?=
 =?utf-8?B?V2k3bTVvUzlDVEtoZk1XQmdCcCtHMkRXM2sxNE05V2R3dWZWVFpjQW9zK1Nx?=
 =?utf-8?B?SWhjT2hWZlhkVVRqaWtKcTlIVzVtSVRsMlBYenA2MnVLUVhycjBoQ3J0RTJv?=
 =?utf-8?B?MGwxc1RWT0pnY1VwQStIV1FSRHN3PT0=?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c34ad2fa-a957-4351-910c-08d9d450aaca
X-MS-Exchange-CrossTenant-AuthSource: BN6PR11MB4049.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jan 2022 15:48:37.9738
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: T5JWlCMRbZVF++RXL1+Qk8FI3SOiHtJ0mwxnk9lEPNqZTKb6TcMUb5vYQb43fQNbkml4bgoNSNxsAGbQcRKjqvv0BkZmoxG4zfrgmFjoDj0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR11MB3905
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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;
>   }
>