Re: [PATCH 3/4] rcar-vin: Stop stream when subdevice signal EOS

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hi Jacopo,

Thanks for your comments.

On 2020-11-16 17:58:14 +0100, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Thu, Nov 12, 2020 at 11:51:46PM +0100, Niklas Söderlund wrote:
> > When a subdevice signals end of stream stop the VIN in addition to
> > informing user-space of the event.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index dca3ab1656a66cef..fcaf68c3428b80fd 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -969,9 +969,23 @@ void rvin_v4l2_unregister(struct rvin_dev *vin)
> >  static void rvin_notify_video_device(struct rvin_dev *vin,
> >  				     unsigned int notification, void *arg)
> >  {
> > +	const struct v4l2_event *event;
> > +
> 
> Can this go inside the switch ?

It could but I dislike creating 'case FOO: { }' blocks as I think they 
are hard to read and un C like ;-P

> 
> >  	switch (notification) {
> >  	case V4L2_DEVICE_NOTIFY_EVENT:
> > -		v4l2_event_queue(&vin->vdev, arg);
> > +		event = arg;
> > +
> > +		switch (event->type) {
> > +		case V4L2_EVENT_EOS:
> 
> As there's only a case where this happen, this could be an if, but I
> see a switch is consistent with the existing one. Up to you.

I been bitten by this in the past so now days I go straight for the 
switch() construct :-)

> 
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks!

> 
> Thanks
>    j
> 
> > +			rvin_stop_streaming(vin);
> > +			v4l2_info(&vin->v4l2_dev,
> > +				  "Subdevice signaled end of stream, stopping.\n");
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +
> > +		v4l2_event_queue(&vin->vdev, event);
> >  		break;
> >  	default:
> >  		break;
> > --
> > 2.29.2
> >

-- 
Regards,
Niklas Söderlund