Re: [PATCH 05/14] media: rzg2l-cru: Remove locking from start/stop routines

[Tommaso Merciai]

Hi Jacopo,
Thanks for your patch.

On Fri, Mar 27, 2026 at 06:10:10PM +0100, Jacopo Mondi wrote:
> From: Jacopo Mondi <jacopo.mondi+renesas@xxxxxxxxxxxxxxxx>
> 
> The start/stop streaming routines do not need to lock the whole function
> body against possible concurrent accesses to the CRU buffers or hardware
> registers.
> 
> The stop function starts by disabling interrupts, and only this portion
> needs to be protected not to race against a possible IRQ.
> 
> Once interrupts are disabled, nothing in the video device driver can race
> and once the peripheral has been disabled we can release all pending
> buffers.
> 

LGTM. Tested on RZ/G3E.

Tested-by: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>
Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>


Kind Regards,
Tommaso

> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@xxxxxxxxxxxxxxxx>
> ---
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index b041c72837c6..43b1d35fb963 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -341,23 +341,19 @@ bool rzg2l_fifo_empty(struct rzg2l_cru_dev *cru)
>  void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
>  {
>  	unsigned int retries = 0;
> -	unsigned long flags;
>  	u32 icnms;
>  
> -	spin_lock_irqsave(&cru->qlock, flags);
> -
> -	/* Disable and clear the interrupt */
> -	cru->info->disable_interrupts(cru);
> +	scoped_guard(spinlock_irq, &cru->qlock) {
> +		/* Disable and clear the interrupt */
> +		cru->info->disable_interrupts(cru);
> +	}
>  
>  	/* Stop the operation of image conversion */
>  	rzg2l_cru_write(cru, ICnEN, 0);
>  
>  	/* Wait for streaming to stop */
> -	while ((rzg2l_cru_read(cru, ICnMS) & ICnMS_IA) && retries++ < RZG2L_RETRIES) {
> -		spin_unlock_irqrestore(&cru->qlock, flags);
> +	while ((rzg2l_cru_read(cru, ICnMS) & ICnMS_IA) && retries++ < RZG2L_RETRIES)
>  		msleep(RZG2L_TIMEOUT_MS);
> -		spin_lock_irqsave(&cru->qlock, flags);
> -	}
>  
>  	icnms = rzg2l_cru_read(cru, ICnMS) & ICnMS_IA;
>  	if (icnms)
> @@ -401,8 +397,6 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
>  
>  	/* Resets the image processing module */
>  	rzg2l_cru_write(cru, CRUnRST, 0);
> -
> -	spin_unlock_irqrestore(&cru->qlock, flags);
>  }
>  
>  static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
> @@ -470,8 +464,6 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
>  	csi_vc = ret;
>  	cru->svc_channel = csi_vc;
>  
> -	guard(spinlock_irqsave)(&cru->qlock);
> -
>  	/* Select a video input */
>  	rzg2l_cru_write(cru, CRUnCTRL, CRUnCTRL_VINSEL(0));
>  
> 
> -- 
> 2.53.0
>