dt: bindings: Add support for CSI1 bus

From: Sakari Ailus <[email protected]>

In the vast majority of cases the bus type is known to the driver(s) since a receiver or transmitter can only support a single one. There are cases however where different options are possible.

Document the CSI1/CCP2 properties strobe_clk_inv and strobe_clock properties. The former tells whether the strobe/clock signal is inverted, while the latter signifies the clock or strobe mode.

Signed-off-by Sakari Ailus <[email protected]>

Signed-off-by Ivaylo Dimitrov <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/media/video-interfaces.txt +0 -0

index 9cd2a36..f0523f7 100644

Documentation/devicetree/bindings/media/video-interfaces.txt +4 -3

 - bus-width: number of data lines actively used, valid for the parallel busses.
 - data-shift: on the parallel data busses, if bus-width is used to specify the
   number of data lines, data-shift can be used to specify which data lines are

@@ -110,9 +114,10 @@ Optional endpoint properties
   lane and followed by the data lanes in the same order as in data-lanes.
   Valid values are 0 (normal) and 1 (inverted). The length of the array
   should be the combined length of data-lanes and clock-lanes properties.
-  If the lane-polarities property is omitted, the value must be interpreted
-  as 0 (normal). This property is valid for serial busses only.
-
+- clock-inv: Clock or strobe signal inversion.
+  Possible values: 0 -- not inverted; 1 -- inverted
+- strobe: Whether the clock signal is used as clock or strobe. Used
+  with CCP2, for instance.
 
 Example
 -------

Hi Pavel,

Quoted 19 lines · Pavel Machek

From: Sakari Ailus <[email protected]>

In the vast majority of cases the bus type is known to the driver(s) since a receiver or transmitter can only support a single one. There are cases however where different options are possible.

Document the CSI1/CCP2 properties strobe_clk_inv and strobe_clock properties. The former tells whether the strobe/clock signal is inverted, while the latter signifies the clock or strobe mode.

Signed-off-by Sakari Ailus <[email protected]>

Signed-off-by Ivaylo Dimitrov <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/media/video-interfaces.txt +0 -0

index 9cd2a36..f0523f7 100644

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 82

Quoted 7 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +4 -0

@@ -76,6 +76,10 @@ Optional endpoint properties
   mode horizontal and vertical synchronization signals are provided to the
   slave device (data source) by the master device (data sink). In the master
   mode the data source device is also the source of the synchronization signals.
+- bus-type: data bus type. Possible values are:
+  0 - CSI2
+  1 - parallel / Bt656
+  2 - CCP2

I wonder if we should make a difference between CCP2 and CSI-1 here, as it may make a difference in hardware configuration. The next patch does recognise that difference, so it should be present here as well.

Perhaps 2 - CSI1; 3 - CCP2. What do you think?

From: Sakari Ailus <[email protected]>

In the vast majority of cases the bus type is known to the driver(s) since a receiver or transmitter can only support a single one. There are cases however where different options are possible.

Document the CSI1/CCP2 properties strobe_clk_inv and strobe_clock properties. The former tells whether the strobe/clock signal is inverted, while the latter signifies the clock or strobe mode.

Signed-off-by Sakari Ailus <[email protected]>

Signed-off-by Ivaylo Dimitrov <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/media/video-interfaces.txt +0 -0

index 9cd2a36..f0523f7 100644

Documentation/devicetree/bindings/media/video-interfaces.txt +2 -0

+- strobe: Whether the clock signal is used as clock or strobe. Used
+  with CCP2, for instance.
 
 Example
 -------

Signature · 3 lines

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Quoted 5 lines · Pavel Machek

From: Sakari Ailus <[email protected]>

In the vast majority of cases the bus type is known to the driver(s) since a receiver or transmitter can only support a single one. There are cases however where different options are possible.

What cases specifically?

Quoted 9 lines

Document the CSI1/CCP2 properties strobe_clk_inv and strobe_clock properties. The former tells whether the strobe/clock signal is inverted, while the latter signifies the clock or strobe mode.

Signed-off-by Sakari Ailus <[email protected]>

Signed-off-by Ivaylo Dimitrov <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 80

Quoted 10 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +2 -0

index 9cd2a36..f0523f7 100644
@@ -76,6 +76,10 @@ Optional endpoint properties
   mode horizontal and vertical synchronization signals are provided to the
   slave device (data source) by the master device (data sink). In the master
   mode the data source device is also the source of the synchronization signals.
+- bus-type: data bus type. Possible values are:
+  0 - CSI2

As in MIPI CSI2?

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 114

Quoted 12 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +2 -2

+  1 - parallel / Bt656
+  2 - CCP2
 - bus-width: number of data lines actively used, valid for the parallel busses.
 - data-shift: on the parallel data busses, if bus-width is used to specify the
   number of data lines, data-shift can be used to specify which data lines are

@@ -110,9 +114,10 @@ Optional endpoint properties
   lane and followed by the data lanes in the same order as in data-lanes.
   Valid values are 0 (normal) and 1 (inverted). The length of the array
   should be the combined length of data-lanes and clock-lanes properties.
-  If the lane-polarities property is omitted, the value must be interpreted
-  as 0 (normal). This property is valid for serial busses only.

Why is this removed?

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 118

Quoted 4 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +2 -1

-
+- clock-inv: Clock or strobe signal inversion.
+  Possible values: 0 -- not inverted; 1 -- inverted

"invert" assumes I know what is normal and I do not. Define what is "normal" and name the property the opposite of that. If normal is data shifted on clock rising edge, then call the the property "clock-shift-falling-edge" for example..

Quoted 8 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +2 -0

+- strobe: Whether the clock signal is used as clock or strobe. Used
+  with CCP2, for instance.
 
 Example
 -------

Thanks for the review.

Quoted 7 lines · Rob Herring

Quoted 5 lines · Pavel Machek

From: Sakari Ailus <[email protected]>

In the vast majority of cases the bus type is known to the driver(s) since a receiver or transmitter can only support a single one. There are cases however where different options are possible.

Quoted 1 line

What cases specifically?

The existing V4L2 OF support tries to figure out the bus type and parse the bus parameters based on that. This does not scale too well as there are multiple serial busses that share common properties.

Some hardware also supports multiple types of busses on the same interfaces.

Quoted 1 line

As in MIPI CSI2?

Yeah, I guess it'd make sense to make this explicit.

Quoted 5 lines

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 114

Quoted 3 lines

should be the combined length of data-lanes and clock-lanes properties.

Documentation/devicetree/bindings/media/video-interfaces.txt +0 -2

-  If the lane-polarities property is omitted, the value must be interpreted
-  as 0 (normal). This property is valid for serial busses only.

Why is this removed?

Must have been by mistake. :-)

Quoted 8 lines

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 118

Quoted 3 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +2 -1

-
+- clock-inv: Clock or strobe signal inversion.
+  Possible values: 0 -- not inverted; 1 -- inverted

"invert" assumes I know what is normal and I do not. Define what is "normal" and name the property the opposite of that. If normal is data shifted on clock rising edge, then call the the property "clock-shift-falling-edge" for example..

The hardware documentation says this is the "strobe/clock inversion control signal". I'm not entirely sure whether this is just signal polarity (it's a differential signal) or inversion of an internal signal of the CCP2 block.

It might make sense to make this a private property for the OMAP 3 ISP instead. If it's seen elsewhere, then think about it again. I doubt it would, as CCP2 is an old bus that's used on Nokia N9, N950 and N900.

As strobe is included, I'd add that to the name. Say, "ti,clock-strobe-inv".

Quoted 16 lines

Quoted 7 lines

Quoted 2 lines

+- clock-inv: Clock or strobe signal inversion.
+  Possible values: 0 -- not inverted; 1 -- inverted

"invert" assumes I know what is normal and I do not. Define what is "normal" and name the property the opposite of that. If normal is data shifted on clock rising edge, then call the the property "clock-shift-falling-edge" for example..

The hardware documentation says this is the "strobe/clock inversion control signal". I'm not entirely sure whether this is just signal polarity (it's a differential signal) or inversion of an internal signal of the CCP2 block.

It might make sense to make this a private property for the OMAP 3 ISP instead. If it's seen elsewhere, then think about it again. I doubt it would, as CCP2 is an old bus that's used on Nokia N9, N950 and N900.

Quoted 2 lines

As strobe is included, I'd add that to the name. Say, "ti,clock-strobe-inv".

Hmm. N900 does not use inversion. Would it make sense to simply hardcode it to "not-inverted" for now?

Device tree changes are PITA :-(.

From: Sakari Ailus <[email protected]>

In the vast majority of cases the bus type is known to the driver(s) since a receiver or transmitter can only support a single one. There are cases however where different options are possible.

The existing V4L2 OF support tries to figure out the bus type and parse the bus parameters based on that. This does not scale too well as there are multiple serial busses that share common properties.

Some hardware also supports multiple types of busses on the same interfaces.

Document the CSI1/CCP2 property strobe. It signifies the clock or strobe mode.

Signed-off-by Sakari Ailus <[email protected]>

Signed-off-by Ivaylo Dimitrov <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/media/video-interfaces.txt +5 -0

index 9cd2a36..08c4498 100644
@@ -76,6 +76,11 @@ Optional endpoint properties
   mode horizontal and vertical synchronization signals are provided to the
   slave device (data source) by the master device (data sink). In the master
   mode the data source device is also the source of the synchronization signals.
+- bus-type: data bus type. Possible values are:
+  0 - MIPI CSI2
+  1 - parallel / Bt656
+  2 - MIPI CSI1
+  3 - CCP2
 - bus-width: number of data lines actively used, valid for the parallel busses.
 - data-shift: on the parallel data busses, if bus-width is used to specify the
   number of data lines, data-shift can be used to specify which data lines are

From: Sakari Ailus <[email protected]>

In the vast majority of cases the bus type is known to the driver(s) since a receiver or transmitter can only support a single one. There are cases however where different options are possible.

The existing V4L2 OF support tries to figure out the bus type and parse the bus parameters based on that. This does not scale too well as there are multiple serial busses that share common properties.

Some hardware also supports multiple types of busses on the same interfaces.

Document the CSI1/CCP2 property strobe. It signifies the clock or strobe mode.

Signed-off-by Sakari Ailus <[email protected]>

Signed-off-by Ivaylo Dimitrov <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/media/video-interfaces.txt +0 -0

index 9cd2a36..08c4498 100644

Documentation/devicetree/bindings/media/video-interfaces.txt +4 -1

+  2 - MIPI CSI1
+  3 - CCP2
 - bus-width: number of data lines actively used, valid for the parallel busses.
 - data-shift: on the parallel data busses, if bus-width is used to specify the
   number of data lines, data-shift can be used to specify which data lines are

@@ -112,7 +117,8 @@ Optional endpoint properties
   should be the combined length of data-lanes and clock-lanes properties.
   If the lane-polarities property is omitted, the value must be interpreted
   as 0 (normal). This property is valid for serial busses only.
-
+- strobe: Whether the clock signal is used as clock or strobe. Used
+  with CCP2, for instance.
 
 Example
 -------

Hi Pavel,

Quoted 20 lines · Pavel Machek

From: Sakari Ailus <[email protected]>

In the vast majority of cases the bus type is known to the driver(s) since a receiver or transmitter can only support a single one. There are cases however where different options are possible.

The existing V4L2 OF support tries to figure out the bus type and parse the bus parameters based on that. This does not scale too well as there are multiple serial busses that share common properties.

Some hardware also supports multiple types of busses on the same interfaces.

Document the CSI1/CCP2 property strobe. It signifies the clock or strobe mode.

Signed-off-by Sakari Ailus <[email protected]>

Signed-off-by Ivaylo Dimitrov <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 81

Quoted 11 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +3 -0

index 9cd2a36..08c4498 100644
@@ -76,6 +76,11 @@ Optional endpoint properties
   mode horizontal and vertical synchronization signals are provided to the
   slave device (data source) by the master device (data sink). In the master
   mode the data source device is also the source of the synchronization signals.
+- bus-type: data bus type. Possible values are:
+  0 - MIPI CSI2
+  1 - parallel / Bt656

Why not have separate values for parallel and BT.656?

From: Sakari Ailus <[email protected]>

In the vast majority of cases the bus type is known to the driver(s) since a receiver or transmitter can only support a single one. There are cases however where different options are possible.

The existing V4L2 OF support tries to figure out the bus type and parse the bus parameters based on that. This does not scale too well as there are multiple serial busses that share common properties.

Some hardware also supports multiple types of busses on the same interfaces.

Document the CSI1/CCP2 property strobe. It signifies the clock or strobe mode.

Signed-off-by Sakari Ailus <[email protected]>

Signed-off-by Ivaylo Dimitrov <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/media/video-interfaces.txt +0 -0

index 9cd2a36..08c4498 100644

Hi Pavel,

Quoted 25 lines · Pavel Machek

From: Sakari Ailus <[email protected]>

In the vast majority of cases the bus type is known to the driver(s) since a receiver or transmitter can only support a single one. There are cases however where different options are possible.

The existing V4L2 OF support tries to figure out the bus type and parse the bus parameters based on that. This does not scale too well as there are multiple serial busses that share common properties.

Some hardware also supports multiple types of busses on the same interfaces.

Document the CSI1/CCP2 property strobe. It signifies the clock or strobe mode.

Signed-off-by Sakari Ailus <[email protected]>

Signed-off-by Ivaylo Dimitrov <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/media/video-interfaces.txt +0 -0

index 9cd2a36..08c4498 100644

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 83

Quoted 8 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +5 -0

@@ -76,6 +76,11 @@ Optional endpoint properties
   mode horizontal and vertical synchronization signals are provided to the
   slave device (data source) by the master device (data sink). In the master
   mode the data source device is also the source of the synchronization signals.
+- bus-type: data bus type. Possible values are:
+  0 - MIPI CSI2
+  1 - parallel / Bt656
+  2 - MIPI CSI1
+  3 - CCP2

Actually, thinking about this again --- we only need to explictly specify busses if we're dealing with either CCP2 or CSI-1. The vast majority of the actual busses are and continue to be CSI-2 or either parallel or Bt.656. As they can be implicitly detected, we would have an option to just drop values 0 and 1 from above, i.e. only leave CSI-1 and CCP2. For now, specifying CSI-2 or parallel / Bt.656 adds no value as the old DT binaries without bus-type will need to be supported anyway.

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 121

Quoted 10 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +2 -1

 - bus-width: number of data lines actively used, valid for the parallel busses.
 - data-shift: on the parallel data busses, if bus-width is used to specify the
   number of data lines, data-shift can be used to specify which data lines are

@@ -112,7 +117,8 @@ Optional endpoint properties
   should be the combined length of data-lanes and clock-lanes properties.
   If the lane-polarities property is omitted, the value must be interpreted
   as 0 (normal). This property is valid for serial busses only.
-
+- strobe: Whether the clock signal is used as clock or strobe. Used
+  with CCP2, for instance.

How about the "ti,strobe-clock-inv" I proposed? No-one seems to know what this really truly means... or just drop it if it's not really needed.

Hi,

Quoted 45 lines · Sakari Ailus

Hi Pavel,

Quoted 25 lines · Pavel Machek

From: Sakari Ailus <[email protected]>

In the vast majority of cases the bus type is known to the driver(s) since a receiver or transmitter can only support a single one. There are cases however where different options are possible.

The existing V4L2 OF support tries to figure out the bus type and parse the bus parameters based on that. This does not scale too well as there are multiple serial busses that share common properties.

Some hardware also supports multiple types of busses on the same interfaces.

Document the CSI1/CCP2 property strobe. It signifies the clock or strobe mode.

Signed-off-by Sakari Ailus <[email protected]>

Signed-off-by Ivaylo Dimitrov <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/media/video-interfaces.txt +0 -0

index 9cd2a36..08c4498 100644

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 83

Quoted 8 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +5 -0

@@ -76,6 +76,11 @@ Optional endpoint properties
  mode horizontal and vertical synchronization signals are provided to the
  slave device (data source) by the master device (data sink). In the master
  mode the data source device is also the source of the synchronization signals.
+- bus-type: data bus type. Possible values are:
+  0 - MIPI CSI2
+  1 - parallel / Bt656
+  2 - MIPI CSI1
+  3 - CCP2

Actually, thinking about this again --- we only need to explictly specify busses if we're dealing with either CCP2 or CSI-1. The vast majority of the actual busses are and continue to be CSI-2 or either parallel or Bt.656. As they can be implicitly detected, we would have an option to just drop values 0 and 1 from above, i.e. only leave CSI-1 and CCP2. For now, specifying CSI-2 or parallel / Bt.656 adds no value as the old DT binaries without bus-type will need to be supported anyway.

Quoted 14 lines

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 121

Quoted 10 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +2 -3

- bus-width: number of data lines actively used, valid for the parallel busses.
- data-shift: on the parallel data busses, if bus-width is used to specify the
  number of data lines, data-shift can be used to specify which data lines are

@@ -112,7 +117,8 @@ Optional endpoint properties
  should be the combined length of data-lanes and clock-lanes properties.
  If the lane-polarities property is omitted, the value must be interpreted
  as 0 (normal). This property is valid for serial busses only.
-
+- strobe: Whether the clock signal is used as clock or strobe. Used
+  with CCP2, for instance.

How about the "ti,strobe-clock-inv" I proposed? No-one seems to know what this really truly means... or just drop it if it's not really needed.

Not really :), see https://www.spinics.net/lists/linux-media/msg99802.html
and https://www.spinics.net/lists/linux-media/msg99800.html

"clock/strobe", and "strobe-inv" are two distinct properties, see CSI1B_CTRL description in OMAP TRM. BTW there is another property that is needed for both n900 cameras to operate correctly (VP_CLK_POL, bit 12 from the same reg), but that can be added later on when we have the other bits in place.

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 83

Quoted 11 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +5 -0

@@ -76,6 +76,11 @@ Optional endpoint properties
   mode horizontal and vertical synchronization signals are provided to the
   slave device (data source) by the master device (data sink). In the master
   mode the data source device is also the source of the synchronization signals.
+- bus-type: data bus type. Possible values are:
+  0 - MIPI CSI2
+  1 - parallel / Bt656
+  2 - MIPI CSI1
+  3 - CCP2

Actually, thinking about this again --- we only need to explictly specify busses if we're dealing with either CCP2 or CSI-1. The vast majority of the actual busses are and continue to be CSI-2 or either parallel or Bt.656. As they can be implicitly detected, we would have an option to just drop values 0 and 1 from above, i.e. only leave CSI-1 and CCP2. For now, specifying CSI-2 or parallel / Bt.656 adds no value as the old DT binaries without bus-type will need to be supported anyway.

Hi!

Quoted 12 lines

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 83

Quoted 11 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +5 -0

@@ -76,6 +76,11 @@ Optional endpoint properties
   mode horizontal and vertical synchronization signals are provided to the
   slave device (data source) by the master device (data sink). In the master
   mode the data source device is also the source of the synchronization signals.
+- bus-type: data bus type. Possible values are:
+  0 - MIPI CSI2
+  1 - parallel / Bt656
+  2 - MIPI CSI1
+  3 - CCP2

Quoted 7 lines

Actually, thinking about this again --- we only need to explictly specify busses if we're dealing with either CCP2 or CSI-1. The vast majority of the actual busses are and continue to be CSI-2 or either parallel or Bt.656. As they can be implicitly detected, we would have an option to just drop values 0 and 1 from above, i.e. only leave CSI-1 and CCP2. For now, specifying CSI-2 or parallel / Bt.656 adds no value as the old DT binaries without bus-type will need to be supported anyway.

Hmm. "Just deleting the others" may be a bit confusing... but what about this? It explains what we can autodetect.

Best regards,
Pavel

Documentation/devicetree/bindings/media/video-interfaces.txt +0 -0

index 08c4498..d54093b 100644

Documentation/devicetree/bindings/media/video-interfaces.txt +3 -0

+  1 - MIPI CSI1
+  2 - CCP2
+  Autodetection is default, and bus-type property may be omitted in that case.
 - bus-width: number of data lines actively used, valid for the parallel busses.
 - data-shift: on the parallel data busses, if bus-width is used to specify the
   number of data lines, data-shift can be used to specify which data lines are

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 80

Quoted 6 lines

- bus-type: data bus type. Possible values are:

Documentation/devicetree/bindings/media/video-interfaces.txt +1 -4

-  0 - MIPI CSI2
-  1 - parallel / Bt656
-  2 - MIPI CSI1
-  3 - CCP2
+  0 - autodetect based on other properties (MIPI CSI2, parallel, Bt656)

In the meantime, I also realised that we need to separate MIPI C-PHY and D-PHY from each other. So I think we'll need that property for CSI-2 nevertheless. How about:

0 - autodetect based on other properties (MIPI CSI-2 D-PHY, parallel or Bt656) 1 - MIPI CSI-2 C-PHY
2 - MIPI CSI1
3 - CCP2

I wouldn't add a separate entry for the parallel case as there are plenty of existing DT binaries with parallel interface configuration without phy-type property. They will need to be continue to be supported anyway, so there's not too much value in adding a type for that purpose.

I do find this a bit annoying; we should have had something like phy-type from day one rather than try to guess which phy is being used...

From: Sakari Ailus <[email protected]>

In the vast majority of cases the bus type is known to the driver(s) since a receiver or transmitter can only support a single one. There are cases however where different options are possible.

The existing V4L2 OF support tries to figure out the bus type and parse the bus parameters based on that. This does not scale too well as there are multiple serial busses that share common properties.

Some hardware also supports multiple types of busses on the same interfaces.

Document the CSI1/CCP2 property strobe. It signifies the clock or strobe mode.

How about the rest? :-) I guess we could get the CCP2 support in omap3isp without the video bus switch. It'd be nice to have this in a single patchset.

Obtain the CSI1/CCP2 bus parameters from the OF node.

ISP CSI1 module needs all the bits correctly set to work.

OMAP3430 needs various syscon CONTROL_CSIRXFE bits set in order to operate. Implement the missing functionality.

Signed-off-by Sakari Ailus <[email protected]>

Signed-off-by Ivaylo Dimitrov <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

---

Quoted 3 lines

How about the rest? :-) I guess we could get the CCP2 support in omap3isp without the video bus switch. It'd be nice to have this in a single patchset.

Ok, here you go, what about this?

Pavel

(quoted diff) +0 -0

b/drivers/media/platform/omap3isp/isp.c index 0321d84..88bc7c6 100644

drivers/media/platform/omap3isp/isp.c +36 -2

+	dev_dbg(dev, "data lane polarity %u, pos %u\n",
+		buscfg->lanecfg.data[0].pol,
+		buscfg->lanecfg.data[0].pos);
+
+	buscfg->strobe_clk_pol = vep->bus.mipi_csi1.clock_inv;
+	buscfg->phy_layer = vep->bus.mipi_csi1.strobe;
+	buscfg->ccp2_mode = vep->bus_type == V4L2_MBUS_CCP2;
+
+	dev_dbg(dev, "clock_inv %u strobe %u ccp2 %u\n",
+		buscfg->strobe_clk_pol,
+		buscfg->phy_layer,
+		buscfg->ccp2_mode);
+	/*
+	 * FIXME: now we assume the CRC is always there.
+	 * Implement a way to obtain this information from the
+	 * sensor. Frame descriptors, perhaps?
+	 */
+	buscfg->crc = 1;
+
+	buscfg->vp_clk_pol = 1;
+}
+
+static void isp_of_parse_node_csi1(struct device *dev,
+				   struct isp_bus_cfg *buscfg,
+				   struct v4l2_of_endpoint *vep)
+{
+	if (vep->base.port == ISP_OF_PHY_CSIPHY1)
+		buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
+	else
+		buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
+	__isp_of_parse_node_csi1(dev, &buscfg->bus.ccp2, vep);
+}
+
+static void isp_of_parse_node_csi2(struct device *dev,
+				   struct isp_bus_cfg *buscfg,
+				   struct v4l2_of_endpoint *vep)
 {
-	struct isp_bus_cfg *buscfg = &isd->bus;
-	struct v4l2_of_endpoint vep;

drivers/media/platform/omap3isp/isp.c +27 -2

+	buscfg->bus.csi2.lanecfg.clk.pos = vep->bus.mipi_csi2.clock_lane;
+	buscfg->bus.csi2.lanecfg.clk.pol =
+			vep->bus.mipi_csi2.lane_polarities[0];
+	dev_dbg(dev, "clock lane polarity %u, pos %u\n",
+		buscfg->bus.csi2.lanecfg.clk.pol,
+		buscfg->bus.csi2.lanecfg.clk.pos);
+
+	for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
+		buscfg->bus.csi2.lanecfg.data[i].pos =
+			vep->bus.mipi_csi2.data_lanes[i];
+		buscfg->bus.csi2.lanecfg.data[i].pol =
+			vep->bus.mipi_csi2.lane_polarities[i + 1];
+		dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
+			buscfg->bus.csi2.lanecfg.data[i].pol,
+				buscfg->bus.csi2.lanecfg.data[i].pos);
+	}
 
-	dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
-		vep.base.port);
+	/*
+	 * FIXME: now we assume the CRC is always there.
+	 * Implement a way to obtain this information from the
+	 * sensor. Frame descriptors, perhaps?
+	 */
+	buscfg->bus.csi2.crc = 1;
+}
 
+static int isp_endpoint_to_buscfg(struct device *dev,
+				  struct v4l2_of_endpoint vep,
+				  struct isp_bus_cfg *buscfg)
+{
 	switch (vep.base.port) {
 	case ISP_OF_PHY_PARALLEL:
 		buscfg->interface = ISP_INTERFACE_PARALLEL;

@@ -2059,45 +2130,42 @@ static int isp_of_parse_node(struct device *dev,
 struct device_node *node,
 
 	case ISP_OF_PHY_CSIPHY1:
 	case ISP_OF_PHY_CSIPHY2:

drivers/media/platform/omap3isp/isp.c +0 -20

-		buscfg->bus.csi2.lanecfg.clk.pol =
-			vep.bus.mipi_csi2.lane_polarities[0];
-		dev_dbg(dev, "clock lane polarity %u, pos %u\n",
-			buscfg->bus.csi2.lanecfg.clk.pol,
-			buscfg->bus.csi2.lanecfg.clk.pos);
-
-		for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
-			buscfg->bus.csi2.lanecfg.data[i].pos =
-				vep.bus.mipi_csi2.data_lanes[i];
-			buscfg->bus.csi2.lanecfg.data[i].pol =
-				vep.bus.mipi_csi2.lane_polarities[i + 1];
-			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
-				buscfg->bus.csi2.lanecfg.data[i].pol,
-				buscfg->bus.csi2.lanecfg.data[i].pos);
-		}
-
-		/*
-		 * FIXME: now we assume the CRC is always there.
-		 * Implement a way to obtain this information from the
-		 * sensor. Frame descriptors, perhaps?

drivers/media/platform/omap3isp/isp.c +0 -2

 		dev_warn(dev, "%s: invalid interface %u\n", node->full_name,
 			 vep.base.port);
-		break;
-	}
 
 	return 0;

}

drivers/media/platform/omap3isp/isp.c +1 -1

@@ -2124,7 +2192,7 @@ static int isp_of_parse_nodes(struct device *dev,
 
 		notifier->subdevs[notifier->num_subdevs] = &isd->asd;
 
-		if (isp_of_parse_node(dev, node, isd)) {
+		if (isp_of_parse_node_endpoint(dev, node, isd)) {
 			of_node_put(node);
 			return -EINVAL;
 		}

(quoted diff) +0 -0

b/drivers/media/platform/omap3isp/isp.h index 7e6f663..c0b9d1d 100644

drivers/media/platform/omap3isp/isp.h +1 -1

@@ -228,7 +228,7 @@ struct isp_device {
 
 struct isp_async_subdev {
 	struct v4l2_subdev *sd;
-	struct isp_bus_cfg bus;
+	struct isp_bus_cfg *bus;
 	struct v4l2_async_subdev asd;
 };
 

(quoted diff) +0 -0

b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4edb55a 100644

drivers/media/platform/omap3isp/ispccp2.c +3 -0

@@ -21,6 +21,9 @@
 #include <linux/mutex.h>
 #include <linux/uaccess.h>
 #include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
+
+#include <media/v4l2-of.h>
 
 #include "isp.h"
 #include "ispreg.h"

drivers/media/platform/omap3isp/ispccp2.c +5 -2

@@ -213,14 +243,17 @@ static int ccp2_phyif_config(struct isp_ccp2_device
 *ccp2, struct isp_device *isp = to_isp_device(ccp2);
 	u32 val;
 
-	/* CCP2B mode */
 	val = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_CTRL) |
-			    ISPCCP2_CTRL_IO_OUT_SEL | ISPCCP2_CTRL_MODE;
+	      ISPCCP2_CTRL_MODE;
 	/* Data/strobe physical layer */
 	BIT_SET(val, ISPCCP2_CTRL_PHY_SEL_SHIFT, ISPCCP2_CTRL_PHY_SEL_MASK,
 		buscfg->phy_layer);
+	BIT_SET(val, ISPCCP2_CTRL_IO_OUT_SEL_SHIFT,
+		ISPCCP2_CTRL_IO_OUT_SEL_MASK, buscfg->ccp2_mode);
 	BIT_SET(val, ISPCCP2_CTRL_INV_SHIFT, ISPCCP2_CTRL_INV_MASK,
 		buscfg->strobe_clk_pol);
+	BIT_SET(val, ISPCCP2_CTRL_VP_CLK_POL_SHIFT,
+		ISPCCP2_CTRL_VP_CLK_POL_MASK, buscfg->vp_clk_pol);
 	isp_reg_writel(isp, val, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_CTRL);
 
 	val = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_CTRL);

include/media/v4l2-mediabus.h +0 -0

 	V4L2_MBUS_CSI2,
 };
 

include/media/v4l2-of.h +1 -0

index 4dc34b2..63a52ee 100644
@@ -53,6 +53,22 @@ struct v4l2_of_bus_parallel {
 };
 
 /**
+ * struct v4l2_of_bus_csi1 - CSI-1/CCP2 data bus structure

include/media/v4l2-of.h +6 -0

+	unsigned char data_lane;
+	unsigned char clock_lane;
+};
+
+/**
  * struct v4l2_of_endpoint - the endpoint data structure
  * @base: struct of_endpoint containing port, id, and local of_node
  * @bus_type: bus type

@@ -66,6 +82,7 @@ struct v4l2_of_endpoint {
 	enum v4l2_mbus_type bus_type;
 	union {
 		struct v4l2_of_bus_parallel parallel;
+		struct v4l2_of_bus_mipi_csi1 mipi_csi1;
 		struct v4l2_of_bus_mipi_csi2 mipi_csi2;
 	} bus;
 	u64 *link_frequencies;

Quoted 8 lines

(quoted diff) +0 -0

b/drivers/media/platform/omap3isp/isp.c index 0321d84..88bc7c6 100644

drivers/media/platform/omap3isp/isp.c +0 -0

@@ -2024,21 +2024,92 @@ enum isp_of_phy {
 	ISP_OF_PHY_CSIPHY2,
 };
 

drivers/media/platform/omap3isp/isp.c enum isp_of_phy { · line 2029

Quoted 5 lines

drivers/media/platform/omap3isp/isp.c +3 -2

-static int isp_of_parse_node(struct device *dev, struct device_node *node,
-			     struct isp_async_subdev *isd)
+void __isp_of_parse_node_csi1(struct device *dev,
+				   struct isp_ccp2_cfg *buscfg,
+				   struct v4l2_of_endpoint *vep)

This function isn't use anywhere else, you can merge it with isp_of_parse_node_csi1().

Quoted 5 lines

drivers/media/platform/omap3isp/isp.c +5 -0

+{
+	buscfg->lanecfg.clk.pos = vep->bus.mipi_csi1.clock_lane;
+	buscfg->lanecfg.clk.pol =
+		vep->bus.mipi_csi1.lane_polarity[0];
+	dev_dbg(dev, "clock lane polarity %u, pos %u\n",

drivers/media/platform/omap3isp/isp.c enum isp_of_phy { · line 2040

Quoted 6 lines

drivers/media/platform/omap3isp/isp.c +6 -0

+		buscfg->lanecfg.clk.pol,
+		buscfg->lanecfg.clk.pos);
+
+	buscfg->lanecfg.data[0].pos = vep->bus.mipi_csi2.data_lanes[0];
+	buscfg->lanecfg.data[0].pol =
+		vep->bus.mipi_csi2.lane_polarities[1];

bus.mipi_csi2 ?

drivers/media/platform/omap3isp/isp.c enum isp_of_phy { · line 2083

Quoted 7 lines

drivers/media/platform/omap3isp/isp.c +4 -3

-	ret = v4l2_of_parse_endpoint(node, &vep);
-	if (ret)
-		return ret;
+	if (vep->base.port == ISP_OF_PHY_CSIPHY1)
+		buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
+	else
+		buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;

I would keep this code in the caller to avoid code duplication with isp_of_parse_node_csi1().

drivers/media/platform/omap3isp/isp.c static int isp_of_parse_node(struct device *dev, struct device_node *node, · line 2140

Quoted 4 lines

drivers/media/platform/omap3isp/isp.c +1 -0

 		break;
 
 	default:
+		return -1;

Please use the appropriate error code.

Quoted 6 lines

drivers/media/platform/omap3isp/isp.c +6 -0

+	return 0;
+}
+
+static int isp_of_parse_node_endpoint(struct device *dev,
+				      struct device_node *node,
+				      struct isp_async_subdev *isd)

drivers/media/platform/omap3isp/isp.c static int isp_of_parse_node(struct device *dev, struct device_node *node, · line 2153

Quoted 6 lines

drivers/media/platform/omap3isp/isp.c +6 -0

+{
+	struct isp_bus_cfg *buscfg;
+	struct v4l2_of_endpoint vep;
+	int ret;
+
+	isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);

Why do you now need to allocate this manually ?

drivers/media/platform/omap3isp/isp.c static int isp_of_parse_node(struct device *dev, struct device_node *node, · line 2166

Quoted 4 lines

drivers/media/platform/omap3isp/isp.c +4 -0

+	dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
+		vep.base.port);
+
+	if (isp_endpoint_to_buscfg(dev, vep, buscfg))

I'm fine splitting the CSI1/CSI2 parsing code to separate functions, but I don't think there's a need to split isp_endpoint_to_buscfg(). You can keep that part inline.

Quoted 8 lines

(quoted diff) +0 -0

b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4edb55a 100644

drivers/media/platform/omap3isp/ispccp2.c +0 -0

@@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device *ccp2,
 u8 enable) return ret;
 	}
 

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable) · line 166

Quoted 1 line

drivers/media/platform/omap3isp/ispccp2.c +1 -0

+	if (isp->revision == ISP_REVISION_2_0) {

The isp_csiphy.c code checks phy->isp->phy_type for the same purpose, shouldn't you use that too ?

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable) · line 177

Quoted 3 lines

drivers/media/platform/omap3isp/ispccp2.c +3 -0

+		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
+
+

One blank line is enough.

Quoted 5 lines

drivers/media/platform/omap3isp/ispccp2.c +5 -0

+		if (enable) {
+			csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
+				  OMAP343X_CONTROL_CSIRXFE_RESET;
+
+			if (buscfg->phy_layer)

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable) · line 188

Quoted 6 lines

drivers/media/platform/omap3isp/ispccp2.c +6 -0

+				csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
+
+			if (buscfg->strobe_clk_pol)
+				csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
+		} else
+			csirxfe = 0;

You need curly braces for the else statement too.

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device *ccp2, · line 182

Quoted 2 lines

drivers/media/platform/omap3isp/ispccp2.c +2 -0

+
+		regmap_write(isp->syscon, isp->syscon_offset, csirxfe);

Isn't this already configured by csiphy_routing_cfg_3430(), called through omap3isp_csiphy_acquire() ? You'll need to add support for the strobe/clock polarity there, but the rest should already be handled.

drivers/media/platform/omap3isp/ispccp2.c line 73

Quoted 6 lines

drivers/media/platform/omap3isp/ispccp2.c +2 -0

@@ -69,11 +69,15 @@
  * @V4L2_MBUS_PARALLEL:	parallel interface with hsync and vsync
  * @V4L2_MBUS_BT656:	parallel interface with embedded synchronisation, can
  *			also be used for BT.1120
+ * @V4L2_MBUS_CSI1:	MIPI CSI-1 serial interface
+ * @V4L2_MBUS_CCP2:	CCP2 (Compact Camera Port 2)

It would help if you could provide, in comments or in the commit message, a few pointers to information about CSI-1 and CCP2.

Quoted 4 lines

drivers/media/platform/omap3isp/ispccp2.c +1 -0

 /**
+ * struct v4l2_of_bus_csi1 - CSI-1/CCP2 data bus structure

drivers/media/platform/omap3isp/ispccp2.c

Quoted 10 lines

drivers/media/platform/omap3isp/ispccp2.c +10 -0

+ * @clock_inv: polarity of clock/strobe signal
+ *	       false - not inverted, true - inverted
+ * @strobe: false - data/clock, true - data/strobe
+ * @data_lane: the number of the data lane
+ * @clock_lane: the number of the clock lane
+ */
+struct v4l2_of_bus_mipi_csi1 {
+	bool clock_inv;
+	bool strobe;
+	bool lane_polarity[2];

This field isn't documented.

Hi!

Quoted 8 lines

Quoted 8 lines

(quoted diff) +0 -0

b/drivers/media/platform/omap3isp/isp.c index 0321d84..88bc7c6 100644

drivers/media/platform/omap3isp/isp.c +0 -0

@@ -2024,21 +2024,92 @@ enum isp_of_phy {
 	ISP_OF_PHY_CSIPHY2,
 };
 

Quoted 9 lines

drivers/media/platform/omap3isp/isp.c enum isp_of_phy { · line 2029

Quoted 6 lines

drivers/media/platform/omap3isp/isp.c +3 -2

-static int isp_of_parse_node(struct device *dev, struct device_node
 *node,
-			     struct isp_async_subdev *isd)
+void __isp_of_parse_node_csi1(struct device *dev,
+				   struct isp_ccp2_cfg *buscfg,
+				   struct v4l2_of_endpoint *vep)

This function isn't use anywhere else, you can merge it with isp_of_parse_node_csi1().

I'd prefer not to. First, it will be used separately in future, and second, expresions would be uglier.

Quoted 5 lines

Quoted 5 lines

drivers/media/platform/omap3isp/isp.c +5 -0

+{
+	buscfg->lanecfg.clk.pos = vep->bus.mipi_csi1.clock_lane;
+	buscfg->lanecfg.clk.pol =
+		vep->bus.mipi_csi1.lane_polarity[0];
+	dev_dbg(dev, "clock lane polarity %u, pos %u\n",

Quoted 8 lines

drivers/media/platform/omap3isp/isp.c enum isp_of_phy { · line 2040

Quoted 6 lines

drivers/media/platform/omap3isp/isp.c +6 -0

+		buscfg->lanecfg.clk.pol,
+		buscfg->lanecfg.clk.pos);
+
+	buscfg->lanecfg.data[0].pos = vep->bus.mipi_csi2.data_lanes[0];
+	buscfg->lanecfg.data[0].pol =
+		vep->bus.mipi_csi2.lane_polarities[1];

bus.mipi_csi2 ?

Good catch. Fixed.

Quoted 10 lines

drivers/media/platform/omap3isp/isp.c enum isp_of_phy { · line 2083

Quoted 7 lines

drivers/media/platform/omap3isp/isp.c +4 -3

-	ret = v4l2_of_parse_endpoint(node, &vep);
-	if (ret)
-		return ret;
+	if (vep->base.port == ISP_OF_PHY_CSIPHY1)
+		buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
+	else
+		buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;

I would keep this code in the caller to avoid code duplication with isp_of_parse_node_csi1().

Take a closer look. Code in _csi1 is different.

Quoted 6 lines

drivers/media/platform/omap3isp/isp.c static int isp_of_parse_node(struct device *dev, struct device_node *node, · line 2140

Quoted 4 lines

drivers/media/platform/omap3isp/isp.c +1 -0

 		break;
 
 	default:
+		return -1;

Please use the appropriate error code.

Ok.

Quoted 6 lines

Quoted 6 lines

drivers/media/platform/omap3isp/isp.c +6 -0

+	return 0;
+}
+
+static int isp_of_parse_node_endpoint(struct device *dev,
+				      struct device_node *node,
+				      struct isp_async_subdev *isd)

Quoted 8 lines

drivers/media/platform/omap3isp/isp.c static int isp_of_parse_node(struct device *dev, struct device_node *node, · line 2153

Quoted 6 lines

drivers/media/platform/omap3isp/isp.c +6 -0

+{
+	struct isp_bus_cfg *buscfg;
+	struct v4l2_of_endpoint vep;
+	int ret;
+
+	isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);

Why do you now need to allocate this manually ?

bus is now a pointer.

Quoted 8 lines

drivers/media/platform/omap3isp/isp.c static int isp_of_parse_node(struct device *dev, struct device_node *node, · line 2166

Quoted 4 lines

drivers/media/platform/omap3isp/isp.c +4 -0

+	dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
+		vep.base.port);
+
+	if (isp_endpoint_to_buscfg(dev, vep, buscfg))

I'm fine splitting the CSI1/CSI2 parsing code to separate functions, but I don't think there's a need to split isp_endpoint_to_buscfg(). You can keep that part inline.

I'd prefer smaller functions here. I tried to read the original and it was not too easy.

Quoted 10 lines

Quoted 10 lines

(quoted diff) +0 -0

b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4edb55a 100644

drivers/media/platform/omap3isp/ispccp2.c +0 -0

@@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device
 *ccp2, u8 enable) return ret;
 
 	}
 

Quoted 4 lines

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable) · line 166

Quoted 1 line

drivers/media/platform/omap3isp/ispccp2.c +1 -0

+	if (isp->revision == ISP_REVISION_2_0) {

The isp_csiphy.c code checks phy->isp->phy_type for the same purpose, shouldn't you use that too ?

Do you want me to do phy->isp->phy_type == ISP_PHY_TYPE_3430 check here? Can do...

Quoted 5 lines

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable) · line 177

Quoted 3 lines

drivers/media/platform/omap3isp/ispccp2.c +3 -0

+		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
+
+

One blank line is enough.

Ok.

Quoted 5 lines

Quoted 5 lines

drivers/media/platform/omap3isp/ispccp2.c +5 -0

+		if (enable) {
+			csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
+				  OMAP343X_CONTROL_CSIRXFE_RESET;
+
+			if (buscfg->phy_layer)

Quoted 8 lines

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable) · line 188

Quoted 6 lines

drivers/media/platform/omap3isp/ispccp2.c +6 -0

+				csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
+
+			if (buscfg->strobe_clk_pol)
+				csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
+		} else
+			csirxfe = 0;

You need curly braces for the else statement too.

Easy enough.

Quoted 6 lines

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device · line 183

Quoted 2 lines

drivers/media/platform/omap3isp/ispccp2.c +2 -0

+
+		regmap_write(isp->syscon, isp->syscon_offset, csirxfe);

Isn't this already configured by csiphy_routing_cfg_3430(), called through omap3isp_csiphy_acquire() ? You'll need to add support for the strobe/clock polarity there, but the rest should already be handled.

Let me check...

Quoted 10 lines

Quoted 7 lines

drivers/media/platform/omap3isp/ispccp2.c +0 -0

@@ -69,11 +69,15 @@
  * @V4L2_MBUS_PARALLEL:	parallel interface with hsync and vsync
  * @V4L2_MBUS_BT656:	parallel interface with embedded

synchronisation, can
* also be used for BT.1120
+ * @V4L2_MBUS_CSI1: MIPI CSI-1 serial interface
+ * @V4L2_MBUS_CCP2: CCP2 (Compact Camera Port 2)

It would help if you could provide, in comments or in the commit message, a few pointers to information about CSI-1 and CCP2.

There's not much good information :-(.

http://electronics.stackexchange.com/questions/134395/differences-between-mi
pi-csi1-and-mipi-csi2

Quoted 15 lines

drivers/media/platform/omap3isp/ispccp2.c

Quoted 13 lines

/**

+ * struct v4l2_of_bus_csi1 - CSI-1/CCP2 data bus structure
+ * @clock_inv: polarity of clock/strobe signal
+ * false - not inverted, true - inverted
+ * @strobe: false - data/clock, true - data/strobe
+ * @data_lane: the number of the data lane
+ * @clock_lane: the number of the clock lane
+ */

drivers/media/platform/omap3isp/ispccp2.c +4 -0

+struct v4l2_of_bus_mipi_csi1 {
+	bool clock_inv;
+	bool strobe;
+	bool lane_polarity[2];

This field isn't documented.

Yep, automatic checker already told me. Plus, similar field elsewhere is called "lane_polarities" but I believe "polarity" is a better name.

Code in ispcsiphy is quite confusing, and does not initialize phy1 in case of isp rev. 2. Set it to zero, to prevent confusion.

Signed-off-by Pavel Machek <[email protected]>

index 8f73f6d..a2474b6 100644

drivers/media/platform/omap3isp/ispcsiphy.c +0 -0

@@ -362,14 +374,16 @@ int omap3isp_csiphy_init(struct isp_device *isp)
 	phy2->phy_regs = OMAP3_ISP_IOMEM_CSIPHY2;
 	mutex_init(&phy2->mutex);
 

drivers/media/platform/omap3isp/ispcsiphy.c +6 -0

 	}
 
+	phy1->isp = isp;
+	phy1->csi2 = &isp->isp_csi2c;
+	phy1->num_data_lanes = ISP_CSIPHY1_NUM_DATA_LANES;
+	phy1->cfg_regs = OMAP3_ISP_IOMEM_CSI2C_REGS1;
+	phy1->phy_regs = OMAP3_ISP_IOMEM_CSIPHY1;
+	mutex_init(&phy1->mutex);
 	return 0;
 }

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.10 next-20170303]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

Hi!

Quoted 4 lines

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.10 next-20170303]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

Yes, the patch is against Sakari's ccp2 branch. It should work ok there.

I don't think you can do much to fix the automated system....

Pavel

Signature · 9 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html



_______________________________________________
kbuild-all mailing list
[email protected]
https://lists.01.org/mailman/listinfo/kbuild-all

ISP CSI1 module needs all the bits correctly set to work.

Signed-off-by Ivaylo Dimitrov <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

Quoted 5 lines · Pavel Machek

ISP CSI1 module needs all the bits correctly set to work.

Signed-off-by Ivaylo Dimitrov <[email protected]>

Signed-off-by Pavel Machek <[email protected]>

How are you sending the patches?

I've applied this to the ccp2 branch.

Quoted 13 lines

Quoted 12 lines

drivers/media/platform/omap3isp/isp.c static int isp_of_parse_node(struct device *dev, struct device_node *node, · line 2153

Quoted 10 lines

drivers/media/platform/omap3isp/isp.c +10 -0

+
+static int isp_of_parse_node_endpoint(struct device *dev,
+				      struct device_node *node,
+				      struct isp_async_subdev *isd)
+{
+	struct isp_bus_cfg *buscfg;
+	struct v4l2_of_endpoint vep;
+	int ret;
+
+	isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);

Why do you now need to allocate this manually ?

Quoted 1 line

bus is now a pointer.

I've seen that, but why have you changed it ?

Quoted 11 lines

Quoted 10 lines

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device · line 167

Quoted 7 lines

+++ b/drivers/media/platform/omap3isp/ispccp2.c

drivers/media/platform/omap3isp/ispccp2.c +1 -0

@@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device
 *ccp2, u8 enable) return ret;
 
 	}
 
+	if (isp->revision == ISP_REVISION_2_0) {

The isp_csiphy.c code checks phy->isp->phy_type for the same purpose, shouldn't you use that too ?

Quoted 2 lines

Do you want me to do phy->isp->phy_type == ISP_PHY_TYPE_3430 check here? Can do...

Yes that's what I meant.

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

Review static int ccp2_if_enable(struct isp_ccp2_device · line 167

Quoted 7 lines

+++ b/drivers/media/platform/omap3isp/ispccp2.c

(quoted diff) +1 -0

@@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device
 *ccp2, u8 enable) return ret;
 
 	}
 
+	if (isp->revision == ISP_REVISION_2_0) {

The isp_csiphy.c code checks phy->isp->phy_type for the same purpose, shouldn't you use that too ?

Do you want me to do phy->isp->phy_type == ISP_PHY_TYPE_3430 check here? Can do...

Quoted 1 line

Yes that's what I meant.

Ok, that's something I can do.

But code is still somewhat "interesting". Code in omap3isp_csiphy_acquire() assumes csi2, and I don't need most of it.. so I'll just not use it, but it looks strange. I'll post new patch shortly.

Hi!

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

Review static int ccp2_if_enable(struct isp_ccp2_device · line 167

Quoted 7 lines

+++ b/drivers/media/platform/omap3isp/ispccp2.c

(quoted diff) +1 -0

@@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device
 *ccp2, u8 enable) return ret;
 
 	}
 
+	if (isp->revision == ISP_REVISION_2_0) {

The isp_csiphy.c code checks phy->isp->phy_type for the same purpose, shouldn't you use that too ?

Do you want me to do phy->isp->phy_type == ISP_PHY_TYPE_3430 check here? Can do...

Quoted 7 lines

Quoted 1 line

Yes that's what I meant.

Ok, that's something I can do.

But code is still somewhat "interesting". Code in omap3isp_csiphy_acquire() assumes csi2, and I don't need most of it.. so I'll just not use it, but it looks strange. I'll post new patch shortly.

Ok, how about this one?

Pavel

omap3isp: add rest of CSI1 support

CSI1 needs one more bit to be set up. Do just that.

It is not as straightforward as I'd like, see the comments in the code for explanation.

Signed-off-by Pavel Machek <[email protected]>

index ca09523..b6e055e 100644

drivers/media/platform/omap3isp/ispccp2.c +5 -0

@@ -21,6 +23,7 @@
 #include <linux/mutex.h>
 #include <linux/uaccess.h>
 #include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
 
 #include "isp.h"
 #include "ispreg.h"

@@ -160,6 +163,22 @@ static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable)
 			return ret;
 	}
 
+	if (isp->phy_type == ISP_PHY_TYPE_3430) {
+		struct media_pad *pad;
+		struct v4l2_subdev *sensor;
+		const struct isp_ccp2_cfg *buscfg;

drivers/media/platform/omap3isp/ispccp2.c +1 -0

+		 */
 	} else if (isp->revision == ISP_REVISION_15_0) {
 		ccp2->phy = &isp->isp_csiphy1;
 	}

drivers/media/platform/omap3isp/ispcsiphy.c +6 -3

index 871d4fe..897097b 100644
@@ -68,8 +68,8 @@ static void csiphy_routing_cfg_3630(struct isp_csiphy *phy,
 	regmap_write(phy->isp->syscon, phy->isp->syscon_offset, reg);
 }
 
-static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
-				    bool ccp2_strobe)
+void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
+			     bool ccp2_strobe, bool strobe_clk_pol)
 {
 	u32 csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
 		| OMAP343X_CONTROL_CSIRXFE_RESET;

@@ -85,6 +85,9 @@ static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
 
 	if (ccp2_strobe)
 		csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
+	
+	if (strobe_clk_pol)
+		csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
 
 	regmap_write(phy->isp->syscon, phy->isp->syscon_offset, csirxfe);
 }

@@ -108,7 +111,7 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
 	if (phy->isp->phy_type == ISP_PHY_TYPE_3630 && on)
 		return csiphy_routing_cfg_3630(phy, iface, ccp2_strobe);
 	if (phy->isp->phy_type == ISP_PHY_TYPE_3430)
-		return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe);
+		return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe, false);
 }
 
 /*

drivers/media/platform/omap3isp/ispcsiphy.h +3 -0

index 28b63b2..88c5c1e8 100644
@@ -40,4 +40,7 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
 void omap3isp_csiphy_release(struct isp_csiphy *phy);
 int omap3isp_csiphy_init(struct isp_device *isp);
 
+void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
+				    bool ccp2_strobe, bool strobe_clk_pol);
+
 #endif	/* OMAP3_ISP_CSI_PHY_H */

Quoted 7 lines

Ok, how about this one?
omap3isp: add rest of CSI1 support

CSI1 needs one more bit to be set up. Do just that.

It is not as straightforward as I'd like, see the comments in the code for explanation.

Quoted 9 lines

drivers/media/platform/omap3isp/ispccp2.c +9 -0

+	if (isp->phy_type == ISP_PHY_TYPE_3430) {
+		struct media_pad *pad;
+		struct v4l2_subdev *sensor;
+		const struct isp_ccp2_cfg *buscfg;
+
+		pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
+		sensor = media_entity_to_v4l2_subdev(pad->entity);
+		/* Struct isp_bus_cfg has union inside */
+		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable) · line 179

Quoted 5 lines

drivers/media/platform/omap3isp/ispccp2.c +5 -0

+
+		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
+					ISP_INTERFACE_CCP2B_PHY1,
+					enable, !!buscfg->phy_layer,
+					buscfg->strobe_clk_pol);

You should do this through omap3isp_csiphy_acquire(), and not call csiphy_routing_cfg_3430() directly from here.

drivers/media/platform/omap3isp/ispccp2.c int omap3isp_ccp2_init(struct isp_device *isp) · line 1163

Quoted 6 lines

drivers/media/platform/omap3isp/ispccp2.c +2 -0

@@ -1137,10 +1159,19 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 	if (isp->revision == ISP_REVISION_2_0) {
 		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
 		if (IS_ERR(ccp2->vdds_csib)) {
+			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;

This should go to a separate patch.

Quoted 4 lines

drivers/media/platform/omap3isp/ispccp2.c +0 -0

 			dev_dbg(isp->dev,
 				"Could not get regulator vdds_csib\n");
 			ccp2->vdds_csib = NULL;
 		}

drivers/media/platform/omap3isp/ispccp2.c int omap3isp_ccp2_init(struct isp_device *isp) · line 1173

Quoted 6 lines

drivers/media/platform/omap3isp/ispccp2.c +6 -0

+		/*
+		 * If we set up ccp2->phy here,
+		 * omap3isp_csiphy_acquire() will go ahead and assume
+		 * csi2, dereferencing some null pointers.
+		 *
+		 * ccp2->phy = &isp->isp_csiphy2;

That needs to be fixed separately.

Hi!

Quoted 7 lines

Quoted 7 lines

Ok, how about this one?
omap3isp: add rest of CSI1 support

CSI1 needs one more bit to be set up. Do just that.

It is not as straightforward as I'd like, see the comments in the code for explanation.

...

Quoted 9 lines

Quoted 9 lines

drivers/media/platform/omap3isp/ispccp2.c +9 -0

+	if (isp->phy_type == ISP_PHY_TYPE_3430) {
+		struct media_pad *pad;
+		struct v4l2_subdev *sensor;
+		const struct isp_ccp2_cfg *buscfg;
+
+		pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
+		sensor = media_entity_to_v4l2_subdev(pad->entity);
+		/* Struct isp_bus_cfg has union inside */
+		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;

Quoted 8 lines

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable) · line 179

Quoted 5 lines

drivers/media/platform/omap3isp/ispccp2.c +5 -0

+
+		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
+					ISP_INTERFACE_CCP2B_PHY1,
+					enable, !!buscfg->phy_layer,
+					buscfg->strobe_clk_pol);

You should do this through omap3isp_csiphy_acquire(), and not call csiphy_routing_cfg_3430() directly from here.

Well, unfortunately omap3isp_csiphy_acquire() does have csi2 assumptions hard-coded :-(.

This will probably fail.

	        rval = omap3isp_csi2_reset(phy->csi2);
	        if (rval < 0)
		                goto done;

And this will oops:

static int omap3isp_csiphy_config(struct isp_csiphy *phy)

{
	struct isp_csi2_device *csi2 = phy->csi2;
        struct isp_pipeline *pipe = to_isp_pipeline(&csi2->subdev.entity);

struct isp_bus_cfg *buscfg = pipe->external->host_priv;

Quoted 9 lines

drivers/media/platform/omap3isp/ispccp2.c int omap3isp_ccp2_init(struct isp_device *isp) · line 1163

Quoted 7 lines

drivers/media/platform/omap3isp/ispccp2.c +2 -0

@@ -1137,10 +1159,19 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 	if (isp->revision == ISP_REVISION_2_0) {
 		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
 		if (IS_ERR(ccp2->vdds_csib)) {
+			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;

This should go to a separate patch.

Ok, easy enough.

Quoted 4 lines

Quoted 4 lines

drivers/media/platform/omap3isp/ispccp2.c +0 -0

 			dev_dbg(isp->dev,
 				"Could not get regulator vdds_csib\n");
 			ccp2->vdds_csib = NULL;
 		}

Quoted 8 lines

drivers/media/platform/omap3isp/ispccp2.c int omap3isp_ccp2_init(struct isp_device *isp) · line 1173

Quoted 6 lines

drivers/media/platform/omap3isp/ispccp2.c +6 -0

+		/*
+		 * If we set up ccp2->phy here,
+		 * omap3isp_csiphy_acquire() will go ahead and assume
+		 * csi2, dereferencing some null pointers.
+		 *
+		 * ccp2->phy = &isp->isp_csiphy2;

That needs to be fixed separately.

See analysis above. Yes, it would be nice to fix it. Can you provide some hints how to do that? Maybe even patch to test? :-).

Hi!

Quoted 7 lines

Quoted 7 lines

Ok, how about this one?
omap3isp: add rest of CSI1 support

CSI1 needs one more bit to be set up. Do just that.

It is not as straightforward as I'd like, see the comments in the code for explanation.

...

Quoted 9 lines

Quoted 9 lines

drivers/media/platform/omap3isp/ispccp2.c +9 -0

+	if (isp->phy_type == ISP_PHY_TYPE_3430) {
+		struct media_pad *pad;
+		struct v4l2_subdev *sensor;
+		const struct isp_ccp2_cfg *buscfg;
+
+		pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
+		sensor = media_entity_to_v4l2_subdev(pad->entity);
+		/* Struct isp_bus_cfg has union inside */
+		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;

Quoted 8 lines

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable) · line 179

Quoted 5 lines

drivers/media/platform/omap3isp/ispccp2.c +5 -0

+
+		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
+					ISP_INTERFACE_CCP2B_PHY1,
+					enable, !!buscfg->phy_layer,
+					buscfg->strobe_clk_pol);

You should do this through omap3isp_csiphy_acquire(), and not call csiphy_routing_cfg_3430() directly from here.

Well, unfortunately omap3isp_csiphy_acquire() does have csi2 assumptions hard-coded :-(.

This will probably fail.

	        rval = omap3isp_csi2_reset(phy->csi2);
	        if (rval < 0)
		                goto done;

Hi Pavel,

Quoted 33 lines · Pavel Machek

Hi!

Quoted 7 lines

Quoted 7 lines

Ok, how about this one?
omap3isp: add rest of CSI1 support

CSI1 needs one more bit to be set up. Do just that.

It is not as straightforward as I'd like, see the comments in the code for explanation.

...

Quoted 9 lines

Quoted 9 lines

drivers/media/platform/omap3isp/ispccp2.c +9 -0

+	if (isp->phy_type == ISP_PHY_TYPE_3430) {
+		struct media_pad *pad;
+		struct v4l2_subdev *sensor;
+		const struct isp_ccp2_cfg *buscfg;
+
+		pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
+		sensor = media_entity_to_v4l2_subdev(pad->entity);
+		/* Struct isp_bus_cfg has union inside */
+		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;

Quoted 8 lines

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable) · line 179

Quoted 5 lines

drivers/media/platform/omap3isp/ispccp2.c +5 -0

+
+		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
+					ISP_INTERFACE_CCP2B_PHY1,
+					enable, !!buscfg->phy_layer,
+					buscfg->strobe_clk_pol);

You should do this through omap3isp_csiphy_acquire(), and not call csiphy_routing_cfg_3430() directly from here.

Well, unfortunately omap3isp_csiphy_acquire() does have csi2 assumptions hard-coded :-(.

This will probably fail.

Quoted 3 lines

	        rval = omap3isp_csi2_reset(phy->csi2);
	        if (rval < 0)
		                goto done;

Could you try to two patches I've applied on the ccp2 branch (I'll remove them if there are issues).

That's compile tested for now only.

Quoted 44 lines · Sakari Ailus

Quoted 31 lines · Pavel Machek

Quoted 8 lines

Quoted 8 lines

Ok, how about this one?
omap3isp: add rest of CSI1 support

CSI1 needs one more bit to be set up. Do just that.

It is not as straightforward as I'd like, see the comments in the code for explanation.

...

Quoted 11 lines

Quoted 11 lines

drivers/media/platform/omap3isp/ispccp2.c +9 -2

+	if (isp->phy_type == ISP_PHY_TYPE_3430) {
+		struct media_pad *pad;
+		struct v4l2_subdev *sensor;
+		const struct isp_ccp2_cfg *buscfg;
+
+		pad = media_entity_remote_pad(&ccp2
->pads[CCP2_PAD_SINK]);
+		sensor = media_entity_to_v4l2_subdev(pad->entity);
+		/* Struct isp_bus_cfg has union inside */
+		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)
->bus.ccp2;

Quoted 8 lines

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable) · line 179

Quoted 5 lines

drivers/media/platform/omap3isp/ispccp2.c +5 -0

+
+		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
+					ISP_INTERFACE_CCP2B_PHY1,
+					enable, !!buscfg->phy_layer,
+					buscfg->strobe_clk_pol);

You should do this through omap3isp_csiphy_acquire(), and not call csiphy_routing_cfg_3430() directly from here.

Quoted 8 lines

Well, unfortunately omap3isp_csiphy_acquire() does have csi2 assumptions hard-coded :-(.

This will probably fail.

	        rval = omap3isp_csi2_reset(phy->csi2);
	        if (rval < 0)
		                goto done;

Could you try to two patches I've applied on the ccp2 branch (I'll remove them if there are issues).

Quoted 1 line

That's compile tested for now only.

One more thing. What's needed for configuring the PHY for CCP2?

For instance, is the CSI-2 PHY regulator still needed in omap3isp_csiphy_acquire()? One way to do this might go to see the original driver for N900; I don't have the TRM at hand right now.

Hi Sakari,

Quoted 45 lines · On Saturday 04 Mar 2017 17:39:46 Sakari Ailus wrote:

Quoted 44 lines · Sakari Ailus

Quoted 31 lines · Pavel Machek

Quoted 8 lines

Quoted 8 lines

Ok, how about this one?
omap3isp: add rest of CSI1 support

CSI1 needs one more bit to be set up. Do just that.

It is not as straightforward as I'd like, see the comments in the code for explanation.

...

Quoted 11 lines

Quoted 11 lines

(quoted diff) +9 -2

+	if (isp->phy_type == ISP_PHY_TYPE_3430) {
+		struct media_pad *pad;
+		struct v4l2_subdev *sensor;
+		const struct isp_ccp2_cfg *buscfg;
+
+		pad = media_entity_remote_pad(&ccp2
->pads[CCP2_PAD_SINK]);
+		sensor = media_entity_to_v4l2_subdev(pad->entity);
+		/* Struct isp_bus_cfg has union inside */
+		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)
->bus.ccp2;

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+
+		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
+					ISP_INTERFACE_CCP2B_PHY1,
+					enable, !!buscfg->phy_layer,
+					buscfg->strobe_clk_pol);

You should do this through omap3isp_csiphy_acquire(), and not call csiphy_routing_cfg_3430() directly from here.

Quoted 8 lines

Well, unfortunately omap3isp_csiphy_acquire() does have csi2 assumptions hard-coded :-(.

This will probably fail.

	        rval = omap3isp_csi2_reset(phy->csi2);
	        if (rval < 0)
		                goto done;

Could you try to two patches I've applied on the ccp2 branch (I'll remove them if there are issues).

Quoted 7 lines

Quoted 1 line

That's compile tested for now only.

One more thing. What's needed for configuring the PHY for CCP2?

For instance, is the CSI-2 PHY regulator still needed in omap3isp_csiphy_acquire()? One way to do this might go to see the original driver for N900; I don't have the TRM at hand right now.

The OMAP34xx TRM and data manual both mention separate VDDS power supplies for the CSIb and CSI2 I/O complexes.

vdds_csi2 CSI2 Complex I/O
vdds_csib CSIb Complex I/O

On OMAP36xx, we instead have

vdda_csiphy1 Input power for camera PHY buffer
vdda_csiphy2 Input power for camera PHY buffer

We need to enable the vds_csib regulator to operate the CSI1/CCP2 PHY, but that regulator gets enabled in ispccp2.c as that module is powered by the vdds_csib supply on OMAP34xx. However, it won't hurt to do so, and the code could be simpler if we manage the regulators the same way on OMAP34xx and OMAP36xx.

Hi!

Currently, ispvideo.c does not support enum_format. This causes problems for example for libv4l2.

Now, I'm pretty sure patch below is not the right fix. But it fixes libv4l2 problem for me.

Pointer to right solution welcome.

Quoted 23 lines

Quoted 9 lines

Quoted 9 lines

drivers/media/platform/omap3isp/ispccp2.c +9 -0

+	if (isp->phy_type == ISP_PHY_TYPE_3430) {
+		struct media_pad *pad;
+		struct v4l2_subdev *sensor;
+		const struct isp_ccp2_cfg *buscfg;
+
+		pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
+		sensor = media_entity_to_v4l2_subdev(pad->entity);
+		/* Struct isp_bus_cfg has union inside */
+		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;

Quoted 8 lines

drivers/media/platform/omap3isp/ispccp2.c static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable) · line 179

Quoted 5 lines

drivers/media/platform/omap3isp/ispccp2.c +5 -0

+
+		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
+					ISP_INTERFACE_CCP2B_PHY1,
+					enable, !!buscfg->phy_layer,
+					buscfg->strobe_clk_pol);

You should do this through omap3isp_csiphy_acquire(), and not call csiphy_routing_cfg_3430() directly from here.

Well, unfortunately omap3isp_csiphy_acquire() does have csi2 assumptions hard-coded :-(.

This will probably fail.

Quoted 3 lines

	        rval = omap3isp_csi2_reset(phy->csi2);
	        if (rval < 0)
		                goto done;

Could you try to two patches I've applied on the ccp2 branch (I'll remove them if there are issues).

That's compile tested for now only.

Quoted 5 lines

This will probably fail.

	        rval = omap3isp_csi2_reset(phy->csi2);
	        if (rval < 0)
		                goto done;

Could you try to two patches I've applied on the ccp2 branch (I'll remove them if there are issues).

That's compile tested for now only.

Fix clock lane parsing in v4l2-fwnode.

Signed-off-by Pavel Machek <[email protected]>

drivers/media/v4l2-core/v4l2-fwnode.c +0 -0

index d6666d3..44036b8 100644

Hi!

Are there any news about the fwnode branch?

I have quite usable camera, but it is still based on 982e8e40390d26430ef106fede41594139a4111c (that's v4.10). It would be good to see fwnode stuff upstream... are there any plans for that?

Is there stable branch to which I could move the stuff?

Quoted 7 lines

Are there any news about the fwnode branch?

I have quite usable camera, but it is still based on 982e8e40390d26430ef106fede41594139a4111c (that's v4.10). It would be good to see fwnode stuff upstream... are there any plans for that?

Is there stable branch to which I could move the stuff?

What's relevant for most V4L2 drivers is in linux-media right now.

There are new features that will take some time to get in. The trouble has been, and continue to be, that the patches need to go through various trees so it'll take some time for them to be merged.

I expect to have most of them in during the next merge window.

Hi!

Quoted 10 lines

Quoted 7 lines

Are there any news about the fwnode branch?

I have quite usable camera, but it is still based on 982e8e40390d26430ef106fede41594139a4111c (that's v4.10). It would be good to see fwnode stuff upstream... are there any plans for that?

Is there stable branch to which I could move the stuff?

What's relevant for most V4L2 drivers is in linux-media right now.

Quoted 5 lines

There are new features that will take some time to get in. The trouble has been, and continue to be, that the patches need to go through various trees so it'll take some time for them to be merged.

I expect to have most of them in during the next merge window.

So git://linuxtv.org/media_tree.git branch master is the right one to work one?

Quoted 16 lines

Quoted 15 lines

Quoted 7 lines

Are there any news about the fwnode branch?

I have quite usable camera, but it is still based on 982e8e40390d26430ef106fede41594139a4111c (that's v4.10). It would be good to see fwnode stuff upstream... are there any plans for that?

Is there stable branch to which I could move the stuff?

What's relevant for most V4L2 drivers is in linux-media right now.

There are new features that will take some time to get in. The trouble has been, and continue to be, that the patches need to go through various trees so it'll take some time for them to be merged.

I expect to have most of them in during the next merge window.

Quoted 2 lines

So git://linuxtv.org/media_tree.git branch master is the right one to work one?

I also pushed the rebased ccp2 branch there:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=ccp2>

It's now right on the top of media-tree master.