Re: [REVIEWv3 PATCH 01/13] vb2: stop_streaming should return void

[Mauro Carvalho Chehab <m.chehab@samsung.com>]

Hi Hans,

Em Wed, 16 Apr 2014 18:38:25 -0300
Mauro Carvalho Chehab <m.chehab@samsung.com> escreveu:

> Em Fri, 11 Apr 2014 10:11:07 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 

> > --- a/drivers/media/platform/blackfin/bfin_capture.c
> > +++ b/drivers/media/platform/blackfin/bfin_capture.c
> > @@ -427,15 +427,12 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count)
> >  	return 0;
> >  }
> >  
> > -static int bcap_stop_streaming(struct vb2_queue *vq)
> > +static void bcap_stop_streaming(struct vb2_queue *vq)
> >  {
> >  	struct bcap_device *bcap_dev = vb2_get_drv_priv(vq);
> >  	struct ppi_if *ppi = bcap_dev->ppi;
> >  	int ret;
> >  
> > -	if (!vb2_is_streaming(vq))
> > -		return 0;
> > -
> 
> Why are you dropping this? IMHO, you should be doing, instead:
> 	if (!vb2_is_streaming(vq))
> 		return;
> 
> Except if you're 100% sure that checking it here can be removed. On
> this case, please put this on a separate patch, clearly explaining
> why we can safely remove this.
> 
> Please notice that on other similar parts of this patch, you didn't remove
> the test, just removed the returned parameter.

As I said before, if you need to remove the vb2_is_streaming(vq) check
above, please do it on a separate patch, properly justifying why you're
doing that.

This hunk is still present on your new pull request.


-- 

Regards,
Mauro