[PATCH] dt: bindings: Add support for CSI1 bus

[PATCH] dt: bindings: Add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]

From: Sakari Ailus <sakari.ailus@iki.fi>

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 <sakari.ailus@iki.fi>
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index 9cd2a36..f0523f7 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -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
 - 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
 -------


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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] dt: bindings: Add support for CSI1 bus

[Sakari Ailus]

Hi Pavel,

On Wed, Dec 28, 2016 at 07:30:36PM +0100, Pavel Machek wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> 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 <sakari.ailus@iki.fi>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 9cd2a36..f0523f7 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -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?

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



-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH] dt: bindings: Add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2091 bytes --]

On Mon 2017-01-02 09:00:10, Sakari Ailus wrote:
> Hi Pavel,
> 
> On Wed, Dec 28, 2016 at 07:30:36PM +0100, Pavel Machek wrote:
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> > 
> > 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 <sakari.ailus@iki.fi>
> > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > index 9cd2a36..f0523f7 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -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?

Me, think..? Nah. :-).

Well, you have way more experience here, and yes, based on future
patches, this makes sense.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] dt: bindings: Add support for CSI1 bus

[Rob Herring]

On Wed, Dec 28, 2016 at 07:30:36PM +0100, Pavel Machek wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> 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?

> 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 <sakari.ailus@iki.fi>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 9cd2a36..f0523f7 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -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?

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

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

> +- strobe: Whether the clock signal is used as clock or strobe. Used
> +  with CCP2, for instance.
>  
>  Example
>  -------
> 
> 
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] dt: bindings: Add support for CSI1 bus

[Sakari Ailus]

Hi Rob,

Thanks for the review.

On Tue, Jan 03, 2017 at 02:38:54PM -0600, Rob Herring wrote:
> On Wed, Dec 28, 2016 at 07:30:36PM +0100, Pavel Machek wrote:
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> > 
> > 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?

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 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 <sakari.ailus@iki.fi>
> > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > index 9cd2a36..f0523f7 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -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?

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

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

Must have been by mistake. :-)

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

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

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH] dt: bindings: Add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2471 bytes --]

Hi!

> Thanks for the review.
> 
> On Tue, Jan 03, 2017 at 02:38:54PM -0600, Rob Herring wrote:
> > On Wed, Dec 28, 2016 at 07:30:36PM +0100, Pavel Machek wrote:
> > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > > 
> > > 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?
> 
> 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.

Ok, I'll include that in the changelog.

> > As in MIPI CSI2?
> 
> Yeah, I guess it'd make sense to make this explicit.

Ok.

> > >    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?
> 
> Must have been by mistake. :-)

Fixed.

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

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

Device tree changes are PITA :-(.
									Pavel

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] dt: bindings: Add support for CSI1 bus

[Sakari Ailus]

Hi Pavel,

On Wed, Jan 11, 2017 at 11:06:48PM +0100, Pavel Machek wrote:
> > > > +- 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".
> 
> Hmm. N900 does not use inversion. Would it make sense to simply
> hardcode it to "not-inverted" for now?
> 
> Device tree changes are PITA :-(.

I can't remember what's used for the N9 secondary camera. As you said, we
can always add it if needed though. So works for me.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCHv2] dt: bindings: Add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]

From: Sakari Ailus <sakari.ailus@iki.fi>

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 <sakari.ailus@iki.fi>
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index 9cd2a36..08c4498 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -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
@@ -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
 -------


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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCHv2] dt: bindings: Add support for CSI1 bus

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2366 bytes --]

Hi,

On Wed, Jan 11, 2017 at 11:53:35PM +0100, Pavel Machek wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> 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 <sakari.ailus@iki.fi>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 9cd2a36..08c4498 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -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
> @@ -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
>  -------
> 
> 

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCHv2] dt: bindings: Add support for CSI1 bus

[Baruch Siach]

Hi Pavel,

On Wed, Jan 11, 2017 at 11:53:35PM +0100, Pavel Machek wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> 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 <sakari.ailus@iki.fi>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 9cd2a36..08c4498 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -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?

baruch

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCHv2] dt: bindings: Add support for CSI1 bus

[Sakari Ailus]

Hi Baruch,

On Thu, Jan 12, 2017 at 02:06:03PM +0200, Baruch Siach wrote:
> Hi Pavel,
> 
> On Wed, Jan 11, 2017 at 11:53:35PM +0100, Pavel Machek wrote:
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> > 
> > 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 <sakari.ailus@iki.fi>
> > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > index 9cd2a36..08c4498 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -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?

The current implementation of V4L2 OF support digs the information from
other properties (hsync-active, vsync-active and field-even-active). If any
of them are present, the bus is considered to be a regular parallel bus ---
the Bt.656 has no such signals.

CSI-2 bus is assumed if CSI-2 specific properties can be found. However,
explicit bus type is needed as the type of the bus isn't anymore implicitly
determinable with the addition of CSI-1 and CCP2 busses: they use the same
properties.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCHv2] dt: bindings: Add support for CSI1 bus

[Sakari Ailus]

Hi Pavel,

On Wed, Jan 11, 2017 at 11:53:35PM +0100, Pavel Machek wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> 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 <sakari.ailus@iki.fi>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 9cd2a36..08c4498 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -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.

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

-- 
Cheers,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCHv2] dt: bindings: Add support for CSI1 bus

[Ivaylo Dimitrov]

Hi,

On 19.01.2017 23:49, Sakari Ailus wrote:
> Hi Pavel,
>
> On Wed, Jan 11, 2017 at 11:53:35PM +0100, Pavel Machek wrote:
>> From: Sakari Ailus <sakari.ailus@iki.fi>
>>
>> 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 <sakari.ailus@iki.fi>
>> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
>> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>>
>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
>> index 9cd2a36..08c4498 100644
>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
>> @@ -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.
>
>>  - 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.

Ivo

Re: [PATCHv2] dt: bindings: Add support for CSI1 bus

[Sakari Ailus]

Hi Ivaylo,

On Fri, Jan 20, 2017 at 09:59:13AM +0200, Ivaylo Dimitrov wrote:
> Hi,
> 
> On 19.01.2017 23:49, Sakari Ailus wrote:
> >Hi Pavel,
> >
> >On Wed, Jan 11, 2017 at 11:53:35PM +0100, Pavel Machek wrote:
> >>From: Sakari Ailus <sakari.ailus@iki.fi>
> >>
> >>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 <sakari.ailus@iki.fi>
> >>Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> >>Signed-off-by: Pavel Machek <pavel@ucw.cz>
> >>
> >>diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>index 9cd2a36..08c4498 100644
> >>--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>@@ -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.
> >
> >> - 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.

Oh, indeed, my mistake. Please ignore that comment.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCHv2] dt: bindings: Add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]

Hi!

> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -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.

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

Best regards,
								Pavel

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index 08c4498..d54093b 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -77,10 +77,10 @@ Optional endpoint properties
   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
+  0 - autodetect based on other properties (MIPI CSI2, parallel, Bt656)
+  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


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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCHv2] dt: bindings: Add support for CSI1 bus

[Sakari Ailus]

Hi Pavel,

On Fri, Feb 03, 2017 at 12:50:45PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > @@ -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.
> 
> Hmm. "Just deleting the others" may be a bit confusing... but what
> about this? It explains what we can autodetect.
> 
> Best regards,
> 								Pavel
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 08c4498..d54093b 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -77,10 +77,10 @@ Optional endpoint properties
>    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
> +  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...

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


-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCHv2] dt: bindings: Add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]

Hi!

> >  - bus-type: data bus type. Possible values are:
> > -  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...

Ok, v3 is in the mail. 

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCHv2] dt: bindings: Add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2465 bytes --]

From: Sakari Ailus <sakari.ailus@iki.fi>

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 <sakari.ailus@iki.fi>
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>
Reviewed-By: Sebastian Reichel <sre@kernel.org>

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index 9cd2a36..6986fde 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -76,6 +76,12 @@ 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 - autodetect based on other properties (MIPI CSI-2 D-PHY, parallel or Bt656)
+  1 - MIPI CSI-2 C-PHY
+  2 - MIPI CSI1
+  3 - 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
@@ -112,7 +118,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
 -------

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCHv2] dt: bindings: Add support for CSI1 bus

[Sakari Ailus]

Hi Pavel,

On Mon, Feb 06, 2017 at 10:49:57AM +0100, Pavel Machek wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> 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.

Thanks. I split this into two (bus type + strobe). The result is in here:

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

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.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH] omap3isp: add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 13583 bytes --]


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 <sakari.ailus@iki.fi>
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

> 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

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 0321d84..88bc7c6 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2024,21 +2024,92 @@ enum isp_of_phy {
 	ISP_OF_PHY_CSIPHY2,
 };
 
-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)
+{
+	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",
+		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];
+	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;
 	unsigned int i;
-	int ret;
 
-	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;
+	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:
-		/* FIXME: always assume CSI-2 for now. */
-		switch (vep.base.port) {
-		case ISP_OF_PHY_CSIPHY1:
-			buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
-			break;
-		case ISP_OF_PHY_CSIPHY2:
-			buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
-			break;
-		}
-		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);
-		}
-
-		/*
-		 * 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;
+		if (vep.bus_type == V4L2_MBUS_CSI2)
+			isp_of_parse_node_csi2(dev, buscfg, &vep);
+		else
+			isp_of_parse_node_csi1(dev, buscfg, &vep);
 		break;
 
 	default:
+		return -1;
+	}
+	return 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);
+	if (!isd->bus)
+		return -ENOMEM;
+
+	buscfg = isd->bus;
+
+	ret = v4l2_of_parse_endpoint(node, &vep);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
+		vep.base.port);
+
+	if (isp_endpoint_to_buscfg(dev, vep, buscfg))
 		dev_warn(dev, "%s: invalid interface %u\n", node->full_name,
 			 vep.base.port);
-		break;
-	}
 
 	return 0;
 }
@@ -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;
 		}
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 7e6f663..c0b9d1d 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -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;
 };
 
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index ca09523..4edb55a 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -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"
@@ -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) {
+		struct media_pad *pad;
+		struct v4l2_subdev *sensor;
+		const struct isp_ccp2_cfg *buscfg;
+		u32 csirxfe;
+
+		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;
+
+
+		if (enable) {
+			csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
+				  OMAP343X_CONTROL_CSIRXFE_RESET;
+
+			if (buscfg->phy_layer)
+				csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
+
+			if (buscfg->strobe_clk_pol)
+				csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
+		} else
+			csirxfe = 0;
+
+		regmap_write(isp->syscon, isp->syscon_offset, csirxfe);
+	}
+
 	/* Enable/Disable all the LCx channels */
 	for (i = 0; i < CCP2_LCx_CHANS_NUM; i++)
 		isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_LCx_CTRL(i),
@@ -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);
@@ -339,6 +372,9 @@ static void ccp2_lcx_config(struct isp_ccp2_device *ccp2,
 	isp_reg_set(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_LC01_IRQENABLE, val);
 }
 
+void __isp_of_parse_node_csi1(struct device *dev,
+			      struct isp_ccp2_cfg *buscfg,
+			      struct v4l2_of_endpoint *vep);
 /*
  * ccp2_if_configure - Configure ccp2 with data from sensor
  * @ccp2: Pointer to ISP CCP2 device
@@ -1137,6 +1173,8 @@ 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;
 			dev_dbg(isp->dev,
 				"Could not get regulator vdds_csib\n");
 			ccp2->vdds_csib = NULL;
diff --git a/drivers/media/platform/omap3isp/ispreg.h b/drivers/media/platform/omap3isp/ispreg.h
index b5ea8da..d084839 100644
--- a/drivers/media/platform/omap3isp/ispreg.h
+++ b/drivers/media/platform/omap3isp/ispreg.h
@@ -87,6 +87,8 @@
 #define ISPCCP2_CTRL_PHY_SEL_MASK	0x1
 #define ISPCCP2_CTRL_PHY_SEL_SHIFT	1
 #define ISPCCP2_CTRL_IO_OUT_SEL		(1 << 2)
+#define ISPCCP2_CTRL_IO_OUT_SEL_MASK	0x1
+#define ISPCCP2_CTRL_IO_OUT_SEL_SHIFT	2
 #define ISPCCP2_CTRL_MODE		(1 << 4)
 #define ISPCCP2_CTRL_VP_CLK_FORCE_ON	(1 << 9)
 #define ISPCCP2_CTRL_INV		(1 << 10)
@@ -94,6 +96,8 @@
 #define ISPCCP2_CTRL_INV_SHIFT		10
 #define ISPCCP2_CTRL_VP_ONLY_EN		(1 << 11)
 #define ISPCCP2_CTRL_VP_CLK_POL		(1 << 12)
+#define ISPCCP2_CTRL_VP_CLK_POL_MASK	0x1
+#define ISPCCP2_CTRL_VP_CLK_POL_SHIFT	12
 #define ISPCCP2_CTRL_VPCLK_DIV_SHIFT	15
 #define ISPCCP2_CTRL_VPCLK_DIV_MASK	0x1ffff /* [31:15] */
 #define ISPCCP2_CTRL_VP_OUT_CTRL_SHIFT	8 /* 3430 bits */
diff --git a/drivers/media/platform/omap3isp/omap3isp.h b/drivers/media/platform/omap3isp/omap3isp.h
index 443e8f7..f6d1d0d 100644
--- a/drivers/media/platform/omap3isp/omap3isp.h
+++ b/drivers/media/platform/omap3isp/omap3isp.h
@@ -108,6 +108,7 @@ struct isp_ccp2_cfg {
 	unsigned int ccp2_mode:1;
 	unsigned int phy_layer:1;
 	unsigned int vpclk_div:2;
+	unsigned int vp_clk_pol:1;
 	struct isp_csiphy_lanes_cfg lanecfg;
 };
 
diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 34cc99e..315c167 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -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)
  * @V4L2_MBUS_CSI2:	MIPI CSI-2 serial interface
  */
 enum v4l2_mbus_type {
 	V4L2_MBUS_PARALLEL,
 	V4L2_MBUS_BT656,
+	V4L2_MBUS_CSI1,
+	V4L2_MBUS_CCP2,
 	V4L2_MBUS_CSI2,
 };
 
diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
index 4dc34b2..63a52ee 100644
--- a/include/media/v4l2-of.h
+++ b/include/media/v4l2-of.h
@@ -53,6 +53,22 @@ struct v4l2_of_bus_parallel {
 };
 
 /**
+ * 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
+ */
+struct v4l2_of_bus_mipi_csi1 {
+	bool clock_inv;
+	bool strobe;
+	bool lane_polarity[2];
+	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;

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] omap3isp: add support for CSI1 bus

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4532 bytes --]

Hi Pavel,

[auto build test WARNING on v4.9-rc8]
[cannot apply to linuxtv-media/master next-20170208]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Pavel-Machek/omap3isp-add-support-for-CSI1-bus/20170208-222249
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   make[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.
   include/linux/init.h:1: warning: no structured comments found
   include/linux/workqueue.h:392: warning: No description found for parameter '...'
   include/linux/workqueue.h:392: warning: Excess function parameter 'args' description in 'alloc_workqueue'
   include/linux/workqueue.h:413: warning: No description found for parameter '...'
   include/linux/workqueue.h:413: warning: Excess function parameter 'args' description in 'alloc_ordered_workqueue'
   include/linux/kthread.h:26: warning: No description found for parameter '...'
   kernel/sys.c:1: warning: no structured comments found
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
   include/linux/fence-array.h:61: warning: No description found for parameter 'fence'
   include/sound/core.h:324: warning: No description found for parameter '...'
   include/sound/core.h:335: warning: No description found for parameter '...'
   include/sound/core.h:388: warning: No description found for parameter '...'
   include/media/media-entity.h:1054: warning: No description found for parameter '...'
>> include/media/v4l2-of.h:69: warning: No description found for parameter 'lane_polarity[2]'
   include/net/mac80211.h:3207: ERROR: Unexpected indentation.
   include/net/mac80211.h:3210: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/net/mac80211.h:3212: ERROR: Unexpected indentation.
   include/net/mac80211.h:3213: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/net/mac80211.h:1772: ERROR: Unexpected indentation.
   include/net/mac80211.h:1776: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/sched/fair.c:7259: WARNING: Inline emphasis start-string without end-string.
   kernel/time/timer.c:1240: ERROR: Unexpected indentation.
   kernel/time/timer.c:1242: ERROR: Unexpected indentation.
   kernel/time/timer.c:1243: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:121: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:124: ERROR: Unexpected indentation.
   include/linux/wait.h:126: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/time/hrtimer.c:1021: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/signal.c:317: WARNING: Inline literal start-string without end-string.
   drivers/base/firmware_class.c:1348: WARNING: Bullet list ends without a blank line; unexpected unindent.
   drivers/message/fusion/mptbase.c:5054: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/tty/serial/serial_core.c:1893: WARNING: Definition list ends without a blank line; unexpected unindent.
   include/linux/spi/spi.h:369: ERROR: Unexpected indentation.
   WARNING: dvipng command 'dvipng' cannot be run (needed for math display), check the imgmath_dvipng setting

vim +69 include/media/v4l2-of.h

    53	};
    54	
    55	/**
    56	 * struct v4l2_of_bus_csi1 - CSI-1/CCP2 data bus structure
    57	 * @clock_inv: polarity of clock/strobe signal
    58	 *	       false - not inverted, true - inverted
    59	 * @strobe: false - data/clock, true - data/strobe
    60	 * @data_lane: the number of the data lane
    61	 * @clock_lane: the number of the clock lane
    62	 */
    63	struct v4l2_of_bus_mipi_csi1 {
    64		bool clock_inv;
    65		bool strobe;
    66		bool lane_polarity[2];
    67		unsigned char data_lane;
    68		unsigned char clock_lane;
  > 69	};
    70	
    71	/**
    72	 * struct v4l2_of_endpoint - the endpoint data structure
    73	 * @base: struct of_endpoint containing port, id, and local of_node
    74	 * @bus_type: bus type
    75	 * @bus: bus configuration data structure
    76	 * @link_frequencies: array of supported link frequencies
    77	 * @nr_of_link_frequencies: number of elements in link_frequenccies array

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6425 bytes --]

Re: [PATCH] omap3isp: add support for CSI1 bus

[Laurent Pinchart]

Hi Pavel,

Thank you for the patch.

On Wednesday 08 Feb 2017 13:57:38 Pavel Machek wrote:
> 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 <sakari.ailus@iki.fi>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> ---
> 
> > 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
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 0321d84..88bc7c6 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2024,21 +2024,92 @@ enum isp_of_phy {
>  	ISP_OF_PHY_CSIPHY2,
>  };
> 
> -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().

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

> +	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;
>  	unsigned int i;
> -	int ret;
> 
> -	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().

> +	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:
> -		/* FIXME: always assume CSI-2 for now. */
> -		switch (vep.base.port) {
> -		case ISP_OF_PHY_CSIPHY1:
> -			buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> -			break;
> -		case ISP_OF_PHY_CSIPHY2:
> -			buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> -			break;
> -		}
> -		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);
> -		}
> -
> -		/*
> -		 * 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;
> +		if (vep.bus_type == V4L2_MBUS_CSI2)
> +			isp_of_parse_node_csi2(dev, buscfg, &vep);
> +		else
> +			isp_of_parse_node_csi1(dev, buscfg, &vep);
>  		break;
> 
>  	default:
> +		return -1;

Please use the appropriate error code.

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

> +	if (!isd->bus)
> +		return -ENOMEM;
> +
> +	buscfg = isd->bus;
> +
> +	ret = (node, &vep);
> +	if (ret)
> +		return ret;
> +
> +	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.

>  		dev_warn(dev, "%s: invalid interface %u\n", node->full_name,
>  			 vep.base.port);
> -		break;
> -	}
> 
>  	return 0;
>  }
> @@ -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;
>  		}
> diff --git a/drivers/media/platform/omap3isp/isp.h
> b/drivers/media/platform/omap3isp/isp.h index 7e6f663..c0b9d1d 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -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;
>  };
> 
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4edb55a 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -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"
> @@ -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 ?

> +		struct media_pad *pad;
> +		struct v4l2_subdev *sensor;
> +		const struct isp_ccp2_cfg *buscfg;
> +		u32 csirxfe;
> +
> +		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;
> +
> +

One blank line is enough.

> +		if (enable) {
> +			csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
> +				  OMAP343X_CONTROL_CSIRXFE_RESET;
> +
> +			if (buscfg->phy_layer)
> +				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.

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

> +	}
> +
>  	/* Enable/Disable all the LCx channels */
>  	for (i = 0; i < CCP2_LCx_CHANS_NUM; i++)
>  		isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCP2, 
ISPCCP2_LCx_CTRL(i),
> @@ -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);
> @@ -339,6 +372,9 @@ static void ccp2_lcx_config(struct isp_ccp2_device
> *ccp2, isp_reg_set(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_LC01_IRQENABLE, val);
> }
> 
> +void __isp_of_parse_node_csi1(struct device *dev,
> +			      struct isp_ccp2_cfg *buscfg,
> +			      struct v4l2_of_endpoint *vep);

This isn't needed.

>  /*
>   * ccp2_if_configure - Configure ccp2 with data from sensor
>   * @ccp2: Pointer to ISP CCP2 device
> @@ -1137,6 +1173,8 @@ 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;
>  			dev_dbg(isp->dev,
>  				"Could not get regulator vdds_csib\n");
>  			ccp2->vdds_csib = NULL;
> diff --git a/drivers/media/platform/omap3isp/ispreg.h
> b/drivers/media/platform/omap3isp/ispreg.h index b5ea8da..d084839 100644
> --- a/drivers/media/platform/omap3isp/ispreg.h
> +++ b/drivers/media/platform/omap3isp/ispreg.h
> @@ -87,6 +87,8 @@
>  #define ISPCCP2_CTRL_PHY_SEL_MASK	0x1
>  #define ISPCCP2_CTRL_PHY_SEL_SHIFT	1
>  #define ISPCCP2_CTRL_IO_OUT_SEL		(1 << 2)
> +#define ISPCCP2_CTRL_IO_OUT_SEL_MASK	0x1
> +#define ISPCCP2_CTRL_IO_OUT_SEL_SHIFT	2
>  #define ISPCCP2_CTRL_MODE		(1 << 4)
>  #define ISPCCP2_CTRL_VP_CLK_FORCE_ON	(1 << 9)
>  #define ISPCCP2_CTRL_INV		(1 << 10)
> @@ -94,6 +96,8 @@
>  #define ISPCCP2_CTRL_INV_SHIFT		10
>  #define ISPCCP2_CTRL_VP_ONLY_EN		(1 << 11)
>  #define ISPCCP2_CTRL_VP_CLK_POL		(1 << 12)
> +#define ISPCCP2_CTRL_VP_CLK_POL_MASK	0x1
> +#define ISPCCP2_CTRL_VP_CLK_POL_SHIFT	12
>  #define ISPCCP2_CTRL_VPCLK_DIV_SHIFT	15
>  #define ISPCCP2_CTRL_VPCLK_DIV_MASK	0x1ffff /* [31:15] */
>  #define ISPCCP2_CTRL_VP_OUT_CTRL_SHIFT	8 /* 3430 bits */
> diff --git a/drivers/media/platform/omap3isp/omap3isp.h
> b/drivers/media/platform/omap3isp/omap3isp.h index 443e8f7..f6d1d0d 100644
> --- a/drivers/media/platform/omap3isp/omap3isp.h
> +++ b/drivers/media/platform/omap3isp/omap3isp.h
> @@ -108,6 +108,7 @@ struct isp_ccp2_cfg {
>  	unsigned int ccp2_mode:1;
>  	unsigned int phy_layer:1;
>  	unsigned int vpclk_div:2;
> +	unsigned int vp_clk_pol:1;
>  	struct isp_csiphy_lanes_cfg lanecfg;
>  };
> 
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 34cc99e..315c167 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h

You should split the V4L2 core changes to a separate patch.

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

>   * @V4L2_MBUS_CSI2:	MIPI CSI-2 serial interface
>   */
>  enum v4l2_mbus_type {
>  	V4L2_MBUS_PARALLEL,
>  	V4L2_MBUS_BT656,
> +	V4L2_MBUS_CSI1,
> +	V4L2_MBUS_CCP2,

That's nice, but v4l2_of_parse_endpoint() never sets the bus type to CSI1 or 
CCP2. You need to fix that (and update the related DT bindings).

>  	V4L2_MBUS_CSI2,
>  };
> 
> diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
> index 4dc34b2..63a52ee 100644
> --- a/include/media/v4l2-of.h
> +++ b/include/media/v4l2-of.h
> @@ -53,6 +53,22 @@ struct v4l2_of_bus_parallel {
>  };
> 
>  /**
> + * 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
> + */
> +struct v4l2_of_bus_mipi_csi1 {
> +	bool clock_inv;
> +	bool strobe;
> +	bool lane_polarity[2];

This field isn't documented.

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH] omap3isp: add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 5567 bytes --]

Hi!

> > diff --git a/drivers/media/platform/omap3isp/isp.c
> > b/drivers/media/platform/omap3isp/isp.c index 0321d84..88bc7c6 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -2024,21 +2024,92 @@ enum isp_of_phy {
> >  	ISP_OF_PHY_CSIPHY2,
> >  };
> > 
> > -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.

> > +{
> > +	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",
> > +		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.

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

> >  		break;
> > 
> >  	default:
> > +		return -1;
> 
> Please use the appropriate error code.

Ok.

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

bus is now a pointer.

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

> > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4edb55a 100644
> > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > @@ -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...

> > +		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> > +
> > +
> 
> One blank line is enough.

Ok.

> > +		if (enable) {
> > +			csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
> > +				  OMAP343X_CONTROL_CSIRXFE_RESET;
> > +
> > +			if (buscfg->phy_layer)
> > +				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.

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

> > @@ -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-mipi-csi1-and-mipi-csi2
> 

> >  /**
> > + * 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
> > + */
> > +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.

									Pavel

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] omap3isp: add support for CSI1 bus

[Laurent Pinchart]

Hi Pavel,

On Wednesday 15 Feb 2017 10:42:29 Pavel Machek wrote:
> Hi!
> 
> >> diff --git a/drivers/media/platform/omap3isp/isp.c
> >> b/drivers/media/platform/omap3isp/isp.c index 0321d84..88bc7c6 100644
> >> --- a/drivers/media/platform/omap3isp/isp.c
> >> +++ b/drivers/media/platform/omap3isp/isp.c
> >> @@ -2024,21 +2024,92 @@ enum isp_of_phy {
> >>  	ISP_OF_PHY_CSIPHY2,
> >>  };
> >> 
> >> -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.

Where will it be used ? As for the uglier part, I don't agree, otherwise I 
wouldn't have proposed it.

> >> +{
> >> +	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",
> >> +		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.
> 
> >> -	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.
>
> >>  		break;
> >>  	
> >>  	default:
> >> +		return -1;
> > 
> > Please use the appropriate error code.
> 
> Ok.
> 
> >> +	return 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 ?
> 
> bus is now a pointer.

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

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

This function became a kzalloc (which I still don't see why you need it), a 
call to v4l2_of_parse_endpoint(), and then isp_endpoint_to_buscfg(). That's 
too small to be a function of its own. Please merge 
isp_of_parse_node_endpoint() and isp_endpoint_to_buscfg().

> >> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> >> b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4edb55a
> >> 100644
> >> --- a/drivers/media/platform/omap3isp/ispccp2.c
> >> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> >> @@ -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...

Yes that's what I meant.

> >> +		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> >> +
> >> +
> > 
> > One blank line is enough.
> 
> Ok.
> 
> >> +		if (enable) {
> >> +			csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
> >> +				  OMAP343X_CONTROL_CSIRXFE_RESET;
> >> +
> >> +			if (buscfg->phy_layer)
> >> +				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.
> 
> >> +
> >> +		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...
> 
> >> @@ -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

> >>  /**
> >> 
> >> + * 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
> >> + */
> >> +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.

-- 
Regards,

Laurent Pinchart

[PATCH] omap3isp: avoid uninitialized memory

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]


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 <pavel@ucw.cz>

index 8f73f6d..a2474b6 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -362,14 +374,16 @@ int omap3isp_csiphy_init(struct isp_device *isp)
 	phy2->phy_regs = OMAP3_ISP_IOMEM_CSIPHY2;
 	mutex_init(&phy2->mutex);
 
-	if (isp->revision == ISP_REVISION_15_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);
+	if (isp->revision != ISP_REVISION_15_0) {
+		memset(phy1, sizeof(*phy1), 0);
+		return 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;
 }


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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] omap3isp: avoid uninitialized memory

[Sakari Ailus]

Hi Pavel,

On Mon, Feb 20, 2017 at 01:06:47PM +0100, Pavel Machek wrote:
> 
> 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 <pavel@ucw.cz>
> 
> index 8f73f6d..a2474b6 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -362,14 +374,16 @@ int omap3isp_csiphy_init(struct isp_device *isp)
>  	phy2->phy_regs = OMAP3_ISP_IOMEM_CSIPHY2;
>  	mutex_init(&phy2->mutex);
>  
> -	if (isp->revision == ISP_REVISION_15_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);
> +	if (isp->revision != ISP_REVISION_15_0) {
> +		memset(phy1, sizeof(*phy1), 0);
> +		return 0;

Isn't the memory allocated using kzalloc(), meaning it's already zero?

>  	}
>  
> +	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;
>  }
> 
> 

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[media] omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2283 bytes --]

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

Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>

index ca09523..e6584a2 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -213,14 +236,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);
diff --git a/drivers/media/platform/omap3isp/ispreg.h b/drivers/media/platform/omap3isp/ispreg.h
index b5ea8da..d084839 100644
--- a/drivers/media/platform/omap3isp/ispreg.h
+++ b/drivers/media/platform/omap3isp/ispreg.h
@@ -87,6 +87,8 @@
 #define ISPCCP2_CTRL_PHY_SEL_MASK	0x1
 #define ISPCCP2_CTRL_PHY_SEL_SHIFT	1
 #define ISPCCP2_CTRL_IO_OUT_SEL		(1 << 2)
+#define ISPCCP2_CTRL_IO_OUT_SEL_MASK	0x1
+#define ISPCCP2_CTRL_IO_OUT_SEL_SHIFT	2
 #define ISPCCP2_CTRL_MODE		(1 << 4)
 #define ISPCCP2_CTRL_VP_CLK_FORCE_ON	(1 << 9)
 #define ISPCCP2_CTRL_INV		(1 << 10)
@@ -94,6 +96,8 @@
 #define ISPCCP2_CTRL_INV_SHIFT		10
 #define ISPCCP2_CTRL_VP_ONLY_EN		(1 << 11)
 #define ISPCCP2_CTRL_VP_CLK_POL		(1 << 12)
+#define ISPCCP2_CTRL_VP_CLK_POL_MASK	0x1
+#define ISPCCP2_CTRL_VP_CLK_POL_SHIFT	12
 #define ISPCCP2_CTRL_VPCLK_DIV_SHIFT	15
 #define ISPCCP2_CTRL_VPCLK_DIV_MASK	0x1ffff /* [31:15] */
 #define ISPCCP2_CTRL_VP_OUT_CTRL_SHIFT	8 /* 3430 bits */


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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [media] omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]

Hi Pavel,

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

url:    https://github.com/0day-ci/linux/commits/Pavel-Machek/omap3isp-Correctly-set-IO_OUT_SEL-and-VP_CLK_POL-for-CCP2-mode/20170303-164739
base:   git://linuxtv.org/media_tree.git master
config: arm-omap2plus_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/media/platform/omap3isp/ispccp2.c: In function 'ccp2_phyif_config':
>> drivers/media/platform/omap3isp/ispccp2.c:226:39: error: 'const struct isp_ccp2_cfg' has no member named 'vp_clk_pol'; did you mean 'vpclk_div'?
      ISPCCP2_CTRL_VP_CLK_POL_MASK, buscfg->vp_clk_pol);
                                          ^
   drivers/media/platform/omap3isp/ispccp2.c:61:8: note: in definition of macro 'BIT_SET'
       | ((val) << (shift));  \
           ^~~

vim +226 drivers/media/platform/omap3isp/ispccp2.c

   220			buscfg->phy_layer);
   221		BIT_SET(val, ISPCCP2_CTRL_IO_OUT_SEL_SHIFT,
   222			ISPCCP2_CTRL_IO_OUT_SEL_MASK, buscfg->ccp2_mode);
   223		BIT_SET(val, ISPCCP2_CTRL_INV_SHIFT, ISPCCP2_CTRL_INV_MASK,
   224			buscfg->strobe_clk_pol);
   225		BIT_SET(val, ISPCCP2_CTRL_VP_CLK_POL_SHIFT,
 > 226			ISPCCP2_CTRL_VP_CLK_POL_MASK, buscfg->vp_clk_pol);
   227		isp_reg_writel(isp, val, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_CTRL);
   228	
   229		val = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_CTRL);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28900 bytes --]

Re: [media] omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 515 bytes --]

Hi!

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [kbuild-all] [media] omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode

[Ye Xiaolong]

On 03/03, Pavel Machek wrote:
>Hi!
>
>> [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.

Could you tell us the url of Sakari's tree? thus we can add it to 0day's
monitoring list.

Thanks,
Xiaolong
>
>I don't think you can do much to fix the automated system....
>
>										Pavel
>
>-- 
>(english) http://www.livejournal.com/~pavelmachek
>(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html



>_______________________________________________
>kbuild-all mailing list
>kbuild-all@lists.01.org
>https://lists.01.org/mailman/listinfo/kbuild-all

Re: [media] omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode

[Fengguang Wu]

On Fri, Mar 03, 2017 at 10:48:38PM +0100, Pavel Machek wrote:
>Hi!
>
>> [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....

We could, if "git format-patch" can be setup to auto append lines

        parent-commit: X
        parent-patch-id: Y

With that information, as long as the parent commit/patch is public --
either by "git push" or posting patch to mailing lists -- we'll have
good chance to find and use it as the base for "git am".

Currently "git format-patch" already has the option "--base=auto" to
auto append the more accurate lines

        base-commit: P
        prerequisite-patch-id: X
        prerequisite-patch-id: Y
        prerequisite-patch-id: Z

That's the best information git can offer. Unfortunately it cannot
ALWAYS work without human aid. What's worse, when it cannot figure out
the base-commit, the whole "git format-patch" command will abort like
this

        $ git format-patch -1
        fatal: base commit shouldn't be in revision list

That fatal error makes it not a viable option to always turn on
"--base=auto" in .gitconfig.

Without a fully-automated solution, I don't think many people will
bother or remember to manually specify base-commit before sending
patches out.

To effectively save the robot from "base commit" guessing works, what
we can do is to

1) append "parent-commit"/"parent-patch-id" lines when git cannot
   figure out and append the "base-commit"/"prerequisite-patch-id"
   lines. So that the test robot always get the information to do
   its job.

2) advise kernel developers to run this once

        git config format.useAutoBase yes

   to configure "--base=auto" as the default behavior.

Thanks,
Fengguang

Re: [media] omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode

[Sakari Ailus]

On Wed, Mar 01, 2017 at 12:45:46PM +0100, Pavel Machek wrote:
> ISP CSI1 module needs all the bits correctly set to work.
> 
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 

How are you sending the patches?

I've applied this to the ccp2 branch.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [media] omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 669 bytes --]

On Sat 2017-03-04 17:15:34, Sakari Ailus wrote:
> On Wed, Mar 01, 2017 at 12:45:46PM +0100, Pavel Machek wrote:
> > ISP CSI1 module needs all the bits correctly set to work.
> > 
> > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> 
> How are you sending the patches?

manually using mutt, for series I do something with git. Hmm. And
script I was using for that disappeared :-(.

> I've applied this to the ccp2 branch.

Thanks,
									Pavel

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] omap3isp: add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]

Hi!

> > >> +
> > >> +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 ?
> > 
> > bus is now a pointer.
> 
> I've seen that, but why have you changed it ?

subdev support. Needs to go into separate patch. Will be done shortly.

> > >> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > >> @@ -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...
> 
> 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.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCHv2] omap3isp: add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 5049 bytes --]

Hi!

> > > >> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > > >> @@ -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...
> > 
> > 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 <pavel@ucw.cz>

index ca09523..b6e055e 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -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;
+
+		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;
+
+		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
+					ISP_INTERFACE_CCP2B_PHY1,
+					enable, !!buscfg->phy_layer,
+					buscfg->strobe_clk_pol);
+	}
+
 	/* Enable/Disable all the LCx channels */
 	for (i = 0; i < CCP2_LCx_CHANS_NUM; i++)
 		isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_LCx_CTRL(i),
@@ -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;
 			dev_dbg(isp->dev,
 				"Could not get regulator vdds_csib\n");
 			ccp2->vdds_csib = NULL;
 		}
+		/*
+		 * 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;
+		 */
 	} else if (isp->revision == ISP_REVISION_15_0) {
 		ccp2->phy = &isp->isp_csiphy1;
 	}
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 871d4fe..897097b 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -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);
 }
 
 /*
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.h b/drivers/media/platform/omap3isp/ispcsiphy.h
index 28b63b2..88c5c1e8 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.h
+++ b/drivers/media/platform/omap3isp/ispcsiphy.h
@@ -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 */



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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCHv2] omap3isp: add support for CSI1 bus

[Sakari Ailus]

Hi Pavel,

On Thu, Mar 02, 2017 at 11:16:04AM +0100, Pavel Machek wrote:
> Hi!
> 
> > > > >> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > > > >> @@ -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...
> > > 
> > > 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 <pavel@ucw.cz>
> 
> index ca09523..b6e055e 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -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;
> +
> +		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;
> +
> +		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.


> +	}
> +
>  	/* Enable/Disable all the LCx channels */
>  	for (i = 0; i < CCP2_LCx_CHANS_NUM; i++)
>  		isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_LCx_CTRL(i),
> @@ -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.

>  			dev_dbg(isp->dev,
>  				"Could not get regulator vdds_csib\n");
>  			ccp2->vdds_csib = NULL;
>  		}
> +		/*
> +		 * 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.

> +		 */
>  	} else if (isp->revision == ISP_REVISION_15_0) {
>  		ccp2->phy = &isp->isp_csiphy1;
>  	}
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
> index 871d4fe..897097b 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -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);
>  }
>  
>  /*
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.h b/drivers/media/platform/omap3isp/ispcsiphy.h
> index 28b63b2..88c5c1e8 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.h
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.h
> @@ -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 */
> 
> 
> 

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCHv2] omap3isp: add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]

Hi!

> > 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.
...
> > +	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;
> > +
> > +		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;

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

> >  			dev_dbg(isp->dev,
> >  				"Could not get regulator vdds_csib\n");
> >  			ccp2->vdds_csib = NULL;
> >  		}
> > +		/*
> > +		 * 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? :-).

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCHv2] omap3isp: add support for CSI1 bus

[Sakari Ailus]

Hi Pavel,

On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > 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.
> ...
> > > +	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;
> > > +
> > > +		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;

Yes. It needs to be fixed. :-)

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

There seems to be some more work left, yes. :-I

> 
> > > @@ -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.
> 
> > >  			dev_dbg(isp->dev,
> > >  				"Could not get regulator vdds_csib\n");
> > >  			ccp2->vdds_csib = NULL;
> > >  		}
> > > +		/*
> > > +		 * 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? :-).

If I only will have the time. Let's see if I can find some time this
week-end.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCHv2] omap3isp: add support for CSI1 bus

[Sakari Ailus]

Hi Pavel,

On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > 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.
> ...
> > > +	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;
> > > +
> > > +		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;

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.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCHv2] omap3isp: add support for CSI1 bus

[Sakari Ailus]

On Sat, Mar 04, 2017 at 03:03:18PM +0200, Sakari Ailus wrote:
> Hi Pavel,
> 
> On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > > 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.
> > ...
> > > > +	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;
> > > > +
> > > > +		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;
> 
> 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.

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.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCHv2] omap3isp: add support for CSI1 bus

[Laurent Pinchart]

Hi Sakari,

On Saturday 04 Mar 2017 17:39:46 Sakari Ailus wrote:
> On Sat, Mar 04, 2017 at 03:03:18PM +0200, Sakari Ailus wrote:
> > On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> >> 
> >>>> 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.
> >>
> >> ...
> >> 
> >>>> +	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;
> >>>> +
> >>>> +		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;
> > 
> > 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.
> 
> 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.

-- 
Regards,

Laurent Pinchart

[bug] omap3isp: missing support for ENUM_FMT

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 5145 bytes --]

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.

Regards,
									Pavel

diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 218e6d7..2ce0327 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -772,6 +772,44 @@ isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
 }
 
 static int
+isp_video_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *format)
+{
+	struct isp_video *video = video_drvdata(file);
+	struct v4l2_subdev_format fmt;
+	struct v4l2_subdev *subdev;
+	u32 pad;
+	int ret;
+
+	printk("ispvideo: enum_fmt\n");
+
+	subdev = isp_video_remote_subdev(video, &pad);
+	if (subdev == NULL) {
+		printk("No subdev\n");
+		//return -EINVAL;
+	}
+
+	//isp_video_pix_to_mbus(&format->fmt.pix, &fmt.format);
+	if (format->index)
+		return -EINVAL;
+	format->type = video->type;
+	format->flags = 0;
+	strcpy(format->description, "subdev description");
+	format->pixelformat = V4L2_PIX_FMT_SGRBG10;
+
+	printk("Returning SRGBG10\n");
+#if 0	
+	fmt.pad = pad;
+	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
+	if (ret)
+		return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
+
+	isp_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
+#endif
+	return 0;
+}
+
+static int
 isp_video_get_selection(struct file *file, void *fh, struct v4l2_selection *sel)
 {
 	struct isp_video *video = video_drvdata(file);
@@ -1276,6 +1314,7 @@ static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
 	.vidioc_g_fmt_vid_cap		= isp_video_get_format,
 	.vidioc_s_fmt_vid_cap		= isp_video_set_format,
 	.vidioc_try_fmt_vid_cap		= isp_video_try_format,
+	.vidioc_enum_fmt_vid_cap        = isp_video_enum_format,
 	.vidioc_g_fmt_vid_out		= isp_video_get_format,
 	.vidioc_s_fmt_vid_out		= isp_video_set_format,
 	.vidioc_try_fmt_vid_out		= isp_video_try_format,

On Sat 2017-03-04 20:44:50, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Saturday 04 Mar 2017 17:39:46 Sakari Ailus wrote:
> > On Sat, Mar 04, 2017 at 03:03:18PM +0200, Sakari Ailus wrote:
> > > On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> > >> 
> > >>>> 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.
> > >>
> > >> ...
> > >> 
> > >>>> +	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;
> > >>>> +
> > >>>> +		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;
> > > 
> > > 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.
> > 
> > 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.
> 

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [bug] omap3isp: missing support for ENUM_FMT

[Sakari Ailus]

Hi Pavel,

On Wed, Apr 26, 2017 at 11:19:33PM +0200, Pavel Machek wrote:
> 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.

The issue with providing ENUM_FMT support on MC-enabled drivers is that the
media bus format has an effect to which pixel formats are available.

What has been discussed in the past but what remains unimplemented is to add
the media bus code to v4l2_fmtdesc for user to choose. That would allow
meaningful ENUM_FMT support.

For users such as libv4l2, i.e. code == 0 --- it should just tell the user
what it can do with the active media bus format.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCHv2] omap3isp: add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1587 bytes --]

Hi!

> > > > +	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;
> > > > +
> > > > +		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;
> 
> 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.

Thanks! They seem to be step in right direction. I still need to call
csiphy_routing_cfg_3430() directly for camera to work, but at least it
does not crash if I set up the phy pointer. I'll debug it some more.

Best regards,

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCHv2] omap3isp: add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4778 bytes --]

Hi!

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

They help a lot. Now I can use similar code paths...

Not yet a mergeable patch, but already better than it was.

Thanks and best regards,
									Pavel


diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 24a9fc5..79838bd 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -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"
@@ -1149,6 +1170,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 				"Could not get regulator vdds_csib\n");
 			ccp2->vdds_csib = NULL;
 		}
+		ccp2->phy = &isp->isp_csiphy2;
 	} else if (isp->revision == ISP_REVISION_15_0) {
 		ccp2->phy = &isp->isp_csiphy1;
 	}
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 50c0f64..94461df 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -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);
 }
 
 /*
@@ -197,27 +200,40 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 	}
 
 	if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
-	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
+	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
 		lanes = &buscfg->bus.ccp2.lanecfg;
-	else
+		phy->num_data_lanes = 1;
+	} else
 		lanes = &buscfg->bus.csi2.lanecfg;
 
+	printk("lane verification... %d\n", phy->num_data_lanes);
+
 	/* Clock and data lanes verification */
 	for (i = 0; i < phy->num_data_lanes; i++) {
-		if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3)
-			return -EINVAL;
+		if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3) {
+			printk("Bad cfg\n");
+			//return -EINVAL;
+		}
 
-		if (used_lanes & (1 << lanes->data[i].pos))
-			return -EINVAL;
+		if (used_lanes & (1 << lanes->data[i].pos)) {
+			printk("Already used\n");
+			//return -EINVAL;
+		}
 
 		used_lanes |= 1 << lanes->data[i].pos;
 	}
 
-	if (lanes->clk.pol > 1 || lanes->clk.pos > 3)
-		return -EINVAL;
+	printk("used lanes... %d\n", used_lanes);	
 
-	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
-		return -EINVAL;
+	if (lanes->clk.pol > 1 || lanes->clk.pos > 3) {
+		printk("Bad clock\n");
+		//return -EINVAL;
+	}
+
+	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos)) {
+		printk("Reused clock\n");
+		//return -EINVAL;
+	}
 
 	/*
 	 * The PHY configuration is lost in off mode, that's not an
@@ -302,13 +318,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
 	if (rval < 0)
 		goto done;
 
-	rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
-	if (rval) {
-		regulator_disable(phy->vdd);
-		goto done;
+	if (phy->isp->revision == ISP_REVISION_15_0) {
+		rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
+		if (rval) {
+			regulator_disable(phy->vdd);
+			goto done;
+		}
+		
+		csiphy_power_autoswitch_enable(phy, true);		
 	}
 
-	csiphy_power_autoswitch_enable(phy, true);
 	phy->phy_in_use = 1;
 
 done:

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH] v4l2-fwnode: Fix clock lane parsing

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 812 bytes --]

Fix clock lane parsing in v4l2-fwnode.
    
Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index d6666d3..44036b8 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -167,7 +167,7 @@ void v4l2_fwnode_endpoint_parse_csi1_bus(struct fwnode_handle *fwn,
                bus->data_lane = v;
 
        if (!fwnode_property_read_u32(fwn, "clock-lanes", &v))
-               bus->data_lane = v;
+               bus->clock_lane = v;
 
        if (bus_type == V4L2_FWNODE_BUS_TYPE_CCP2)
 	       vfwn->bus_type = V4L2_MBUS_CCP2;

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] v4l2-fwnode: Fix clock lane parsing

[Sakari Ailus]

On Mon, Mar 06, 2017 at 08:23:24AM +0100, Pavel Machek wrote:
> Fix clock lane parsing in v4l2-fwnode.
>     
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index d6666d3..44036b8 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -167,7 +167,7 @@ void v4l2_fwnode_endpoint_parse_csi1_bus(struct fwnode_handle *fwn,
>                 bus->data_lane = v;
>  
>         if (!fwnode_property_read_u32(fwn, "clock-lanes", &v))
> -               bus->data_lane = v;
> +               bus->clock_lane = v;

Oh my. Did I really write it like that?

I'll merge your fix next Monday. Thanks!

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

v4l2-fwnode: status, plans for merge, any branch to merge against?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]

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?

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Sakari Ailus]

Hi Pavel,

On Tue, Jun 13, 2017 at 02:22:40PM +0200, Pavel Machek wrote:
> 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?

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.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

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

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

Thanks,
									Pavel


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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Sakari Ailus]

Hi, Pavel!

On Tue, Jun 13, 2017 at 11:09:00PM +0200, Pavel Machek wrote:
> 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?
> > 
> > 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.
> 
> 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.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3119 bytes --]

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

Thanks, that's what I was looking for.

Unfortunately, it does not compile.

  CC      drivers/media/platform/omap3isp/ispcsiphy.o
  drivers/media/platform/omap3isp/isp.c: In function
  'isp_fwnode_parse':
  drivers/media/platform/omap3isp/isp.c:2029:35: error: 'fwn'
  undeclared (first use in this function)
  drivers/media/platform/omap3isp/isp.c:2029:35: note: each undeclared
  identifier is reported only once for each function it appears in
  drivers/media/platform/omap3isp/isp.c:2029:2: error: incompatible
  type for argument 2 of 'v4l2_fwnode_endpoint_parse'
  In file included from drivers/media/platform/omap3isp/isp.c:67:0:
  ./include/media/v4l2-fwnode.h:112:5: note: expected 'struct
  v4l2_fwnode_endpoint *' but argument is of type 'struct
  v4l2_fwnode_endpoint'
  scripts/Makefile.build:302: recipe for target
  'drivers/media/platform/omap3isp/isp.o' failed
  make[4]: *** [drivers/media/platform/omap3isp/isp.o] Error 1
  make[4]: *** Waiting for unfinished jobs....
  scripts/Makefile.build:561: recipe for target
  'drivers/media/platform/omap3isp' failed
  make[3]: *** [drivers/media/platform/omap3isp] Error 2

You can get my config if needed. Now let me try to fix it... It was
not too bad, good.

commit 364340e7aa037535a65d2ef2a1711c97d233fede
Author: Pavel <pavel@ucw.cz>
Date:   Wed Jun 14 21:40:37 2017 +0200

Fix compilation of omap3isp/isp.c.
    
Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 4ca3fc9..b80debf 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2026,7 +2026,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
 
 	isd->bus = buscfg;
 
-	ret = v4l2_fwnode_endpoint_parse(fwn, vep);
+	ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
 	if (ret)
 		return ret;
 

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Sakari Ailus]

On Wed, Jun 14, 2017 at 09:41:29PM +0200, Pavel Machek wrote:
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 4ca3fc9..b80debf 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2026,7 +2026,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
>  
>  	isd->bus = buscfg;
>  
> -	ret = v4l2_fwnode_endpoint_parse(fwn, vep);
> +	ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
>  	if (ret)
>  		return ret;

I just pushed the fix there.

Btw. I think we should probably drop the change allocating the sub-device
configuration separately. It's better to associate the lens, flash and
eeprom (where it exists) to the sensor than to the CSI-2 receiver. In that
case there are no async sub-devices without bus configuration.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1820 bytes --]

On Fri 2017-06-16 01:07:00, Sakari Ailus wrote:
> On Wed, Jun 14, 2017 at 09:41:29PM +0200, Pavel Machek wrote:
> > diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> > index 4ca3fc9..b80debf 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -2026,7 +2026,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
> >  
> >  	isd->bus = buscfg;
> >  
> > -	ret = v4l2_fwnode_endpoint_parse(fwn, vep);
> > +	ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
> >  	if (ret)
> >  		return ret;
> 
> I just pushed the fix there.
> 
> Btw. I think we should probably drop the change allocating the sub-device
> configuration separately. It's better to associate the lens, flash and
> eeprom (where it exists) to the sensor than to the CSI-2 receiver. In that
> case there are no async sub-devices without bus configuration.

Actually I thought about that a bit, and am not sure about that.

CSI-2 receiver may not be good place to associate lens and flash with,
agreed.

But is sensor a good place? In particular, phones with two cameras
cooperating (for example one black&white and one color) are getting
common. It seems to be true that each sensor has a lens and autofocus
motor associated, but flash LED is common, and both sensors are
designed to work as one device.

But yes, that's still better than placing it at CSI-2 receiver. But I
guess we should make sure that flash LED can associated with more than
one sensor, and maybe we should have some kind of "camera package"
entity.

Best regards,

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

n900 camera on v4.12-rc (was Re: v4l2-fwnode: status, plans for merge, any branch to merge against?)

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

Hi!

Ok, so I played a bit, and now I have working camera in v4.12-rc3.
https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-n900.git/
camera-fw5-3 is recommended branch to play with.

Sakari, should I attempt to clean/send you patches, or would it be
better to wait till ccp2 branch is merged upstream? There's one
compile fix, I'll submit that one in following email.

I even have patches for v4l2-utils, so digital camera can be used as
... digital camera :-). (With rather slow autofocus, and 1Mpix only at
the moment, but hey, its a start, and I already have _one_ nice
picture from it.)

Best regards,

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH] omap3isp: fix compilation

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]


Fix compilation of isp.c
    
Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 4ca3fc9..b80debf 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2026,7 +2026,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
 
 	isd->bus = buscfg;
 
-	ret = v4l2_fwnode_endpoint_parse(fwn, vep);
+	ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
 	if (ret)
 		return ret;
 

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] omap3isp: fix compilation

[Hans Verkuil]

On 06/16/2017 12:23 AM, Pavel Machek wrote:
> 
> Fix compilation of isp.c
>      
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 4ca3fc9..b80debf 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2026,7 +2026,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
>   
>   	isd->bus = buscfg;
>   
> -	ret = v4l2_fwnode_endpoint_parse(fwn, vep);
> +	ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
>   	if (ret)
>   		return ret;
>   
> 

You're using something old since the media tree master already uses &vep.

Regards,

	Hans

Re: [PATCH] omap3isp: fix compilation

[Sakari Ailus]

On Fri, Jun 16, 2017 at 10:03:41AM +0200, Hans Verkuil wrote:
> On 06/16/2017 12:23 AM, Pavel Machek wrote:
> >
> >Fix compilation of isp.c
> >Signed-off-by: Pavel Machek <pavel@ucw.cz>
> >
> >diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> >index 4ca3fc9..b80debf 100644
> >--- a/drivers/media/platform/omap3isp/isp.c
> >+++ b/drivers/media/platform/omap3isp/isp.c
> >@@ -2026,7 +2026,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
> >  	isd->bus = buscfg;
> >-	ret = v4l2_fwnode_endpoint_parse(fwn, vep);
> >+	ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
> >  	if (ret)
> >  		return ret;
> >
> 
> You're using something old since the media tree master already uses &vep.

Well, yes and no. Pavel is using my ccp2 support branch I recently rebased.
:-)

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Pavel Machek]

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

Is ccp2 branch expected to go into 4.13, too?

Best regards,
									Pavel

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

Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Sakari Ailus]

On Tue, Jul 04, 2017 at 05:08:19PM +0200, Pavel Machek wrote:
> 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?
> > > > 
> > > > 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.
> > > 
> > > 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.
> 
> Is ccp2 branch expected to go into 4.13, too?

Hi Pavel,

What I've done is just rebased the ccp2 branch. In other words, the patches
in that branch are no more ready than they were.

To get these merged we should ideally

1) Make sure there will be no regressions,

2) clean things up in the omap3isp; which resources are needed and when
(e.g. regulators, PHY configuration) isn't clear at the moment and

2) have one driver using the implementation.

At least 1) is needed. I think a number of framework patches could be
mergeable before 2) and 3) are done. I can prepare a set later this week.
But even that'd be likely for 4.14, not 4.13.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 5336 bytes --]

Hi!

> > > > > 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?
> > > 
> > > 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.
> > 
> > Is ccp2 branch expected to go into 4.13, too?
> 
> Hi Pavel,
> 
> What I've done is just rebased the ccp2 branch. In other words, the patches
> in that branch are no more ready than they were.

I thought they were ready even back then :-).

> To get these merged we should ideally
> 
> 1) Make sure there will be no regressions,

Well, all I have running recent kernels is N900. If ccp branch works
for you on N9, that's probably as much testing as we can get.

> 2) clean things up in the omap3isp; which resources are needed and when
> (e.g. regulators, PHY configuration) isn't clear at the moment and
> 
> 2) have one driver using the implementation.
> 
> At least 1) is needed. I think a number of framework patches could be
> mergeable before 2) and 3) are done. I can prepare a set later this week.
> But even that'd be likely for 4.14, not 4.13.

Yep, it is too late for v4.13 now. But getting stuff ready for v4.14
would be good.

I started looking through the patches; I believe they are safe, but it
is probably better to review the series you've just mailed.

The driver using the implementation -- yes, I have it all working on
n900 (incuding userland, I can actually take photos.) I can post the
series, or better link to kernel.org.

Right now, my goal would be to get sensor working on N900 with
mainline (without flash and focus).

I'd very much like any comment on patch attached below.

Age   Commit message (Expand)	Author	Files	Lines
2017-06-16   omap3isp: Destroy CSI-2 phy mutexes in error and module
2017-06-16	omap3isp: Skip CSI-2 receiver initialisation in CCP2
2017-06-16	omap3isp: Correctly put the last iterated endpoint
2017-06-16	omap3isp: Always initialise isp and mutex for csiphy1
2017-06-16	omap3isp: Return -EPROBE_DEFER if the required
2017-06-16 omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2
2017-06-16    omap3isp: Make external sub-device bus configuration a
2017-06-15    omap3isp: Parse CSI1 configuration from the device tree
2017-06-15    omap3isp: Check for valid port in endpoints	Sakari
2017-06-15	omap3isp: Ignore endpoints with invalid configuration

# Nothing changes for bus_type == V4L2_MBUS_CSI2. FIXME: Is bus_type
  set correctly?

2017-06-15	smiapp: add CCP2 support	Pavel Machek	1

# bus_type will be guess, so no code changes on existing system:

2017-06-15	v4l: Add support for CSI-1 and CCP2 busses	Sakari

# Reads unused value -> can't break anything:

2017-06-13	v4l: fwnode: Obtain data bus type from FW	Sakari

# No code changes -> totally safe:

2017-06-13	v4l: fwnode: Call CSI2 bus csi2, not csi	Sakari
2017-06-13	dt: bindings: Add strobe property for CCP2	Sakari
2017-06-13	dt: bindings: Explicitly specify bus type

Best regards,
								Pavel

commit 1220492dd4c1872c8036caa573680f95aabc69bc
Author: Pavel <pavel@ucw.cz>
Date:   Tue Feb 28 12:02:26 2017 +0100

    omap3isp: add CSI1 support
    
    Use proper code path for csi1/ccp2 support.
    
    Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 24a9fc5..47210b1 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1149,6 +1149,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 				"Could not get regulator vdds_csib\n");
 			ccp2->vdds_csib = NULL;
 		}
+		ccp2->phy = &isp->isp_csiphy2;
 	} else if (isp->revision == ISP_REVISION_15_0) {
 		ccp2->phy = &isp->isp_csiphy1;
 	}
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 50c0f64..862fdd3 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -197,9 +197,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 	}
 
 	if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
-	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
+	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
 		lanes = &buscfg->bus.ccp2.lanecfg;
-	else
+		phy->num_data_lanes = 1;
+	} else
 		lanes = &buscfg->bus.csi2.lanecfg;
 
 	/* Clock and data lanes verification */
@@ -302,13 +303,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
 	if (rval < 0)
 		goto done;
 
-	rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
-	if (rval) {
-		regulator_disable(phy->vdd);
-		goto done;
+	if (phy->isp->revision == ISP_REVISION_15_0) {
+		rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
+		if (rval) {
+			regulator_disable(phy->vdd);
+			goto done;
+		}
+
+		csiphy_power_autoswitch_enable(phy, true);
 	}
 
-	csiphy_power_autoswitch_enable(phy, true);
 	phy->phy_in_use = 1;
 
 done:

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Sakari Ailus]

Hi Pavel,

On Thu, Jul 06, 2017 at 12:38:51PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > > > 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?
> > > > 
> > > > 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.
> > > 
> > > Is ccp2 branch expected to go into 4.13, too?
> > 
> > Hi Pavel,
> > 
> > What I've done is just rebased the ccp2 branch. In other words, the patches
> > in that branch are no more ready than they were.
> 
> I thought they were ready even back then :-).
> 
> > To get these merged we should ideally
> > 
> > 1) Make sure there will be no regressions,
> 
> Well, all I have running recent kernels is N900. If ccp branch works
> for you on N9, that's probably as much testing as we can get.
> 
> > 2) clean things up in the omap3isp; which resources are needed and when
> > (e.g. regulators, PHY configuration) isn't clear at the moment and
> > 
> > 2) have one driver using the implementation.
> > 
> > At least 1) is needed. I think a number of framework patches could be
> > mergeable before 2) and 3) are done. I can prepare a set later this week.
> > But even that'd be likely for 4.14, not 4.13.
> 
> Yep, it is too late for v4.13 now. But getting stuff ready for v4.14
> would be good.
> 
> I started looking through the patches; I believe they are safe, but it
> is probably better to review the series you've just mailed.
> 
> The driver using the implementation -- yes, I have it all working on
> n900 (incuding userland, I can actually take photos.) I can post the
> series, or better link to kernel.org.
> 
> Right now, my goal would be to get sensor working on N900 with
> mainline (without flash and focus).
> 
> I'd very much like any comment on patch attached below.
> 
> Age   Commit message (Expand)	Author	Files	Lines
> 2017-06-16   omap3isp: Destroy CSI-2 phy mutexes in error and module
> 2017-06-16	omap3isp: Skip CSI-2 receiver initialisation in CCP2
> 2017-06-16	omap3isp: Correctly put the last iterated endpoint
> 2017-06-16	omap3isp: Always initialise isp and mutex for csiphy1
> 2017-06-16	omap3isp: Return -EPROBE_DEFER if the required
> 2017-06-16 omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2
> 2017-06-16    omap3isp: Make external sub-device bus configuration a
> 2017-06-15    omap3isp: Parse CSI1 configuration from the device tree
> 2017-06-15    omap3isp: Check for valid port in endpoints	Sakari
> 2017-06-15	omap3isp: Ignore endpoints with invalid configuration
> 
> # Nothing changes for bus_type == V4L2_MBUS_CSI2. FIXME: Is bus_type
>   set correctly?
> 
> 2017-06-15	smiapp: add CCP2 support	Pavel Machek	1
> 
> # bus_type will be guess, so no code changes on existing system:
> 
> 2017-06-15	v4l: Add support for CSI-1 and CCP2 busses	Sakari
> 
> # Reads unused value -> can't break anything:
> 
> 2017-06-13	v4l: fwnode: Obtain data bus type from FW	Sakari
> 
> # No code changes -> totally safe:
> 
> 2017-06-13	v4l: fwnode: Call CSI2 bus csi2, not csi	Sakari
> 2017-06-13	dt: bindings: Add strobe property for CCP2	Sakari
> 2017-06-13	dt: bindings: Explicitly specify bus type
> 
> Best regards,
> 								Pavel
> 
> commit 1220492dd4c1872c8036caa573680f95aabc69bc
> Author: Pavel <pavel@ucw.cz>
> Date:   Tue Feb 28 12:02:26 2017 +0100
> 
>     omap3isp: add CSI1 support
>     
>     Use proper code path for csi1/ccp2 support.
>     
>     Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
> index 24a9fc5..47210b1 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -1149,6 +1149,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
>  				"Could not get regulator vdds_csib\n");
>  			ccp2->vdds_csib = NULL;
>  		}
> +		ccp2->phy = &isp->isp_csiphy2;
>  	} else if (isp->revision == ISP_REVISION_15_0) {
>  		ccp2->phy = &isp->isp_csiphy1;
>  	}
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
> index 50c0f64..862fdd3 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -197,9 +197,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
>  	}
>  
>  	if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
> -	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
> +	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
>  		lanes = &buscfg->bus.ccp2.lanecfg;
> -	else
> +		phy->num_data_lanes = 1;
> +	} else
>  		lanes = &buscfg->bus.csi2.lanecfg;
>  
>  	/* Clock and data lanes verification */
> @@ -302,13 +303,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
>  	if (rval < 0)
>  		goto done;
>  
> -	rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> -	if (rval) {
> -		regulator_disable(phy->vdd);
> -		goto done;
> +	if (phy->isp->revision == ISP_REVISION_15_0) {

Shouldn't you make the related changes to omap3isp_csiphy_release() as
well?

Other than that the patch looks good to me.

> +		rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> +		if (rval) {
> +			regulator_disable(phy->vdd);
> +			goto done;
> +		}
> +
> +		csiphy_power_autoswitch_enable(phy, true);
>  	}
>  
> -	csiphy_power_autoswitch_enable(phy, true);
>  	phy->phy_in_use = 1;
>  
>  done:
> 

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1619 bytes --]

Hi!


> > > 1) Make sure there will be no regressions,
> > 
> > Well, all I have running recent kernels is N900. If ccp branch works
> > for you on N9, that's probably as much testing as we can get.
> > 
> > > 2) clean things up in the omap3isp; which resources are needed and when
> > > (e.g. regulators, PHY configuration) isn't clear at the moment and
> > > 
> > > 2) have one driver using the implementation.
> > > 
> > > At least 1) is needed. I think a number of framework patches could be
> > > mergeable before 2) and 3) are done. I can prepare a set later this week.
> > > But even that'd be likely for 4.14, not 4.13.
> > 
> > Yep, it is too late for v4.13 now. But getting stuff ready for v4.14
> > would be good.
...
> > @@ -302,13 +303,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
> >  	if (rval < 0)
> >  		goto done;
> >  
> > -	rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> > -	if (rval) {
> > -		regulator_disable(phy->vdd);
> > -		goto done;
> > +	if (phy->isp->revision == ISP_REVISION_15_0) {
> 
> Shouldn't you make the related changes to omap3isp_csiphy_release() as
> well?
> 
> Other than that the patch looks good to me.

Ah, yes, that needs to be fixed. Thanks for review.

I'll refresh the series. I believe we now have everything neccessary
to have useful driver for 4.14. Series is still based on 4.12-rc3, I
can rebase it when there's better base.

Best regards,

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

omap3isp: is capture mode working? what hardware? was Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1696 bytes --]

Hi!

> What I've done is just rebased the ccp2 branch. In other words, the patches
> in that branch are no more ready than they were.
> 
> To get these merged we should ideally
> 
> 1) Make sure there will be no regressions,

I grepped dts trees a bit... where is omap3isp currently used?
Anything besides N9 and N950?

Does the capture mode currently work for you?

Because as far as I can tell, formatter is disabled, so video is in
wrong format for the userspace.

So something like patch below is needed; (of course after adjusting
the comment etc.)

Thanks,
								Pavel

commit eb81524b8b44bbff2518b272cb3de304157bd3ba
Author: Pavel <pavel@ucw.cz>
Date:   Mon Feb 13 21:26:51 2017 +0100

    omap3isp: fix VP2SDR bit so capture (not preview) works
    
    This is neccessary for capture (not preview) to work properly on
    N900. Why is unknown.

diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 7207558..2fb755f 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1186,7 +1186,8 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	/* Use the raw, unprocessed data when writing to memory. The H3A and
 	 * histogram modules are still fed with lens shading corrected data.
 	 */
-	syn_mode &= ~ISPCCDC_SYN_MODE_VP2SDR;
+//	syn_mode &= ~ISPCCDC_SYN_MODE_VP2SDR;
+	syn_mode |= ISPCCDC_SYN_MODE_VP2SDR;
 
 	if (ccdc->output & CCDC_OUTPUT_MEMORY)
 		syn_mode |= ISPCCDC_SYN_MODE_WEN;

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[RFC] omap3isp: add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]

omap3isp: add rest of CSI1 support

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

Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

Hmm. Looking at that... num_data_lanes probably should be modified in
local variable, not globally like this. Should I do that?

Anything else that needs fixing?

index 24a9fc5..6feba36 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -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"
@@ -1149,6 +1152,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 				"Could not get regulator vdds_csib\n");
 			ccp2->vdds_csib = NULL;
 		}
+		ccp2->phy = &isp->isp_csiphy2;
 	} else if (isp->revision == ISP_REVISION_15_0) {
 		ccp2->phy = &isp->isp_csiphy1;
 	}
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 50c0f64..cd6351b 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -197,9 +200,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 	}
 
 	if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
-	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
+	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
 		lanes = &buscfg->bus.ccp2.lanecfg;
-	else
+		phy->num_data_lanes = 1;
+	} else
 		lanes = &buscfg->bus.csi2.lanecfg;
 
 	/* Clock and data lanes verification */
@@ -302,13 +306,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
 	if (rval < 0)
 		goto done;
 
-	rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
-	if (rval) {
-		regulator_disable(phy->vdd);
-		goto done;
+	if (phy->isp->revision == ISP_REVISION_15_0) {
+		rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
+		if (rval) {
+			regulator_disable(phy->vdd);
+			goto done;
+		}
+		
+		csiphy_power_autoswitch_enable(phy, true);		
 	}
 
-	csiphy_power_autoswitch_enable(phy, true);
 	phy->phy_in_use = 1;
 
 done:

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RFC] omap3isp: add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2603 bytes --]

On Mon 2017-03-06 08:56:59, Pavel Machek wrote:
> omap3isp: add rest of CSI1 support
> 
> CSI1 needs one more bit to be set up. Do just that.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> ---
> 
> Hmm. Looking at that... num_data_lanes probably should be modified in
> local variable, not globally like this. Should I do that?
> 
> Anything else that needs fixing?

Ping? Feedback here would be nice. This is last "interesting" piece of
the hardware support...

Best regards,
								Pavel

> index 24a9fc5..6feba36 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -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"
> @@ -1149,6 +1152,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
>  				"Could not get regulator vdds_csib\n");
>  			ccp2->vdds_csib = NULL;
>  		}
> +		ccp2->phy = &isp->isp_csiphy2;
>  	} else if (isp->revision == ISP_REVISION_15_0) {
>  		ccp2->phy = &isp->isp_csiphy1;
>  	}
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
> index 50c0f64..cd6351b 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -197,9 +200,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
>  	}
>  
>  	if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
> -	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
> +	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
>  		lanes = &buscfg->bus.ccp2.lanecfg;
> -	else
> +		phy->num_data_lanes = 1;
> +	} else
>  		lanes = &buscfg->bus.csi2.lanecfg;
>  
>  	/* Clock and data lanes verification */
> @@ -302,13 +306,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
>  	if (rval < 0)
>  		goto done;
>  
> -	rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> -	if (rval) {
> -		regulator_disable(phy->vdd);
> -		goto done;
> +	if (phy->isp->revision == ISP_REVISION_15_0) {
> +		rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> +		if (rval) {
> +			regulator_disable(phy->vdd);
> +			goto done;
> +		}
> +		
> +		csiphy_power_autoswitch_enable(phy, true);		
>  	}
>  
> -	csiphy_power_autoswitch_enable(phy, true);
>  	phy->phy_in_use = 1;
>  
>  done:
> 



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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RFC] omap3isp: add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2886 bytes --]

On Fri 2017-03-10 14:41:31, Pavel Machek wrote:
> On Mon 2017-03-06 08:56:59, Pavel Machek wrote:
> > omap3isp: add rest of CSI1 support
> > 
> > CSI1 needs one more bit to be set up. Do just that.
> > 
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> > ---
> > 
> > Hmm. Looking at that... num_data_lanes probably should be modified in
> > local variable, not globally like this. Should I do that?
> > 
> > Anything else that needs fixing?
> 
> Ping? Feedback here would be nice. This is last "interesting" piece of
> the hardware support...

Any news here? You complained that I was not pushy enough in the past
;-).

								Pavel
								
> > index 24a9fc5..6feba36 100644
> > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > @@ -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"
> > @@ -1149,6 +1152,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> >  				"Could not get regulator vdds_csib\n");
> >  			ccp2->vdds_csib = NULL;
> >  		}
> > +		ccp2->phy = &isp->isp_csiphy2;
> >  	} else if (isp->revision == ISP_REVISION_15_0) {
> >  		ccp2->phy = &isp->isp_csiphy1;
> >  	}
> > diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
> > index 50c0f64..cd6351b 100644
> > --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> > +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> > @@ -197,9 +200,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
> >  	}
> >  
> >  	if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
> > -	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
> > +	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
> >  		lanes = &buscfg->bus.ccp2.lanecfg;
> > -	else
> > +		phy->num_data_lanes = 1;
> > +	} else
> >  		lanes = &buscfg->bus.csi2.lanecfg;
> >  
> >  	/* Clock and data lanes verification */
> > @@ -302,13 +306,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
> >  	if (rval < 0)
> >  		goto done;
> >  
> > -	rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> > -	if (rval) {
> > -		regulator_disable(phy->vdd);
> > -		goto done;
> > +	if (phy->isp->revision == ISP_REVISION_15_0) {
> > +		rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> > +		if (rval) {
> > +			regulator_disable(phy->vdd);
> > +			goto done;
> > +		}
> > +		
> > +		csiphy_power_autoswitch_enable(phy, true);		
> >  	}
> >  
> > -	csiphy_power_autoswitch_enable(phy, true);
> >  	phy->phy_in_use = 1;
> >  
> >  done:
> > 
> 
> 
> 



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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCHv2] omap3isp: add support for CSI1 bus

[Sakari Ailus]

Hi Pavel,

On 03/04/17 15:03, Sakari Ailus wrote:
> Hi Pavel,
> 
> On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
>> Hi!
>>
>>>> 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.
>> ...
>>>> +	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;
>>>> +
>>>> +		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;
> 
> 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.
> 

I've updated the CCP2 patches here on top of the latest fwnode patches:

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

No even compile testing this time though. I'm afraid I haven't had the
time to otherwise to work on the CCP2 support, so there are no other
changes besides the rebase.

I intend to send a pull request for the fwnode patches once we have the
next rc1 in media tree so then we can have the patches on plain media
tree master branch.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi     XMPP: sailus@retiisi.org.uk

Re: [PATCHv2] omap3isp: add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]

Hi!

> > 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.
> > 
> 
> I've updated the CCP2 patches here on top of the latest fwnode patches:
> 
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=ccp2>
> 
> No even compile testing this time though. I'm afraid I haven't had the
> time to otherwise to work on the CCP2 support, so there are no other
> changes besides the rebase.

It seems they don't compile. Hmmm. Did I do something wrong? "struct
fwnode_endpoint" seems to be only used in v4l2-fwnode.h; that can't be right...?

  CC      drivers/media/i2c/smiapp/smiapp-core.o
  In file included from drivers/media/i2c/smiapp/smiapp-core.c:35:0:
  ./include/media/v4l2-fwnode.h:83:25: error: field 'base' has
  incomplete type
  drivers/media/i2c/smiapp/smiapp-core.c: In function
  'smiapp_get_hwconfig':
  drivers/media/i2c/smiapp/smiapp-core.c:2790:9: error: implicit
  declaration of function 'dev_fwnode'
  [-Werror=implicit-function-declaration]
  drivers/media/i2c/smiapp/smiapp-core.c:2790:33: warning:
  initialization makes pointer from integer without a cast [enabled by
  default]
  drivers/media/i2c/smiapp/smiapp-core.c:2797:2: error: implicit
  declaration of function 'fwnode_graph_get_next_endpoint'
  [-Werror=implicit-function-declaration]

Best regards,
									Pavel

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCHv2] omap3isp: add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]

Hi!

> It seems they don't compile. Hmmm. Did I do something wrong? "struct
> fwnode_endpoint" seems to be only used in v4l2-fwnode.h; that can't be right...?

Next problem is missing dev_fwnode; fixed. Next problem is

pavel@duo:/data/l/linux-n900$ git grep fwnode_graph_get_next_endpoint
.
drivers/media/i2c/smiapp/smiapp-core.c: ep =
fwnode_graph_get_next_endpoint(fwnode, NULL);
drivers/media/platform/omap3isp/isp.c:  while ((fwnode =
fwnode_graph_get_next_endpoint(dev_fwnode(dev),

So sorry, I guess I should wait for version that compiles ;-).
									Pavel

diff --git a/drivers/base/property.c b/drivers/base/property.c
index c458c63..f52a260 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -182,11 +182,6 @@ static int pset_prop_read_string(struct property_set *pset,
 	return 0;
 }
 
-static inline struct fwnode_handle *dev_fwnode(struct device *dev)
-{
-	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
-		&dev->of_node->fwnode : dev->fwnode;
-}
 
 /**
  * device_property_present - check if a property of a device is present
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 8bd28ce..9215e23 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -27,4 +27,10 @@ struct fwnode_handle {
 	struct fwnode_handle *secondary;
 };
 
+static inline struct fwnode_handle *dev_fwnode(struct device *dev)
+{
+	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
+		&dev->of_node->fwnode : dev->fwnode;
+}
+
 #endif
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index d762a55..9e9cfbc 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -80,7 +80,7 @@ struct v4l2_fwnode_bus_mipi_csi1 {
  * @nr_of_link_frequencies: number of elements in link_frequenccies array
  */
 struct v4l2_fwnode_endpoint {
-	struct fwnode_endpoint base;
+	/*struct fwnode_endpoint base; */
 	/*
 	 * Fields below this line will be zeroed by
 	 * v4l2_fwnode_parse_endpoint()




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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH] omap3isp: wait for regulators to come up

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]


If regulator returns -EPROBE_DEFER, we need to return it too, so that
omap3isp will be re-probed when regulator is ready.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index ca09523..b6e055e 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1137,10 +1159,12 @@ 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;
 			dev_dbg(isp->dev,
 				"Could not get regulator vdds_csib\n");
 			ccp2->vdds_csib = NULL;
 		}
 	} else if (isp->revision == ISP_REVISION_15_0) {
 		ccp2->phy = &isp->isp_csiphy1;
 	}

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] omap3isp: wait for regulators to come up

[Laurent Pinchart]

Hi Pavel,

Thank you for the patch.

On Thursday 02 Mar 2017 13:45:32 Pavel Machek wrote:
> If regulator returns -EPROBE_DEFER, we need to return it too, so that
> omap3isp will be re-probed when regulator is ready.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..b6e055e 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -1137,10 +1159,12 @@ 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 looks good to me, but it will result in the caller printing a "CCP2 
initialization failed" error message, which I'm not sure is right. Maybe we 
should move that message to the omap3isp_ccp2_init() function ?

In any case, this change is fine, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  			dev_dbg(isp->dev,
>  				"Could not get regulator vdds_csib\n");
>  			ccp2->vdds_csib = NULL;
>  		}
>  	} else if (isp->revision == ISP_REVISION_15_0) {
>  		ccp2->phy = &isp->isp_csiphy1;
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH] omap3isp: wait for regulators to come up

[Sakari Ailus]

On Thu, Mar 02, 2017 at 04:46:42PM +0200, Laurent Pinchart wrote:
> Hi Pavel,
> 
> Thank you for the patch.
> 
> On Thursday 02 Mar 2017 13:45:32 Pavel Machek wrote:
> > If regulator returns -EPROBE_DEFER, we need to return it too, so that
> > omap3isp will be re-probed when regulator is ready.
> > 
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..b6e055e 100644
> > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > @@ -1137,10 +1159,12 @@ 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 looks good to me, but it will result in the caller printing a "CCP2 
> initialization failed" error message, which I'm not sure is right. Maybe we 
> should move that message to the omap3isp_ccp2_init() function ?
> 
> In any case, this change is fine, so
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Applied to ccp2 branch with the error message moved. The old message is
still printed if the error code is different.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH] omap3isp: add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2367 bytes --]

Hi!

> > +		if (enable) {
> > +			csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
> > +				  OMAP343X_CONTROL_CSIRXFE_RESET;
> > +
> > +			if (buscfg->phy_layer)
> > +				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.
> 
> > +
> > +		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.

It seems csiphy_routing_cfg_3430 is not called at all. I added
printks, but they don't trigger. If you have an idea what is going on
there, it would help...
									Pavel

diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 6b814e1..fe9303a 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -30,6 +30,8 @@ static void csiphy_routing_cfg_3630(struct isp_csiphy *phy,
 	u32 reg;
 	u32 shift, mode;
 
+	printk("routing cfg 3630: iface %d, %d\n", iface, ISP_INTERFACE_CCP2B_PHY1);
+	
 	regmap_read(phy->isp->syscon, phy->isp->syscon_offset, &reg);
 
 	switch (iface) {
@@ -74,6 +76,9 @@ static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
 	u32 csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
 		| OMAP343X_CONTROL_CSIRXFE_RESET;
 
+	/* FIXME: can this be used instead of if (isp->revision) in ispccp2.c? */
+	
+	printk("routing cfg: iface %d, %d\n", iface, ISP_INTERFACE_CCP2B_PHY1);
 	/* Only the CCP2B on PHY1 is configurable. */
 	if (iface != ISP_INTERFACE_CCP2B_PHY1)
 		return;
@@ -105,6 +110,7 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
 			       enum isp_interface_type iface, bool on,
 			       bool ccp2_strobe)
 {
+	printk("csiphy_routing_cfg\n");
 	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)



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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] omap3isp: add support for CSI1 bus

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]

Hi,

On Wed, Feb 15, 2017 at 11:23:01AM +0100, Pavel Machek wrote:
> It seems csiphy_routing_cfg_3430 is not called at all. I added
> printks, but they don't trigger. If you have an idea what is going on
> there, it would help...

You added printk to csiphy_routing_cfg_3630 instead of csiphy_routing_cfg_3430
and N900 has OMAP3430. Function should be called when you start (or
stop) using the camera:

csiphy_routing_cfg_3430(...)
csiphy_routing_cfg(...)
omap3isp_csiphy_config(...)
omap3isp_csiphy_acquire(...) & omap3isp_csiphy_release(...)
ccp2_s_stream(...)

-- Sebastian

> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
> index 6b814e1..fe9303a 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -30,6 +30,8 @@ static void csiphy_routing_cfg_3630(struct isp_csiphy *phy,
>  	u32 reg;
>  	u32 shift, mode;
>  
> +	printk("routing cfg 3630: iface %d, %d\n", iface, ISP_INTERFACE_CCP2B_PHY1);
> +	
>  	regmap_read(phy->isp->syscon, phy->isp->syscon_offset, &reg);
>  
>  	switch (iface) {
> @@ -74,6 +76,9 @@ static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
>  	u32 csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
>  		| OMAP343X_CONTROL_CSIRXFE_RESET;
>  
> +	/* FIXME: can this be used instead of if (isp->revision) in ispccp2.c? */
> +	
> +	printk("routing cfg: iface %d, %d\n", iface, ISP_INTERFACE_CCP2B_PHY1);
>  	/* Only the CCP2B on PHY1 is configurable. */
>  	if (iface != ISP_INTERFACE_CCP2B_PHY1)
>  		return;
> @@ -105,6 +110,7 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
>  			       enum isp_interface_type iface, bool on,
>  			       bool ccp2_strobe)
>  {
> +	printk("csiphy_routing_cfg\n");
>  	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)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] omap3isp: add support for CSI1 bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2423 bytes --]

On Wed 2017-02-15 17:57:46, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Feb 15, 2017 at 11:23:01AM +0100, Pavel Machek wrote:
> > It seems csiphy_routing_cfg_3430 is not called at all. I added
> > printks, but they don't trigger. If you have an idea what is going on
> > there, it would help...
> 
> You added printk to csiphy_routing_cfg_3630 instead of csiphy_routing_cfg_3430
> and N900 has OMAP3430. Function should be called when you start (or
> stop) using the camera:
> 
> csiphy_routing_cfg_3430(...)
> csiphy_routing_cfg(...)
> omap3isp_csiphy_config(...)
> omap3isp_csiphy_acquire(...) & omap3isp_csiphy_release(...)
> ccp2_s_stream(...)

Take another look, I believe I added printk to both of them.

Thanks for the expected backtrace, that should help figuring it out.

> -- Sebastian
> 
> > diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
> > index 6b814e1..fe9303a 100644
> > --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> > +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> > @@ -30,6 +30,8 @@ static void csiphy_routing_cfg_3630(struct isp_csiphy *phy,
> >  	u32 reg;
> >  	u32 shift, mode;
> >  
> > +	printk("routing cfg 3630: iface %d, %d\n", iface, ISP_INTERFACE_CCP2B_PHY1);
> > +	
> >  	regmap_read(phy->isp->syscon, phy->isp->syscon_offset, &reg);
> >  
> >  	switch (iface) {
> > @@ -74,6 +76,9 @@ static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
> >  	u32 csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
> >  		| OMAP343X_CONTROL_CSIRXFE_RESET;
> >  
> > +	/* FIXME: can this be used instead of if (isp->revision) in ispccp2.c? */
> > +	
> > +	printk("routing cfg: iface %d, %d\n", iface, ISP_INTERFACE_CCP2B_PHY1);
> >  	/* Only the CCP2B on PHY1 is configurable. */
> >  	if (iface != ISP_INTERFACE_CCP2B_PHY1)
> >  		return;
> > @@ -105,6 +110,7 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
> >  			       enum isp_interface_type iface, bool on,
> >  			       bool ccp2_strobe)
> >  {
> > +	printk("csiphy_routing_cfg\n");
> >  	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)



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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCHv2] dt: bindings: Add support for CSI1 bus

[Rob Herring]

On Mon, Feb 06, 2017 at 10:49:57AM +0100, Pavel Machek wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> 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 <sakari.ailus@iki.fi>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-By: Sebastian Reichel <sre@kernel.org>

Acked-by: Rob Herring <robh@kernel.org>