[PATCH v2 0/5] i2c: adv748x: add support for CSI-2 TXA to work in 1-, 2- and 4-lane mode

[PATCH v2 0/5] i2c: adv748x: add support for CSI-2 TXA to work in 1-, 2- and 4-lane mode

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Hi,

This series allows the TXA CSI-2 transmitter of the adv748x to function
in 1-, 2- and 4- lane mode. Currently the driver fixes the hardware in
4-lane mode. The driver looks at the standard DT property 'data-lanes'
to determine which mode it should operate in.

Patch 1/5 lists the 'data-lanes' DT property as mandatory for endpoints 
describing the CSI-2 transmitters. Patch 2/5 and 3/5 refactors the 
initialization sequence of the adv748x to be able to reuse more code. 
Patch 4/5 adds the DT parsing and storing of the number of lanes. Patch
5/5 merges the TXA and TXB power up/down procedure while also taking the 
configurable number of lanes into account.

The series is based on the latest media-tree master and is tested on
Renesas M3-N in 1-, 2- and 4- lane mode.

Niklas Söderlund (5):
  dt-bindings: adv748x: make data-lanes property mandatory for CSI-2
    endpoints
  i2c: adv748x: reorder register writes for CSI-2 transmitters
    initialization
  i2c: adv748x: reuse power up sequence when initializing CSI-2
  i2c: adv748x: store number of CSI-2 lanes described in device tree
  i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter

 .../devicetree/bindings/media/i2c/adv748x.txt |   3 +
 drivers/media/i2c/adv748x/adv748x-core.c      | 207 ++++++++++--------
 drivers/media/i2c/adv748x/adv748x.h           |   1 +
 3 files changed, 122 insertions(+), 89 deletions(-)

-- 
2.19.0

[PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

The CSI-2 transmitters can use a different number of lanes to transmit
data. Make the data-lanes mandatory for the endpoints describe the
transmitters as no good default can be set to fallback on.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 Documentation/devicetree/bindings/media/i2c/adv748x.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
index 5dddc95f9cc46084..f9dac01ab795fc28 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
@@ -50,6 +50,9 @@ are numbered as follows.
 
 The digital output port nodes must contain at least one endpoint.
 
+The endpoints described in TXA and TXB ports must if present contain
+the data-lanes property as described in video-interfaces.txt.
+
 Ports are optional if they are not connected to anything at the hardware level.
 
 Example:
-- 
2.19.0

Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thursday, 4 October 2018 23:41:34 EEST Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> The CSI-2 transmitters can use a different number of lanes to transmit
> data. Make the data-lanes mandatory for the endpoints describe the

s/describe/that describe/ ?

> transmitters as no good default can be set to fallback on.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  Documentation/devicetree/bindings/media/i2c/adv748x.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index
> 5dddc95f9cc46084..f9dac01ab795fc28 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> @@ -50,6 +50,9 @@ are numbered as follows.
> 
>  The digital output port nodes must contain at least one endpoint.
> 
> +The endpoints described in TXA and TXB ports must if present contain
> +the data-lanes property as described in video-interfaces.txt.
> +

Would it make sense to merge those two paragraphs, as they refer to the same 
endpoint ?

"The digital output port nodes, when present, shall contain at least one 
endpoint. Each of those endpoints shall contain the data-lanes property as 
described in video-interfaces.txt."

(DT bindings normally use "shall" instead of "must", but that hasn't really 
been enforced.)

If you want to keep the paragraphs separate, I would recommend using "digital 
output ports" instead of "TXA and TXB" in the second paragraph for consistency 
(or the other way around).

I'm fine with any of the above option, so please pick your favourite, and add

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

>  Ports are optional if they are not connected to anything at the hardware
> level.
> 
>  Example:

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints

[Laurent Pinchart]

Hello again,

On Friday, 5 October 2018 00:42:17 EEST Laurent Pinchart wrote:
> On Thursday, 4 October 2018 23:41:34 EEST Niklas Söderlund wrote:
> > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > The CSI-2 transmitters can use a different number of lanes to transmit
> > data. Make the data-lanes mandatory for the endpoints describe the
> 
> s/describe/that describe/ ?
> 
> > transmitters as no good default can be set to fallback on.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> > 
> >  Documentation/devicetree/bindings/media/i2c/adv748x.txt | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index
> > 5dddc95f9cc46084..f9dac01ab795fc28 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > @@ -50,6 +50,9 @@ are numbered as follows.
> > 
> >  The digital output port nodes must contain at least one endpoint.
> > 
> > +The endpoints described in TXA and TXB ports must if present contain
> > +the data-lanes property as described in video-interfaces.txt.
> > +
> 
> Would it make sense to merge those two paragraphs, as they refer to the same
> endpoint ?
> 
> "The digital output port nodes, when present, shall contain at least one
> endpoint. Each of those endpoints shall contain the data-lanes property as
> described in video-interfaces.txt."
> 
> (DT bindings normally use "shall" instead of "must", but that hasn't really
> been enforced.)
> 
> If you want to keep the paragraphs separate, I would recommend using
> "digital output ports" instead of "TXA and TXB" in the second paragraph for
> consistency (or the other way around).
> 
> I'm fine with any of the above option, so please pick your favourite, and
> add
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I just realized that TXB only supports a single data lane, so we may want not 
to have a data-lanes property for TXB.

> >  Ports are optional if they are not connected to anything at the hardware
> > 
> > level.
> > 
> >  Example:

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints

[jacopo mondi]

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

Hi Laurent, Niklas,

On Fri, Oct 05, 2018 at 01:00:47AM +0300, Laurent Pinchart wrote:
> Hello again,
>
> On Friday, 5 October 2018 00:42:17 EEST Laurent Pinchart wrote:
> > On Thursday, 4 October 2018 23:41:34 EEST Niklas Söderlund wrote:
> > > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >
> > > The CSI-2 transmitters can use a different number of lanes to transmit
> > > data. Make the data-lanes mandatory for the endpoints describe the
> >
> > s/describe/that describe/ ?
> >
> > > transmitters as no good default can be set to fallback on.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >
> > >  Documentation/devicetree/bindings/media/i2c/adv748x.txt | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > > b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index
> > > 5dddc95f9cc46084..f9dac01ab795fc28 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > > +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > > @@ -50,6 +50,9 @@ are numbered as follows.
> > >
> > >  The digital output port nodes must contain at least one endpoint.
> > >
> > > +The endpoints described in TXA and TXB ports must if present contain
> > > +the data-lanes property as described in video-interfaces.txt.
> > > +
> >
> > Would it make sense to merge those two paragraphs, as they refer to the same
> > endpoint ?
> >
> > "The digital output port nodes, when present, shall contain at least one
> > endpoint. Each of those endpoints shall contain the data-lanes property as
> > described in video-interfaces.txt."
> >
> > (DT bindings normally use "shall" instead of "must", but that hasn't really
> > been enforced.)
> >
> > If you want to keep the paragraphs separate, I would recommend using
> > "digital output ports" instead of "TXA and TXB" in the second paragraph for
> > consistency (or the other way around).
> >
> > I'm fine with any of the above option, so please pick your favourite, and
> > add
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> I just realized that TXB only supports a single data lane, so we may want not
> to have a data-lanes property for TXB.
>

Isn't it better to restrict its value to <1> but make it mandatory
anyhow? I understand conceptually that property should not be there,
as it has a single acceptable value, but otherwise we need to traeat
differently the two output ports, in documentation and code.

Why not inserting a paragraph with the required endpoint properties
description?

Eg:

 Required endpoint properties:
 - data-lanes: See "video-interfaces.txt" for description. The property
   is mandatory for CSI-2 output endpoints and the accepted values
   depends on which endpoint the property is applied to:
   - TXA: accepted values are <1>, <2>, <4>
   - TXB: accepted value is <1>

> > >  Ports are optional if they are not connected to anything at the hardware
> > >
> > > level.
> > >
> > >  Example:
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

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

Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints

[Laurent Pinchart]

Hi Jacopo,

On Friday, 5 October 2018 11:49:45 EEST jacopo mondi wrote:
> On Fri, Oct 05, 2018 at 01:00:47AM +0300, Laurent Pinchart wrote:
> > On Friday, 5 October 2018 00:42:17 EEST Laurent Pinchart wrote:
> >> On Thursday, 4 October 2018 23:41:34 EEST Niklas Söderlund wrote:
> >>> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>> 
> >>> The CSI-2 transmitters can use a different number of lanes to transmit
> >>> data. Make the data-lanes mandatory for the endpoints describe the
> >> 
> >> s/describe/that describe/ ?
> >> 
> >>> transmitters as no good default can be set to fallback on.
> >>> 
> >>> Signed-off-by: Niklas Söderlund
> >>> <niklas.soderlund+renesas@ragnatech.se>
> >>> ---
> >>> 
> >>>  Documentation/devicetree/bindings/media/i2c/adv748x.txt | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>> 
> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> >>> b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index
> >>> 5dddc95f9cc46084..f9dac01ab795fc28 100644
> >>> --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> >>> +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> >>> @@ -50,6 +50,9 @@ are numbered as follows.
> >>> 
> >>>  The digital output port nodes must contain at least one endpoint.
> >>> 
> >>> +The endpoints described in TXA and TXB ports must if present contain
> >>> +the data-lanes property as described in video-interfaces.txt.
> >>> +
> >> 
> >> Would it make sense to merge those two paragraphs, as they refer to the
> >> same endpoint ?
> >> 
> >> "The digital output port nodes, when present, shall contain at least one
> >> endpoint. Each of those endpoints shall contain the data-lanes property
> >> as described in video-interfaces.txt."
> >> 
> >> (DT bindings normally use "shall" instead of "must", but that hasn't
> >> really been enforced.)
> >> 
> >> If you want to keep the paragraphs separate, I would recommend using
> >> "digital output ports" instead of "TXA and TXB" in the second paragraph
> >> for consistency (or the other way around).
> >> 
> >> I'm fine with any of the above option, so please pick your favourite,
> >> and add
> >> 
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > I just realized that TXB only supports a single data lane, so we may want
> > not to have a data-lanes property for TXB.
> 
> Isn't it better to restrict its value to <1> but make it mandatory
> anyhow? I understand conceptually that property should not be there,
> as it has a single acceptable value, but otherwise we need to traeat
> differently the two output ports, in documentation and code.

The two ports are different, so I wouldn't be shocked if we handled them 
differently :-) I believe it would actually reduce the code size (and save CPU 
cycles at runtime).

> Why not inserting a paragraph with the required endpoint properties
> description?
> 
> Eg:
> 
>  Required endpoint properties:
>  - data-lanes: See "video-interfaces.txt" for description. The property
>    is mandatory for CSI-2 output endpoints and the accepted values
>    depends on which endpoint the property is applied to:
>    - TXA: accepted values are <1>, <2>, <4>
>    - TXB: accepted value is <1>
> 
> >>>  Ports are optional if they are not connected to anything at the
> >>>  hardware level.
> >>> 
> >>>  Example:

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints

[Niklas Söderlund]

Hi Laurent, Jacopo

Thanks for your comments.

On 2018-10-05 13:02:53 +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Friday, 5 October 2018 11:49:45 EEST jacopo mondi wrote:
> > On Fri, Oct 05, 2018 at 01:00:47AM +0300, Laurent Pinchart wrote:
> > > On Friday, 5 October 2018 00:42:17 EEST Laurent Pinchart wrote:
> > >> On Thursday, 4 October 2018 23:41:34 EEST Niklas S�derlund wrote:
> > >>> From: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > >>> 
> > >>> The CSI-2 transmitters can use a different number of lanes to transmit
> > >>> data. Make the data-lanes mandatory for the endpoints describe the
> > >> 
> > >> s/describe/that describe/ ?
> > >> 
> > >>> transmitters as no good default can be set to fallback on.
> > >>> 
> > >>> Signed-off-by: Niklas S�derlund
> > >>> <niklas.soderlund+renesas@ragnatech.se>
> > >>> ---
> > >>> 
> > >>>  Documentation/devicetree/bindings/media/i2c/adv748x.txt | 3 +++
> > >>>  1 file changed, 3 insertions(+)
> > >>> 
> > >>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > >>> b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index
> > >>> 5dddc95f9cc46084..f9dac01ab795fc28 100644
> > >>> --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > >>> +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > >>> @@ -50,6 +50,9 @@ are numbered as follows.
> > >>> 
> > >>>  The digital output port nodes must contain at least one endpoint.
> > >>> 
> > >>> +The endpoints described in TXA and TXB ports must if present contain
> > >>> +the data-lanes property as described in video-interfaces.txt.
> > >>> +
> > >> 
> > >> Would it make sense to merge those two paragraphs, as they refer to the
> > >> same endpoint ?
> > >> 
> > >> "The digital output port nodes, when present, shall contain at least one
> > >> endpoint. Each of those endpoints shall contain the data-lanes property
> > >> as described in video-interfaces.txt."
> > >> 
> > >> (DT bindings normally use "shall" instead of "must", but that hasn't
> > >> really been enforced.)
> > >> 
> > >> If you want to keep the paragraphs separate, I would recommend using
> > >> "digital output ports" instead of "TXA and TXB" in the second paragraph
> > >> for consistency (or the other way around).
> > >> 
> > >> I'm fine with any of the above option, so please pick your favourite,
> > >> and add
> > >> 
> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > I just realized that TXB only supports a single data lane, so we may want
> > > not to have a data-lanes property for TXB.
> > 
> > Isn't it better to restrict its value to <1> but make it mandatory
> > anyhow? I understand conceptually that property should not be there,
> > as it has a single acceptable value, but otherwise we need to traeat
> > differently the two output ports, in documentation and code.
> 
> The two ports are different, so I wouldn't be shocked if we handled them 
> differently :-) I believe it would actually reduce the code size (and save CPU 
> cycles at runtime).

I'm leaning towards Jacopo on this that I think it's more clear to treat 
the two the same. I also think it adheres to the notion that DT shall 
describe hardware which I think this adds value. Also I only have 
datasheets for adv7482 so I can't be sure other adv748x don't support 
more then one lane on TXB.

I do not feel strongly about this so I'm open to treating them 
differently. I might keep it as is for v3 if no one screams to loud :-)

> 
> > Why not inserting a paragraph with the required endpoint properties
> > description?
> > 
> > Eg:
> > 
> >  Required endpoint properties:
> >  - data-lanes: See "video-interfaces.txt" for description. The property
> >    is mandatory for CSI-2 output endpoints and the accepted values
> >    depends on which endpoint the property is applied to:
> >    - TXA: accepted values are <1>, <2>, <4>
> >    - TXB: accepted value is <1>
> > 
> > >>>  Ports are optional if they are not connected to anything at the
> > >>>  hardware level.
> > >>> 
> > >>>  Example:
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints

[Niklas Söderlund]

Hi Laurent, Jacopo

Thanks for your comments.

On 2018-10-05 13:02:53 +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Friday, 5 October 2018 11:49:45 EEST jacopo mondi wrote:
> > On Fri, Oct 05, 2018 at 01:00:47AM +0300, Laurent Pinchart wrote:
> > > On Friday, 5 October 2018 00:42:17 EEST Laurent Pinchart wrote:
> > >> On Thursday, 4 October 2018 23:41:34 EEST Niklas Söderlund wrote:
> > >>> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >>> 
> > >>> The CSI-2 transmitters can use a different number of lanes to transmit
> > >>> data. Make the data-lanes mandatory for the endpoints describe the
> > >> 
> > >> s/describe/that describe/ ?
> > >> 
> > >>> transmitters as no good default can be set to fallback on.
> > >>> 
> > >>> Signed-off-by: Niklas Söderlund
> > >>> <niklas.soderlund+renesas@ragnatech.se>
> > >>> ---
> > >>> 
> > >>>  Documentation/devicetree/bindings/media/i2c/adv748x.txt | 3 +++
> > >>>  1 file changed, 3 insertions(+)
> > >>> 
> > >>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > >>> b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index
> > >>> 5dddc95f9cc46084..f9dac01ab795fc28 100644
> > >>> --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > >>> +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > >>> @@ -50,6 +50,9 @@ are numbered as follows.
> > >>> 
> > >>>  The digital output port nodes must contain at least one endpoint.
> > >>> 
> > >>> +The endpoints described in TXA and TXB ports must if present contain
> > >>> +the data-lanes property as described in video-interfaces.txt.
> > >>> +
> > >> 
> > >> Would it make sense to merge those two paragraphs, as they refer to the
> > >> same endpoint ?
> > >> 
> > >> "The digital output port nodes, when present, shall contain at least one
> > >> endpoint. Each of those endpoints shall contain the data-lanes property
> > >> as described in video-interfaces.txt."
> > >> 
> > >> (DT bindings normally use "shall" instead of "must", but that hasn't
> > >> really been enforced.)
> > >> 
> > >> If you want to keep the paragraphs separate, I would recommend using
> > >> "digital output ports" instead of "TXA and TXB" in the second paragraph
> > >> for consistency (or the other way around).
> > >> 
> > >> I'm fine with any of the above option, so please pick your favourite,
> > >> and add
> > >> 
> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > I just realized that TXB only supports a single data lane, so we may want
> > > not to have a data-lanes property for TXB.
> > 
> > Isn't it better to restrict its value to <1> but make it mandatory
> > anyhow? I understand conceptually that property should not be there,
> > as it has a single acceptable value, but otherwise we need to traeat
> > differently the two output ports, in documentation and code.
> 
> The two ports are different, so I wouldn't be shocked if we handled them 
> differently :-) I believe it would actually reduce the code size (and save CPU 
> cycles at runtime).

I'm leaning towards Jacopo on this that I think it's more clear to treat 
the two the same. I also think it adheres to the notion that DT shall 
describe hardware which I think this adds value. Also I only have 
datasheets for adv7482 so I can't be sure other adv748x don't support 
more then one lane on TXB.

I do not feel strongly about this so I'm open to treating them 
differently. I might keep it as is for v3 if no one screams to loud :-)

> 
> > Why not inserting a paragraph with the required endpoint properties
> > description?
> > 
> > Eg:
> > 
> >  Required endpoint properties:
> >  - data-lanes: See "video-interfaces.txt" for description. The property
> >    is mandatory for CSI-2 output endpoints and the accepted values
> >    depends on which endpoint the property is applied to:
> >    - TXA: accepted values are <1>, <2>, <4>
> >    - TXB: accepted value is <1>
> > 
> > >>>  Ports are optional if they are not connected to anything at the
> > >>>  hardware level.
> > >>> 
> > >>>  Example:
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints

[Kieran Bingham]

Hi Niklas,

On 02/11/2018 10:34, Niklas Söderlund wrote:
> Hi Laurent, Jacopo
> 
> Thanks for your comments.
> 
> On 2018-10-05 13:02:53 +0300, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> On Friday, 5 October 2018 11:49:45 EEST jacopo mondi wrote:
>>> On Fri, Oct 05, 2018 at 01:00:47AM +0300, Laurent Pinchart wrote:
>>>> On Friday, 5 October 2018 00:42:17 EEST Laurent Pinchart wrote:
>>>>> On Thursday, 4 October 2018 23:41:34 EEST Niklas Söderlund wrote:
>>>>>> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>>>>
>>>>>> The CSI-2 transmitters can use a different number of lanes to transmit
>>>>>> data. Make the data-lanes mandatory for the endpoints describe the
>>>>>
>>>>> s/describe/that describe/ ?
>>>>>
>>>>>> transmitters as no good default can be set to fallback on.
>>>>>>
>>>>>> Signed-off-by: Niklas Söderlund
>>>>>> <niklas.soderlund+renesas@ragnatech.se>
>>>>>> ---
>>>>>>
>>>>>>  Documentation/devicetree/bindings/media/i2c/adv748x.txt | 3 +++
>>>>>>  1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
>>>>>> b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index
>>>>>> 5dddc95f9cc46084..f9dac01ab795fc28 100644
>>>>>> --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
>>>>>> @@ -50,6 +50,9 @@ are numbered as follows.
>>>>>>
>>>>>>  The digital output port nodes must contain at least one endpoint.
>>>>>>
>>>>>> +The endpoints described in TXA and TXB ports must if present contain
>>>>>> +the data-lanes property as described in video-interfaces.txt.
>>>>>> +
>>>>>
>>>>> Would it make sense to merge those two paragraphs, as they refer to the
>>>>> same endpoint ?
>>>>>
>>>>> "The digital output port nodes, when present, shall contain at least one
>>>>> endpoint. Each of those endpoints shall contain the data-lanes property
>>>>> as described in video-interfaces.txt."
>>>>>
>>>>> (DT bindings normally use "shall" instead of "must", but that hasn't
>>>>> really been enforced.)
>>>>>
>>>>> If you want to keep the paragraphs separate, I would recommend using
>>>>> "digital output ports" instead of "TXA and TXB" in the second paragraph
>>>>> for consistency (or the other way around).
>>>>>
>>>>> I'm fine with any of the above option, so please pick your favourite,
>>>>> and add
>>>>>
>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>
>>>> I just realized that TXB only supports a single data lane, so we may want
>>>> not to have a data-lanes property for TXB.
>>>
>>> Isn't it better to restrict its value to <1> but make it mandatory
>>> anyhow? I understand conceptually that property should not be there,
>>> as it has a single acceptable value, but otherwise we need to traeat
>>> differently the two output ports, in documentation and code.
>>
>> The two ports are different, so I wouldn't be shocked if we handled them 
>> differently :-) I believe it would actually reduce the code size (and save CPU 
>> cycles at runtime).
> 
> I'm leaning towards Jacopo on this that I think it's more clear to treat 
> the two the same. I also think it adheres to the notion that DT shall 
> describe hardware which I think this adds value. Also I only have 
> datasheets for adv7482 so I can't be sure other adv748x don't support 
> more then one lane on TXB.
> 
> I do not feel strongly about this so I'm open to treating them 
> differently. I might keep it as is for v3 if no one screams to loud :-)

I personally think that TXA and TXB are 'the same' entities, just in
different configurations, and so I would say they are 'the same' too.

For reference, from the adv748x.h header file:

 * The ADV748x range of receivers have the following configurations:
 *
 *                  Analog   HDMI  MHL  4-Lane  1-Lane
 *                    In      In         CSI     CSI
 *       ADV7480               X    X     X
 *       ADV7481      X        X    X     X       X
 *       ADV7482      X        X          X       X


So both the ADV7481 and ADV7482 have both a TXA and a TXB, where the
ADV7480 has only the TXA.

TXB is always only 'one-lane'

--
Kieran



> 
>>
>>> Why not inserting a paragraph with the required endpoint properties
>>> description?
>>>
>>> Eg:
>>>
>>>  Required endpoint properties:
>>>  - data-lanes: See "video-interfaces.txt" for description. The property
>>>    is mandatory for CSI-2 output endpoints and the accepted values
>>>    depends on which endpoint the property is applied to:
>>>    - TXA: accepted values are <1>, <2>, <4>
>>>    - TXB: accepted value is <1>
>>>
>>>>>>  Ports are optional if they are not connected to anything at the
>>>>>>  hardware level.
>>>>>>
>>>>>>  Example:
>>
>> -- 
>> Regards,
>>
>> Laurent Pinchart
>>

[PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reorder the initialization order of registers to allow for refactoring.
The move could have been done at the same time as the refactoring but
since the documentation about some registers involved are missing do it
separately.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 6854d898fdd1f192..721ed6552bc1cde6 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -383,8 +383,6 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
 	{ADV748X_PAGE_IO, 0x0c, 0xe0},	/* Enable LLC_DLL & Double LLC Timing */
 	{ADV748X_PAGE_IO, 0x0e, 0xdd},	/* LLC/PIX/SPI PINS TRISTATED AUD */
 
-	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
-	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
 	{ADV748X_PAGE_TXA, 0xdb, 0x10},	/* ADI Required Write */
 	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
 	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
@@ -392,6 +390,9 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
 	{ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI Required Write */
 	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
 
+	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
+	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
+
 	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
 	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
 	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
@@ -435,17 +436,18 @@ static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
 	{ADV748X_PAGE_SDP, 0x31, 0x12},	/* ADI Required Write */
 	{ADV748X_PAGE_SDP, 0xe6, 0x4f},  /* V bit end pos manually in NTSC */
 
-	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
-	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
 	{ADV748X_PAGE_TXB, 0xd2, 0x40},	/* ADI Required Write */
 	{ADV748X_PAGE_TXB, 0xc4, 0x0a},	/* ADI Required Write */
 	{ADV748X_PAGE_TXB, 0x71, 0x33},	/* ADI Required Write */
 	{ADV748X_PAGE_TXB, 0x72, 0x11},	/* ADI Required Write */
 	{ADV748X_PAGE_TXB, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
+
+	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
+	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
+
 	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
 	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
 	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
-
 	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
 	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
 	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
-- 
2.19.0

Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thursday, 4 October 2018 23:41:35 EEST Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Reorder the initialization order of registers to allow for refactoring.
> The move could have been done at the same time as the refactoring but
> since the documentation about some registers involved are missing do it
> separately.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 6854d898fdd1f192..721ed6552bc1cde6 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -383,8 +383,6 @@ static const struct adv748x_reg_value
> adv748x_init_txa_4lane[] = { {ADV748X_PAGE_IO, 0x0c, 0xe0},	/* Enable
> LLC_DLL & Double LLC Timing */ {ADV748X_PAGE_IO, 0x0e, 0xdd},	/*
> LLC/PIX/SPI PINS TRISTATED AUD */
> 
> -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
>  	{ADV748X_PAGE_TXA, 0xdb, 0x10},	/* ADI Required Write */
>  	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
>  	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
> @@ -392,6 +390,9 @@ static const struct adv748x_reg_value
> adv748x_init_txa_4lane[] = { {ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI
> Required Write */
>  	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> 
> +	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> +	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> +
>  	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
>  	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
>  	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> @@ -435,17 +436,18 @@ static const struct adv748x_reg_value
> adv748x_init_txb_1lane[] = { {ADV748X_PAGE_SDP, 0x31, 0x12},	/* ADI
> Required Write */
>  	{ADV748X_PAGE_SDP, 0xe6, 0x4f},  /* V bit end pos manually in NTSC */
> 
> -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> -	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
>  	{ADV748X_PAGE_TXB, 0xd2, 0x40},	/* ADI Required Write */
>  	{ADV748X_PAGE_TXB, 0xc4, 0x0a},	/* ADI Required Write */
>  	{ADV748X_PAGE_TXB, 0x71, 0x33},	/* ADI Required Write */
>  	{ADV748X_PAGE_TXB, 0x72, 0x11},	/* ADI Required Write */
>  	{ADV748X_PAGE_TXB, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> +
> +	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> +	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> +

This is pretty hard to review, as there's a bunch of undocumented register 
writes. I think the first write is safe, as the tables are written immediately 
following a software reset, and the default value of the register is 0x81 
(CSI-TX disabled, 1 lane). The second write, however, enables usage of the 
computed DPHY parameters, and I don't know whether the undocumented register 
writes in-between may interact with that.

That being said, this change enables further important refactoring, so I'm 
tempted to accept it. I assume you've tested it and haven't noticed a 
regression. The part that still bothers me in particular is that the write to 
register 0xf0 just above this takes the DPHY out of power down according to 
the datasheet, and I wonder whether at that point the DPHY might not react to 
that information. Have you analyzed the power-up sequence in section 9.5.1 of 
the hardware manual ? I wonder whether the dphy_pwdn shouldn't be handled in 
the power up and power down sequences, which might involve also moving the 
above four (and five for TXA) undocumented writes to the power up sequence as 
well.

>  	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
>  	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
>  	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> -
>  	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
>  	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
>  	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-10-05 01:36:11 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thursday, 4 October 2018 23:41:35 EEST Niklas Söderlund wrote:
> > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > Reorder the initialization order of registers to allow for refactoring.
> > The move could have been done at the same time as the refactoring but
> > since the documentation about some registers involved are missing do it
> > separately.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > b/drivers/media/i2c/adv748x/adv748x-core.c index
> > 6854d898fdd1f192..721ed6552bc1cde6 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -383,8 +383,6 @@ static const struct adv748x_reg_value
> > adv748x_init_txa_4lane[] = { {ADV748X_PAGE_IO, 0x0c, 0xe0},	/* Enable
> > LLC_DLL & Double LLC Timing */ {ADV748X_PAGE_IO, 0x0e, 0xdd},	/*
> > LLC/PIX/SPI PINS TRISTATED AUD */
> > 
> > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> >  	{ADV748X_PAGE_TXA, 0xdb, 0x10},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
> > @@ -392,6 +390,9 @@ static const struct adv748x_reg_value
> > adv748x_init_txa_4lane[] = { {ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI
> > Required Write */
> >  	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> > 
> > +	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > +	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > +
> >  	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > @@ -435,17 +436,18 @@ static const struct adv748x_reg_value
> > adv748x_init_txb_1lane[] = { {ADV748X_PAGE_SDP, 0x31, 0x12},	/* ADI
> > Required Write */
> >  	{ADV748X_PAGE_SDP, 0xe6, 0x4f},  /* V bit end pos manually in NTSC */
> > 
> > -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > -	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> >  	{ADV748X_PAGE_TXB, 0xd2, 0x40},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXB, 0xc4, 0x0a},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXB, 0x71, 0x33},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXB, 0x72, 0x11},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXB, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> > +
> > +	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > +	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> > +
> 
> This is pretty hard to review, as there's a bunch of undocumented register 
> writes. I think the first write is safe, as the tables are written immediately 
> following a software reset, and the default value of the register is 0x81 
> (CSI-TX disabled, 1 lane). The second write, however, enables usage of the 
> computed DPHY parameters, and I don't know whether the undocumented register 
> writes in-between may interact with that.

I agree it's hard to grasp all implications with undocumented registers 
involved. That is why I choose to do it in a separate commit so if 
regressions are found it could be bisectable to this change.

> 
> That being said, this change enables further important refactoring, so I'm 
> tempted to accept it. I assume you've tested it and haven't noticed a 
> regression. The part that still bothers me in particular is that the write to 
> register 0xf0 just above this takes the DPHY out of power down according to 
> the datasheet, and I wonder whether at that point the DPHY might not react to 
> that information. Have you analyzed the power-up sequence in section 9.5.1 of 
> the hardware manual ? I wonder whether the dphy_pwdn shouldn't be handled in 
> the power up and power down sequences, which might involve also moving the 
> above four (and five for TXA) undocumented writes to the power up sequence as 
> well.

I looked at the documentation and ran lots of tests based on this change 
and noticed no change in behavior.

> 
> >  	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > -
> >  	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> >  	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> >  	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-10-05 01:36:11 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thursday, 4 October 2018 23:41:35 EEST Niklas S�derlund wrote:
> > From: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > Reorder the initialization order of registers to allow for refactoring.
> > The move could have been done at the same time as the refactoring but
> > since the documentation about some registers involved are missing do it
> > separately.
> > 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > b/drivers/media/i2c/adv748x/adv748x-core.c index
> > 6854d898fdd1f192..721ed6552bc1cde6 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -383,8 +383,6 @@ static const struct adv748x_reg_value
> > adv748x_init_txa_4lane[] = { {ADV748X_PAGE_IO, 0x0c, 0xe0},	/* Enable
> > LLC_DLL & Double LLC Timing */ {ADV748X_PAGE_IO, 0x0e, 0xdd},	/*
> > LLC/PIX/SPI PINS TRISTATED AUD */
> > 
> > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> >  	{ADV748X_PAGE_TXA, 0xdb, 0x10},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
> > @@ -392,6 +390,9 @@ static const struct adv748x_reg_value
> > adv748x_init_txa_4lane[] = { {ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI
> > Required Write */
> >  	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> > 
> > +	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > +	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > +
> >  	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > @@ -435,17 +436,18 @@ static const struct adv748x_reg_value
> > adv748x_init_txb_1lane[] = { {ADV748X_PAGE_SDP, 0x31, 0x12},	/* ADI
> > Required Write */
> >  	{ADV748X_PAGE_SDP, 0xe6, 0x4f},  /* V bit end pos manually in NTSC */
> > 
> > -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > -	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> >  	{ADV748X_PAGE_TXB, 0xd2, 0x40},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXB, 0xc4, 0x0a},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXB, 0x71, 0x33},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXB, 0x72, 0x11},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXB, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> > +
> > +	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > +	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> > +
> 
> This is pretty hard to review, as there's a bunch of undocumented register 
> writes. I think the first write is safe, as the tables are written immediately 
> following a software reset, and the default value of the register is 0x81 
> (CSI-TX disabled, 1 lane). The second write, however, enables usage of the 
> computed DPHY parameters, and I don't know whether the undocumented register 
> writes in-between may interact with that.

I agree it's hard to grasp all implications with undocumented registers 
involved. That is why I choose to do it in a separate commit so if 
regressions are found it could be bisectable to this change.

> 
> That being said, this change enables further important refactoring, so I'm 
> tempted to accept it. I assume you've tested it and haven't noticed a 
> regression. The part that still bothers me in particular is that the write to 
> register 0xf0 just above this takes the DPHY out of power down according to 
> the datasheet, and I wonder whether at that point the DPHY might not react to 
> that information. Have you analyzed the power-up sequence in section 9.5.1 of 
> the hardware manual ? I wonder whether the dphy_pwdn shouldn't be handled in 
> the power up and power down sequences, which might involve also moving the 
> above four (and five for TXA) undocumented writes to the power up sequence as 
> well.

I looked at the documentation and ran lots of tests based on this change 
and noticed no change in behavior.

> 
> >  	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
> >  	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > -
> >  	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> >  	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> >  	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization

[Laurent Pinchart]

Hi Niklas,

On Friday, 2 November 2018 12:38:34 EET Niklas Söderlund wrote:
> On 2018-10-05 01:36:11 +0300, Laurent Pinchart wrote:
> > On Thursday, 4 October 2018 23:41:35 EEST Niklas Söderlund wrote:
> > > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > 
> > > Reorder the initialization order of registers to allow for refactoring.
> > > The move could have been done at the same time as the refactoring but
> > > since the documentation about some registers involved are missing do it
> > > separately.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > > 
> > >  drivers/media/i2c/adv748x/adv748x-core.c | 12 +++++++-----
> > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > > b/drivers/media/i2c/adv748x/adv748x-core.c index
> > > 6854d898fdd1f192..721ed6552bc1cde6 100644
> > > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > > @@ -383,8 +383,6 @@ static const struct adv748x_reg_value
> > > adv748x_init_txa_4lane[] = { {ADV748X_PAGE_IO, 0x0c, 0xe0},	/* Enable
> > > LLC_DLL & Double LLC Timing */ {ADV748X_PAGE_IO, 0x0e, 0xdd},	/*
> > > LLC/PIX/SPI PINS TRISTATED AUD */
> > > 
> > > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > > -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > > 
> > >  	{ADV748X_PAGE_TXA, 0xdb, 0x10},	/* ADI Required Write */
> > >  	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
> > >  	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
> > > 
> > > @@ -392,6 +390,9 @@ static const struct adv748x_reg_value
> > > adv748x_init_txa_4lane[] = { {ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI
> > > Required Write */
> > > 
> > >  	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> > > 
> > > +	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > > +	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > > +
> > > 
> > >  	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> > >  	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
> > >  	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > > 
> > > @@ -435,17 +436,18 @@ static const struct adv748x_reg_value
> > > adv748x_init_txb_1lane[] = { {ADV748X_PAGE_SDP, 0x31, 0x12},	/* ADI
> > > Required Write */
> > > 
> > >  	{ADV748X_PAGE_SDP, 0xe6, 0x4f},  /* V bit end pos manually in NTSC 
*/
> > > 
> > > -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > > -	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> > > 
> > >  	{ADV748X_PAGE_TXB, 0xd2, 0x40},	/* ADI Required Write */
> > >  	{ADV748X_PAGE_TXB, 0xc4, 0x0a},	/* ADI Required Write */
> > >  	{ADV748X_PAGE_TXB, 0x71, 0x33},	/* ADI Required Write */
> > >  	{ADV748X_PAGE_TXB, 0x72, 0x11},	/* ADI Required Write */
> > >  	{ADV748X_PAGE_TXB, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> > > 
> > > +
> > > +	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > > +	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> > > +
> > 
> > This is pretty hard to review, as there's a bunch of undocumented register
> > writes. I think the first write is safe, as the tables are written
> > immediately following a software reset, and the default value of the
> > register is 0x81 (CSI-TX disabled, 1 lane). The second write, however,
> > enables usage of the computed DPHY parameters, and I don't know whether
> > the undocumented register writes in-between may interact with that.
> 
> I agree it's hard to grasp all implications with undocumented registers
> involved. That is why I choose to do it in a separate commit so if
> regressions are found it could be bisectable to this change.
> 
> > That being said, this change enables further important refactoring, so I'm
> > tempted to accept it. I assume you've tested it and haven't noticed a
> > regression. The part that still bothers me in particular is that the write
> > to register 0xf0 just above this takes the DPHY out of power down
> > according to the datasheet, and I wonder whether at that point the DPHY
> > might not react to that information. Have you analyzed the power-up
> > sequence in section 9.5.1 of the hardware manual ? I wonder whether the
> > dphy_pwdn shouldn't be handled in the power up and power down sequences,
> > which might involve also moving the above four (and five for TXA)
> > undocumented writes to the power up sequence as well.
> 
> I looked at the documentation and ran lots of tests based on this change
> and noticed no change in behavior.

As a last test, could you try programming completely invalid values to the 
undocumented registers before taking the DPHY out of power down ? I'm worried 
that things might work just because the registers happen to contain acceptable 
values when the DPHY is powered up, and that it might break later because the 
sequence of operations resulting from starting and stopping the video streams 
in different configurations would end up taking the DPHY out of power down 
with invalid values in those registers.

> >>  	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> >>  	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
> >>  	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> >> -
> >>  	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> >>  	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> >>  	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-11-02 13:43:21 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Friday, 2 November 2018 12:38:34 EET Niklas Söderlund wrote:
> > On 2018-10-05 01:36:11 +0300, Laurent Pinchart wrote:
> > > On Thursday, 4 October 2018 23:41:35 EEST Niklas Söderlund wrote:
> > > > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > 
> > > > Reorder the initialization order of registers to allow for refactoring.
> > > > The move could have been done at the same time as the refactoring but
> > > > since the documentation about some registers involved are missing do it
> > > > separately.
> > > > 
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > ---
> > > > 
> > > >  drivers/media/i2c/adv748x/adv748x-core.c | 12 +++++++-----
> > > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > > > b/drivers/media/i2c/adv748x/adv748x-core.c index
> > > > 6854d898fdd1f192..721ed6552bc1cde6 100644
> > > > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > > > @@ -383,8 +383,6 @@ static const struct adv748x_reg_value
> > > > adv748x_init_txa_4lane[] = { {ADV748X_PAGE_IO, 0x0c, 0xe0},	/* Enable
> > > > LLC_DLL & Double LLC Timing */ {ADV748X_PAGE_IO, 0x0e, 0xdd},	/*
> > > > LLC/PIX/SPI PINS TRISTATED AUD */
> > > > 
> > > > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > > > -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > > > 
> > > >  	{ADV748X_PAGE_TXA, 0xdb, 0x10},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
> > > > 
> > > > @@ -392,6 +390,9 @@ static const struct adv748x_reg_value
> > > > adv748x_init_txa_4lane[] = { {ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI
> > > > Required Write */
> > > > 
> > > >  	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> > > > 
> > > > +	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > > > +	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > > > +
> > > > 
> > > >  	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > > > 
> > > > @@ -435,17 +436,18 @@ static const struct adv748x_reg_value
> > > > adv748x_init_txb_1lane[] = { {ADV748X_PAGE_SDP, 0x31, 0x12},	/* ADI
> > > > Required Write */
> > > > 
> > > >  	{ADV748X_PAGE_SDP, 0xe6, 0x4f},  /* V bit end pos manually in NTSC 
> */
> > > > 
> > > > -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > > > -	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> > > > 
> > > >  	{ADV748X_PAGE_TXB, 0xd2, 0x40},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXB, 0xc4, 0x0a},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXB, 0x71, 0x33},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXB, 0x72, 0x11},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXB, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> > > > 
> > > > +
> > > > +	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > > > +	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> > > > +
> > > 
> > > This is pretty hard to review, as there's a bunch of undocumented register
> > > writes. I think the first write is safe, as the tables are written
> > > immediately following a software reset, and the default value of the
> > > register is 0x81 (CSI-TX disabled, 1 lane). The second write, however,
> > > enables usage of the computed DPHY parameters, and I don't know whether
> > > the undocumented register writes in-between may interact with that.
> > 
> > I agree it's hard to grasp all implications with undocumented registers
> > involved. That is why I choose to do it in a separate commit so if
> > regressions are found it could be bisectable to this change.
> > 
> > > That being said, this change enables further important refactoring, so I'm
> > > tempted to accept it. I assume you've tested it and haven't noticed a
> > > regression. The part that still bothers me in particular is that the write
> > > to register 0xf0 just above this takes the DPHY out of power down
> > > according to the datasheet, and I wonder whether at that point the DPHY
> > > might not react to that information. Have you analyzed the power-up
> > > sequence in section 9.5.1 of the hardware manual ? I wonder whether the
> > > dphy_pwdn shouldn't be handled in the power up and power down sequences,
> > > which might involve also moving the above four (and five for TXA)
> > > undocumented writes to the power up sequence as well.
> > 
> > I looked at the documentation and ran lots of tests based on this change
> > and noticed no change in behavior.
> 
> As a last test, could you try programming completely invalid values to the 
> undocumented registers before taking the DPHY out of power down ? I'm worried 
> that things might work just because the registers happen to contain acceptable 
> values when the DPHY is powered up, and that it might break later because the 
> sequence of operations resulting from starting and stopping the video streams 
> in different configurations would end up taking the DPHY out of power down 
> with invalid values in those registers.

I did the test you describe above and the ordering have no effect on the 
end result.

However looking at the problem with fresh eyes I realised that the more 
correct option is to take your first suggestion and bring the 
undocumented registers into the power up function and that is what I 
will do in v3. Thanks for pointing this out!

> 
> > >>  	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> > >>  	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
> > >>  	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > >> -
> > >>  	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> > >>  	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> > >>  	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-11-02 13:43:21 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Friday, 2 November 2018 12:38:34 EET Niklas S�derlund wrote:
> > On 2018-10-05 01:36:11 +0300, Laurent Pinchart wrote:
> > > On Thursday, 4 October 2018 23:41:35 EEST Niklas S�derlund wrote:
> > > > From: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > > > 
> > > > Reorder the initialization order of registers to allow for refactoring.
> > > > The move could have been done at the same time as the refactoring but
> > > > since the documentation about some registers involved are missing do it
> > > > separately.
> > > > 
> > > > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > > > ---
> > > > 
> > > >  drivers/media/i2c/adv748x/adv748x-core.c | 12 +++++++-----
> > > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > > > b/drivers/media/i2c/adv748x/adv748x-core.c index
> > > > 6854d898fdd1f192..721ed6552bc1cde6 100644
> > > > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > > > @@ -383,8 +383,6 @@ static const struct adv748x_reg_value
> > > > adv748x_init_txa_4lane[] = { {ADV748X_PAGE_IO, 0x0c, 0xe0},	/* Enable
> > > > LLC_DLL & Double LLC Timing */ {ADV748X_PAGE_IO, 0x0e, 0xdd},	/*
> > > > LLC/PIX/SPI PINS TRISTATED AUD */
> > > > 
> > > > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > > > -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > > > 
> > > >  	{ADV748X_PAGE_TXA, 0xdb, 0x10},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
> > > > 
> > > > @@ -392,6 +390,9 @@ static const struct adv748x_reg_value
> > > > adv748x_init_txa_4lane[] = { {ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI
> > > > Required Write */
> > > > 
> > > >  	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> > > > 
> > > > +	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > > > +	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > > > +
> > > > 
> > > >  	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > > > 
> > > > @@ -435,17 +436,18 @@ static const struct adv748x_reg_value
> > > > adv748x_init_txb_1lane[] = { {ADV748X_PAGE_SDP, 0x31, 0x12},	/* ADI
> > > > Required Write */
> > > > 
> > > >  	{ADV748X_PAGE_SDP, 0xe6, 0x4f},  /* V bit end pos manually in NTSC 
> */
> > > > 
> > > > -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > > > -	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> > > > 
> > > >  	{ADV748X_PAGE_TXB, 0xd2, 0x40},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXB, 0xc4, 0x0a},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXB, 0x71, 0x33},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXB, 0x72, 0x11},	/* ADI Required Write */
> > > >  	{ADV748X_PAGE_TXB, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> > > > 
> > > > +
> > > > +	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > > > +	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> > > > +
> > > 
> > > This is pretty hard to review, as there's a bunch of undocumented register
> > > writes. I think the first write is safe, as the tables are written
> > > immediately following a software reset, and the default value of the
> > > register is 0x81 (CSI-TX disabled, 1 lane). The second write, however,
> > > enables usage of the computed DPHY parameters, and I don't know whether
> > > the undocumented register writes in-between may interact with that.
> > 
> > I agree it's hard to grasp all implications with undocumented registers
> > involved. That is why I choose to do it in a separate commit so if
> > regressions are found it could be bisectable to this change.
> > 
> > > That being said, this change enables further important refactoring, so I'm
> > > tempted to accept it. I assume you've tested it and haven't noticed a
> > > regression. The part that still bothers me in particular is that the write
> > > to register 0xf0 just above this takes the DPHY out of power down
> > > according to the datasheet, and I wonder whether at that point the DPHY
> > > might not react to that information. Have you analyzed the power-up
> > > sequence in section 9.5.1 of the hardware manual ? I wonder whether the
> > > dphy_pwdn shouldn't be handled in the power up and power down sequences,
> > > which might involve also moving the above four (and five for TXA)
> > > undocumented writes to the power up sequence as well.
> > 
> > I looked at the documentation and ran lots of tests based on this change
> > and noticed no change in behavior.
> 
> As a last test, could you try programming completely invalid values to the 
> undocumented registers before taking the DPHY out of power down ? I'm worried 
> that things might work just because the registers happen to contain acceptable 
> values when the DPHY is powered up, and that it might break later because the 
> sequence of operations resulting from starting and stopping the video streams 
> in different configurations would end up taking the DPHY out of power down 
> with invalid values in those registers.

I did the test you describe above and the ordering have no effect on the 
end result.

However looking at the problem with fresh eyes I realised that the more 
correct option is to take your first suggestion and bring the 
undocumented registers into the power up function and that is what I 
will do in v3. Thanks for pointing this out!

> 
> > >>  	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> > >>  	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
> > >>  	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > >> -
> > >>  	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> > >>  	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> > >>  	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Regards,
Niklas S�derlund

[PATCH v2 3/5] i2c: adv748x: reuse power up sequence when initializing CSI-2

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Instead of duplicate the register writes to power on the CSI-2
transmitter when initialization the hardware reuse the dedicated power
control functions.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 28 ++----------------------
 1 file changed, 2 insertions(+), 26 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 721ed6552bc1cde6..41cc0cdd6a5fcef5 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -390,19 +390,6 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
 	{ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI Required Write */
 	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
 
-	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
-	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
-
-	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
-	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
-	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
-	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
-	{ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
-	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
-	{ADV748X_PAGE_TXA, 0xc1, 0x2b},	/* ADI Required Write */
-	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
-	{ADV748X_PAGE_TXA, 0x31, 0x80},	/* ADI Required Write */
-
 	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
 };
 
@@ -442,19 +429,6 @@ static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
 	{ADV748X_PAGE_TXB, 0x72, 0x11},	/* ADI Required Write */
 	{ADV748X_PAGE_TXB, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
 
-	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
-	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
-
-	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
-	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
-	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
-	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
-	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
-	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
-	{ADV748X_PAGE_TXB, 0xc1, 0x2b},	/* ADI Required Write */
-	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
-	{ADV748X_PAGE_TXB, 0x31, 0x80},	/* ADI Required Write */
-
 	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
 };
 
@@ -476,6 +450,7 @@ static int adv748x_reset(struct adv748x_state *state)
 	if (ret)
 		return ret;
 
+	adv748x_tx_power(&state->txa, 1);
 	adv748x_tx_power(&state->txa, 0);
 
 	/* Init and power down TXB */
@@ -483,6 +458,7 @@ static int adv748x_reset(struct adv748x_state *state)
 	if (ret)
 		return ret;
 
+	adv748x_tx_power(&state->txb, 1);
 	adv748x_tx_power(&state->txb, 0);
 
 	/* Disable chip powerdown & Enable HDMI Rx block */
-- 
2.19.0

Re: [PATCH v2 3/5] i2c: adv748x: reuse power up sequence when initializing CSI-2

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thursday, 4 October 2018 23:41:36 EEST Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Instead of duplicate the register writes to power on the CSI-2
> transmitter when initialization the hardware reuse the dedicated power
> control functions.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 28 ++----------------------
>  1 file changed, 2 insertions(+), 26 deletions(-)

Nice diffstat.

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

Please see below for an additional comment.

> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 721ed6552bc1cde6..41cc0cdd6a5fcef5 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -390,19 +390,6 @@ static const struct adv748x_reg_value
> adv748x_init_txa_4lane[] = { {ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI
> Required Write */
>  	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> 
> -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> -
> -	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> -	{ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> -	{ADV748X_PAGE_TXA, 0xc1, 0x2b},	/* ADI Required Write */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> -	{ADV748X_PAGE_TXA, 0x31, 0x80},	/* ADI Required Write */
> -
>  	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
>  };
> 
> @@ -442,19 +429,6 @@ static const struct adv748x_reg_value
> adv748x_init_txb_1lane[] = { {ADV748X_PAGE_TXB, 0x72, 0x11},	/* ADI
> Required Write */
>  	{ADV748X_PAGE_TXB, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> 
> -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> -	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> -
> -	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> -	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> -	{ADV748X_PAGE_TXB, 0xc1, 0x2b},	/* ADI Required Write */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> -	{ADV748X_PAGE_TXB, 0x31, 0x80},	/* ADI Required Write */
> -
>  	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
>  };
> 
> @@ -476,6 +450,7 @@ static int adv748x_reset(struct adv748x_state *state)
>  	if (ret)
>  		return ret;
> 
> +	adv748x_tx_power(&state->txa, 1);
>  	adv748x_tx_power(&state->txa, 0);

This makes me think there's room for further improvement :-)

>  	/* Init and power down TXB */
> @@ -483,6 +458,7 @@ static int adv748x_reset(struct adv748x_state *state)
>  	if (ret)
>  		return ret;
> 
> +	adv748x_tx_power(&state->txb, 1);
>  	adv748x_tx_power(&state->txb, 0);
> 
>  	/* Disable chip powerdown & Enable HDMI Rx block */


-- 
Regards,

Laurent Pinchart

[PATCH v2 4/5] i2c: adv748x: store number of CSI-2 lanes described in device tree

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

The adv748x CSI-2 transmitters TXA and TXB can use different number of
lanes to transmit data. In order to be able to configure the device
correctly this information need to be parsed from device tree and stored
in each TX private data structure.

TXA supports 1, 2 and 4 lanes while TXB supports 1 lane.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
* Changes since v1
- Use %u instead of %d to print unsigned int.
- Fix spelling in commit message, thanks Laurent.
---
 drivers/media/i2c/adv748x/adv748x-core.c | 49 ++++++++++++++++++++++++
 drivers/media/i2c/adv748x/adv748x.h      |  1 +
 2 files changed, 50 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 41cc0cdd6a5fcef5..3836dd3025d6ffb7 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -23,6 +23,7 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-dv-timings.h>
+#include <media/v4l2-fwnode.h>
 #include <media/v4l2-ioctl.h>
 
 #include "adv748x.h"
@@ -523,12 +524,55 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, struct adv748x_state *state,
 	sd->entity.ops = &adv748x_media_ops;
 }
 
+static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
+				    unsigned int port,
+				    struct device_node *ep)
+{
+	struct v4l2_fwnode_endpoint vep;
+	unsigned int num_lanes;
+	int ret;
+
+	if (port != ADV748X_PORT_TXA && port != ADV748X_PORT_TXB)
+		return 0;
+
+	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &vep);
+	if (ret)
+		return ret;
+
+	num_lanes = vep.bus.mipi_csi2.num_data_lanes;
+
+	if (vep.base.port == ADV748X_PORT_TXA) {
+		if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
+			adv_err(state, "TXA: Invalid number (%u) of lanes\n",
+				num_lanes);
+			return -EINVAL;
+		}
+
+		state->txa.num_lanes = num_lanes;
+		adv_dbg(state, "TXA: using %u lanes\n", state->txa.num_lanes);
+	}
+
+	if (vep.base.port == ADV748X_PORT_TXB) {
+		if (num_lanes != 1) {
+			adv_err(state, "TXB: Invalid number (%u) of lanes\n",
+				num_lanes);
+			return -EINVAL;
+		}
+
+		state->txb.num_lanes = num_lanes;
+		adv_dbg(state, "TXB: using %u lanes\n", state->txb.num_lanes);
+	}
+
+	return 0;
+}
+
 static int adv748x_parse_dt(struct adv748x_state *state)
 {
 	struct device_node *ep_np = NULL;
 	struct of_endpoint ep;
 	bool out_found = false;
 	bool in_found = false;
+	int ret;
 
 	for_each_endpoint_of_node(state->dev->of_node, ep_np) {
 		of_graph_parse_endpoint(ep_np, &ep);
@@ -559,6 +603,11 @@ static int adv748x_parse_dt(struct adv748x_state *state)
 			in_found = true;
 		else
 			out_found = true;
+
+		/* Store number of CSI-2 lanes used for TXA and TXB. */
+		ret = adv748x_parse_csi2_lanes(state, ep.port, ep_np);
+		if (ret)
+			return ret;
 	}
 
 	return in_found && out_found ? 0 : -ENODEV;
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 39c2fdc3b41667d8..b482c7fe6957eb85 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -79,6 +79,7 @@ struct adv748x_csi2 {
 	struct v4l2_mbus_framefmt format;
 	unsigned int page;
 	unsigned int port;
+	unsigned int num_lanes;
 
 	struct media_pad pads[ADV748X_CSI2_NR_PADS];
 	struct v4l2_ctrl_handler ctrl_hdl;
-- 
2.19.0

Re: [PATCH v2 4/5] i2c: adv748x: store number of CSI-2 lanes described in device tree

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thursday, 4 October 2018 23:41:37 EEST Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> The adv748x CSI-2 transmitters TXA and TXB can use different number of
> lanes to transmit data. In order to be able to configure the device
> correctly this information need to be parsed from device tree and stored
> in each TX private data structure.
> 
> TXA supports 1, 2 and 4 lanes while TXB supports 1 lane.

TXB doesn't really support a configurable number of lanes then, does it ? :-) 
Should we skip the parsing for TXB ?

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> ---
> * Changes since v1
> - Use %u instead of %d to print unsigned int.
> - Fix spelling in commit message, thanks Laurent.
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 49 ++++++++++++++++++++++++
>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
>  2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 41cc0cdd6a5fcef5..3836dd3025d6ffb7 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -23,6 +23,7 @@
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-dv-timings.h>
> +#include <media/v4l2-fwnode.h>
>  #include <media/v4l2-ioctl.h>
> 
>  #include "adv748x.h"
> @@ -523,12 +524,55 @@ void adv748x_subdev_init(struct v4l2_subdev *sd,
> struct adv748x_state *state, sd->entity.ops = &adv748x_media_ops;
>  }
> 
> +static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
> +				    unsigned int port,
> +				    struct device_node *ep)
> +{
> +	struct v4l2_fwnode_endpoint vep;
> +	unsigned int num_lanes;
> +	int ret;
> +
> +	if (port != ADV748X_PORT_TXA && port != ADV748X_PORT_TXB)
> +		return 0;
> +
> +	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &vep);
> +	if (ret)
> +		return ret;
> +
> +	num_lanes = vep.bus.mipi_csi2.num_data_lanes;
> +
> +	if (vep.base.port == ADV748X_PORT_TXA) {
> +		if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
> +			adv_err(state, "TXA: Invalid number (%u) of lanes\n",
> +				num_lanes);
> +			return -EINVAL;
> +		}
> +
> +		state->txa.num_lanes = num_lanes;
> +		adv_dbg(state, "TXA: using %u lanes\n", state->txa.num_lanes);
> +	}
> +
> +	if (vep.base.port == ADV748X_PORT_TXB) {
> +		if (num_lanes != 1) {
> +			adv_err(state, "TXB: Invalid number (%u) of lanes\n",
> +				num_lanes);
> +			return -EINVAL;
> +		}
> +
> +		state->txb.num_lanes = num_lanes;
> +		adv_dbg(state, "TXB: using %u lanes\n", state->txb.num_lanes);
> +	}
> +
> +	return 0;
> +}
> +
>  static int adv748x_parse_dt(struct adv748x_state *state)
>  {
>  	struct device_node *ep_np = NULL;
>  	struct of_endpoint ep;
>  	bool out_found = false;
>  	bool in_found = false;
> +	int ret;
> 
>  	for_each_endpoint_of_node(state->dev->of_node, ep_np) {
>  		of_graph_parse_endpoint(ep_np, &ep);
> @@ -559,6 +603,11 @@ static int adv748x_parse_dt(struct adv748x_state
> *state) in_found = true;
>  		else
>  			out_found = true;
> +
> +		/* Store number of CSI-2 lanes used for TXA and TXB. */
> +		ret = adv748x_parse_csi2_lanes(state, ep.port, ep_np);
> +		if (ret)
> +			return ret;
>  	}
> 
>  	return in_found && out_found ? 0 : -ENODEV;
> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h index
> 39c2fdc3b41667d8..b482c7fe6957eb85 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -79,6 +79,7 @@ struct adv748x_csi2 {
>  	struct v4l2_mbus_framefmt format;
>  	unsigned int page;
>  	unsigned int port;
> +	unsigned int num_lanes;
> 
>  	struct media_pad pads[ADV748X_CSI2_NR_PADS];
>  	struct v4l2_ctrl_handler ctrl_hdl;

-- 
Regards,

Laurent Pinchart

[PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

The driver fixed the TXA CSI-2 transmitter in 4-lane mode while it could
operate using 1-, 2- and 4-lanes. Update the driver to support all modes
the hardware does.

The driver make use of large tables of static register/value writes when
powering up/down the TXA and TXB transmitters which include the write to
the NUM_LANES register. By converting the tables into functions and
using parameters the power up/down functions for TXA and TXB power
up/down can be merged and used for both transmitters.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

---
* Changes since v1
- Convert tables of register/value writes into functions instead of
  intercepting and modifying the writes to the NUM_LANES register.
---
 drivers/media/i2c/adv748x/adv748x-core.c | 132 ++++++++++++-----------
 1 file changed, 67 insertions(+), 65 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 3836dd3025d6ffb7..fe29781368a3a6b6 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -125,6 +125,16 @@ int adv748x_write(struct adv748x_state *state, u8 page, u8 reg, u8 value)
 	return regmap_write(state->regmap[page], reg, value);
 }
 
+static int adv748x_write_check(struct adv748x_state *state, u8 page, u8 reg,
+			       u8 value, int *error)
+{
+	if (*error)
+		return *error;
+
+	*error = adv748x_write(state, page, reg, value);
+	return *error;
+}
+
 /* adv748x_write_block(): Write raw data with a maximum of I2C_SMBUS_BLOCK_MAX
  * size to one or more registers.
  *
@@ -231,69 +241,63 @@ static int adv748x_write_regs(struct adv748x_state *state,
  * TXA and TXB
  */
 
-static const struct adv748x_reg_value adv748x_power_up_txa_4lane[] = {
-
-	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
-	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
-
-	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
-	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
-	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
-	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
-	{ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
-	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
-	{ADV748X_PAGE_TXA, 0xc1, 0x2b},	/* ADI Required Write */
-	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
-	{ADV748X_PAGE_TXA, 0x31, 0x80},	/* ADI Required Write */
-
-	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
-};
-
-static const struct adv748x_reg_value adv748x_power_down_txa_4lane[] = {
-
-	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
-	{ADV748X_PAGE_TXA, 0x1e, 0x00},	/* ADI Required Write */
-	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
-	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
-	{ADV748X_PAGE_TXA, 0xc1, 0x3b},	/* ADI Required Write */
-
-	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
-};
-
-static const struct adv748x_reg_value adv748x_power_up_txb_1lane[] = {
-
-	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
-	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
-
-	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
-	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
-	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
-	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
-	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
-	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
-	{ADV748X_PAGE_TXB, 0xc1, 0x2b},	/* ADI Required Write */
-	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
-	{ADV748X_PAGE_TXB, 0x31, 0x80},	/* ADI Required Write */
-
-	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
-};
-
-static const struct adv748x_reg_value adv748x_power_down_txb_1lane[] = {
-
-	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
-	{ADV748X_PAGE_TXB, 0x1e, 0x00},	/* ADI Required Write */
-	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
-	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
-	{ADV748X_PAGE_TXB, 0xc1, 0x3b},	/* ADI Required Write */
-
-	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
-};
+static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
+{
+	struct adv748x_state *state = tx->state;
+	u8 page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
+	int ret = 0;
+
+	/* Enable 4-lane MIPI */
+	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
+
+	/* Set Auto DPHY Timing */
+	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
+
+	/* ADI Required Writes*/
+	adv748x_write_check(state, page, 0x31, 0x82, &ret);
+	adv748x_write_check(state, page, 0x1e, 0x40, &ret);
+
+	/* i2c_mipi_pll_en - 1'b1 */
+	adv748x_write_check(state, page, 0xda, 0x01, &ret);
+	usleep_range(2000, 2500);
+
+	/* Power-up CSI-TX */
+	adv748x_write_check(state, page, 0x00, 0x20 | tx->num_lanes, &ret);
+	usleep_range(1000, 1500);
+
+	/* ADI Required Writes*/
+	adv748x_write_check(state, page, 0xc1, 0x2b, &ret);
+	usleep_range(1000, 1500);
+	adv748x_write_check(state, page, 0x31, 0x80, &ret);
+
+	return ret;
+}
+
+static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
+{
+	struct adv748x_state *state = tx->state;
+	u8 page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
+	int ret = 0;
+
+	/* ADI Required Writes */
+	adv748x_write_check(state, page, 0x31, 0x82, &ret);
+	adv748x_write_check(state, page, 0x1e, 0x00, &ret);
+
+	/* Enable 4-lane MIPI */
+	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
+
+	/* i2c_mipi_pll_en - 1'b1 */
+	adv748x_write_check(state, page, 0xda, 0x01, &ret);
+
+	/* ADI Required Write */
+	adv748x_write_check(state, page, 0xc1, 0x3b, &ret);
+
+	return ret;
+}
 
 int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
 {
-	struct adv748x_state *state = tx->state;
-	const struct adv748x_reg_value *reglist;
-	int val;
+	int val, ret;
 
 	if (!is_tx_enabled(tx))
 		return 0;
@@ -311,13 +315,11 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
 			"Enabling with unknown bit set");
 
 	if (on)
-		reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
-				       adv748x_power_up_txb_1lane;
+		ret = adv748x_power_up_tx(tx);
 	else
-		reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
-				       adv748x_power_down_txb_1lane;
+		ret = adv748x_power_down_tx(tx);
 
-	return adv748x_write_regs(state, reglist);
+	return ret;
 }
 
 /* -----------------------------------------------------------------------------
-- 
2.19.0

Re: [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thursday, 4 October 2018 23:41:38 EEST Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> The driver fixed the TXA CSI-2 transmitter in 4-lane mode while it could
> operate using 1-, 2- and 4-lanes. Update the driver to support all modes
> the hardware does.
> 
> The driver make use of large tables of static register/value writes when
> powering up/down the TXA and TXB transmitters which include the write to
> the NUM_LANES register. By converting the tables into functions and
> using parameters the power up/down functions for TXA and TXB power
> up/down can be merged and used for both transmitters.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> ---
> * Changes since v1
> - Convert tables of register/value writes into functions instead of
>   intercepting and modifying the writes to the NUM_LANES register.
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 132 ++++++++++++-----------
>  1 file changed, 67 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 3836dd3025d6ffb7..fe29781368a3a6b6 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -125,6 +125,16 @@ int adv748x_write(struct adv748x_state *state, u8 page,
> u8 reg, u8 value) return regmap_write(state->regmap[page], reg, value);
>  }
> 
> +static int adv748x_write_check(struct adv748x_state *state, u8 page, u8
> reg,
> +			       u8 value, int *error)
> +{
> +	if (*error)
> +		return *error;

We could also store the error as a write_error field in the state structure to 
avoid passing it around as a parameter all the time. I'll let you and Kieran 
decide.

> +
> +	*error = adv748x_write(state, page, reg, value);
> +	return *error;
> +}
> +
>  /* adv748x_write_block(): Write raw data with a maximum of
> I2C_SMBUS_BLOCK_MAX * size to one or more registers.
>   *
> @@ -231,69 +241,63 @@ static int adv748x_write_regs(struct adv748x_state
> *state, * TXA and TXB
>   */
> 
> -static const struct adv748x_reg_value adv748x_power_up_txa_4lane[] = {
> -
> -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> -
> -	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> -	{ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> -	{ADV748X_PAGE_TXA, 0xc1, 0x2b},	/* ADI Required Write */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> -	{ADV748X_PAGE_TXA, 0x31, 0x80},	/* ADI Required Write */
> -
> -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> -};
> -
> -static const struct adv748x_reg_value adv748x_power_down_txa_4lane[] = {
> -
> -	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0x1e, 0x00},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> -	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> -	{ADV748X_PAGE_TXA, 0xc1, 0x3b},	/* ADI Required Write */
> -
> -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> -};
> -
> -static const struct adv748x_reg_value adv748x_power_up_txb_1lane[] = {
> -
> -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> -	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> -
> -	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> -	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> -	{ADV748X_PAGE_TXB, 0xc1, 0x2b},	/* ADI Required Write */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> -	{ADV748X_PAGE_TXB, 0x31, 0x80},	/* ADI Required Write */
> -
> -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> -};
> -
> -static const struct adv748x_reg_value adv748x_power_down_txb_1lane[] = {
> -
> -	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXB, 0x1e, 0x00},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> -	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> -	{ADV748X_PAGE_TXB, 0xc1, 0x3b},	/* ADI Required Write */
> -
> -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> -};
> +static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
> +{
> +	struct adv748x_state *state = tx->state;
> +	u8 page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
> +	int ret = 0;
> +
> +	/* Enable 4-lane MIPI */

Not necessarily 4 lanes anymore.

> +	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
> +
> +	/* Set Auto DPHY Timing */
> +	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
> +
> +	/* ADI Required Writes*/

s/\*\// *\//

> +	adv748x_write_check(state, page, 0x31, 0x82, &ret);
> +	adv748x_write_check(state, page, 0x1e, 0x40, &ret);
> +
> +	/* i2c_mipi_pll_en - 1'b1 */
> +	adv748x_write_check(state, page, 0xda, 0x01, &ret);
> +	usleep_range(2000, 2500);
> +
> +	/* Power-up CSI-TX */
> +	adv748x_write_check(state, page, 0x00, 0x20 | tx->num_lanes, &ret);
> +	usleep_range(1000, 1500);
> +
> +	/* ADI Required Writes*/
> +	adv748x_write_check(state, page, 0xc1, 0x2b, &ret);
> +	usleep_range(1000, 1500);
> +	adv748x_write_check(state, page, 0x31, 0x80, &ret);
> +
> +	return ret;
> +}
> +
> +static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
> +{
> +	struct adv748x_state *state = tx->state;
> +	u8 page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
> +	int ret = 0;
> +
> +	/* ADI Required Writes */
> +	adv748x_write_check(state, page, 0x31, 0x82, &ret);
> +	adv748x_write_check(state, page, 0x1e, 0x00, &ret);
> +
> +	/* Enable 4-lane MIPI */

Same here.

> +	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
> +
> +	/* i2c_mipi_pll_en - 1'b1 */
> +	adv748x_write_check(state, page, 0xda, 0x01, &ret);
> +
> +	/* ADI Required Write */
> +	adv748x_write_check(state, page, 0xc1, 0x3b, &ret);
> +
> +	return ret;
> +}
> 
>  int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>  {
> -	struct adv748x_state *state = tx->state;
> -	const struct adv748x_reg_value *reglist;
> -	int val;
> +	int val, ret;
> 
>  	if (!is_tx_enabled(tx))
>  		return 0;
> @@ -311,13 +315,11 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> "Enabling with unknown bit set");
> 
>  	if (on)
> -		reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
> -				       adv748x_power_up_txb_1lane;
> +		ret = adv748x_power_up_tx(tx);
>  	else
> -		reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
> -				       adv748x_power_down_txb_1lane;
> +		ret = adv748x_power_down_tx(tx);

You could also return directly here and avoid adding a ret variable.

> 
> -	return adv748x_write_regs(state, reglist);
> +	return ret;
>  }
> 
>  /* ------------------------------------------------------------------------

Very nice change overall. With the above small issues fixed,

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter

[jacopo mondi]

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

Hi Niklas,

On Thu, Oct 04, 2018 at 10:41:38PM +0200, Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> The driver fixed the TXA CSI-2 transmitter in 4-lane mode while it could
> operate using 1-, 2- and 4-lanes. Update the driver to support all modes
> the hardware does.
>
> The driver make use of large tables of static register/value writes when
> powering up/down the TXA and TXB transmitters which include the write to
> the NUM_LANES register. By converting the tables into functions and
> using parameters the power up/down functions for TXA and TXB power
> up/down can be merged and used for both transmitters.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> ---
> * Changes since v1
> - Convert tables of register/value writes into functions instead of
>   intercepting and modifying the writes to the NUM_LANES register.
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 132 ++++++++++++-----------
>  1 file changed, 67 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 3836dd3025d6ffb7..fe29781368a3a6b6 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -125,6 +125,16 @@ int adv748x_write(struct adv748x_state *state, u8 page, u8 reg, u8 value)
>  	return regmap_write(state->regmap[page], reg, value);
>  }
>
> +static int adv748x_write_check(struct adv748x_state *state, u8 page, u8 reg,
> +			       u8 value, int *error)

Am I wrong or the return error is ignored and could be dropped?

> +{
> +	if (*error)
> +		return *error;
> +
> +	*error = adv748x_write(state, page, reg, value);
> +	return *error;
> +}
> +
>  /* adv748x_write_block(): Write raw data with a maximum of I2C_SMBUS_BLOCK_MAX
>   * size to one or more registers.
>   *
> @@ -231,69 +241,63 @@ static int adv748x_write_regs(struct adv748x_state *state,
>   * TXA and TXB
>   */
>
> -static const struct adv748x_reg_value adv748x_power_up_txa_4lane[] = {
> -
> -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> -
> -	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> -	{ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> -	{ADV748X_PAGE_TXA, 0xc1, 0x2b},	/* ADI Required Write */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> -	{ADV748X_PAGE_TXA, 0x31, 0x80},	/* ADI Required Write */
> -
> -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> -};
> -
> -static const struct adv748x_reg_value adv748x_power_down_txa_4lane[] = {
> -
> -	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0x1e, 0x00},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> -	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> -	{ADV748X_PAGE_TXA, 0xc1, 0x3b},	/* ADI Required Write */
> -
> -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> -};
> -
> -static const struct adv748x_reg_value adv748x_power_up_txb_1lane[] = {
> -
> -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> -	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> -
> -	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> -	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> -	{ADV748X_PAGE_TXB, 0xc1, 0x2b},	/* ADI Required Write */
> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> -	{ADV748X_PAGE_TXB, 0x31, 0x80},	/* ADI Required Write */
> -
> -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> -};
> -
> -static const struct adv748x_reg_value adv748x_power_down_txb_1lane[] = {
> -
> -	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXB, 0x1e, 0x00},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> -	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> -	{ADV748X_PAGE_TXB, 0xc1, 0x3b},	/* ADI Required Write */
> -
> -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> -};
> +static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
> +{
> +	struct adv748x_state *state = tx->state;
> +	u8 page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
> +	int ret = 0;
> +
> +	/* Enable 4-lane MIPI */
> +	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
> +
> +	/* Set Auto DPHY Timing */
> +	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
> +
> +	/* ADI Required Writes*/
> +	adv748x_write_check(state, page, 0x31, 0x82, &ret);
> +	adv748x_write_check(state, page, 0x1e, 0x40, &ret);
> +
> +	/* i2c_mipi_pll_en - 1'b1 */
> +	adv748x_write_check(state, page, 0xda, 0x01, &ret);
> +	usleep_range(2000, 2500);
> +
> +	/* Power-up CSI-TX */
> +	adv748x_write_check(state, page, 0x00, 0x20 | tx->num_lanes, &ret);
> +	usleep_range(1000, 1500);
> +
> +	/* ADI Required Writes*/
> +	adv748x_write_check(state, page, 0xc1, 0x2b, &ret);
> +	usleep_range(1000, 1500);
> +	adv748x_write_check(state, page, 0x31, 0x80, &ret);
> +
> +	return ret;
> +}
> +
> +static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
> +{
> +	struct adv748x_state *state = tx->state;
> +	u8 page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
> +	int ret = 0;
> +
> +	/* ADI Required Writes */
> +	adv748x_write_check(state, page, 0x31, 0x82, &ret);
> +	adv748x_write_check(state, page, 0x1e, 0x00, &ret);
> +
> +	/* Enable 4-lane MIPI */
> +	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
> +
> +	/* i2c_mipi_pll_en - 1'b1 */
> +	adv748x_write_check(state, page, 0xda, 0x01, &ret);
> +
> +	/* ADI Required Write */
> +	adv748x_write_check(state, page, 0xc1, 0x3b, &ret);
> +
> +	return ret;
> +}

This one looks good to me

>
>  int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>  {
> -	struct adv748x_state *state = tx->state;
> -	const struct adv748x_reg_value *reglist;
> -	int val;
> +	int val, ret;
>
>  	if (!is_tx_enabled(tx))
>  		return 0;
> @@ -311,13 +315,11 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>  			"Enabling with unknown bit set");
>
>  	if (on)
> -		reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
> -				       adv748x_power_up_txb_1lane;
> +		ret = adv748x_power_up_tx(tx);
>  	else
> -		reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
> -				       adv748x_power_down_txb_1lane;
> +		ret = adv748x_power_down_tx(tx);
>
> -	return adv748x_write_regs(state, reglist);
> +	return ret;

As Laurent suggested, or even
        return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);

Thanks
   j

>  }
>
>  /* -----------------------------------------------------------------------------
> --
> 2.19.0
>

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

Re: [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2018-10-05 12:46:15 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Thu, Oct 04, 2018 at 10:41:38PM +0200, Niklas Söderlund wrote:
> > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > The driver fixed the TXA CSI-2 transmitter in 4-lane mode while it could
> > operate using 1-, 2- and 4-lanes. Update the driver to support all modes
> > the hardware does.
> >
> > The driver make use of large tables of static register/value writes when
> > powering up/down the TXA and TXB transmitters which include the write to
> > the NUM_LANES register. By converting the tables into functions and
> > using parameters the power up/down functions for TXA and TXB power
> > up/down can be merged and used for both transmitters.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > ---
> > * Changes since v1
> > - Convert tables of register/value writes into functions instead of
> >   intercepting and modifying the writes to the NUM_LANES register.
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 132 ++++++++++++-----------
> >  1 file changed, 67 insertions(+), 65 deletions(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index 3836dd3025d6ffb7..fe29781368a3a6b6 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -125,6 +125,16 @@ int adv748x_write(struct adv748x_state *state, u8 page, u8 reg, u8 value)
> >  	return regmap_write(state->regmap[page], reg, value);
> >  }
> >
> > +static int adv748x_write_check(struct adv748x_state *state, u8 page, u8 reg,
> > +			       u8 value, int *error)
> 
> Am I wrong or the return error is ignored and could be dropped?

No it's used to reduce boiler plate code, see adv748x_power_up_tx() 
bellow for one example.

    int ret = 0;
    ...
    adv748x_write_check(state, page, 0x31, 0x82, &ret);
    adv748x_write_check(state, page, 0x1e, 0x40, &ret);
    ...
    return ret;

If the first adv748x_write_check() fails all later calls to 
adv748x_write_check() will do nothing. This is do avoid the rather hard 
to read:

    ret = adv748x_write(state, page, 0x31, 0x82);
    if (ret)
        return ret;

    ret = adv748x_write(state, page, 0x1e, 0x40);
    if (ret)
        return ret;

> 
> > +{
> > +	if (*error)
> > +		return *error;
> > +
> > +	*error = adv748x_write(state, page, reg, value);
> > +	return *error;
> > +}
> > +
> >  /* adv748x_write_block(): Write raw data with a maximum of I2C_SMBUS_BLOCK_MAX
> >   * size to one or more registers.
> >   *
> > @@ -231,69 +241,63 @@ static int adv748x_write_regs(struct adv748x_state *state,
> >   * TXA and TXB
> >   */
> >
> > -static const struct adv748x_reg_value adv748x_power_up_txa_4lane[] = {
> > -
> > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > -
> > -	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > -	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> > -	{ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
> > -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> > -	{ADV748X_PAGE_TXA, 0xc1, 0x2b},	/* ADI Required Write */
> > -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> > -	{ADV748X_PAGE_TXA, 0x31, 0x80},	/* ADI Required Write */
> > -
> > -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> > -};
> > -
> > -static const struct adv748x_reg_value adv748x_power_down_txa_4lane[] = {
> > -
> > -	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXA, 0x1e, 0x00},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > -	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > -	{ADV748X_PAGE_TXA, 0xc1, 0x3b},	/* ADI Required Write */
> > -
> > -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> > -};
> > -
> > -static const struct adv748x_reg_value adv748x_power_up_txb_1lane[] = {
> > -
> > -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > -	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> > -
> > -	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > -	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> > -	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> > -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> > -	{ADV748X_PAGE_TXB, 0xc1, 0x2b},	/* ADI Required Write */
> > -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> > -	{ADV748X_PAGE_TXB, 0x31, 0x80},	/* ADI Required Write */
> > -
> > -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> > -};
> > -
> > -static const struct adv748x_reg_value adv748x_power_down_txb_1lane[] = {
> > -
> > -	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXB, 0x1e, 0x00},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > -	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > -	{ADV748X_PAGE_TXB, 0xc1, 0x3b},	/* ADI Required Write */
> > -
> > -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> > -};
> > +static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
> > +{
> > +	struct adv748x_state *state = tx->state;
> > +	u8 page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
> > +	int ret = 0;
> > +
> > +	/* Enable 4-lane MIPI */
> > +	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
> > +
> > +	/* Set Auto DPHY Timing */
> > +	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
> > +
> > +	/* ADI Required Writes*/
> > +	adv748x_write_check(state, page, 0x31, 0x82, &ret);
> > +	adv748x_write_check(state, page, 0x1e, 0x40, &ret);
> > +
> > +	/* i2c_mipi_pll_en - 1'b1 */
> > +	adv748x_write_check(state, page, 0xda, 0x01, &ret);
> > +	usleep_range(2000, 2500);
> > +
> > +	/* Power-up CSI-TX */
> > +	adv748x_write_check(state, page, 0x00, 0x20 | tx->num_lanes, &ret);
> > +	usleep_range(1000, 1500);
> > +
> > +	/* ADI Required Writes*/
> > +	adv748x_write_check(state, page, 0xc1, 0x2b, &ret);
> > +	usleep_range(1000, 1500);
> > +	adv748x_write_check(state, page, 0x31, 0x80, &ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
> > +{
> > +	struct adv748x_state *state = tx->state;
> > +	u8 page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
> > +	int ret = 0;
> > +
> > +	/* ADI Required Writes */
> > +	adv748x_write_check(state, page, 0x31, 0x82, &ret);
> > +	adv748x_write_check(state, page, 0x1e, 0x00, &ret);
> > +
> > +	/* Enable 4-lane MIPI */
> > +	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
> > +
> > +	/* i2c_mipi_pll_en - 1'b1 */
> > +	adv748x_write_check(state, page, 0xda, 0x01, &ret);
> > +
> > +	/* ADI Required Write */
> > +	adv748x_write_check(state, page, 0xc1, 0x3b, &ret);
> > +
> > +	return ret;
> > +}
> 
> This one looks good to me
> 
> >
> >  int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> >  {
> > -	struct adv748x_state *state = tx->state;
> > -	const struct adv748x_reg_value *reglist;
> > -	int val;
> > +	int val, ret;
> >
> >  	if (!is_tx_enabled(tx))
> >  		return 0;
> > @@ -311,13 +315,11 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> >  			"Enabling with unknown bit set");
> >
> >  	if (on)
> > -		reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
> > -				       adv748x_power_up_txb_1lane;
> > +		ret = adv748x_power_up_tx(tx);
> >  	else
> > -		reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
> > -				       adv748x_power_down_txb_1lane;
> > +		ret = adv748x_power_down_tx(tx);
> >
> > -	return adv748x_write_regs(state, reglist);
> > +	return ret;
> 
> As Laurent suggested, or even
>         return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);

Good idea!

> 
> Thanks
>    j
> 
> >  }
> >
> >  /* -----------------------------------------------------------------------------
> > --
> > 2.19.0
> >



-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2018-10-05 12:46:15 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Thu, Oct 04, 2018 at 10:41:38PM +0200, Niklas S�derlund wrote:
> > From: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > The driver fixed the TXA CSI-2 transmitter in 4-lane mode while it could
> > operate using 1-, 2- and 4-lanes. Update the driver to support all modes
> > the hardware does.
> >
> > The driver make use of large tables of static register/value writes when
> > powering up/down the TXA and TXB transmitters which include the write to
> > the NUM_LANES register. By converting the tables into functions and
> > using parameters the power up/down functions for TXA and TXB power
> > up/down can be merged and used for both transmitters.
> >
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > ---
> > * Changes since v1
> > - Convert tables of register/value writes into functions instead of
> >   intercepting and modifying the writes to the NUM_LANES register.
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 132 ++++++++++++-----------
> >  1 file changed, 67 insertions(+), 65 deletions(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index 3836dd3025d6ffb7..fe29781368a3a6b6 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -125,6 +125,16 @@ int adv748x_write(struct adv748x_state *state, u8 page, u8 reg, u8 value)
> >  	return regmap_write(state->regmap[page], reg, value);
> >  }
> >
> > +static int adv748x_write_check(struct adv748x_state *state, u8 page, u8 reg,
> > +			       u8 value, int *error)
> 
> Am I wrong or the return error is ignored and could be dropped?

No it's used to reduce boiler plate code, see adv748x_power_up_tx() 
bellow for one example.

    int ret = 0;
    ...
    adv748x_write_check(state, page, 0x31, 0x82, &ret);
    adv748x_write_check(state, page, 0x1e, 0x40, &ret);
    ...
    return ret;

If the first adv748x_write_check() fails all later calls to 
adv748x_write_check() will do nothing. This is do avoid the rather hard 
to read:

    ret = adv748x_write(state, page, 0x31, 0x82);
    if (ret)
        return ret;

    ret = adv748x_write(state, page, 0x1e, 0x40);
    if (ret)
        return ret;

> 
> > +{
> > +	if (*error)
> > +		return *error;
> > +
> > +	*error = adv748x_write(state, page, reg, value);
> > +	return *error;
> > +}
> > +
> >  /* adv748x_write_block(): Write raw data with a maximum of I2C_SMBUS_BLOCK_MAX
> >   * size to one or more registers.
> >   *
> > @@ -231,69 +241,63 @@ static int adv748x_write_regs(struct adv748x_state *state,
> >   * TXA and TXB
> >   */
> >
> > -static const struct adv748x_reg_value adv748x_power_up_txa_4lane[] = {
> > -
> > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > -
> > -	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > -	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> > -	{ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
> > -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> > -	{ADV748X_PAGE_TXA, 0xc1, 0x2b},	/* ADI Required Write */
> > -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> > -	{ADV748X_PAGE_TXA, 0x31, 0x80},	/* ADI Required Write */
> > -
> > -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> > -};
> > -
> > -static const struct adv748x_reg_value adv748x_power_down_txa_4lane[] = {
> > -
> > -	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXA, 0x1e, 0x00},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > -	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > -	{ADV748X_PAGE_TXA, 0xc1, 0x3b},	/* ADI Required Write */
> > -
> > -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> > -};
> > -
> > -static const struct adv748x_reg_value adv748x_power_up_txb_1lane[] = {
> > -
> > -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > -	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> > -
> > -	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > -	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> > -	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> > -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> > -	{ADV748X_PAGE_TXB, 0xc1, 0x2b},	/* ADI Required Write */
> > -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> > -	{ADV748X_PAGE_TXB, 0x31, 0x80},	/* ADI Required Write */
> > -
> > -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> > -};
> > -
> > -static const struct adv748x_reg_value adv748x_power_down_txb_1lane[] = {
> > -
> > -	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXB, 0x1e, 0x00},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > -	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > -	{ADV748X_PAGE_TXB, 0xc1, 0x3b},	/* ADI Required Write */
> > -
> > -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> > -};
> > +static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
> > +{
> > +	struct adv748x_state *state = tx->state;
> > +	u8 page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
> > +	int ret = 0;
> > +
> > +	/* Enable 4-lane MIPI */
> > +	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
> > +
> > +	/* Set Auto DPHY Timing */
> > +	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
> > +
> > +	/* ADI Required Writes*/
> > +	adv748x_write_check(state, page, 0x31, 0x82, &ret);
> > +	adv748x_write_check(state, page, 0x1e, 0x40, &ret);
> > +
> > +	/* i2c_mipi_pll_en - 1'b1 */
> > +	adv748x_write_check(state, page, 0xda, 0x01, &ret);
> > +	usleep_range(2000, 2500);
> > +
> > +	/* Power-up CSI-TX */
> > +	adv748x_write_check(state, page, 0x00, 0x20 | tx->num_lanes, &ret);
> > +	usleep_range(1000, 1500);
> > +
> > +	/* ADI Required Writes*/
> > +	adv748x_write_check(state, page, 0xc1, 0x2b, &ret);
> > +	usleep_range(1000, 1500);
> > +	adv748x_write_check(state, page, 0x31, 0x80, &ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
> > +{
> > +	struct adv748x_state *state = tx->state;
> > +	u8 page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
> > +	int ret = 0;
> > +
> > +	/* ADI Required Writes */
> > +	adv748x_write_check(state, page, 0x31, 0x82, &ret);
> > +	adv748x_write_check(state, page, 0x1e, 0x00, &ret);
> > +
> > +	/* Enable 4-lane MIPI */
> > +	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
> > +
> > +	/* i2c_mipi_pll_en - 1'b1 */
> > +	adv748x_write_check(state, page, 0xda, 0x01, &ret);
> > +
> > +	/* ADI Required Write */
> > +	adv748x_write_check(state, page, 0xc1, 0x3b, &ret);
> > +
> > +	return ret;
> > +}
> 
> This one looks good to me
> 
> >
> >  int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> >  {
> > -	struct adv748x_state *state = tx->state;
> > -	const struct adv748x_reg_value *reglist;
> > -	int val;
> > +	int val, ret;
> >
> >  	if (!is_tx_enabled(tx))
> >  		return 0;
> > @@ -311,13 +315,11 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> >  			"Enabling with unknown bit set");
> >
> >  	if (on)
> > -		reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
> > -				       adv748x_power_up_txb_1lane;
> > +		ret = adv748x_power_up_tx(tx);
> >  	else
> > -		reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
> > -				       adv748x_power_down_txb_1lane;
> > +		ret = adv748x_power_down_tx(tx);
> >
> > -	return adv748x_write_regs(state, reglist);
> > +	return ret;
> 
> As Laurent suggested, or even
>         return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);

Good idea!

> 
> Thanks
>    j
> 
> >  }
> >
> >  /* -----------------------------------------------------------------------------
> > --
> > 2.19.0
> >



-- 
Regards,
Niklas S�derlund

Re: [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter

[jacopo mondi]

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

Hi Niklas,

On Fri, Nov 02, 2018 at 11:44:25AM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2018-10-05 12:46:15 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Thu, Oct 04, 2018 at 10:41:38PM +0200, Niklas Söderlund wrote:
> > > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >
> > > The driver fixed the TXA CSI-2 transmitter in 4-lane mode while it could
> > > operate using 1-, 2- and 4-lanes. Update the driver to support all modes
> > > the hardware does.
> > >
> > > The driver make use of large tables of static register/value writes when
> > > powering up/down the TXA and TXB transmitters which include the write to
> > > the NUM_LANES register. By converting the tables into functions and
> > > using parameters the power up/down functions for TXA and TXB power
> > > up/down can be merged and used for both transmitters.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >
> > > ---
> > > * Changes since v1
> > > - Convert tables of register/value writes into functions instead of
> > >   intercepting and modifying the writes to the NUM_LANES register.
> > > ---
> > >  drivers/media/i2c/adv748x/adv748x-core.c | 132 ++++++++++++-----------
> > >  1 file changed, 67 insertions(+), 65 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > > index 3836dd3025d6ffb7..fe29781368a3a6b6 100644
> > > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > > @@ -125,6 +125,16 @@ int adv748x_write(struct adv748x_state *state, u8 page, u8 reg, u8 value)
> > >  	return regmap_write(state->regmap[page], reg, value);
> > >  }
> > >
> > > +static int adv748x_write_check(struct adv748x_state *state, u8 page, u8 reg,
> > > +			       u8 value, int *error)
> >
> > Am I wrong or the return error is ignored and could be dropped?
>
> No it's used to reduce boiler plate code, see adv748x_power_up_tx()
> bellow for one example.

I meant the function return value. It is never checked:

        adv748x_write_check(state, page, 0x31, 0x82, &ret);

>
>     int ret = 0;
>     ...
>     adv748x_write_check(state, page, 0x31, 0x82, &ret);
>     adv748x_write_check(state, page, 0x1e, 0x40, &ret);
>     ...
>     return ret;
>
> If the first adv748x_write_check() fails all later calls to
> adv748x_write_check() will do nothing. This is do avoid the rather hard
> to read:
>
>     ret = adv748x_write(state, page, 0x31, 0x82);
>     if (ret)
>         return ret;
>
>     ret = adv748x_write(state, page, 0x1e, 0x40);
>     if (ret)
>         return ret;
>
> >
> > > +{
> > > +	if (*error)
> > > +		return *error;
> > > +
> > > +	*error = adv748x_write(state, page, reg, value);
> > > +	return *error;
> > > +}
> > > +
> > >  /* adv748x_write_block(): Write raw data with a maximum of I2C_SMBUS_BLOCK_MAX
> > >   * size to one or more registers.
> > >   *
> > > @@ -231,69 +241,63 @@ static int adv748x_write_regs(struct adv748x_state *state,
> > >   * TXA and TXB
> > >   */
> > >
> > > -static const struct adv748x_reg_value adv748x_power_up_txa_4lane[] = {
> > > -
> > > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > > -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > > -
> > > -	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > > -	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> > > -	{ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
> > > -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> > > -	{ADV748X_PAGE_TXA, 0xc1, 0x2b},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> > > -	{ADV748X_PAGE_TXA, 0x31, 0x80},	/* ADI Required Write */
> > > -
> > > -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> > > -};
> > > -
> > > -static const struct adv748x_reg_value adv748x_power_down_txa_4lane[] = {
> > > -
> > > -	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_TXA, 0x1e, 0x00},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > > -	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > > -	{ADV748X_PAGE_TXA, 0xc1, 0x3b},	/* ADI Required Write */
> > > -
> > > -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> > > -};
> > > -
> > > -static const struct adv748x_reg_value adv748x_power_up_txb_1lane[] = {
> > > -
> > > -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > > -	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
> > > -
> > > -	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > > -	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> > > -	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> > > -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> > > -	{ADV748X_PAGE_TXB, 0xc1, 0x2b},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> > > -	{ADV748X_PAGE_TXB, 0x31, 0x80},	/* ADI Required Write */
> > > -
> > > -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> > > -};
> > > -
> > > -static const struct adv748x_reg_value adv748x_power_down_txb_1lane[] = {
> > > -
> > > -	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_TXB, 0x1e, 0x00},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
> > > -	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > > -	{ADV748X_PAGE_TXB, 0xc1, 0x3b},	/* ADI Required Write */
> > > -
> > > -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> > > -};
> > > +static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
> > > +{
> > > +	struct adv748x_state *state = tx->state;
> > > +	u8 page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
> > > +	int ret = 0;
> > > +
> > > +	/* Enable 4-lane MIPI */
> > > +	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
> > > +
> > > +	/* Set Auto DPHY Timing */
> > > +	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
> > > +
> > > +	/* ADI Required Writes*/
> > > +	adv748x_write_check(state, page, 0x31, 0x82, &ret);
> > > +	adv748x_write_check(state, page, 0x1e, 0x40, &ret);
> > > +
> > > +	/* i2c_mipi_pll_en - 1'b1 */
> > > +	adv748x_write_check(state, page, 0xda, 0x01, &ret);
> > > +	usleep_range(2000, 2500);
> > > +
> > > +	/* Power-up CSI-TX */
> > > +	adv748x_write_check(state, page, 0x00, 0x20 | tx->num_lanes, &ret);
> > > +	usleep_range(1000, 1500);
> > > +
> > > +	/* ADI Required Writes*/
> > > +	adv748x_write_check(state, page, 0xc1, 0x2b, &ret);
> > > +	usleep_range(1000, 1500);
> > > +	adv748x_write_check(state, page, 0x31, 0x80, &ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
> > > +{
> > > +	struct adv748x_state *state = tx->state;
> > > +	u8 page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
> > > +	int ret = 0;
> > > +
> > > +	/* ADI Required Writes */
> > > +	adv748x_write_check(state, page, 0x31, 0x82, &ret);
> > > +	adv748x_write_check(state, page, 0x1e, 0x00, &ret);
> > > +
> > > +	/* Enable 4-lane MIPI */
> > > +	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
> > > +
> > > +	/* i2c_mipi_pll_en - 1'b1 */
> > > +	adv748x_write_check(state, page, 0xda, 0x01, &ret);
> > > +
> > > +	/* ADI Required Write */
> > > +	adv748x_write_check(state, page, 0xc1, 0x3b, &ret);
> > > +
> > > +	return ret;
> > > +}
> >
> > This one looks good to me
> >
> > >
> > >  int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> > >  {
> > > -	struct adv748x_state *state = tx->state;
> > > -	const struct adv748x_reg_value *reglist;
> > > -	int val;
> > > +	int val, ret;
> > >
> > >  	if (!is_tx_enabled(tx))
> > >  		return 0;
> > > @@ -311,13 +315,11 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> > >  			"Enabling with unknown bit set");
> > >
> > >  	if (on)
> > > -		reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
> > > -				       adv748x_power_up_txb_1lane;
> > > +		ret = adv748x_power_up_tx(tx);
> > >  	else
> > > -		reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
> > > -				       adv748x_power_down_txb_1lane;
> > > +		ret = adv748x_power_down_tx(tx);
> > >
> > > -	return adv748x_write_regs(state, reglist);
> > > +	return ret;
> >
> > As Laurent suggested, or even
> >         return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
>
> Good idea!
>
> >
> > Thanks
> >    j
> >
> > >  }
> > >
> > >  /* -----------------------------------------------------------------------------
> > > --
> > > 2.19.0
> > >
>
>
>
> --
> Regards,
> Niklas Söderlund

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

Re: [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter

[Kieran Bingham]

Hi Jacopo,

On 02/11/2018 11:04, jacopo mondi wrote:
> Hi Niklas,
> 
> On Fri, Nov 02, 2018 at 11:44:25AM +0100, Niklas Söderlund wrote:
>> Hi Jacopo,
>>
>> Thanks for your feedback.
>>
>> On 2018-10-05 12:46:15 +0200, Jacopo Mondi wrote:
>>> Hi Niklas,
>>>
>>> On Thu, Oct 04, 2018 at 10:41:38PM +0200, Niklas Söderlund wrote:
>>>> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>>
>>>> The driver fixed the TXA CSI-2 transmitter in 4-lane mode while it could
>>>> operate using 1-, 2- and 4-lanes. Update the driver to support all modes
>>>> the hardware does.
>>>>
>>>> The driver make use of large tables of static register/value writes when
>>>> powering up/down the TXA and TXB transmitters which include the write to
>>>> the NUM_LANES register. By converting the tables into functions and
>>>> using parameters the power up/down functions for TXA and TXB power
>>>> up/down can be merged and used for both transmitters.
>>>>
>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>>
>>>> ---
>>>> * Changes since v1
>>>> - Convert tables of register/value writes into functions instead of
>>>>   intercepting and modifying the writes to the NUM_LANES register.
>>>> ---
>>>>  drivers/media/i2c/adv748x/adv748x-core.c | 132 ++++++++++++-----------
>>>>  1 file changed, 67 insertions(+), 65 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
>>>> index 3836dd3025d6ffb7..fe29781368a3a6b6 100644
>>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
>>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
>>>> @@ -125,6 +125,16 @@ int adv748x_write(struct adv748x_state *state, u8 page, u8 reg, u8 value)
>>>>  	return regmap_write(state->regmap[page], reg, value);
>>>>  }
>>>>
>>>> +static int adv748x_write_check(struct adv748x_state *state, u8 page, u8 reg,
>>>> +			       u8 value, int *error)
>>>
>>> Am I wrong or the return error is ignored and could be dropped?
>>
>> No it's used to reduce boiler plate code, see adv748x_power_up_tx()
>> bellow for one example.
> 
> I meant the function return value. It is never checked:
> 
>         adv748x_write_check(state, page, 0x31, 0x82, &ret);


I would vote to keep the return value - even if in most cases it's unused.

It will allow for the last statement in a function to be :

{
 ...

 return adv748x_write_check(state, page, 0x1e, 0x40, &ret);
}


Which might be desirable in some cases.

I'll bet the compiler will optimise the value out if it's not used anyway.

--
Kieran


> 
>>
>>     int ret = 0;
>>     ...
>>     adv748x_write_check(state, page, 0x31, 0x82, &ret);
>>     adv748x_write_check(state, page, 0x1e, 0x40, &ret);
>>     ...
>>     return ret;
>>
>> If the first adv748x_write_check() fails all later calls to
>> adv748x_write_check() will do nothing. This is do avoid the rather hard
>> to read:
>>
>>     ret = adv748x_write(state, page, 0x31, 0x82);
>>     if (ret)
>>         return ret;
>>
>>     ret = adv748x_write(state, page, 0x1e, 0x40);
>>     if (ret)
>>         return ret;
>>
>>>
>>>> +{
>>>> +	if (*error)
>>>> +		return *error;
>>>> +
>>>> +	*error = adv748x_write(state, page, reg, value);
>>>> +	return *error;
>>>> +}
>>>> +
>>>>  /* adv748x_write_block(): Write raw data with a maximum of I2C_SMBUS_BLOCK_MAX
>>>>   * size to one or more registers.
>>>>   *
>>>> @@ -231,69 +241,63 @@ static int adv748x_write_regs(struct adv748x_state *state,
>>>>   * TXA and TXB
>>>>   */
>>>>
>>>> -static const struct adv748x_reg_value adv748x_power_up_txa_4lane[] = {
>>>> -
>>>> -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
>>>> -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
>>>> -
>>>> -	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
>>>> -	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
>>>> -	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
>>>> -	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
>>>> -	{ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
>>>> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
>>>> -	{ADV748X_PAGE_TXA, 0xc1, 0x2b},	/* ADI Required Write */
>>>> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
>>>> -	{ADV748X_PAGE_TXA, 0x31, 0x80},	/* ADI Required Write */
>>>> -
>>>> -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
>>>> -};
>>>> -
>>>> -static const struct adv748x_reg_value adv748x_power_down_txa_4lane[] = {
>>>> -
>>>> -	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
>>>> -	{ADV748X_PAGE_TXA, 0x1e, 0x00},	/* ADI Required Write */
>>>> -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
>>>> -	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
>>>> -	{ADV748X_PAGE_TXA, 0xc1, 0x3b},	/* ADI Required Write */
>>>> -
>>>> -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
>>>> -};
>>>> -
>>>> -static const struct adv748x_reg_value adv748x_power_up_txb_1lane[] = {
>>>> -
>>>> -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
>>>> -	{ADV748X_PAGE_TXB, 0x00, 0xa1},	/* Set Auto DPHY Timing */
>>>> -
>>>> -	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
>>>> -	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
>>>> -	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
>>>> -	{ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
>>>> -	{ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
>>>> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
>>>> -	{ADV748X_PAGE_TXB, 0xc1, 0x2b},	/* ADI Required Write */
>>>> -	{ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
>>>> -	{ADV748X_PAGE_TXB, 0x31, 0x80},	/* ADI Required Write */
>>>> -
>>>> -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
>>>> -};
>>>> -
>>>> -static const struct adv748x_reg_value adv748x_power_down_txb_1lane[] = {
>>>> -
>>>> -	{ADV748X_PAGE_TXB, 0x31, 0x82},	/* ADI Required Write */
>>>> -	{ADV748X_PAGE_TXB, 0x1e, 0x00},	/* ADI Required Write */
>>>> -	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */
>>>> -	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
>>>> -	{ADV748X_PAGE_TXB, 0xc1, 0x3b},	/* ADI Required Write */
>>>> -
>>>> -	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
>>>> -};
>>>> +static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
>>>> +{
>>>> +	struct adv748x_state *state = tx->state;
>>>> +	u8 page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
>>>> +	int ret = 0;
>>>> +
>>>> +	/* Enable 4-lane MIPI */
>>>> +	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
>>>> +
>>>> +	/* Set Auto DPHY Timing */
>>>> +	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
>>>> +
>>>> +	/* ADI Required Writes*/
>>>> +	adv748x_write_check(state, page, 0x31, 0x82, &ret);
>>>> +	adv748x_write_check(state, page, 0x1e, 0x40, &ret);
>>>> +
>>>> +	/* i2c_mipi_pll_en - 1'b1 */
>>>> +	adv748x_write_check(state, page, 0xda, 0x01, &ret);
>>>> +	usleep_range(2000, 2500);
>>>> +
>>>> +	/* Power-up CSI-TX */
>>>> +	adv748x_write_check(state, page, 0x00, 0x20 | tx->num_lanes, &ret);
>>>> +	usleep_range(1000, 1500);
>>>> +
>>>> +	/* ADI Required Writes*/
>>>> +	adv748x_write_check(state, page, 0xc1, 0x2b, &ret);
>>>> +	usleep_range(1000, 1500);
>>>> +	adv748x_write_check(state, page, 0x31, 0x80, &ret);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
>>>> +{
>>>> +	struct adv748x_state *state = tx->state;
>>>> +	u8 page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
>>>> +	int ret = 0;
>>>> +
>>>> +	/* ADI Required Writes */
>>>> +	adv748x_write_check(state, page, 0x31, 0x82, &ret);
>>>> +	adv748x_write_check(state, page, 0x1e, 0x00, &ret);
>>>> +
>>>> +	/* Enable 4-lane MIPI */
>>>> +	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
>>>> +
>>>> +	/* i2c_mipi_pll_en - 1'b1 */
>>>> +	adv748x_write_check(state, page, 0xda, 0x01, &ret);
>>>> +
>>>> +	/* ADI Required Write */
>>>> +	adv748x_write_check(state, page, 0xc1, 0x3b, &ret);
>>>> +
>>>> +	return ret;
>>>> +}
>>>
>>> This one looks good to me
>>>
>>>>
>>>>  int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>>>>  {
>>>> -	struct adv748x_state *state = tx->state;
>>>> -	const struct adv748x_reg_value *reglist;
>>>> -	int val;
>>>> +	int val, ret;
>>>>
>>>>  	if (!is_tx_enabled(tx))
>>>>  		return 0;
>>>> @@ -311,13 +315,11 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>>>>  			"Enabling with unknown bit set");
>>>>
>>>>  	if (on)
>>>> -		reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
>>>> -				       adv748x_power_up_txb_1lane;
>>>> +		ret = adv748x_power_up_tx(tx);
>>>>  	else
>>>> -		reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
>>>> -				       adv748x_power_down_txb_1lane;
>>>> +		ret = adv748x_power_down_tx(tx);
>>>>
>>>> -	return adv748x_write_regs(state, reglist);
>>>> +	return ret;
>>>
>>> As Laurent suggested, or even
>>>         return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
>>
>> Good idea!
>>
>>>
>>> Thanks
>>>    j
>>>
>>>>  }
>>>>
>>>>  /* -----------------------------------------------------------------------------
>>>> --
>>>> 2.19.0
>>>>
>>
>>
>>
>> --
>> Regards,
>> Niklas Söderlund

-- 
Regards
--
Kieran