Re: [PATCH 1/3] media: rcar-vin: Fold interrupt helpers into only callers

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Niklas,

Thank you for the patch.

On Sat, Jun 14, 2025 at 04:15:43PM +0200, Niklas Söderlund wrote:
> The call sites using the interrupt helper functions have all been
> reworked to only one for each. Fold echo of them into the only call
> sites left.
> 
> While at it rename the variable holding the current interrupt status to
> make the code easier to read.
> 
> There is no functional change.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  .../platform/renesas/rcar-vin/rcar-dma.c      | 27 +++++--------------
>  1 file changed, 6 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> index 5c08ee2c9807..585b8b3dcfd8 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> @@ -912,21 +912,6 @@ static int rvin_setup(struct rvin_dev *vin)
>  	return 0;
>  }
>  
> -static void rvin_disable_interrupts(struct rvin_dev *vin)
> -{
> -	rvin_write(vin, 0, VNIE_REG);
> -}
> -
> -static u32 rvin_get_interrupt_status(struct rvin_dev *vin)
> -{
> -	return rvin_read(vin, VNINTS_REG);
> -}
> -
> -static void rvin_ack_interrupt(struct rvin_dev *vin)
> -{
> -	rvin_write(vin, rvin_read(vin, VNINTS_REG), VNINTS_REG);
> -}
> -
>  static bool rvin_capture_active(struct rvin_dev *vin)
>  {
>  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
> @@ -1049,22 +1034,22 @@ static void rvin_capture_stop(struct rvin_dev *vin)
>  static irqreturn_t rvin_irq(int irq, void *data)
>  {
>  	struct rvin_dev *vin = data;
> -	u32 int_status, vnms;
> +	u32 status, vnms;
>  	int slot;
>  	unsigned int handled = 0;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&vin->qlock, flags);
>  
> -	int_status = rvin_get_interrupt_status(vin);
> -	if (!int_status)
> +	status = rvin_read(vin, VNINTS_REG);
> +	if (!status)
>  		goto done;
>  
> -	rvin_ack_interrupt(vin);
> +	rvin_write(vin, status, VNINTS_REG);

Actually there is a functional change here. Before this change, if an
interrupt occured between reading VNINTS_REG in
rvin_get_interrupt_status() and reading it again in
rvin_ack_interrupt(), it would get lost. This patch fixes a possible
bug.

With an updated commit message,

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

>  	handled = 1;
>  
>  	/* Nothing to do if nothing was captured. */
> -	if (!(int_status & VNINTS_FIS))
> +	if (!(status & VNINTS_FIS))
>  		goto done;
>  
>  	/* Nothing to do if not running. */
> @@ -1417,7 +1402,7 @@ void rvin_stop_streaming(struct rvin_dev *vin)
>  	rvin_set_stream(vin, 0);
>  
>  	/* disable interrupts */
> -	rvin_disable_interrupts(vin);
> +	rvin_write(vin, 0, VNIE_REG);
>  
>  	/* Return unprocessed buffers from hardware. */
>  	for (unsigned int i = 0; i < HW_BUFFER_NUM; i++) {

-- 
Regards,

Laurent Pinchart