Hi Guennadi,
On 07/16/2012 10:55 AM, Guennadi Liakhovetski wrote:
> Hi Sylwester
>
> Thanks for your comments to my RFC and for pointing out to this your
> earlier patch series. Unfortunately, I missed in in May, let me try to
> provide some thoughts about this, we should really try to converge our
> proposals. Maybe a discussion at KS would help too.
Thank you for the review. I was happy to see your RFC, as previously
there seemed to be not much interest in DT among the media guys.
Certainly, we need to work on a common approach to ensure interoperability
of existing drivers and to avoid having people inventing different
bindings for common features. I would also expect some share of device
specific bindings, as diversity of media devices is significant.
I'd be great to discuss these things at KS, especially support for
proper suspend/resume sequences. Also having common sessions with
other subsystems folks, like ASoC, for example, might be a good idea.
I'm not sure if I'll be travelling to the KS though. :)
> On Fri, 25 May 2012, Sylwester Nawrocki wrote:
>
>> s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
>> (camera host interface DMA engine and image processor). This patch
>> adds support for instantiating the MIPI-CSIS devices from DT and
>> parsing all SoC and board specific properties from device tree.
>> The MIPI DPHY control callback is now called directly from within
>> the driver, the platform code must ensure this callback does the
>> right thing for each SoC.
>>
>> The cell-index property is used to ensure proper signal routing,
>> from physical camera port, through MIPI-CSI2 receiver to the DMA
>> engine (FIMC?). It's also helpful in exposing the device topology
>> in user space at /dev/media? devnode (Media Controller API).
>>
>> This patch also defines a common property ("data-lanes") for MIPI-CSI
>> receivers and transmitters.
>>
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@xxxxxxxxxxx>
>> Signed-off-by: Kyungmin Park<kyungmin.park@xxxxxxxxxxx>
>> ---
>> Documentation/devicetree/bindings/video/mipi.txt | 5 +
>> .../bindings/video/samsung-mipi-csis.txt | 47 ++++++++++
>> drivers/media/video/s5p-fimc/mipi-csis.c | 97 +++++++++++++++-----
>> drivers/media/video/s5p-fimc/mipi-csis.h | 1 +
>> 4 files changed, 126 insertions(+), 24 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/video/mipi.txt
>> create mode 100644 Documentation/devicetree/bindings/video/samsung-mipi-csis.txt
>>
>> diff --git a/Documentation/devicetree/bindings/video/mipi.txt b/Documentation/devicetree/bindings/video/mipi.txt
>> new file mode 100644
>> index 0000000..5aed285
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/video/mipi.txt
>> @@ -0,0 +1,5 @@
>> +Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and transmitters
>> +
>> + - data-lanes : number of differential data lanes wired and actively used in
>> + communication between the transmitter and the receiver, this
>> + excludes the clock lane;
>
> Wouldn't it be better to use the standard "bus-width" DT property?
I can't see any problems with using "bus-width". It seems sufficient
and could indeed be better, without a need to invent new MIPI-CSI
specific names. That was my first RFC on that and my perspective
wasn't probably broad enough. :)
>> diff --git a/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt b/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt
>> new file mode 100644
>> index 0000000..7bce6f4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt
>> @@ -0,0 +1,47 @@
>> +Samsung S5P/EXYNOS SoC MIPI-CSI2 receiver (MIPI CSIS)
>> +-----------------------------------------------------
>> +
>> +Required properties:
>> +
>> +- compatible - one of :
>> + "samsung,s5pv210-csis",
>> + "samsung,exynos4210-csis",
>> + "samsung,exynos4212-csis",
>> + "samsung,exynos4412-csis";
>> +- reg : physical base address and size of the device memory mapped registers;
>> +- interrupts : should contain MIPI CSIS interrupt; the format of the
>> + interrupt specifier depends on the interrupt controller;
>> +- cell-index : the hardware instance index;
>
> Not sure whether this is absolutely needed... Wouldn't it be sufficient to
> just enumerate them during probing?
As Grant pointed to me, the "cell-index" property is something that we should
be avoiding. But I needed something like this in the driver,
to differentiate between the multiple IP instances. I cannot simply assign
the indexes in random way to the hardware instances. Each of the MIPI-CSI
Slaves has different properties (e.g. supporting max. 2 or 4 data lanes).
Additionally, a particular MIPI-CSI Slave instance is hard-wired inside the
SoC to the FIMC input mux (cross-bar) and physical video port. IOW, an image
sensor/video port/MIPI-CSIS instance assignment is fixed. If two MIPI-CSI
image sensors are connected, one of them will work only with MIPI-CSIS0, and
the other only with MIPI-CSIS1.
So the driver must be able to identify it's physical device (IO region +
IRQ) precisely.
That said, I found out recently that a proper entries in the "aliases"
could be used instead, and I could finally abandon the "cell-index" idea.
Not sure how aliases approach is better but from what I can see it is
a preferred way to handle things like the above.
Please see Documentation/devicetree/bindings/sound/mxs-saif.txt and users
of of_alias_get_id() for more details.
So to summarize, the indexes are needed, but the current implementation
is not necessarily good, and AFAICT the aliases approach could be the way
to go.
>> +- clock-frequency : The IP's main (system bus) clock frequency in Hz, the default
>> + value when this property is not specified is 166 MHz;
>> +- data-lanes : number of physical MIPI-CSI2 lanes used;
>
> ditto - bus-width?
Yes, agreed. Let's drop it in favour of "bus-width".
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
