Re: [PATCH v3 4/4] omap3isp: lane shifter support

[Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>]

Laurent Pinchart wrote:
> Hi Sakari,

Hi Laurent and Michael!

...
>>> +	return in_info->bpp - out_info->bpp + additional_shift <= 6;
>>
>> Currently there are no formats that would behave badly in this check?
>> Perhaps it'd be good idea to take that into consideration. The shift
>> that can be done is even.
> 
> I've asked Michael to remove the check because we have no misbehaving formats 
> :-) Do you think we need to add a check back ?

I think it would be helpful in debugging if someone decides to attach a
sensor which supports a shift of non-even bits (8 and 9 bits, for
example). In any case an invalid configuration is possible in such case,
and I don't think that should be allowed, should it?

>>> @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct
>>> isp_pipeline *pipe)
>>>
>>>  		return -EPIPE;
>>>  	
>>>  	while (1) {
>>>
>>> +		unsigned int link_has_shifter;
>>
>> link_has_shifter is only used in one place. Would it be cleaner to test
>> below if it's the CCDC? A comment there could be nice, too.
> 
> I would like that better as well, but between the line where link_has_shifter 
> is set and the line where it is checked, the subdev variable changes so we 
> can't just check subdev == &isp->isp_ccdc.subdev there.

That's definitely valid. I take my comment back. The variable could be
called is_ccdc, though, since only the CCDC has that feature. No need to
generalise. :-)

Cheers,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com