Re: [PATCH v2 5/5] v4l: Renesas R-Car VSP1 driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

On Thursday 25 July 2013 16:43:28 Sakari Ailus wrote:
> On Thu, Jul 25, 2013 at 01:46:54PM +0200, Laurent Pinchart wrote:
> > > On Wed, Jul 17, 2013 at 04:54:42PM +0200, Laurent Pinchart wrote:
> > > ...
> > > 
> > > > +static void vsp1_device_init(struct vsp1_device *vsp1)
> > > > +{
> > > > +	unsigned int i;
> > > > +	u32 status;
> > > > +
> > > > +	/* Reset any channel that might be running. */
> > > > +	status = vsp1_read(vsp1, VI6_STATUS);
> > > > +
> > > > +	for (i = 0; i < VPS1_MAX_WPF; ++i) {
> > > > +		unsigned int timeout;
> > > > +
> > > > +		if (!(status & VI6_STATUS_SYS_ACT(i)))
> > > > +			continue;
> > > > +
> > > > +		vsp1_write(vsp1, VI6_SRESET, VI6_SRESET_SRTS(i));
> > > > +		for (timeout = 10; timeout > 0; --timeout) {
> > > > +			status = vsp1_read(vsp1, VI6_STATUS);
> > > > +			if (!(status & VI6_STATUS_SYS_ACT(i)))
> > > > +				break;
> > > > +
> > > > +			usleep_range(1000, 2000);
> > > > +		}
> > > > +
> > > > +		if (timeout)
> > > > +			dev_err(vsp1->dev, "failed to reset wpf.%u\n", i);
> > > 
> > > Have you seen this happening in practice? Do you expect the device to
> > > function if resetting it fails?
> > 
> > I've seen this happening during development when I had messed up register
> > values, but not otherwise. I don't expect the deviec to still function if
> > resetting the WPF fails, but I need to make sure that the busy loop exits.
> 
> Shouldn't you also return an error in this case? The function currently
> returns void.

I will fix that.

> ...
> 
> > > > +	/* Follow links downstream for each input and make sure the graph
> > > > +	 * contains no loop and that all branches end at the output WPF.
> > > > +	 */
> > > 
> > > I wonder if checking for loops should be done already in pipeline
> > > validation done by the framework. That's fine to do later on IMHO, too.
> > 
> > It would have to be performed by the core, as the callbacks are local to
> > links. That's feasible (but should be optional, as some devices might
> > support circular graphs), feel free to submit a patch :-)
> 
> As a matter of fact I think I will. I'd like you to test it though since I
> have no hardware with such media graph. :-)

Sure :-)

> But please don't expect to see that in time for your driver to get in. Let's
> think about that later on.

OK.

-- 
Regards,

Laurent Pinchart