Hi Dan,

Thank you for the comments.

On Friday 04 October 2013 11:16:38 Dan Carpenter wrote:
> On Thu, Oct 03, 2013 at 01:55:29AM +0200, Laurent Pinchart wrote:
> > +
> > +   ret = vb2_streamon(&vfh->queue, type);
> > +   if (ret < 0)
> > +           goto err_iss_video_check_format;
> > +
> > +   /* In sensor-to-memory mode, the stream can be started synchronously
> > +    * to the stream on command. In memory-to-memory mode, it will be
> > +    * started when buffers are queued on both the input and output.
> > +    */
> > +   if (pipe->input == NULL) {
> > +           unsigned long flags;
> > +           ret = omap4iss_pipeline_set_stream(pipe,
> > +                                         ISS_PIPELINE_STREAM_CONTINUOUS);
> > +           if (ret < 0)
> > +                   goto err_omap4iss_set_stream;
> > +           spin_lock_irqsave(&video->qlock, flags);
> > +           if (list_empty(&video->dmaqueue))
> > +                   video->dmaqueue_flags |= ISS_VIDEO_DMAQUEUE_UNDERRUN;
> > +           spin_unlock_irqrestore(&video->qlock, flags);
> > +   }
> > +
> > +   if (ret < 0) {
> > +err_omap4iss_set_stream:
> > +           vb2_streamoff(&vfh->queue, type);
> > +err_iss_video_check_format:
> > +           media_entity_pipeline_stop(&video->video.entity);
> > +err_media_entity_pipeline_start:
> > +           if (video->iss->pdata->set_constraints)
> > +                   video->iss->pdata->set_constraints(video->iss, false);
> > +           video->queue = NULL;
> > +   }
> > +
> > +   if (!ret)
> > +           video->streaming = 1;
> > +
> > +   mutex_unlock(&video->stream_lock);
> > +   return ret;
> > +}
> 
> This driver is mostly really nice code, but this error handling is
> spaghetti-al-nasty.  Just split up the success and failure returns.
> 
>       video->streaming = 1;
>       mutex_unlock(&video->stream_lock);
>       return 0;
> 
> err_omap4iss_set_stream:
>       vb2_streamoff(&vfh->queue, type);
> err_iss_video_check_format:
>       media_entity_pipeline_stop(&video->video.entity);
> err_media_entity_pipeline_start:
>       if (video->iss->pdata->set_constraints)
>               video->iss->pdata->set_constraints(video->iss, false);
>       video->queue = NULL;
> 
>       mutex_unlock(&video->stream_lock);
>       return ret;
> }

Sure, I'll fix that.

-- 
Regards,

Laurent Pinchart

_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to