Re: [PATCH 2/3] media: rcar-vin: Check for correct capture interrupt event

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

Hi Niklas,

On Sat, Jun 14, 2025 at 04:15:44PM +0200, Niklas Söderlund wrote:
> Depending on if the capture session deals with fields or whole frames
> interrupts can be generated at an end of field, or end of frame event.
> The interrupt mask is setup to generate an interrupt on one of the two
> events depending on what is needed when the VIN is started. The end of
> field bit is set in both cases so controlling the mask that generates an
> interrupt have been enough to control the two use-cases.
> 
> Before extending the interrupt handler to deal with other types of
> interrupt events it is needs to extended to "capture complete" check for
> correct the use-case in operation. Without this the simplification in
> the handler can result in corrupted frames when the mask on what type of
> events can generate an interrupt generated can no longer be assumed to
> only be an "capture complete" event.
> 
> Which bit is checked matches which bit is enabled at configuration time
> as which event can generate an interrupt for "capture complete". There
> is no functional change.
> 
> While at it switch to use the BIT() macro to describe the bit positions
> for the interrupt functions.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> index 585b8b3dcfd8..85e44a00e0fc 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> @@ -115,11 +115,12 @@
>  #define VNFC_S_FRAME		(1 << 0)
>  
>  /* Video n Interrupt Enable Register bits */
> -#define VNIE_FIE		(1 << 4)
> -#define VNIE_EFE		(1 << 1)
> +#define VNIE_FIE		BIT(4)
> +#define VNIE_EFE		BIT(1)
>  
>  /* Video n Interrupt Status Register bits */
> -#define VNINTS_FIS		(1 << 4)
> +#define VNINTS_FIS		BIT(4)
> +#define VNINTS_EFS		BIT(1)
>  
>  /* Video n Data Mode Register bits */
>  #define VNDMR_A8BIT(n)		(((n) & 0xff) << 24)
> @@ -1034,7 +1035,7 @@ static void rvin_capture_stop(struct rvin_dev *vin)
>  static irqreturn_t rvin_irq(int irq, void *data)
>  {
>  	struct rvin_dev *vin = data;
> -	u32 status, vnms;
> +	u32 capture, status, vnms;
>  	int slot;
>  	unsigned int handled = 0;
>  	unsigned long flags;
> @@ -1049,7 +1050,10 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  	handled = 1;
>  
>  	/* Nothing to do if nothing was captured. */
> -	if (!(status & VNINTS_FIS))
> +	capture = vin->format.field == V4L2_FIELD_NONE ||
> +		vin->format.field == V4L2_FIELD_ALTERNATE ?
> +		VNINTS_FIS : VNINTS_EFS;

I would find

	capture = vin->format.field == V4L2_FIELD_NONE ||
		  vin->format.field == V4L2_FIELD_ALTERNATE
		? VNINTS_FIS : VNINTS_EFS;

easier to read, but it could be just me.

Do I read it correctly that you use the "Field Interrupt" with
V4L2_FIELD_NONE ? That seems a bit weird to me, but maybe I don't get
how the hardware operates.

> +	if (!(status & capture))
>  		goto done;
>  
>  	/* Nothing to do if not running. */

-- 
Regards,

Laurent Pinchart