Re: [PATCH v7] s5k5baf: add camera sensor driver

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

Hi Mark,

On 08/27/2013 11:14 AM, Mark Rutland wrote:
>> +endpoint node
>> +-------------
>> +
>> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
>> +  video-interfaces.txt. This property can be only used to specify number
>> +  of data lanes, i.e. the array's content is unused, only its length is
>> +  meaningful. When this property is not specified default value of 1 lane
>> +  will be used.
> 
> Apologies for having not replied to the last posting, but having looked
> at the documentation I was provided last time [1], I don't think the
> values in the data-lanes property should be described as unused. That
> may be the way the Linux driver functions at present, but it's not how
> the generic video-interfaces binding documentation describes the
> property.
> 
> If the CSI transmitter hardware doesn't support logical remapping of
> lanes, then the only valid values for data-lanes would be a contiguous
> list of lane IDs starting at 1, ending at 4 at most. Valid values for
> the property would be one of:
> 
> data-lanes = <1>;
> data-lanes = <1>, <2>;
> data-lanes = <1>, <2>, <3>;
> data-lanes = <1>, <2>, <3>, <4>;
> 
> We can mention the fact the hardware doesn't support remapping of lanes,
> and therefore the list must start with lane 1 and end with (at most)
> lane 4. That way a dts will match the generic binding and actually
> describe the hardware, and it's possible for Linux (or any other OS) to
> factor out the parsing of data-lanes later as desired.
> 
> I don't think we should offer freedom to encode garbage in the dt when
> we can just as easily encourage more standard use of bindings that will
> make our lives easier in the long-term.

I entirely agree, that's a very accurate analysis.

Presumably the data-lanes property's descriptions could be improved so
it is said explicitly that array elements 0...N - 1, where N = 4, 
correspond to logical data lanes 1...N.

Then considering the values in the data-lanes property, I didn't make
the description terribly specific about the fact that pool of indexes
0...4 is used for the clock lane and 4 data lanes. The values could well
be H/W specific, but it seems more sensible to enforce common range.
It may not match exactly with documentation of various hardware. E.g.
OMAP, see page 1661, register CSI2_COMPLEXIO_CFG [1], uses indexes 
1..5 to indicate position of a data lane and 0 is used to mark a lane 
as unused.

I think we should have similarly defined value 0 to indicate an unused 
lane. None of drivers in mainline uses this line remapping feature, so 
changing meaning of the array values wouldn't presumably have any bad 
side effects. I'm not sure if it's OK to make a change like this now. 
IIUC the MIPI CSI-2 standard requires consecutive data lane indexes, 
so valid set of data lanes could be only: <1>, <1, 2>, <1, 2, 3>, 
<1, 2, 3, 4>. So there seems to be no issue for MIPI CSI-2. But for 
future protocols the current convention might not have been flexible 
enough.

> [1] http://www.mipi.org/specifications/camera-interface#CSI2

[2] http://www.ti.com/general/docs/lit/getliterature.tsp?literatureNumber=swpu231ao&fileType=pdf

--
Regards,
Sylwester