* [PATCH v2 0/5] i2c: adv748x: add support for CSI-2 TXA to work in 1-, 2- and 4-lane mode @ 2018-10-04 20:41 Niklas Söderlund 2018-10-04 20:41 ` [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints Niklas Söderlund ` (4 more replies) 0 siblings, 5 replies; 27+ messages in thread From: Niklas Söderlund @ 2018-10-04 20:41 UTC (permalink / raw) To: Kieran Bingham, Laurent Pinchart, Jacopo Mondi, linux-media Cc: linux-renesas-soc, 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints 2018-10-04 20:41 [PATCH v2 0/5] i2c: adv748x: add support for CSI-2 TXA to work in 1-, 2- and 4-lane mode Niklas Söderlund @ 2018-10-04 20:41 ` Niklas Söderlund 2018-10-04 21:42 ` Laurent Pinchart 2018-10-04 20:41 ` [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization Niklas Söderlund ` (3 subsequent siblings) 4 siblings, 1 reply; 27+ messages in thread From: Niklas Söderlund @ 2018-10-04 20:41 UTC (permalink / raw) To: Kieran Bingham, Laurent Pinchart, Jacopo Mondi, linux-media Cc: linux-renesas-soc, Niklas Söderlund, Rob Herring, devicetree 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints 2018-10-04 20:41 ` [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints Niklas Söderlund @ 2018-10-04 21:42 ` Laurent Pinchart 2018-10-04 22:00 ` Laurent Pinchart 0 siblings, 1 reply; 27+ messages in thread From: Laurent Pinchart @ 2018-10-04 21:42 UTC (permalink / raw) To: Niklas Söderlund Cc: Kieran Bingham, Jacopo Mondi, linux-media, linux-renesas-soc, Niklas Söderlund, Rob Herring, devicetree 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints 2018-10-04 21:42 ` Laurent Pinchart @ 2018-10-04 22:00 ` Laurent Pinchart 2018-10-05 8:49 ` jacopo mondi 0 siblings, 1 reply; 27+ messages in thread From: Laurent Pinchart @ 2018-10-04 22:00 UTC (permalink / raw) To: Niklas Söderlund Cc: Kieran Bingham, Jacopo Mondi, linux-media, linux-renesas-soc, Niklas Söderlund, Rob Herring, devicetree 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints 2018-10-04 22:00 ` Laurent Pinchart @ 2018-10-05 8:49 ` jacopo mondi 2018-10-05 10:02 ` Laurent Pinchart 0 siblings, 1 reply; 27+ messages in thread From: jacopo mondi @ 2018-10-05 8:49 UTC (permalink / raw) To: Laurent Pinchart Cc: Niklas Söderlund, Kieran Bingham, linux-media, linux-renesas-soc, Niklas Söderlund, Rob Herring, devicetree [-- 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 --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints 2018-10-05 8:49 ` jacopo mondi @ 2018-10-05 10:02 ` Laurent Pinchart 2018-11-02 10:34 ` Niklas Söderlund 0 siblings, 1 reply; 27+ messages in thread From: Laurent Pinchart @ 2018-10-05 10:02 UTC (permalink / raw) To: jacopo mondi Cc: Niklas Söderlund, Kieran Bingham, linux-media, linux-renesas-soc, Niklas Söderlund, Rob Herring, devicetree 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints 2018-10-05 10:02 ` Laurent Pinchart @ 2018-11-02 10:34 ` Niklas Söderlund 0 siblings, 0 replies; 27+ messages in thread From: Niklas Söderlund @ 2018-11-02 10:34 UTC (permalink / raw) To: Laurent Pinchart Cc: jacopo mondi, Kieran Bingham, linux-media, linux-renesas-soc, Rob Herring, devicetree 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints @ 2018-11-02 10:34 ` Niklas Söderlund 0 siblings, 0 replies; 27+ messages in thread From: Niklas Söderlund @ 2018-11-02 10:34 UTC (permalink / raw) To: Laurent Pinchart Cc: jacopo mondi, Kieran Bingham, linux-media, linux-renesas-soc, Rob Herring, devicetree 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints 2018-11-02 10:34 ` Niklas Söderlund (?) @ 2018-11-02 10:57 ` Kieran Bingham -1 siblings, 0 replies; 27+ messages in thread From: Kieran Bingham @ 2018-11-02 10:57 UTC (permalink / raw) To: Niklas Söderlund, Laurent Pinchart Cc: jacopo mondi, linux-media, linux-renesas-soc, Rob Herring, devicetree 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 >> ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization 2018-10-04 20:41 [PATCH v2 0/5] i2c: adv748x: add support for CSI-2 TXA to work in 1-, 2- and 4-lane mode Niklas Söderlund 2018-10-04 20:41 ` [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints Niklas Söderlund @ 2018-10-04 20:41 ` Niklas Söderlund 2018-10-04 22:36 ` Laurent Pinchart 2018-10-04 20:41 ` [PATCH v2 3/5] i2c: adv748x: reuse power up sequence when initializing CSI-2 Niklas Söderlund ` (2 subsequent siblings) 4 siblings, 1 reply; 27+ messages in thread From: Niklas Söderlund @ 2018-10-04 20:41 UTC (permalink / raw) To: Kieran Bingham, Laurent Pinchart, Jacopo Mondi, linux-media Cc: linux-renesas-soc, 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization 2018-10-04 20:41 ` [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization Niklas Söderlund @ 2018-10-04 22:36 ` Laurent Pinchart 2018-11-02 10:38 ` Niklas Söderlund 0 siblings, 1 reply; 27+ messages in thread From: Laurent Pinchart @ 2018-10-04 22:36 UTC (permalink / raw) To: Niklas Söderlund Cc: Kieran Bingham, Jacopo Mondi, linux-media, linux-renesas-soc, Niklas Söderlund 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization 2018-10-04 22:36 ` Laurent Pinchart @ 2018-11-02 10:38 ` Niklas Söderlund 0 siblings, 0 replies; 27+ messages in thread From: Niklas Söderlund @ 2018-11-02 10:38 UTC (permalink / raw) To: Laurent Pinchart Cc: Kieran Bingham, Jacopo Mondi, linux-media, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization @ 2018-11-02 10:38 ` Niklas Söderlund 0 siblings, 0 replies; 27+ messages in thread From: Niklas Söderlund @ 2018-11-02 10:38 UTC (permalink / raw) To: Laurent Pinchart Cc: Kieran Bingham, Jacopo Mondi, linux-media, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization 2018-11-02 10:38 ` Niklas Söderlund (?) @ 2018-11-02 11:43 ` Laurent Pinchart 2018-11-02 15:40 ` Niklas Söderlund -1 siblings, 1 reply; 27+ messages in thread From: Laurent Pinchart @ 2018-11-02 11:43 UTC (permalink / raw) To: Niklas Söderlund Cc: Kieran Bingham, Jacopo Mondi, linux-media, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization 2018-11-02 11:43 ` Laurent Pinchart @ 2018-11-02 15:40 ` Niklas Söderlund 0 siblings, 0 replies; 27+ messages in thread From: Niklas Söderlund @ 2018-11-02 15:40 UTC (permalink / raw) To: Laurent Pinchart Cc: Kieran Bingham, Jacopo Mondi, linux-media, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization @ 2018-11-02 15:40 ` Niklas Söderlund 0 siblings, 0 replies; 27+ messages in thread From: Niklas Söderlund @ 2018-11-02 15:40 UTC (permalink / raw) To: Laurent Pinchart Cc: Kieran Bingham, Jacopo Mondi, linux-media, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 3/5] i2c: adv748x: reuse power up sequence when initializing CSI-2 2018-10-04 20:41 [PATCH v2 0/5] i2c: adv748x: add support for CSI-2 TXA to work in 1-, 2- and 4-lane mode Niklas Söderlund 2018-10-04 20:41 ` [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints Niklas Söderlund 2018-10-04 20:41 ` [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization Niklas Söderlund @ 2018-10-04 20:41 ` Niklas Söderlund 2018-10-04 21:58 ` Laurent Pinchart 2018-10-04 20:41 ` [PATCH v2 4/5] i2c: adv748x: store number of CSI-2 lanes described in device tree Niklas Söderlund 2018-10-04 20:41 ` [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter Niklas Söderlund 4 siblings, 1 reply; 27+ messages in thread From: Niklas Söderlund @ 2018-10-04 20:41 UTC (permalink / raw) To: Kieran Bingham, Laurent Pinchart, Jacopo Mondi, linux-media Cc: linux-renesas-soc, 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/5] i2c: adv748x: reuse power up sequence when initializing CSI-2 2018-10-04 20:41 ` [PATCH v2 3/5] i2c: adv748x: reuse power up sequence when initializing CSI-2 Niklas Söderlund @ 2018-10-04 21:58 ` Laurent Pinchart 0 siblings, 0 replies; 27+ messages in thread From: Laurent Pinchart @ 2018-10-04 21:58 UTC (permalink / raw) To: Niklas Söderlund Cc: Kieran Bingham, Jacopo Mondi, linux-media, linux-renesas-soc, Niklas Söderlund 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 4/5] i2c: adv748x: store number of CSI-2 lanes described in device tree 2018-10-04 20:41 [PATCH v2 0/5] i2c: adv748x: add support for CSI-2 TXA to work in 1-, 2- and 4-lane mode Niklas Söderlund ` (2 preceding siblings ...) 2018-10-04 20:41 ` [PATCH v2 3/5] i2c: adv748x: reuse power up sequence when initializing CSI-2 Niklas Söderlund @ 2018-10-04 20:41 ` Niklas Söderlund 2018-10-04 22:01 ` Laurent Pinchart 2018-10-04 20:41 ` [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter Niklas Söderlund 4 siblings, 1 reply; 27+ messages in thread From: Niklas Söderlund @ 2018-10-04 20:41 UTC (permalink / raw) To: Kieran Bingham, Laurent Pinchart, Jacopo Mondi, linux-media Cc: linux-renesas-soc, 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/5] i2c: adv748x: store number of CSI-2 lanes described in device tree 2018-10-04 20:41 ` [PATCH v2 4/5] i2c: adv748x: store number of CSI-2 lanes described in device tree Niklas Söderlund @ 2018-10-04 22:01 ` Laurent Pinchart 0 siblings, 0 replies; 27+ messages in thread From: Laurent Pinchart @ 2018-10-04 22:01 UTC (permalink / raw) To: Niklas Söderlund Cc: Kieran Bingham, Jacopo Mondi, linux-media, linux-renesas-soc, Niklas Söderlund 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter 2018-10-04 20:41 [PATCH v2 0/5] i2c: adv748x: add support for CSI-2 TXA to work in 1-, 2- and 4-lane mode Niklas Söderlund ` (3 preceding siblings ...) 2018-10-04 20:41 ` [PATCH v2 4/5] i2c: adv748x: store number of CSI-2 lanes described in device tree Niklas Söderlund @ 2018-10-04 20:41 ` Niklas Söderlund 2018-10-04 22:08 ` Laurent Pinchart 2018-10-05 10:46 ` jacopo mondi 4 siblings, 2 replies; 27+ messages in thread From: Niklas Söderlund @ 2018-10-04 20:41 UTC (permalink / raw) To: Kieran Bingham, Laurent Pinchart, Jacopo Mondi, linux-media Cc: linux-renesas-soc, 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter 2018-10-04 20:41 ` [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter Niklas Söderlund @ 2018-10-04 22:08 ` Laurent Pinchart 2018-10-05 10:46 ` jacopo mondi 1 sibling, 0 replies; 27+ messages in thread From: Laurent Pinchart @ 2018-10-04 22:08 UTC (permalink / raw) To: Niklas Söderlund Cc: Kieran Bingham, Jacopo Mondi, linux-media, linux-renesas-soc, Niklas Söderlund 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter 2018-10-04 20:41 ` [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter Niklas Söderlund 2018-10-04 22:08 ` Laurent Pinchart @ 2018-10-05 10:46 ` jacopo mondi 2018-11-02 10:44 ` Niklas Söderlund 1 sibling, 1 reply; 27+ messages in thread From: jacopo mondi @ 2018-10-05 10:46 UTC (permalink / raw) To: Niklas Söderlund Cc: Kieran Bingham, Laurent Pinchart, linux-media, linux-renesas-soc, Niklas Söderlund [-- 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 --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter 2018-10-05 10:46 ` jacopo mondi @ 2018-11-02 10:44 ` Niklas Söderlund 0 siblings, 0 replies; 27+ messages in thread From: Niklas Söderlund @ 2018-11-02 10:44 UTC (permalink / raw) To: jacopo mondi Cc: Kieran Bingham, Laurent Pinchart, linux-media, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter @ 2018-11-02 10:44 ` Niklas Söderlund 0 siblings, 0 replies; 27+ messages in thread From: Niklas Söderlund @ 2018-11-02 10:44 UTC (permalink / raw) To: jacopo mondi Cc: Kieran Bingham, Laurent Pinchart, linux-media, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter 2018-11-02 10:44 ` Niklas Söderlund (?) @ 2018-11-02 11:04 ` jacopo mondi 2018-11-02 11:46 ` Kieran Bingham -1 siblings, 1 reply; 27+ messages in thread From: jacopo mondi @ 2018-11-02 11:04 UTC (permalink / raw) To: Niklas Söderlund Cc: Kieran Bingham, Laurent Pinchart, linux-media, linux-renesas-soc [-- 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 --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter 2018-11-02 11:04 ` jacopo mondi @ 2018-11-02 11:46 ` Kieran Bingham 0 siblings, 0 replies; 27+ messages in thread From: Kieran Bingham @ 2018-11-02 11:46 UTC (permalink / raw) To: jacopo mondi, Niklas Söderlund Cc: Laurent Pinchart, linux-media, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2018-11-03 0:48 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-04 20:41 [PATCH v2 0/5] i2c: adv748x: add support for CSI-2 TXA to work in 1-, 2- and 4-lane mode Niklas Söderlund 2018-10-04 20:41 ` [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints Niklas Söderlund 2018-10-04 21:42 ` Laurent Pinchart 2018-10-04 22:00 ` Laurent Pinchart 2018-10-05 8:49 ` jacopo mondi 2018-10-05 10:02 ` Laurent Pinchart 2018-11-02 10:34 ` Niklas Söderlund 2018-11-02 10:34 ` Niklas Söderlund 2018-11-02 10:57 ` Kieran Bingham 2018-10-04 20:41 ` [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization Niklas Söderlund 2018-10-04 22:36 ` Laurent Pinchart 2018-11-02 10:38 ` Niklas Söderlund 2018-11-02 10:38 ` Niklas Söderlund 2018-11-02 11:43 ` Laurent Pinchart 2018-11-02 15:40 ` Niklas Söderlund 2018-11-02 15:40 ` Niklas Söderlund 2018-10-04 20:41 ` [PATCH v2 3/5] i2c: adv748x: reuse power up sequence when initializing CSI-2 Niklas Söderlund 2018-10-04 21:58 ` Laurent Pinchart 2018-10-04 20:41 ` [PATCH v2 4/5] i2c: adv748x: store number of CSI-2 lanes described in device tree Niklas Söderlund 2018-10-04 22:01 ` Laurent Pinchart 2018-10-04 20:41 ` [PATCH v2 5/5] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter Niklas Söderlund 2018-10-04 22:08 ` Laurent Pinchart 2018-10-05 10:46 ` jacopo mondi 2018-11-02 10:44 ` Niklas Söderlund 2018-11-02 10:44 ` Niklas Söderlund 2018-11-02 11:04 ` jacopo mondi 2018-11-02 11:46 ` Kieran Bingham
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.