Add device tree support for s5p-fimc SoC camera host interface driver

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 <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 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

Documentation/devicetree/bindings/video/mipi.txt new file +0 -0

new file mode 100644
index 0000000..5aed285

Documentation/devicetree/bindings/video/samsung-mipi-csis.txt new file +4 -0

new file mode 100644
index 0000000..7bce6f4
@@ -0,0 +1,47 @@
+Samsung S5P/EXYNOS SoC MIPI-CSI2 receiver (MIPI CSIS)
+-----------------------------------------------------
+
+Required properties:

Documentation/devicetree/bindings/video/samsung-mipi-csis.txt new file +20 -0

+- samsung,csis-hs-settle : differential receiver (HS-RX) settle time;
+- vddio-supply    : MIPI CSIS I/O and PLL voltage supply (e.g. 1.8V);
+- vddcore-supply  : MIPI CSIS Core voltage supply (e.g. 1.1V).
+
+Example:
+
+	reg0: regulator@0 {
+	};
+
+	reg1: regulator@1 {
+	};
+
+/* SoC properties */
+
+	csis@11880000 {
+		compatible = "samsung,exynos4210-csis";
+		reg = <0x11880000 0x1000>;
+		interrupts = <0 78 0>;
+		cell-index = <0>;
+	};

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.

Quoted 28 lines

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<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---
  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

Documentation/devicetree/bindings/video/mipi.txt line 5

Quoted 11 lines

Documentation/devicetree/bindings/video/mipi.txt new file +5 -0

new file mode 100644
index 0000000..5aed285
@@ -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?

Quoted 15 lines

Documentation/devicetree/bindings/video/samsung-mipi-csis.txt new file +9 -0

new file mode 100644
index 0000000..7bce6f4
@@ -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",

Documentation/devicetree/bindings/video/samsung-mipi-csis.txt line 14

Quoted 5 lines

Documentation/devicetree/bindings/video/samsung-mipi-csis.txt new file +5 -0

+		"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?

Documentation/devicetree/bindings/video/samsung-mipi-csis.txt line 17

Quoted 3 lines

Documentation/devicetree/bindings/video/samsung-mipi-csis.txt new file +3 -0

+- 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?

Quoted 6 lines · Guennadi Liakhovetski

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

Quoted 36 lines · Sylwester Nawrocki

Quoted 35 lines

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<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---

  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>>

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/video/mipi.txt new file mode 100644 index 0000000..5aed285

Documentation/devicetree/bindings/video/mipi.txt new file +0 -0

@@ -0,0 +1,5 @@

Quoted 8 lines

Documentation/devicetree/bindings/video/mipi.txt line 5

Quoted 6 lines

Documentation/devicetree/bindings/video/mipi.txt new file +3 -0

+Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and transmitters
+
+ - data-lanes : number of differential data lanes wired and actively

used in

Documentation/devicetree/bindings/video/mipi.txt new file +2 -0

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

Quoted 12 lines

Quoted 11 lines

Documentation/devicetree/bindings/video/mipi.txt line 5

Quoted 9 lines

Documentation/devicetree/bindings/video/mipi.txt new file +3 -0

@@ -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

Documentation/devicetree/bindings/video/mipi.txt new file +2 -0

+		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?

Quoted 4 lines

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

What about CSI receivers that can reroute the lanes internally ? We would need to specify lane indices for each lane then, maybe with something like

clock-lane =<0>;
data-lanes =<2 3 1>;

For receivers that can't reroute lanes internally, the data-lanes property would be need to specify lanes in sequence.

data-lanes =<1 2 3>;

Quoted 18 lines · Laurent Pinchart

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

Documentation/devicetree/bindings/video/mipi.txt new file +1 -0

@@ -0,0 +1,5 @@
+Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and

Quoted 8 lines

Documentation/devicetree/bindings/video/mipi.txt line 5

Quoted 6 lines

transmitters
+
+ - data-lanes : number of differential data lanes wired and actively used in

Documentation/devicetree/bindings/video/mipi.txt new file +2 -0

+		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?

Quoted 4 lines

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

Quoted 6 lines

What about CSI receivers that can reroute the lanes internally ? We would need to specify lane indices for each lane then, maybe with something like

clock-lane =<0>;
data-lanes =<2 3 1>;

Sounds good to me. And the clock-lane could be made optional, as not all devices would need it.

However, as far as I can see, there is currently no generic API for handling this kind of data structure. E.g. number of cells for the "interrupts" property is specified with an additional "#interrupt-cells" property.

It would have been much easier to handle something like:

data-lanes = <2>, <3>, <1>;

i.e. an array of the lane indexes.

Quoted 4 lines

For receivers that can't reroute lanes internally, the data-lanes property would be need to specify lanes in sequence.

data-lanes =<1 2 3>;

In this case we would be only interested in the number of cells in this property, but how it could be retrieved ? With an array, it could have been calculated from property length returned by of_property_find() (divided by sizof(u32)).

Hi Sylwester,

Quoted 33 lines · On Thursday 26 July 2012 21:51:30 Sylwester Nawrocki wrote:

Quoted 18 lines · Laurent Pinchart

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

Documentation/devicetree/bindings/video/mipi.txt new file +1 -0

@@ -0,0 +1,5 @@
+Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and

Quoted 8 lines

Documentation/devicetree/bindings/video/mipi.txt line 5

Quoted 6 lines

transmitters
+
+ - data-lanes : number of differential data lanes wired and actively used in

Documentation/devicetree/bindings/video/mipi.txt new file +2 -0

+		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?

Quoted 4 lines

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

Quoted 6 lines

What about CSI receivers that can reroute the lanes internally ? We would need to specify lane indices for each lane then, maybe with something like

clock-lane =<0>;
data-lanes =<2 3 1>;

Sounds good to me. And the clock-lane could be made optional, as not all devices would need it.

However, as far as I can see, there is currently no generic API for handling this kind of data structure. E.g. number of cells for the "interrupts" property is specified with an additional "#interrupt-cells" property.

Quoted 5 lines

It would have been much easier to handle something like:

data-lanes = <2>, <3>, <1>;

i.e. an array of the lane indexes.

I'm fine with that.

Quoted 9 lines

Quoted 4 lines

For receivers that can't reroute lanes internally, the data-lanes property would be need to specify lanes in sequence.

data-lanes =<1 2 3>;

In this case we would be only interested in the number of cells in this property, but how it could be retrieved ? With an array, it could have been calculated from property length returned by of_property_find() (divided by sizof(u32)).

Agreed.

Signature · 3 lines

Regards,

Laurent Pinchart

Quoted 41 lines · Laurent Pinchart

Quoted 40 lines · On Thursday 26 July 2012 21:51:30 Sylwester Nawrocki wrote:

Quoted 19 lines · Laurent Pinchart

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

Documentation/devicetree/bindings/video/mipi.txt new file +1 -0

@@ -0,0 +1,5 @@
+Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and

transmitters

Quoted 8 lines

Documentation/devicetree/bindings/video/mipi.txt

Quoted 6 lines

+
+ - data-lanes : number of differential data lanes wired and actively
used in

Documentation/devicetree/bindings/video/mipi.txt new file +2 -0

+		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?

Quoted 4 lines

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

Quoted 7 lines

What about CSI receivers that can reroute the lanes internally ? We would
need to specify lane indices for each lane then, maybe with something like

clock-lane =<0>;
data-lanes =<2 3 1>;

Sounds good to me. And the clock-lane could be made optional, as not all devices would need it.

However, as far as I can see, there is currently no generic API for handling this kind of data structure. E.g. number of cells for the "interrupts" property is specified with an additional "#interrupt-cells" property.

It would have been much easier to handle something like:

data-lanes = <2>, <3>, <1>;

Quoted 3 lines

Quoted 1 line

i.e. an array of the lane indexes.

I'm fine with that.

...on a second thought: shouldn't this be handled by pinctrl? Or is it some CSI-2 module internal lane switching, not the global SoC pin function configuration?

Quoted 22 lines

Quoted 22 lines

Quoted 21 lines

Quoted 7 lines

What about CSI receivers that can reroute the lanes internally ? We would
need to specify lane indices for each lane then, maybe with something like

clock-lane =<0>;
data-lanes =<2 3 1>;

Sounds good to me. And the clock-lane could be made optional, as not all devices would need it.

However, as far as I can see, there is currently no generic API for handling this kind of data structure. E.g. number of cells for the "interrupts" property is specified with an additional "#interrupt-cells" property.

It would have been much easier to handle something like:

data-lanes = <2>, <3>, <1>;

i.e. an array of the lane indexes.

Quoted 5 lines

Quoted 1 line

I'm fine with that.

...on a second thought: shouldn't this be handled by pinctrl? Or is it some CSI-2 module internal lane switching, not the global SoC pin function configuration?

On the hardware I came across, it's handled by the CSI2 receiver, not the SoC pinmux feature.

Quoted 27 lines · Laurent Pinchart

Quoted 26 lines

Quoted 15 lines

Quoted 15 lines

Quoted 6 lines

What about CSI receivers that can reroute the lanes internally ? We would need to specify lane indices for each lane then, maybe with something like

clock-lane =<0>;
data-lanes =<2 3 1>;

Sounds good to me. And the clock-lane could be made optional, as not all devices would need it.

However, as far as I can see, there is currently no generic API for handling this kind of data structure. E.g. number of cells for the "interrupts" property is specified with an additional "#interrupt-cells" property.

Quoted 7 lines

Quoted 5 lines

It would have been much easier to handle something like:

data-lanes = <2>, <3>, <1>;

i.e. an array of the lane indexes.

I'm fine with that.

...on a second thought: shouldn't this be handled by pinctrl? Or is it some CSI-2 module internal lane switching, not the global SoC pin function configuration?

Quoted 2 lines

On the hardware I came across, it's handled by the CSI2 receiver, not the SoC pinmux feature.

Same here. Is there any hardware known to mux those MIPI-CSI D-PHY high speed differential signals with general purpose IO pins ?

Or are there mostly dedicated pins used ?

However, if there are cases the lane configuration is done solely at CSI2 receiver level, there seems little point in involving the pinctrl API.

Hi Sylwester,

Quoted 66 lines · On Tuesday 17 July 2012 20:16:23 Sylwester Nawrocki wrote:

Quoted 6 lines · Guennadi Liakhovetski

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

Quoted 36 lines · Sylwester Nawrocki

Quoted 35 lines

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<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---

  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>>

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/video/mipi.txt new file mode 100644 index 0000000..5aed285

Documentation/devicetree/bindings/video/mipi.txt new file +0 -0

@@ -0,0 +1,5 @@

Quoted 8 lines

Documentation/devicetree/bindings/video/mipi.txt line 5

Quoted 6 lines

Documentation/devicetree/bindings/video/mipi.txt new file +3 -0

+Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and transmitters
+
+ - data-lanes : number of differential data lanes wired and actively

used in

Documentation/devicetree/bindings/video/mipi.txt new file +2 -0

+		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?

Quoted 4 lines

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

What about CSI receivers that can reroute the lanes internally ? We would need to specify lane indices for each lane then, maybe with something like

clock-lane = <0>;

data-lanes = <2 3 1>;

For receivers that can't reroute lanes internally, the data-lanes property would be need to specify lanes in sequence.

data-lanes = <1 2 3>;

Signature · 3 lines

Regards,

Laurent Pinchart

Signed-off-by Sylwester Nawrocki <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 Documentation/devicetree/bindings/video/video.txt |   10 ++++++++++
 1 file changed, 10 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/video.txt

Documentation/devicetree/bindings/video/video.txt new file +0 -0

new file mode 100644
index 0000000..9f6a637

Quoted 17 lines · Sylwester Nawrocki

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---
  Documentation/devicetree/bindings/video/video.txt |   10 ++++++++++
  1 file changed, 10 insertions(+)

create mode 100644 Documentation/devicetree/bindings/video/video.txt

Documentation/devicetree/bindings/video/video.txt new file +4 -0

new file mode 100644
index 0000000..9f6a637
@@ -0,0 +1,10 @@
+Common properties of video data source devices (image sensor, video encoders, etc.)
+
+Video bus types
+---------------

Documentation/devicetree/bindings/video/video.txt line 9

Quoted 5 lines

Documentation/devicetree/bindings/video/video.txt new file +5 -0

+
+- video-bus-type : must be one of:
+
+    - itu-601   : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
+    - itu-656   : parallel bus with embedded synchronization - ITU-R BT.656;

wikipedia tells me, that bt.601 is mostly a data encoding standard, it also defines bus-types, but recent versions also include serial busses.

Documentation/devicetree/bindings/video/video.txt line 10

Quoted 1 line

Documentation/devicetree/bindings/video/video.txt new file +1 -0

+    - mipi-csi2 : MIPI-CSI2 serial bus;

In general: are these at all needed? Wouldn't it be better to specify pads on sensors and interfaces to differentiate between serial and parallel connections. As for whether HSYNC and VSYNC are used - I see 3

possibilities there:

1. real sync signals are used - the default.

2. embedded syncs shall be used, because sync signals are missing. This should (arguably) be derived from pinctrl - see this discussion:

http://lkml.indiana.edu/hypermail/linux/kernel/1205.2/index.html#03893

3. sync signals are present, but cannot be used, because one of the partners doesn't support them - .g_mbus_config() can be used to retrieve this information.

4. sync signals are available and supported by both peers, but for some reason the user prefers to use embedded sync data - is such a case feasible? Even if so, this should be run-time configurable then?

So, maybe we don't need these at all.

Signed-off-by Sylwester Nawrocki <[email protected]>

Signed-off-by Karol Lewandowski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

From the documentation below I think, I understand what it does, but why

---
 .../bindings/camera/soc/samsung-fimc.txt           |   66 ++++
 drivers/media/video/s5p-fimc/fimc-capture.c        |    2 +-
 drivers/media/video/s5p-fimc/fimc-core.c           |  410 +++++++++++---------
 drivers/media/video/s5p-fimc/fimc-core.h           |    2 -
 drivers/media/video/s5p-fimc/fimc-mdevice.c        |    8 +-
 5 files changed, 291 insertions(+), 197 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt new file +56 -0

new file mode 100644
index 0000000..1ec48e9
@@ -0,0 +1,66 @@
+Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
+----------------------------------------------
+
+The Exynos Camera subsystem uses a dedicated device node associated with
+top level device driver that manages common properties of the whole subsystem,
+like common camera port pins or clocks for external image sensors. This
+aggregate node references related platform sub-devices - FIMC, FIMC-LITE,
+MIPI-CSIS [1], and it also contains nodes describing image sensors wired to
+the host SoC's video port and using I2C or SPI as the control bus.
+
+
+Common 'camera' node
+--------------------
+
+Required properties:
+
+- compatible	   : must be "samsung,fimc"
+- fimc-controllers : an array of phandles to 'fimc' device nodes,
+		     size of this array must be at least 1;
+
+Optional properties:
+
+- csi-rx-controllers : an array of phandles to 'csis' device nodes,
+		       it is required for sensors with MIPI-CSI2 bus;
+
+'fimc' device node
+------------------
+
+Required properties:
+
+- compatible : should be one of:
+		"samsung,s5pv210-fimc"
+		"samsung,exynos4210-fimc";
+		"samsung,exynos4412-fimc";
+- reg	     : physical base address and size of the device memory mapped
+	       registers;
+- interrupts : FIMC interrupt to the CPU should be described here;
+- cell-index : FIMC IP instance index, the number of available instances
+	       depends on the SoC revision. For S5PV210 valid values are:
+	       0...2, for Exynos4x1x: 0...3.
+
+Example:
+
+	fimc0: fimc@11800000 {
+		compatible = "samsung,exynos4210-fimc";
+		reg = <0x11800000 0x1000>;
+		interrupts = <0 85 0>;
+		cell-index = <0>;
+	};
+
+	csis0: csis@11880000 {
+		compatible = "samsung,exynos4210-csis";
+		reg = <0x11880000 0x1000>;
+		interrupts = <0 78 0>;
+		cell-index = <0>;
+	};

Quoted 3 lines · Sylwester Nawrocki

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Karol Lewandowski<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

From the documentation below I think, I understand what it does, but why
is it needed? It doesn't describe your video subsystem topology, right? How various subdevices are connected. It just lists them all in one node... A description for this patch would be very welcome IMHO and, maybe, such a node can be completely avoided?

Quoted 11 lines · Guennadi Liakhovetski

Quoted 3 lines · Sylwester Nawrocki

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Karol Lewandowski<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

From the documentation below I think, I understand what it does, but why
is it needed? It doesn't describe your video subsystem topology, right? How various subdevices are connected. It just lists them all in one node... A description for this patch would be very welcome IMHO and, maybe, such a node can be completely avoided?

Sorry, I'll provide better description in next iteration. It's true it doesn't describe the topology in detail, as there are multiple one-to many possible connections between sub-devices. An exact topology is coded in the driver and can be changed through MC API. The "samsung,camif-mux-id" and "video-bus-type" properties at "sensor" nodes just specify to which physical SoC camera port an image sensor is connected.

Originally the all camera devices were supposed to land under common 'camera' node. And a top level driver would be registering all platform devices. With this approach it would be possible to better control PM handling (which currently depends on an order of registering devices to the driver core). But then we discovered that we couldn't use OF_DEV_AUXDATA in such case, which was required to preserve platform device names, in order for the clock API to work. So I've moved some sub-devices out of 'camera' node and have added only a list of phandles to them in that node. This is rather a cheap workaround..

I think all camera sub-devices should be placed under common node, as there are some properties that don't belong to any sub-node: GPIO config, clocks, to name a few. Of course simpler devices might not need such a composite node. I think we can treat the sub-device interdependencies as an issue separate from a need for a common node.

If some devices need to reflect the topology better, we probably could include in some nodes (a list of) phandles to other nodes. This could ease parsing the topology at the drivers, by using existing OF infrastructure.

Quoted 13 lines · Sylwester Nawrocki

Quoted 11 lines · Guennadi Liakhovetski

Quoted 3 lines · Sylwester Nawrocki

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Karol Lewandowski<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

From the documentation below I think, I understand what it does, but why
is it needed? It doesn't describe your video subsystem topology, right? How various subdevices are connected. It just lists them all in one node... A description for this patch would be very welcome IMHO and, maybe, such a node can be completely avoided?

Quoted 7 lines

Sorry, I'll provide better description in next iteration. It's true it doesn't describe the topology in detail, as there are multiple one-to many possible connections between sub-devices. An exact topology is coded in the driver and can be changed through MC API. The "samsung,camif-mux-id" and "video-bus-type" properties at "sensor" nodes just specify to which physical SoC camera port an image sensor is connected.

So, don't you think my media-link child nodes are a good solution for this?

Quoted 16 lines

Originally the all camera devices were supposed to land under common 'camera' node. And a top level driver would be registering all platform devices. With this approach it would be possible to better control PM handling (which currently depends on an order of registering devices to the driver core). But then we discovered that we couldn't use OF_DEV_AUXDATA in such case, which was required to preserve platform device names, in order for the clock API to work. So I've moved some sub-devices out of 'camera' node and have added only a list of phandles to them in that node. This is rather a cheap workaround..

I think all camera sub-devices should be placed under common node, as there are some properties that don't belong to any sub-node: GPIO config, clocks, to name a few. Of course simpler devices might not need such a composite node. I think we can treat the sub-device interdependencies as an issue separate from a need for a common node.

Quoted 3 lines

If some devices need to reflect the topology better, we probably could include in some nodes (a list of) phandles to other nodes. This could ease parsing the topology at the drivers, by using existing OF infrastructure.

Ok, I think you have some good ideas in your RFC's, an interesting question now is - how to proceed. Do you think we'd be able to work out a combined RFC? Or would you prefer to make two versions and then see what others think? In either case it would be nice, I think, if you could try to separate what you see as common V4L DT bindings, then we could discuss that separately. Whereas what you think is private to your hardware, we can also look at for common ideas, or maybe even some of those properties we'll wake to make common too.

Quoted 23 lines · Guennadi Liakhovetski

Quoted 14 lines · Sylwester Nawrocki

Quoted 5 lines · Guennadi Liakhovetski

Quoted 4 lines · Sylwester Nawrocki

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Karol Lewandowski<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

Quoted 7 lines

From the documentation below I think, I understand what it does, but why

is it needed? It doesn't describe your video subsystem topology, right? How various subdevices are connected. It just lists them all in one node... A description for this patch would be very welcome IMHO and, maybe, such a node can be completely avoided?

Quoted 7 lines

Sorry, I'll provide better description in next iteration. It's true it doesn't describe the topology in detail, as there are multiple one-to many possible connections between sub-devices. An exact topology is coded in the driver and can be changed through MC API. The "samsung,camif-mux-id" and "video-bus-type" properties at "sensor" nodes just specify to which physical SoC camera port an image sensor is connected.

Quoted 2 lines

So, don't you think my media-link child nodes are a good solution for this?

Not quite yet ;) It would be good to see some example implementation and how it actually works.

Quoted 23 lines

Quoted 18 lines

Originally the all camera devices were supposed to land under common 'camera' node. And a top level driver would be registering all platform devices. With this approach it would be possible to better control PM handling (which currently depends on an order of registering devices to the driver core). But then we discovered that we couldn't use OF_DEV_AUXDATA in such case, which was required to preserve platform device names, in order for the clock API to work. So I've moved some sub-devices out of 'camera' node and have added only a list of phandles to them in that node. This is rather a cheap workaround..

I think all camera sub-devices should be placed under common node, as there
are some properties that don't belong to any sub-node: GPIO config, clocks,
to name a few. Of course simpler devices might not need such a composite node. I think we can treat the sub-device interdependencies as an issue separate from a need for a common node.

Quoted 4 lines

If some devices need to reflect the topology better, we probably could include in some nodes (a list of) phandles to other nodes. This could ease
parsing the topology at the drivers, by using existing OF infrastructure.

Quoted 8 lines

Ok, I think you have some good ideas in your RFC's, an interesting question now is - how to proceed. Do you think we'd be able to work out a combined RFC? Or would you prefer to make two versions and then see what others think? In either case it would be nice, I think, if you could try to separate what you see as common V4L DT bindings, then we could discuss that separately. Whereas what you think is private to your hardware, we can also look at for common ideas, or maybe even some of those properties we'll wake to make common too.

I think we need a one combined RFC and continue discussions in one thread.

Still, our proposals are quite different, but I believe we need something in between. I presume we should focus more to have common bindings for subdevs that are reused among different host/ISP devices, i.e. sensors and encoders. For simple host interfaces we can likely come up with common binding patterns, but more complex processing pipelines may require a sort of individual approach.

The suspend/resume handling is still something I don't have an idea on how the solution for might look like..
Instantiating all devices from a top level driver could help, but it is only going to work when platforms are converted to the common clock framework and have their clocks instantiated from device tree.

This week I'm out of office, and next one or two I have some pending assignments. So there might be some delay before I can dedicate some reasonable amount of time to carry on with that topic.

I unfortunately won't be attending KS this time.

I'll try to prepare some summary with topics that appear common. And also to restructure my RFC series to better separate new common features and specific H/W support.

In the meantime we could possibly continue discussions in your RFC thread.

Quoted 1 line · On Wednesday 18 July 2012 21:53:34 Sylwester Nawrocki wrote:

I think we need a one combined RFC and continue discussions in one thread.

Agreed.

Quoted 13 lines

Still, our proposals are quite different, but I believe we need something in between. I presume we should focus more to have common bindings for subdevs that are reused among different host/ISP devices, i.e. sensors and encoders. For simple host interfaces we can likely come up with common binding patterns, but more complex processing pipelines may require a sort of individual approach.

The suspend/resume handling is still something I don't have an idea on how the solution for might look like..
Instantiating all devices from a top level driver could help, but it is only going to work when platforms are converted to the common clock framework and have their clocks instantiated from device tree.

Quoted 5 lines

This week I'm out of office, and next one or two I have some pending assignments. So there might be some delay before I can dedicate some reasonable amount of time to carry on with that topic.

I unfortunately won't be attending KS this time.

That's bad news :-( I still think this topic should be discussed during KS, I

expect several developers to be interested. The media workshop might not be the best venue though, as we might need quite a lot of time.

Until KS let's continue the discussion by e-mail.

Signed-off-by Sylwester Nawrocki <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 .../bindings/camera/soc/samsung-fimc.txt           |   15 ++++
 drivers/media/video/s5p-fimc/fimc-lite.c           |   73 ++++++++++++++------
 2 files changed, 67 insertions(+), 21 deletions(-)

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt +3 -0

index 1ec48e9..b459da2 100644
@@ -39,6 +39,21 @@ Required properties:
 	       depends on the SoC revision. For S5PV210 valid values are:
 	       0...2, for Exynos4x1x: 0...3.
 
+
+'fimc-lite' device node
+-----------------------

Quoted 6 lines

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt +0 -0

@@ -39,6 +39,21 @@ Required properties:
  	       depends on the SoC revision. For S5PV210 valid values are:
  	       0...2, for Exynos4x1x: 0...3.
 

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt Required properties: · line 54

Quoted 13 lines

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt +13 -0

+
+'fimc-lite' device node
+-----------------------
+
+Required properties:
+
+- compatible : should be one of:
+		"samsung,exynos4212-fimc";
+		"samsung,exynos4412-fimc";
+- reg	     : physical base address and size of the device's memory mapped
+	       registers;
+- interrupts : should contain FIMC-LITE interrupt;
+- cell-index : FIMC-LITE IP instance index;

Same as in an earlier patch - not sure this is needed.

Quoted 6 lines · Guennadi Liakhovetski

Quoted 6 lines

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt +0 -0

@@ -39,6 +39,21 @@ Required properties:
  	       depends on the SoC revision. For S5PV210 valid values are:
  	       0...2, for Exynos4x1x: 0...3.
 

Quoted 16 lines

Quoted 11 lines

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt +10 -0

+
+'fimc-lite' device node
+-----------------------
+
+Required properties:
+
+- compatible : should be one of:
+		"samsung,exynos4212-fimc";
+		"samsung,exynos4412-fimc";
+- reg	     : physical base address and size of the device's memory

mapped

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt Required properties: · line 54

Quoted 3 lines

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt +3 -0

+	       registers;
+- interrupts : should contain FIMC-LITE interrupt;
+- cell-index : FIMC-LITE IP instance index;

Same as in an earlier patch - not sure this is needed.

It is needed for setting up a pipeline of multiple sub-device within a SoC. As I commented on patch 2/13 I'd like to replace this with proper entries in the "aliases" node. Some sub-devices have registers that these indexes need to be directly written to.

Quoted 24 lines · Sylwester Nawrocki

Quoted 6 lines · Guennadi Liakhovetski

Quoted 6 lines

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt +0 -0

@@ -39,6 +39,21 @@ Required properties:
   	       depends on the SoC revision. For S5PV210 valid values are:
   	       0...2, for Exynos4x1x: 0...3.
 

Quoted 16 lines

Quoted 11 lines

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt +10 -0

+
+'fimc-lite' device node
+-----------------------
+
+Required properties:
+
+- compatible : should be one of:
+		"samsung,exynos4212-fimc";
+		"samsung,exynos4412-fimc";
+- reg	     : physical base address and size of the device's memory

mapped

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt Required properties: · line 54

Quoted 3 lines

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt +3 -0

+	       registers;
+- interrupts : should contain FIMC-LITE interrupt;
+- cell-index : FIMC-LITE IP instance index;

Same as in an earlier patch - not sure this is needed.

Quoted 4 lines

It is needed for setting up a pipeline of multiple sub-device within a SoC. As I commented on patch 2/13 I'd like to replace this with proper entries in the "aliases" node. Some sub-devices have registers that these indexes need to be directly written to.

Aha, so, these are not purely software indices, that you just use to identify your devices, these are actual hardware values? You really have to write, e.g., "1" into a resizer register to connect it to a certain capture engine? Ok, in this case it indeed might make sense to have these

values in the DT. Not sure how best to call these properties, or whether aliases is the best solution, but that's already a detail. And btw, if you

do decide to keep this as a property, maybe this is one of the cases, where you'd want to mark it with your hardware type, like "fimc,cell-id" or similar?

The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
 drivers/media/video/s5k6aa.c                       |  129 ++++++++++++++------
 2 files changed, 146 insertions(+), 40 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +0 -0

new file mode 100644
index 0000000..6685a9c

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +2 -0

+- gpio-stby : specifies the S5K6AA_STBY GPIO
+- gpio-rst : specifies the S5K6AA_RST GPIO

From Documentation/devicetree/bindings/gpio/gpio.txt:

+- samsung,s5k6aa-inv-stby : set inverted S5K6AA_STBY GPIO level;
+- samsung,s5k6aa-inv-rst : set inverted S5K6AA_RST GPIO level;

+- samsung,s5k6aa-hflip : set the default horizontal image flipping;
+- samsung,s5k6aa-vflip : set the default vertical image flipping;

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +21 -0

+
+
+Example:
+
+	gpl2: gpio-controller@0 {
+	};
+
+	reg0: regulator@0 {
+	};
+
+	reg1: regulator@1 {
+	};
+
+	reg2: regulator@2 {
+	};
+
+	reg3: regulator@3 {
+	};
+
+	s5k6aafx@3c {
+		compatible = "samsung,s5k6aafx";

Quoted 13 lines · Sylwester Nawrocki

The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---
  .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
  drivers/media/video/s5k6aa.c                       |  129 ++++++++++++++------
  2 files changed, 146 insertions(+), 40 deletions(-)

create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt line 7

Quoted 13 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +7 -0

new file mode 100644
index 0000000..6685a9c
@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+------------------------------
+
+Required properties:
+
+- compatible : "samsung,s5k6aafx";
+- reg : base address of the device on I2C bus;

You said you ended up putting your sensors outside of I2C busses, is this one of changes, that are present in your git-tree but not in this series?

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt line 8

Quoted 1 line

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +1 -0

+- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;

If this is a boolean property, it cannot be required? Otherwise, as discussed in a different patch comment, this property might not be required, and if it is, I would use the same definition for both the camera interface and for sensors.

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt line 12

Quoted 4 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +4 -0

+- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
+- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
+- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to 1.9V)
+		   or 2.8V (2.6V to 3.0);

I think, underscores in property names are generally frowned upon.

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt line 20

Quoted 8 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +8 -0

+- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)
+		 or 2.8V (2.5V to 3.1V);
+
+Optional properties:
+
+- clock-frequency : the IP's main (system bus) clock frequency in Hz, the default
+		    value when this property is not specified is 24 MHz;
+- data-lanes : number of physical lanes used (default 2 if not specified);

bus-width?

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt line 22

Quoted 2 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +2 -0

+- gpio-stby : specifies the S5K6AA_STBY GPIO
+- gpio-rst : specifies the S5K6AA_RST GPIO

From Documentation/devicetree/bindings/gpio/gpio.txt:

<quote>
GPIO properties should be named "[<name>-]gpios".
</quote>

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt line 24

Quoted 2 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +2 -0

+- samsung,s5k6aa-inv-stby : set inverted S5K6AA_STBY GPIO level;
+- samsung,s5k6aa-inv-rst : set inverted S5K6AA_RST GPIO level;

Isn't this provided by GPIO flags as described in include/linux/of_gpio.h by using the OF_GPIO_ACTIVE_LOW bit?

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt line 26

Quoted 2 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +2 -0

+- samsung,s5k6aa-hflip : set the default horizontal image flipping;
+- samsung,s5k6aa-vflip : set the default vertical image flipping;

This is a board property, specifying how the sensor is wired and mounted on the casing, right? IIRC, libv4l has a database of these for USB

cameras. So, maybe it belongs to the user-space for embedded systems too?

Or at least this shouldn't be Samsung-specific?

Quoted 26 lines · Guennadi Liakhovetski

Quoted 25 lines · Sylwester Nawrocki

The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---

  .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
  drivers/media/video/s5k6aa.c                       |  129

++++++++++++++------ 2 files changed, 146 insertions(+), 40 deletions(-)
create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt>>
diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file mode 100644
index 0000000..6685a9c

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +2 -0

@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+------------------------------

Quoted 8 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt line 7

Quoted 5 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +5 -0

+
+Required properties:
+
+- compatible : "samsung,s5k6aafx";
+- reg : base address of the device on I2C bus;

You said you ended up putting your sensors outside of I2C busses, is this one of changes, that are present in your git-tree but not in this series?

No, I must have been not clear enough on that. Our idea was to keep I2C slave device nodes as an I2C controller's child nodes, according to the current convention.
The 'sensor' nodes (the 'camera''s children) would only contain a phandle to a respective I2C slave node.

This implies that we cannot access I2C bus in I2C client's device probe() callback. An actual H/W access could begin only from within and after invocation of v4l2_subdev .registered callback..

Quoted 6 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt line 8

Quoted 1 line

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +1 -0

+- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;

If this is a boolean property, it cannot be required? Otherwise, as discussed in a different patch comment, this property might not be required, and if it is, I would use the same definition for both the camera interface and for sensors.

Yeah, that's an omission, a leftover from previous versions. It should have been 'video-bus-type', for this series.

Here is the updated binding documentation:

8<---------------------------------------------------------------------- Required properties:

- compatible : "samsung,s5k6aafx";
- reg : base address of the device on I2C bus;
- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
- vdda-supply : analog circuits voltage supply 2.8V (2.6V to 3.0V);
- vdd_reg-supply : regulator input voltage supply 1.8V (1.7V to 1.9V)

                   or 2.8V (2.6V to 3.0);
- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)
                 or 2.8V (2.5V to 3.1V);
- video-bus-type : sensor video output bus type, must be one of:
                   "itu-601", "mipi-csi2";

- clock-frequency : the sensor's master clock frequency in Hz;

Optional properties:

- data-lanes : number of physical MIPI-CSI2 data lanes used

               (default is 2 when this property is not specified);

- gpios : specifies gpios connected to the sensor's reset
reset (RSTN) and standby (STBYN) pins, the order
of gpios is: <RSTN, STBYN>; If any of these gpios
is not used, its specifier shall be 0;
- samsung,s5k6aa-gpio-lvls : this property (bits [1:0]) specifies an active state of the S5K6AAFX reset and stand-by
signals; the meaning of bits is: bit[1:0] = [RST, STBY], e.g. bit[1] = 0 indicates the GPIO driving the S5K6AAFX's standby pin should be set to 1 to bring the sensor out from stand-by to normal operation state.
- samsung,s5k6aa-hflip : sets the default horizontal image flipping;
- samsung,s5k6aa-vflip : sets the default vertical image flipping; 8<----------------------------------------------------------------------

Quoted 6 lines

Quoted 4 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +3 -0

+- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
+- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
+- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to

1.9V) + or 2.8V (2.6V to 3.0);

I think, underscores in property names are generally frowned upon.

Indeed, I felt something might be wrong here;) It's in this form because I just copied regulator supply names from the existing driver. And I didn't want to change them, to avoid updating them in all board files where they're used. Thanks for pointing it out, I'll fix that in next iteration.

Quoted 11 lines

Quoted 9 lines

+- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +5 -0

+		 or 2.8V (2.5V to 3.1V);
+
+Optional properties:
+
+- clock-frequency : the IP's main (system bus) clock frequency in Hz,

the default + value when this property is not specified is 24 MHz;
+- data-lanes : number of physical lanes used (default 2 if not specified);

bus-width?

OK.

Quoted 7 lines

Quoted 3 lines

+- gpio-stby : specifies the S5K6AA_STBY GPIO
+- gpio-rst : specifies the S5K6AA_RST GPIO

From Documentation/devicetree/bindings/gpio/gpio.txt:
<quote>
GPIO properties should be named "[<name>-]gpios". </quote>

Thanks, these have been already removed in the next version, as can be seen above.

Quoted 5 lines

Quoted 2 lines

+- samsung,s5k6aa-inv-stby : set inverted S5K6AA_STBY GPIO level;
+- samsung,s5k6aa-inv-rst : set inverted S5K6AA_RST GPIO level;

Isn't this provided by GPIO flags as described in include/linux/of_gpio.h by using the OF_GPIO_ACTIVE_LOW bit?

Hmm, I wasn't aware of that. I'll see how this flag can be used.

Quoted 5 lines

Quoted 2 lines

+- samsung,s5k6aa-hflip : set the default horizontal image flipping;
+- samsung,s5k6aa-vflip : set the default vertical image flipping;

This is a board property, specifying how the sensor is wired and mounted on the casing, right? IIRC, libv4l has a database of these for USB

Yeah, that's exactly it. It could be used for compensating for an "upside down" mounting.

Quoted 1 line

cameras. So, maybe it belongs to the user-space for embedded systems too?

Using library for these things is one of the solutions, which works well for USB webcams. However, I wouldn't like enforcing that. Those properties still describe hardware IHMO, then why not add them into DT blob that is usually distributed with each board ? There wouldn't be a need to rely on any user-space database lookup, before the hardware can actually be used.

Quoted 1 line

Or at least this shouldn't be Samsung-specific?

Yes, good point. If we agree we can keep them, perhaps we could standardize those as boolean properties: horizontal-flip; and vertical-flip; ?

Hi Sylwester,

Quoted 42 lines · On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:

Quoted 26 lines · Guennadi Liakhovetski

Quoted 25 lines · Sylwester Nawrocki

The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---

  .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
  drivers/media/video/s5k6aa.c                       |  129

++++++++++++++------ 2 files changed, 146 insertions(+), 40 deletions(-)
create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt>>
diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file mode 100644
index 0000000..6685a9c

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +2 -0

@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+------------------------------

Quoted 8 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt line 7

Quoted 5 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +5 -0

+
+Required properties:
+
+- compatible : "samsung,s5k6aafx";
+- reg : base address of the device on I2C bus;

You said you ended up putting your sensors outside of I2C busses, is this one of changes, that are present in your git-tree but not in this series?

No, I must have been not clear enough on that. Our idea was to keep I2C slave device nodes as an I2C controller's child nodes, according to the current convention.
The 'sensor' nodes (the 'camera''s children) would only contain a phandle to a respective I2C slave node.

Quoted 3 lines

This implies that we cannot access I2C bus in I2C client's device probe() callback. An actual H/W access could begin only from within and after invocation of v4l2_subdev .registered callback..

That's how I've envisioned the DT bindings for sensors as well, this sounds good. The real challenge will be to get hold of the subdev to register it without race conditions.

Quoted 47 lines · Laurent Pinchart

Quoted 46 lines · On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:

Quoted 30 lines · Guennadi Liakhovetski

Quoted 29 lines · Sylwester Nawrocki

The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---

  .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
  drivers/media/video/s5k6aa.c                       |  129

++++++++++++++------ 2 files changed, 146 insertions(+), 40 deletions(-)
create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt>>

diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file
mode 100644
index 0000000..6685a9c

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +4 -0

@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+------------------------------
+
+Required properties:

Quoted 8 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt line 7

Quoted 3 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +3 -0

+
+- compatible : "samsung,s5k6aafx";
+- reg : base address of the device on I2C bus;

You said you ended up putting your sensors outside of I2C busses, is this
one of changes, that are present in your git-tree but not in this series?

No, I must have been not clear enough on that. Our idea was to keep I2C slave device nodes as an I2C controller's child nodes, according to the current convention.
The 'sensor' nodes (the 'camera''s children) would only contain a phandle to a respective I2C slave node.

Quoted 7 lines

Quoted 3 lines

This implies that we cannot access I2C bus in I2C client's device probe() callback. An actual H/W access could begin only from within and after invocation of v4l2_subdev .registered callback..

That's how I've envisioned the DT bindings for sensors as well, this sounds good. The real challenge will be to get hold of the subdev to register it without race conditions.

Hrm... That's how early pre-subdev versions of soc-camera used to work, that's where all the <device>_video_probe() functions come from. But then we switched to dynamic i2c device registration. Do we want to switch all drivers back now?... Couldn't we "temporarily" use references from subdevs to hosts until the clock API is available?

Hi Guennadi,

Quoted 56 lines · On Tuesday 31 July 2012 11:56:44 Guennadi Liakhovetski wrote:

Quoted 47 lines · Laurent Pinchart

Quoted 46 lines · On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:

Quoted 30 lines · Guennadi Liakhovetski

Quoted 29 lines · Sylwester Nawrocki

The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---

  .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
  drivers/media/video/s5k6aa.c                       |  129

++++++++++++++------ 2 files changed, 146 insertions(+), 40 deletions(-)
create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt>>

diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file
mode 100644
index 0000000..6685a9c

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +4 -0

@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+------------------------------
+
+Required properties:

Quoted 8 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt line 7

Quoted 3 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +3 -0

+
+- compatible : "samsung,s5k6aafx";
+- reg : base address of the device on I2C bus;

You said you ended up putting your sensors outside of I2C busses, is this
one of changes, that are present in your git-tree but not in this series?

No, I must have been not clear enough on that. Our idea was to keep I2C slave device nodes as an I2C controller's child nodes, according to the current convention.
The 'sensor' nodes (the 'camera''s children) would only contain a phandle to a respective I2C slave node.

Quoted 7 lines

Quoted 3 lines

This implies that we cannot access I2C bus in I2C client's device probe() callback. An actual H/W access could begin only from within and after invocation of v4l2_subdev .registered callback..

That's how I've envisioned the DT bindings for sensors as well, this sounds good. The real challenge will be to get hold of the subdev to register it without race conditions.

Quoted 5 lines

Hrm... That's how early pre-subdev versions of soc-camera used to work, that's where all the <device>_video_probe() functions come from. But then we switched to dynamic i2c device registration. Do we want to switch all drivers back now?... Couldn't we "temporarily" use references from subdevs to hosts until the clock API is available?

I don't think that requires a reference from subdevs to hosts in the DT. The subdev will need the host to be probed before a clock can be available so you won't be able to access the hardware in the probe() function in the generic case. You will need to wait until the registered() subdev operation is called, at which point the host can be accessed through the v4l2_device.

Quoted 66 lines · Laurent Pinchart

Quoted 59 lines · On Tuesday 31 July 2012 11:56:44 Guennadi Liakhovetski wrote:

Quoted 50 lines · Laurent Pinchart

Quoted 49 lines · On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:

Quoted 33 lines · Guennadi Liakhovetski

Quoted 32 lines · Sylwester Nawrocki

The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Bartlomiej

Zolnierkiewicz<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---

  .../bindings/camera/samsung-s5k6aafx.txt           |   57
  +++++++++
  drivers/media/video/s5k6aa.c                       |  129

++++++++++++++------ 2 files changed, 146 insertions(+), 40 deletions(-)
create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt>>

diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new
file
mode 100644
index 0000000..6685a9c
--- /dev/null
+++
b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt

(quoted diff) +3 -0

@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+------------------------------
+

Quoted 8 lines

Review line 7

Quoted 4 lines

(quoted diff) +4 -0

+Required properties:
+
+- compatible : "samsung,s5k6aafx";
+- reg : base address of the device on I2C bus;

You said you ended up putting your sensors outside of I2C busses, is this one of changes, that are present in your git-tree but not in this series?

No, I must have been not clear enough on that. Our idea was to keep I2C slave device nodes as an I2C controller's child nodes, according to the current convention.
The 'sensor' nodes (the 'camera''s children) would only contain a phandle to a respective I2C slave node.

Quoted 7 lines

Quoted 3 lines

This implies that we cannot access I2C bus in I2C client's device probe() callback. An actual H/W access could begin only from within and after invocation of v4l2_subdev .registered callback..

That's how I've envisioned the DT bindings for sensors as well, this sounds good. The real challenge will be to get hold of the subdev to register it without race conditions.

Quoted 5 lines

Hrm... That's how early pre-subdev versions of soc-camera used to work, that's where all the <device>_video_probe() functions come from. But then we switched to dynamic i2c device registration. Do we want to switch all drivers back now?... Couldn't we "temporarily" use references from subdevs to hosts until the clock API is available?

Quoted 6 lines

I don't think that requires a reference from subdevs to hosts in the DT. The subdev will need the host to be probed before a clock can be available so you won't be able to access the hardware in the probe() function in the generic case. You will need to wait until the registered() subdev operation is called, at which point the host can be accessed through the v4l2_device.

Sure, I understand, but that's exactly what we wanted to avoid - succeeding client's i2c .probe() without even touching the hardware.

Hi Guennadi,

Quoted 74 lines · On Tuesday 31 July 2012 13:14:13 Guennadi Liakhovetski wrote:

Quoted 72 lines · Laurent Pinchart

Quoted 59 lines · On Tuesday 31 July 2012 11:56:44 Guennadi Liakhovetski wrote:

Quoted 50 lines · Laurent Pinchart

Quoted 49 lines · On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:

Quoted 33 lines · Guennadi Liakhovetski

Quoted 32 lines · Sylwester Nawrocki

The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Bartlomiej

Zolnierkiewicz<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---

  .../bindings/camera/samsung-s5k6aafx.txt           |   57
  +++++++++
  drivers/media/video/s5k6aa.c                       |  129

++++++++++++++------ 2 files changed, 146 insertions(+), 40 deletions(-)
create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt>>

diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new
file
mode 100644
index 0000000..6685a9c
--- /dev/null
+++
b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt

(quoted diff) +3 -0

@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+------------------------------
+

Quoted 8 lines

Review line 7

Quoted 4 lines

(quoted diff) +4 -0

+Required properties:
+
+- compatible : "samsung,s5k6aafx";
+- reg : base address of the device on I2C bus;

You said you ended up putting your sensors outside of I2C busses, is this one of changes, that are present in your git-tree but not in this series?

No, I must have been not clear enough on that. Our idea was to keep I2C slave device nodes as an I2C controller's child nodes, according to the current convention.
The 'sensor' nodes (the 'camera''s children) would only contain a phandle to a respective I2C slave node.

Quoted 7 lines

Quoted 3 lines

This implies that we cannot access I2C bus in I2C client's device probe() callback. An actual H/W access could begin only from within and after invocation of v4l2_subdev .registered callback..

That's how I've envisioned the DT bindings for sensors as well, this sounds good. The real challenge will be to get hold of the subdev to register it without race conditions.

Quoted 5 lines

Hrm... That's how early pre-subdev versions of soc-camera used to work, that's where all the <device>_video_probe() functions come from. But then we switched to dynamic i2c device registration. Do we want to switch all drivers back now?... Couldn't we "temporarily" use references from subdevs to hosts until the clock API is available?

I don't think that requires a reference from subdevs to hosts in the DT. The subdev will need the host to be probed before a clock can be available so you won't be able to access the hardware in the probe() function in the generic case. You will need to wait until the registered() subdev operation is called, at which point the host can be accessed through the v4l2_device.

Quoted 2 lines

Sure, I understand, but that's exactly what we wanted to avoid - succeeding client's i2c .probe() without even touching the hardware.

But should we allow host probe() to succeed if the sensor isn't present ?

Quoted 85 lines · Laurent Pinchart

Quoted 83 lines · On Tuesday 31 July 2012 13:14:13 Guennadi Liakhovetski wrote:

Quoted 73 lines · Laurent Pinchart

Quoted 65 lines · On Tuesday 31 July 2012 11:56:44 Guennadi Liakhovetski wrote:

Quoted 55 lines · Laurent Pinchart

Quoted 54 lines · On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:

Quoted 38 lines · Guennadi Liakhovetski

Quoted 37 lines · Sylwester Nawrocki

The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Bartlomiej

Zolnierkiewicz<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---

  .../bindings/camera/samsung-s5k6aafx.txt           |   57
  +++++++++
  drivers/media/video/s5k6aa.c                       |  129

++++++++++++++------ 2 files changed, 146 insertions(+), 40 deletions(-)
create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t xt>>

diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t xt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t xt
new
file
mode 100644
index 0000000..6685a9c
--- /dev/null
+++
b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t xt

(quoted diff) +3 -0

@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+------------------------------
+

Quoted 8 lines

Review line 7

Quoted 4 lines

(quoted diff) +4 -0

+Required properties:
+
+- compatible : "samsung,s5k6aafx";
+- reg : base address of the device on I2C bus;

You said you ended up putting your sensors outside of I2C busses, is this one of changes, that are present in your git- tree but not in this series?

No, I must have been not clear enough on that. Our idea was to keep I2C slave device nodes as an I2C controller's child nodes, according to the current convention. The 'sensor' nodes (the 'camera''s children) would only contain a phandle to a respective I2C slave node.

Quoted 8 lines

Quoted 4 lines

This implies that we cannot access I2C bus in I2C client's device probe() callback. An actual H/W access could begin only from within and after invocation of v4l2_subdev .registered callback..

That's how I've envisioned the DT bindings for sensors as well, this sounds good. The real challenge will be to get hold of the subdev to register it without race conditions.

Quoted 6 lines

Hrm... That's how early pre-subdev versions of soc-camera used to work, that's where all the <device>_video_probe() functions come from. But then we switched to dynamic i2c device registration. Do we want to switch all drivers back now?... Couldn't we "temporarily" use references from subdevs to hosts until the clock API is available?

Quoted 6 lines

I don't think that requires a reference from subdevs to hosts in the DT. The subdev will need the host to be probed before a clock can be available so you won't be able to access the hardware in the probe() function in the generic case. You will need to wait until the registered() subdev operation is called, at which point the host can be accessed through the v4l2_device.

Sure, I understand, but that's exactly what we wanted to avoid - succeeding client's i2c .probe() without even touching the hardware.

Quoted 1 line

But should we allow host probe() to succeed if the sensor isn't present ?

I think we should, yes. The host hardware is there and functional - whether or not all or some of the clients are failing. Theoretically clients can also be hot-plugged. Whether and how many video device nodes we create, that's a different question.

Hi Guennadi,

Quoted 88 lines · On Tuesday 31 July 2012 13:29:55 Guennadi Liakhovetski wrote:

Quoted 86 lines · Laurent Pinchart

Quoted 81 lines · On Tuesday 31 July 2012 13:14:13 Guennadi Liakhovetski wrote:

Quoted 73 lines · Laurent Pinchart

Quoted 65 lines · On Tuesday 31 July 2012 11:56:44 Guennadi Liakhovetski wrote:

Quoted 55 lines · Laurent Pinchart

Quoted 54 lines · On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:

Quoted 38 lines · Guennadi Liakhovetski

Quoted 37 lines · Sylwester Nawrocki

The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Bartlomiej

Zolnierkiewicz<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---

  .../bindings/camera/samsung-s5k6aafx.txt           |   57
  +++++++++
  drivers/media/video/s5k6aa.c                       |  129

++++++++++++++------ 2 files changed, 146 insertions(+), 40 deletions(-)
create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t xt>>

diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t xt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t xt
new
file
mode 100644
index 0000000..6685a9c
--- /dev/null
+++
b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t xt

(quoted diff) +3 -0

@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+------------------------------
+

Quoted 8 lines

Review line 7

Quoted 4 lines

(quoted diff) +4 -0

+Required properties:
+
+- compatible : "samsung,s5k6aafx";
+- reg : base address of the device on I2C bus;

You said you ended up putting your sensors outside of I2C busses, is this one of changes, that are present in your git- tree but not in this series?

No, I must have been not clear enough on that. Our idea was to keep I2C slave device nodes as an I2C controller's child nodes, according to the current convention. The 'sensor' nodes (the 'camera''s children) would only contain a phandle to a respective I2C slave node.

Quoted 8 lines

Quoted 4 lines

This implies that we cannot access I2C bus in I2C client's device probe() callback. An actual H/W access could begin only from within and after invocation of v4l2_subdev .registered callback..

That's how I've envisioned the DT bindings for sensors as well, this sounds good. The real challenge will be to get hold of the subdev to register it without race conditions.

Quoted 6 lines

Hrm... That's how early pre-subdev versions of soc-camera used to work, that's where all the <device>_video_probe() functions come from. But then we switched to dynamic i2c device registration. Do we want to switch all drivers back now?... Couldn't we "temporarily" use references from subdevs to hosts until the clock API is available?

Quoted 6 lines

I don't think that requires a reference from subdevs to hosts in the DT. The subdev will need the host to be probed before a clock can be available so you won't be able to access the hardware in the probe() function in the generic case. You will need to wait until the registered() subdev operation is called, at which point the host can be accessed through the v4l2_device.

Quoted 2 lines

Sure, I understand, but that's exactly what we wanted to avoid - succeeding client's i2c .probe() without even touching the hardware.

But should we allow host probe() to succeed if the sensor isn't present ?

Quoted 4 lines

I think we should, yes. The host hardware is there and functional - whether or not all or some of the clients are failing. Theoretically clients can also be hot-plugged. Whether and how many video device nodes we create, that's a different question.

I think I can agree with you on this (although I could change my mind if this architecture turns out to result in unsolvable technical issues). That will involve a lot of work though.

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

But should we allow host probe() to succeed if the sensor isn't present ?

I think we should, yes. The host hardware is there and functional - whether or not all or some of the clients are failing. Theoretically clients can also be hot-plugged. Whether and how many video device nodes we create, that's a different question.

Quoted 3 lines

I think I can agree with you on this (although I could change my mind if this architecture turns out to result in unsolvable technical issues). That will involve a lot of work though.

There's however at least one more gotcha that occurs to me with this
approach: if clients fail to probe, how do we find out about that and turn clocks back off? One improvement to turning clocks on immediately in

host's probe() is to only do it in a BUS_NOTIFY_BIND_DRIVER notifier. But how do we find out, that probing failed? No notifier is called in this case. We could use a time-out, but that's ugly. I think, we could ever request a new notifier for this case. We could also require client drivers to call a V4L2 function in this case, but that's not very pretty either.

Quoted 11 lines · Guennadi Liakhovetski

Quoted 10 lines

Quoted 6 lines

Quoted 1 line

But should we allow host probe() to succeed if the sensor isn't present ?

I think we should, yes. The host hardware is there and functional - whether or not all or some of the clients are failing. Theoretically clients can also be hot-plugged. Whether and how many video device nodes we create, that's a different question.

I think I can agree with you on this (although I could change my mind if this architecture turns out to result in unsolvable technical issues). That will involve a lot of work though.

Quoted 3 lines

There's however at least one more gotcha that occurs to me with this
approach: if clients fail to probe, how do we find out about that and turn clocks back off? One improvement to turning clocks on immediately in

Hmm, wouldn't it be the client that turns a clock on/off when needed ? I'd like to preserve this functionality, so client drivers can have full control on the power up/down sequences. While we are trying to improve the current situation...

Quoted 5 lines

host's probe() is to only do it in a BUS_NOTIFY_BIND_DRIVER notifier. But how do we find out, that probing failed? No notifier is called in this case. We could use a time-out, but that's ugly. I think, we could ever request a new notifier for this case. We could also require client drivers to call a V4L2 function in this case, but that's not very pretty either.

Quoted 16 lines · Sylwester Nawrocki

Quoted 14 lines · Guennadi Liakhovetski

Quoted 10 lines

Quoted 6 lines

Quoted 1 line

But should we allow host probe() to succeed if the sensor isn't present ?

I think we should, yes. The host hardware is there and functional - whether or not all or some of the clients are failing. Theoretically clients can also be hot-plugged. Whether and how many video device nodes we create, that's a different question.

I think I can agree with you on this (although I could change my mind if this architecture turns out to result in unsolvable technical issues). That will involve a lot of work though.

There's however at least one more gotcha that occurs to me with this
approach: if clients fail to probe, how do we find out about that and turn clocks back off? One improvement to turning clocks on immediately in

Quoted 4 lines

Hmm, wouldn't it be the client that turns a clock on/off when needed ? I'd like to preserve this functionality, so client drivers can have full control on the power up/down sequences. While we are trying to improve the current situation...

Eventually, when the clock API is available - yes, the client would just call clk_enable() / clk_disable(). But for now isn't it host's job to do that? Or you want to add new API for that?

Quoted 20 lines · Guennadi Liakhovetski

Quoted 19 lines · Sylwester Nawrocki

Quoted 17 lines · Guennadi Liakhovetski

Quoted 9 lines

Quoted 8 lines

Quoted 2 lines

But should we allow host probe() to succeed if the sensor isn't present ?

I think we should, yes. The host hardware is there and functional - whether or not all or some of the clients are failing. Theoretically clients can also be hot-plugged. Whether and how many video device nodes
we create, that's a different question.

Quoted 3 lines

I think I can agree with you on this (although I could change my mind if this architecture turns out to result in unsolvable technical issues). That will involve a lot of work though.

There's however at least one more gotcha that occurs to me with this
approach: if clients fail to probe, how do we find out about that and turn
clocks back off? One improvement to turning clocks on immediately in

Quoted 8 lines

Quoted 4 lines

Hmm, wouldn't it be the client that turns a clock on/off when needed ? I'd like to preserve this functionality, so client drivers can have full control on the power up/down sequences. While we are trying to improve the current situation...

Eventually, when the clock API is available - yes, the client would just call clk_enable() / clk_disable(). But for now isn't it host's job to do that? Or you want to add new API for that?

Indeed, looking at existing drivers, the clocks' handling is now mostly done in the host drivers. Only the omap3isp appears to do the right thing, i.e. delegating control over the clock to client drivers, but it does it with platform_data callbacks.

We've already discussed adding a new API for that,
http://www.mail-archive.com/[email protected]/msg35359.html

However using common clock API and binding a clock to client device (either DT based or not) sounds like a best approach to me.

Waiting for the common clock API to be generally available maybe we could add some clock ops at the v4l2_device ? Just a humble suggestion, I'm not sure whether it is really good and needed or not.

The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
 drivers/media/video/s5k6aa.c                       |  129 ++++++++++++-----
 2 files changed, 146 insertions(+), 40 deletions(-)
 create mode 100644

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file mode 100644
index 0000000..6685a9c

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +18 -0

@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+------------------------------
+
+Required properties:
+
+- compatible : "samsung,s5k6aafx";
+- reg : base address of the device on I2C bus;
+- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
+- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
+- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
+- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to 1.9V)
+		   or 2.8V (2.6V to 3.0);
+- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)
+		 or 2.8V (2.5V to 3.1V);
+
+Optional properties:
+
+- clock-frequency : the IP's main (system bus) clock frequency in Hz, the

default

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +39 -0

+		    value when this property is not specified is 24 MHz;
+- data-lanes : number of physical lanes used (default 2 if not specified);
+- gpio-stby : specifies the S5K6AA_STBY GPIO
+- gpio-rst : specifies the S5K6AA_RST GPIO
+- samsung,s5k6aa-inv-stby : set inverted S5K6AA_STBY GPIO level;
+- samsung,s5k6aa-inv-rst : set inverted S5K6AA_RST GPIO level;
+- samsung,s5k6aa-hflip : set the default horizontal image flipping;
+- samsung,s5k6aa-vflip : set the default vertical image flipping;
+
+
+Example:
+
+	gpl2: gpio-controller@0 {
+	};
+
+	reg0: regulator@0 {
+	};
+
+	reg1: regulator@1 {
+	};
+
+	reg2: regulator@2 {
+	};
+
+	reg3: regulator@3 {
+	};
+
+	s5k6aafx@3c {
+		compatible = "samsung,s5k6aafx";
+		reg = <0x3c>;
+		clock-frequency = <24000000>;
+		gpio-rst = <&gpl2 0 2 0 3>;
+		gpio-stby = <&gpl2 1 2 0 3>;
+		video-itu-601-bus;
+		vdd_core-supply = <&reg0>;
+		vdda-supply = <&reg1>;
+		vdd_reg-supply = <&reg2>;
+		vddio-supply = <&reg3>;
+	};

Quoted 34 lines · On Friday 25 May 2012 21:52:48 Sylwester Nawrocki wrote:

The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---
  .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
  drivers/media/video/s5k6aa.c                       |  129 ++++++++++++-----
  2 files changed, 146 insertions(+), 40 deletions(-)

create mode 100644
Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file mode 100644
index 0000000..6685a9c

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +13 -0

@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+------------------------------
+
+Required properties:
+
+- compatible : "samsung,s5k6aafx";
+- reg : base address of the device on I2C bus;
+- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
+- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
+- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
+- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to 1.9V)
+		   or 2.8V (2.6V to 3.0);
+- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)

Quoted 6 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +5 -0

+		 or 2.8V (2.5V to 3.1V);
+
+Optional properties:
+
+- clock-frequency : the IP's main (system bus) clock frequency in Hz, the

default

Is that the input clock frequency ? Can't it vary ? Instead of accessing the

sensor clock frequency from the FIMC driver you should reference a clock in the sensor DT node. That obviously requires generic clock support, which might not be available for your platform yet (that's one of the reasons the OMAP3 ISP driver doesn't support DT yet).

Quoted 39 lines · Laurent Pinchart

Quoted 38 lines · On Friday 25 May 2012 21:52:48 Sylwester Nawrocki wrote:

The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---

  .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
  drivers/media/video/s5k6aa.c                       |  129 +++++++++----
  2 files changed, 146 insertions(+), 40 deletions(-)

create mode 100644

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt

diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file mode 100644
index 0000000..6685a9c

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +11 -0

@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+------------------------------
+
+Required properties:
+
+- compatible : "samsung,s5k6aafx";
+- reg : base address of the device on I2C bus;
+- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
+- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
+- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
+- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to

1.9V) + or 2.8V (2.6V to 3.0);
+- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +1 -0

+		 or 2.8V (2.5V to 3.1V);

Quoted 8 lines

Quoted 5 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +4 -0

+
+Optional properties:
+
+- clock-frequency : the IP's main (system bus) clock frequency in Hz,

the default

Is that the input clock frequency ? Can't it vary ? Instead of accessing the

Yes, the description is incorrect in this patch, it should read:

+- clock-frequency : the sensor's master clock frequency in Hz;

and be a required property. As in this patch:
https://github.com/snawrocki/linux/commit/e8a5f890dec0d7414b656bb1d1ac97d5e7
abe563

It could vary (as this is a PLL input frequency), so probably a range would be a better alternative. Given that host device won't always be able to set this exact value...

Quoted 4 lines

sensor clock frequency from the FIMC driver you should reference a clock in the sensor DT node. That obviously requires generic clock support, which might not be available for your platform yet (that's one of the reasons the OMAP3 ISP driver doesn't support DT yet).

I agree it might be better, but waiting unknown number of kernel releases for the platforms to get converted to common clock API is not a good alternative either. I guess we could have some transitional solutions while other subsystems are getting adapted.

Yet we need to specify the clock frequency range per sensor, so

1. either we specify it at a sensor node and host device driver references it, or
2. it could be added to a sensor specific child node of a host device mode, and then only the host would reference it, and sensor would reference a clock in its DT node; I guess it's not a problem that in most cases the camera host device is a clock provider.

Hi Sylwester,

Quoted 56 lines · On Thursday 26 July 2012 22:39:31 Sylwester Nawrocki wrote:

Quoted 39 lines · Laurent Pinchart

Quoted 38 lines · On Friday 25 May 2012 21:52:48 Sylwester Nawrocki wrote:

The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---

  .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
  drivers/media/video/s5k6aa.c                       |  129 +++++++++----
  2 files changed, 146 insertions(+), 40 deletions(-)

create mode 100644

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt

diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file mode 100644
index 0000000..6685a9c

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +11 -0

@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+------------------------------
+
+Required properties:
+
+- compatible : "samsung,s5k6aafx";
+- reg : base address of the device on I2C bus;
+- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
+- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
+- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
+- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to

1.9V) + or 2.8V (2.6V to 3.0);
+- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +1 -0

+		 or 2.8V (2.5V to 3.1V);

Quoted 8 lines

Quoted 5 lines

Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file +4 -0

+
+Optional properties:
+
+- clock-frequency : the IP's main (system bus) clock frequency in Hz,

the default

Is that the input clock frequency ? Can't it vary ? Instead of accessing the

Yes, the description is incorrect in this patch, it should read:

+- clock-frequency : the sensor's master clock frequency in Hz;

and be a required property. As in this patch:
https://github.com/snawrocki/linux/commit/e8a5f890dec0d7414b656bb1d1ac97d5e7
abe563

Quoted 3 lines

It could vary (as this is a PLL input frequency), so probably a range would be a better alternative. Given that host device won't always be able to set this exact value...

A range sounds good, or perhaps a list of ranges. Sakari, what would you need for the SMIA++ driver ?

Quoted 9 lines

Quoted 4 lines

sensor clock frequency from the FIMC driver you should reference a clock in the sensor DT node. That obviously requires generic clock support, which might not be available for your platform yet (that's one of the reasons the OMAP3 ISP driver doesn't support DT yet).

I agree it might be better, but waiting unknown number of kernel releases for the platforms to get converted to common clock API is not a good alternative either. I guess we could have some transitional solutions while other subsystems are getting adapted.

I agree, we need an interim solution.

Add parsing of 'sensor' nodes specified as 'camera' child nodes. Each 'sensor' node should contain a phandle indicating sensor I2C client device. Sensors with SPI control bus are not yet supported.

Additionally it is required that the I2C client node (child node of I2C bus controller node) contains 'clock-frequency' and 'video-bus-type' properties. These properties allow the host controller to switch to proper video bus and set proper master clock frequency for a sensor.

Signed-off-by Sylwester Nawrocki <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---

We might need something like this as a V4L2 core function. But I wanted to have something settled first until proposing any addtions to the V4L2 core modules.

---
 .../bindings/camera/soc/samsung-fimc.txt           |   22 ++++
 drivers/media/video/s5p-fimc/fimc-mdevice.c        |  114 +++++++++++++++++++-
 2 files changed, 133 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt +3 -0

index b459da2..ffe09ac 100644
@@ -54,6 +54,28 @@ Required properties:
 - cell-index : FIMC-LITE IP instance index;
 
 
+The 'sensor' nodes
+------------------
+

Quoted 18 lines

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt +10 -0

index b459da2..ffe09ac 100644
@@ -54,6 +54,28 @@ Required properties:
  - cell-index : FIMC-LITE IP instance index;
 
 
+The 'sensor' nodes
+------------------
+
+Required properties:
+
+ - i2c-client : a phandle to an image sensor I2C client device;
+
+Optional properties:
+
+- samsung,camif-mux-id : FIMC video multiplexer input index; for camera

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt Required properties: · line 69

Quoted 3 lines

Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt +3 -0

+			 port A, B, C the indexes are 0, 1, 0 respectively.
+			 If this property is not specified a default 0
+			 value will be used by driver.

Isn't it possible to have several clients connected to different mux inputs and switch between them at run-time? Even if only one of them can be active at any time? That's why I've introduced link nodes in my RFC to specify exactly which pads are connected.

For systems instantiated from device tree struct platform_device id field is always -1, add an 'id' argument to the s5p_csis_phy_enable() function so the MIPI-CSIS hardware instance index can be passed in by driver, for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 arch/arm/plat-s5p/setup-mipiphy.c              |   20 ++++++++------------
 arch/arm/plat-samsung/include/plat/mipi_csis.h |   10 ++++++----
 2 files changed, 14 insertions(+), 16 deletions(-)

(quoted diff) +0 -0

b/arch/arm/plat-s5p/setup-mipiphy.c index 683c466..146ecc3 100644

Hi Sylwester,

Quoted 20 lines · On Friday 25 May 2012 21:52:40 Sylwester Nawrocki wrote:

For systems instantiated from device tree struct platform_device id field is always -1, add an 'id' argument to the s5p_csis_phy_enable() function so the MIPI-CSIS hardware instance index can be passed in by driver, for CONFIG_OF=y.

Signed-off-by Sylwester Nawrocki<[email protected]>

Signed-off-by Kyungmin Park<[email protected]>

---
  arch/arm/plat-s5p/setup-mipiphy.c              |   20 ++++++++------------
  arch/arm/plat-samsung/include/plat/mipi_csis.h |   10 ++++++----
  2 files changed, 14 insertions(+), 16 deletions(-)

(quoted diff) +0 -0

b/arch/arm/plat-s5p/setup-mipiphy.c index 683c466..146ecc3 100644

arch/arm/plat-s5p/setup-mipiphy.c +0 -0

@@ -14,24 +14,19 @@
  #include<linux/spinlock.h>
  #include<mach/regs-clock.h>
 

arch/arm/plat-s5p/setup-mipiphy.c line 18

Quoted 3 lines

arch/arm/plat-s5p/setup-mipiphy.c +1 -1

-static int __s5p_mipi_phy_control(struct platform_device *pdev,
+static int __s5p_mipi_phy_control(struct platform_device *pdev, int id,
  				  bool on, u32 reset)

What about removing the pdev argument, as it's now not needed ?