Hi Laurent,
Thanks for your review time.
On 2025-06-15 00:23:14 +0300, Laurent Pinchart wrote:
> 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@xxxxxxxxxxxx>
> > ---
> > .../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.
Indeed, thanks for pointing it out. I will update the commit message to
mention this.
>
> With an updated commit message,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>
> > 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
--
Kind Regards,
Niklas Söderlund
