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

[Michael Jones <michael.jones@matrix-vision.de>]

Hi Sakari,

Thanks for the review.

On 03/16/2011 06:46 PM, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wednesday 16 March 2011 18:08:04 Sakari Ailus wrote:
>> Laurent Pinchart wrote:
>>> Hi Sakari,
>>>>> +	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?
> 
> I agree it shouldn't be allowed, but the ISP driver doesn't support non-even 
> widths at the moment, so there's no big risk. There could be an issue when a 
> non-even width is added to the driver if the developer forgets to update the 
> shift code. Maybe a comment in ispvideo.c above the big formats array would 
> help making sure this is not forgotten ?

I think now that additional_shift is also being considered which comes
from the board file, it makes sense to reintroduce the check for an even
shift.  As Sakari points out, this would be helpful for debugging if
someone tries using .data_lane_shift which is odd.

> 
>>>>> @@ -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. :-)
> 

But this is not a feature of the CCDC, the lane shifter is outside of
the CCDC.  Each 'while (1)' iteration handles 2 subdevs on each side of
one link, so I think it makes sense for a particular iteration to say
"this link has", especially when the subdev ptr changes values between
the assignment of this var and its usage.  "is_ccdc" is vague as to
which side of the CCDC we're on.  'link_has_shifter' wasn't intended to
be general, it was supposed to mean 'this_is_the_link_with_the_shifter'.
 If you want to be more specific where that is in the pipeline, maybe
'ccdc_sink_link'?  If you just want it to sound less like "this is one
of the links with a shifter" and more like "We've found _the_ link with
_the_ shifter", it could just be 'shifter_link'.

After we iron these two things out, are you guys ready to see v4?

-Michael

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner