Re: [PATCH 1/3] rcar-vin: remove duplicated check of state in irq handler

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 3410 bytes --]

Hi Niklas, Kieran,

On Tue, Mar 13, 2018 at 06:56:54PM +0100, Niklas Söderlund wrote:
> Hi Kieran,
>
> Thanks for your feedback.
>
> On 2018-03-13 17:42:25 +0100, Kieran Bingham wrote:
> > Hi Niklas,
> >
> > Thanks for the patch series :) - I've been looking forward to seeing this one !
> >
> > On 10/03/18 01:09, Niklas Söderlund wrote:
> > > This is an error from when the driver where converted from soc-camera.
> > > There is absolutely no gain to check the state variable two times to be
> > > extra sure if the hardware is stopped.
> >
> > I'll not say this isn't a redundant check ... but isn't the check against two
> > different states, and thus the remaining check doesn't actually catch the case
> > now where state == STOPPED ?
>
> Thanks for noticing this, you are correct. I think I need to refresh my
> glasses subscription after missing this :-)
>
> >
> > (Perhaps state != RUNNING would be better ?, but I haven't checked the rest of
> > the code)
>
> I will respin this in a v2 and either use state != RUNNING or at least
> combine the two checks to prevent future embarrassing mistakes like
> this.

I am sorry I have missed this comment, but I think your patch has some
merits. Ofc no need to hold on v2 of this series for this, but still I
think you can re-propose this later (and I didn't get from
your commit message you were confusing STOPPED/STOPPING).

In rvin_stop_streaming(), you enter STOPPING state, disable the
interface cleaning ME bit in VnMC and single/continuous capture mode
in VnFC, and then poll on CA bit of VnMS until the VIN peripheral has
not been stopped, at this  point you set interface state to STOPPED.

As you loop you can still receive interrupts, as you are releasing the
spinlock when sleeping before testing the ME bit again, so it's fine
checking for STOPPING state in irq handler.
It seems to me though, that once you enter STOPPED state, you are sure the
peripheral has stopped and you should not receive any more interrupt, spurious
ones apart or when the peripheral fails to stop at all, but things went
south already at that point.

Again no need to have this part of this series, but you may want to
take into consideration this for the future, as with this change you can
remove the STOPPED state at all from the driver.

Thanks
   j

>
> >
> > --
> > Kieran
> >
> >
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-dma.c | 6 ------
> > >  1 file changed, 6 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > index 23fdff7a7370842e..b4be75d5009080f7 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > @@ -916,12 +916,6 @@ static irqreturn_t rvin_irq(int irq, void *data)
> > >  	rvin_ack_interrupt(vin);
> > >  	handled = 1;
> > >
> > > -	/* Nothing to do if capture status is 'STOPPED' */
> > > -	if (vin->state == STOPPED) {
> > > -		vin_dbg(vin, "IRQ while state stopped\n");
> > > -		goto done;
> > > -	}
> > > -
> > >  	/* Nothing to do if capture status is 'STOPPING' */
> > >  	if (vin->state == STOPPING) {
> > >  		vin_dbg(vin, "IRQ while state stopping\n");
> > >
>
> --
> Regards,
> Niklas Söderlund

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]