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

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

On 03/17/2011 04:40 PM, Sakari Ailus wrote:
> Hi Laurent and Michael,
> 
> Laurent Pinchart wrote:
>> On Thursday 17 March 2011 11:07:40 Michael Jones wrote:
>>> On 03/16/2011 06:46 PM, Laurent Pinchart wrote:
>>>> 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.
>>
>> How should we handle such a broken .data_lane_shift value ? Always refuse to 
>> start streaming (maybe with a kernel log message) ? Or should we catch it in 
>> isp_register_entities() instead ?
> 
> If I understand correctly it's not possible to shift odd bits in any
> case. It's a hardware limitation.
> 
> I'd perhaps have just the appropriate register bits in the platform data
> so that leaves no room for accidental misconfiguration, but this is
> perhaps just too much work for not much gain.
> 

Actually, that's the way .data_lane_shift was originally defined
(0,1,2,3), and I left it that way to minimize confusion.  I was mistaken
above when I said that .data_lane_shift could sneak an odd shift to
isp_video_is_shiftable(), because .data_lane_shift is multiplied by 2
before getting passed there.  So I would like to leave this as is, and
it sounds like we have a consensus on this.

I'll submit v4 soon, then.

thanks,
Michael

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