[PATCH v3 0/4] i2c: adv748x: add support for CSI-2 TXA to work

[PATCH v3 0/4] i2c: adv748x: add support for CSI-2 TXA to work

[Niklas Söderlund]

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/4 lists the 'data-lanes' DT property as mandatory for endpoints
describing the CSI-2 transmitters. Patch 2/4 refactors the 
initialization sequence of the adv748x to be able to reuse more code.
Patch 3/4 adds the DT parsing and storing of the number of lanes. Patch
4/4 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 (4):
  dt-bindings: adv748x: make data-lanes property mandatory for CSI-2
    endpoints
  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 |   4 +-
 drivers/media/i2c/adv748x/adv748x-core.c      | 235 ++++++++++--------
 drivers/media/i2c/adv748x/adv748x.h           |   1 +
 3 files changed, 135 insertions(+), 105 deletions(-)

-- 
2.19.1

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

[Niklas Söderlund]

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

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

---
* Changes since v2
- Update paragraph according to Laurents comments.
---
 Documentation/devicetree/bindings/media/i2c/adv748x.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
index 5dddc95f9cc46084..bffbabc879efd86c 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
@@ -48,7 +48,9 @@ are numbered as follows.
 	  TXA		source		10
 	  TXB		source		11
 
-The digital output port nodes must contain at least one 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.
 
 Ports are optional if they are not connected to anything at the hardware level.
 
-- 
2.19.1

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

[jacopo mondi]

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

Hi Niklas,

On Fri, Nov 02, 2018 at 05:00:06PM +0100, Niklas Söderlund wrote:
> The CSI-2 transmitters can use a different number of lanes to transmit
> data. Make the data-lanes mandatory for the endpoints that describe the
> transmitters as no good default can be set to fallback on.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> ---
> * Changes since v2
> - Update paragraph according to Laurents comments.
> ---
>  Documentation/devicetree/bindings/media/i2c/adv748x.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> index 5dddc95f9cc46084..bffbabc879efd86c 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> @@ -48,7 +48,9 @@ are numbered as follows.
>  	  TXA		source		10
>  	  TXB		source		11
>
> -The digital output port nodes must contain at least one 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.
>
>  Ports are optional if they are not connected to anything at the hardware level.
>

Re-vamping my ignored comment on v2, I still think you should list here the
accepted values for each TX as they're actually a property of the hw
device itself.

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

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

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

[Rob Herring]

On Mon, Nov 05, 2018 at 09:41:06AM +0100, jacopo mondi wrote:
> Hi Niklas,
> 
> On Fri, Nov 02, 2018 at 05:00:06PM +0100, Niklas S�derlund wrote:
> > The CSI-2 transmitters can use a different number of lanes to transmit
> > data. Make the data-lanes mandatory for the endpoints that describe the
> > transmitters as no good default can be set to fallback on.
> >
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > ---
> > * Changes since v2
> > - Update paragraph according to Laurents comments.
> > ---
> >  Documentation/devicetree/bindings/media/i2c/adv748x.txt | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > index 5dddc95f9cc46084..bffbabc879efd86c 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > @@ -48,7 +48,9 @@ are numbered as follows.
> >  	  TXA		source		10
> >  	  TXB		source		11
> >
> > -The digital output port nodes must contain at least one 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.
> >
> >  Ports are optional if they are not connected to anything at the hardware level.
> >
> 
> Re-vamping my ignored comment on v2, I still think you should list here the
> accepted values for each TX as they're actually a property of the hw
> device itself.
> 
>  Required endpoint properties:
> - data-lanes: See "video-interfaces.txt" for description. The property
>   is mandatory for CSI-2 output endpoints and the accepted value
>   depends on which endpoint the property is applied to:
>   - TXA: accepted values are <1>, <2>, <4>
>   - TXB: accepted value is <1>

+1

Rob

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

[Rob Herring]

On Mon, Nov 05, 2018 at 09:41:06AM +0100, jacopo mondi wrote:
> Hi Niklas,
> 
> On Fri, Nov 02, 2018 at 05:00:06PM +0100, Niklas Söderlund wrote:
> > The CSI-2 transmitters can use a different number of lanes to transmit
> > data. Make the data-lanes mandatory for the endpoints that describe the
> > transmitters as no good default can be set to fallback on.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > ---
> > * Changes since v2
> > - Update paragraph according to Laurents comments.
> > ---
> >  Documentation/devicetree/bindings/media/i2c/adv748x.txt | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > index 5dddc95f9cc46084..bffbabc879efd86c 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> > @@ -48,7 +48,9 @@ are numbered as follows.
> >  	  TXA		source		10
> >  	  TXB		source		11
> >
> > -The digital output port nodes must contain at least one 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.
> >
> >  Ports are optional if they are not connected to anything at the hardware level.
> >
> 
> Re-vamping my ignored comment on v2, I still think you should list here the
> accepted values for each TX as they're actually a property of the hw
> device itself.
> 
>  Required endpoint properties:
> - data-lanes: See "video-interfaces.txt" for description. The property
>   is mandatory for CSI-2 output endpoints and the accepted value
>   depends on which endpoint the property is applied to:
>   - TXA: accepted values are <1>, <2>, <4>
>   - TXB: accepted value is <1>

+1

Rob

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Friday, 2 November 2018 18:00:06 EET Niklas Söderlund wrote:
> The CSI-2 transmitters can use a different number of lanes to transmit
> data. Make the data-lanes mandatory for the endpoints that describe the
> transmitters as no good default can be set to fallback on.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

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

> 
> ---
> * Changes since v2
> - Update paragraph according to Laurents comments.
> ---
>  Documentation/devicetree/bindings/media/i2c/adv748x.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index
> 5dddc95f9cc46084..bffbabc879efd86c 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> @@ -48,7 +48,9 @@ are numbered as follows.
>  	  TXA		source		10
>  	  TXB		source		11
> 
> -The digital output port nodes must contain at least one 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.
> 
>  Ports are optional if they are not connected to anything at the hardware
> level.


-- 
Regards,

Laurent Pinchart

[PATCH v3 2/4] i2c: adv748x: reuse power up sequence when initializing CSI-2

[Niklas Söderlund]

Extend the MIPI CSI-2 power up sequence to match the power up sequence
in the hardware manual chapter "9.5.1 Power Up Sequence". This change
allows the power up functions to be reused when initializing the
hardware reducing code duplicating as well aligning with the
documentation.

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

---
* Changes since v2
- Bring in the undocumented registers in the power on sequence from the
  initialization sequence after confirming in the hardware manual that
  this is the correct behavior.
---
 drivers/media/i2c/adv748x/adv748x-core.c | 50 ++++++------------------
 1 file changed, 13 insertions(+), 37 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 6854d898fdd1f192..2384f50dacb0ccff 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -234,6 +234,12 @@ 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, 0xdb, 0x10},	/* ADI Required Write */
+	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
+	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
+	{ADV748X_PAGE_TXA, 0x71, 0x33},	/* ADI Required Write */
+	{ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI Required Write */
+	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
 
 	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
 	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
@@ -263,6 +269,11 @@ 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, 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, 0x31, 0x82},	/* ADI Required Write */
 	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
@@ -383,25 +394,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 */
-	{ADV748X_PAGE_TXA, 0x71, 0x33},	/* ADI Required Write */
-	{ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI Required Write */
-	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
-
-	{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 */
 };
 
@@ -435,24 +427,6 @@ 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, 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 */
 };
 
@@ -474,6 +448,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 */
@@ -481,6 +456,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.1

Re: [PATCH v3 2/4] i2c: adv748x: reuse power up sequence when initializing CSI-2

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Friday, 2 November 2018 18:00:07 EET Niklas Söderlund wrote:
> Extend the MIPI CSI-2 power up sequence to match the power up sequence
> in the hardware manual chapter "9.5.1 Power Up Sequence". This change
> allows the power up functions to be reused when initializing the
> hardware reducing code duplicating as well aligning with the
> documentation.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

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

> 
> ---
> * Changes since v2
> - Bring in the undocumented registers in the power on sequence from the
>   initialization sequence after confirming in the hardware manual that
>   this is the correct behavior.
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 50 ++++++------------------
>  1 file changed, 13 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 6854d898fdd1f192..2384f50dacb0ccff 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -234,6 +234,12 @@ 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, 0xdb, 0x10},	/* ADI Required Write */
> +	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
> +	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
> +	{ADV748X_PAGE_TXA, 0x71, 0x33},	/* ADI Required Write */
> +	{ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI Required Write */
> +	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> 
>  	{ADV748X_PAGE_TXA, 0x31, 0x82},	/* ADI Required Write */
>  	{ADV748X_PAGE_TXA, 0x1e, 0x40},	/* ADI Required Write */
> @@ -263,6 +269,11 @@ 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, 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, 0x31, 0x82},	/* ADI Required Write */
>  	{ADV748X_PAGE_TXB, 0x1e, 0x40},	/* ADI Required Write */
> @@ -383,25 +394,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 */
> -	{ADV748X_PAGE_TXA, 0x71, 0x33},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> -
> -	{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 */
>  };
> 
> @@ -435,24 +427,6 @@ 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, 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 */
>  };
> 
> @@ -474,6 +448,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 */
> @@ -481,6 +456,7 @@ static int adv748x_reset(struct adv748x_state *state)
>  	if (ret)
>  		return ret;
> 
> +	adv748x_tx_power(&state->txb, 1);
>  	adv748x_tx_power(&state->txb, 0);
> 
>  	/* Disable chip powerdown & Enable HDMI Rx block */


-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

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>

---
* Changes since v2
- Rebase to latest media-tree requires the bus_type filed in struct
  v4l2_fwnode_endpoint to be initialized, set it to V4L2_MBUS_CSI2_DPHY.

* 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 | 50 ++++++++++++++++++++++++
 drivers/media/i2c/adv748x/adv748x.h      |  1 +
 2 files changed, 51 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 2384f50dacb0ccff..9d80d7f3062b16bc 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"
@@ -521,12 +522,56 @@ 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;
+
+	vep.bus_type = V4L2_MBUS_CSI2_DPHY;
+	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);
@@ -557,6 +602,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.1

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

[jacopo mondi]

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

Hi Niklas,
   thanks for the patches

On Fri, Nov 02, 2018 at 05:00:08PM +0100, Niklas Söderlund wrote:
> 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: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j

>
> ---
> * Changes since v2
> - Rebase to latest media-tree requires the bus_type filed in struct
>   v4l2_fwnode_endpoint to be initialized, set it to V4L2_MBUS_CSI2_DPHY.
>
> * 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 | 50 ++++++++++++++++++++++++
>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
>  2 files changed, 51 insertions(+)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 2384f50dacb0ccff..9d80d7f3062b16bc 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"
> @@ -521,12 +522,56 @@ 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;
> +
> +	vep.bus_type = V4L2_MBUS_CSI2_DPHY;
> +	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);
> @@ -557,6 +602,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.1
>

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

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Friday, 2 November 2018 18:00:08 EET Niklas Söderlund wrote:
> 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>
> 
> ---
> * Changes since v2
> - Rebase to latest media-tree requires the bus_type filed in struct
>   v4l2_fwnode_endpoint to be initialized, set it to V4L2_MBUS_CSI2_DPHY.
> 
> * 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 | 50 ++++++++++++++++++++++++
>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
>  2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 2384f50dacb0ccff..9d80d7f3062b16bc 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"
> @@ -521,12 +522,56 @@ 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;
> +
> +	vep.bus_type = V4L2_MBUS_CSI2_DPHY;
> +	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);
> +	}

Should we set the number of lanes to 4 and 1 respectively by default to 
support old DTs ? Apart from that,

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

> +	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);
> @@ -557,6 +602,11 @@ static int adv748x_parse_dt(struct adv748x_state
> *state) in_found = true;
>  		else
>  			out_found = true;
> +
> +		/* Store number of CSI-2 lanes used for TXA and TXB. */
> +		ret = adv748x_parse_csi2_lanes(state, ep.port, ep_np);
> +		if (ret)
> +			return ret;
>  	}
> 
>  	return in_found && out_found ? 0 : -ENODEV;
> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h index
> 39c2fdc3b41667d8..b482c7fe6957eb85 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -79,6 +79,7 @@ struct adv748x_csi2 {
>  	struct v4l2_mbus_framefmt format;
>  	unsigned int page;
>  	unsigned int port;
> +	unsigned int num_lanes;
> 
>  	struct media_pad pads[ADV748X_CSI2_NR_PADS];
>  	struct v4l2_ctrl_handler ctrl_hdl;

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

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 v2
- Fix typos in comments.
- Remove unneeded boiler plait code in adv748x_tx_power() as suggested
  by Jacopo and Laurent.
- Take into account the two different register used when powering up TXA
  and TXB due to an earlier patch in this series aligns the power
  sequence with the manual.

* 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 | 157 ++++++++++++-----------
 1 file changed, 79 insertions(+), 78 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 9d80d7f3062b16bc..d94c63cb6a2efdba 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,79 +241,77 @@ 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, 0xdb, 0x10},	/* ADI Required Write */
-	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
-	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
-	{ADV748X_PAGE_TXA, 0x71, 0x33},	/* ADI Required Write */
-	{ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI Required Write */
-	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
-
-	{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, 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, 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 n-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 Write */
+	if (is_txa(tx)) {
+		adv748x_write_check(state, page, 0xdb, 0x10, &ret);
+		adv748x_write_check(state, page, 0xd6, 0x07, &ret);
+	} else {
+		adv748x_write_check(state, page, 0xd2, 0x40, &ret);
+	}
+
+	adv748x_write_check(state, page, 0xc4, 0x0a, &ret);
+	adv748x_write_check(state, page, 0x71, 0x33, &ret);
+	adv748x_write_check(state, page, 0x72, 0x11, &ret);
+
+	/* i2c_dphy_pwdn - 1'b0 */
+	adv748x_write_check(state, page, 0xf0, 0x00, &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 n-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;
 
 	if (!is_tx_enabled(tx))
@@ -321,14 +329,7 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
 	WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
 			"Enabling with unknown bit set");
 
-	if (on)
-		reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
-				       adv748x_power_up_txb_1lane;
-	else
-		reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
-				       adv748x_power_down_txb_1lane;
-
-	return adv748x_write_regs(state, reglist);
+	return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
 }
 
 /* -----------------------------------------------------------------------------
-- 
2.19.1

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

[jacopo mondi]

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

Hi Niklas,

On Fri, Nov 02, 2018 at 05:00:09PM +0100, Niklas Söderlund wrote:
> 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 v2
> - Fix typos in comments.
> - Remove unneeded boiler plait code in adv748x_tx_power() as suggested
>   by Jacopo and Laurent.
> - Take into account the two different register used when powering up TXA
>   and TXB due to an earlier patch in this series aligns the power
>   sequence with the manual.
>
> * 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 | 157 ++++++++++++-----------
>  1 file changed, 79 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 9d80d7f3062b16bc..d94c63cb6a2efdba 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,79 +241,77 @@ 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, 0xdb, 0x10},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0x71, 0x33},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> -
> -	{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, 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, 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 n-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 Write */
> +	if (is_txa(tx)) {
> +		adv748x_write_check(state, page, 0xdb, 0x10, &ret);
> +		adv748x_write_check(state, page, 0xd6, 0x07, &ret);
> +	} else {
> +		adv748x_write_check(state, page, 0xd2, 0x40, &ret);
> +	}
> +
> +	adv748x_write_check(state, page, 0xc4, 0x0a, &ret);
> +	adv748x_write_check(state, page, 0x71, 0x33, &ret);
> +	adv748x_write_check(state, page, 0x72, 0x11, &ret);
> +
> +	/* i2c_dphy_pwdn - 1'b0 */
> +	adv748x_write_check(state, page, 0xf0, 0x00, &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 n-lane MIPI */

Nit: "Enable" in a power_down sequence? I know it was like this before
already though.

> +	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;
>
>  	if (!is_tx_enabled(tx))
> @@ -321,14 +329,7 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>  	WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
>  			"Enabling with unknown bit set");
>
> -	if (on)
> -		reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
> -				       adv748x_power_up_txb_1lane;
> -	else
> -		reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
> -				       adv748x_power_down_txb_1lane;
> -
> -	return adv748x_write_regs(state, reglist);
> +	return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);

Nit apart, this seems very nice to me, so please add
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j

>  }
>
>  /* -----------------------------------------------------------------------------
> --
> 2.19.1
>

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

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Friday, 2 November 2018 18:00:09 EET Niklas Söderlund wrote:
> 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.

"all modes the hardware does" sounds weird. How about simple "all available 
modes" ?

> The driver make use of large tables of static register/value writes when

s/make/makes/

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

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

The code looks so much better now :-)

> ---
> * Changes since v2
> - Fix typos in comments.
> - Remove unneeded boiler plait code in adv748x_tx_power() as suggested
>   by Jacopo and Laurent.
> - Take into account the two different register used when powering up TXA
>   and TXB due to an earlier patch in this series aligns the power
>   sequence with the manual.
> 
> * 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 | 157 ++++++++++++-----------
>  1 file changed, 79 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 9d80d7f3062b16bc..d94c63cb6a2efdba 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,79 +241,77 @@ 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, 0xdb, 0x10},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0x71, 0x33},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> -
> -	{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, 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, 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 n-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 Write */
> +	if (is_txa(tx)) {
> +		adv748x_write_check(state, page, 0xdb, 0x10, &ret);
> +		adv748x_write_check(state, page, 0xd6, 0x07, &ret);
> +	} else {
> +		adv748x_write_check(state, page, 0xd2, 0x40, &ret);
> +	}
> +
> +	adv748x_write_check(state, page, 0xc4, 0x0a, &ret);
> +	adv748x_write_check(state, page, 0x71, 0x33, &ret);
> +	adv748x_write_check(state, page, 0x72, 0x11, &ret);
> +
> +	/* i2c_dphy_pwdn - 1'b0 */
> +	adv748x_write_check(state, page, 0xf0, 0x00, &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 n-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;
> 
>  	if (!is_tx_enabled(tx))
> @@ -321,14 +329,7 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>  	WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
>  			"Enabling with unknown bit set");
> 
> -	if (on)
> -		reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
> -				       adv748x_power_up_txb_1lane;
> -	else
> -		reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
> -				       adv748x_power_down_txb_1lane;
> -
> -	return adv748x_write_regs(state, reglist);
> +	return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
>  }
> 
>  /* ------------------------------------------------------------------------

-- 
Regards,

Laurent Pinchart

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

[jacopo mondi]

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

Hi Niklas,

On Fri, Nov 02, 2018 at 05:00:09PM +0100, Niklas Söderlund wrote:
> 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 v2
> - Fix typos in comments.
> - Remove unneeded boiler plait code in adv748x_tx_power() as suggested
>   by Jacopo and Laurent.
> - Take into account the two different register used when powering up TXA
>   and TXB due to an earlier patch in this series aligns the power
>   sequence with the manual.
>
> * 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 | 157 ++++++++++++-----------
>  1 file changed, 79 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 9d80d7f3062b16bc..d94c63cb6a2efdba 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,79 +241,77 @@ 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, 0xdb, 0x10},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0x71, 0x33},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI Required Write */
> -	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> -
> -	{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, 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, 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 n-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 Write */
> +	if (is_txa(tx)) {
> +		adv748x_write_check(state, page, 0xdb, 0x10, &ret);
> +		adv748x_write_check(state, page, 0xd6, 0x07, &ret);
> +	} else {
> +		adv748x_write_check(state, page, 0xd2, 0x40, &ret);
> +	}
> +
> +	adv748x_write_check(state, page, 0xc4, 0x0a, &ret);
> +	adv748x_write_check(state, page, 0x71, 0x33, &ret);
> +	adv748x_write_check(state, page, 0x72, 0x11, &ret);
> +
> +	/* i2c_dphy_pwdn - 1'b0 */
> +	adv748x_write_check(state, page, 0xf0, 0x00, &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);

Where does the 0x20 come from? I don't see it in the register
description in the HW manual..

> +	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 n-lane MIPI */

The comment is wrong here: this write disables the CSI TX interface.

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

Re-looking at power up/down sequences, this should actually be 0xda = 0x00,
as specified in seciont 9.5.2 of the HW manual.

I know all of these come from tables in the previous version and has not
been introduced by this patch, but while at there it might be worth
fixing them.

And actually, I don't like much the comments for registers pll_en and
dphy_pdn registers, and they might be improved, since you're rewriting
this sequence anyhow.

I had a patch pending, before I realized you could change this in your
next v4. In case you want to have a look:
https://paste.debian.net/1052965/

Thanks
   j

> +
> +	/* 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;
>
>  	if (!is_tx_enabled(tx))
> @@ -321,14 +329,7 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>  	WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
>  			"Enabling with unknown bit set");
>
> -	if (on)
> -		reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
> -				       adv748x_power_up_txb_1lane;
> -	else
> -		reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
> -				       adv748x_power_down_txb_1lane;
> -
> -	return adv748x_write_regs(state, reglist);
> +	return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
>  }
>
>  /* -----------------------------------------------------------------------------
> --
> 2.19.1
>

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

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

[Laurent Pinchart]

Hi Jacopo,

On Friday, 23 November 2018 16:01:54 EET jacopo mondi wrote:
> On Fri, Nov 02, 2018 at 05:00:09PM +0100, Niklas Söderlund wrote:
> > 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 v2
> > - Fix typos in comments.
> > - Remove unneeded boiler plait code in adv748x_tx_power() as suggested
> > 
> >   by Jacopo and Laurent.
> > 
> > - Take into account the two different register used when powering up TXA
> > 
> >   and TXB due to an earlier patch in this series aligns the power
> >   sequence with the manual.
> > 
> > * 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 | 157 ++++++++++++-----------
> >  1 file changed, 79 insertions(+), 78 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > b/drivers/media/i2c/adv748x/adv748x-core.c index
> > 9d80d7f3062b16bc..d94c63cb6a2efdba 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,79 +241,77 @@ 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, 0xdb, 0x10},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXA, 0x71, 0x33},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI Required Write */
> > -	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> > -
> > -	{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, 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, 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 n-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 Write */
> > +	if (is_txa(tx)) {
> > +		adv748x_write_check(state, page, 0xdb, 0x10, &ret);
> > +		adv748x_write_check(state, page, 0xd6, 0x07, &ret);
> > +	} else {
> > +		adv748x_write_check(state, page, 0xd2, 0x40, &ret);
> > +	}
> > +
> > +	adv748x_write_check(state, page, 0xc4, 0x0a, &ret);
> > +	adv748x_write_check(state, page, 0x71, 0x33, &ret);
> > +	adv748x_write_check(state, page, 0x72, 0x11, &ret);
> > +
> > +	/* i2c_dphy_pwdn - 1'b0 */
> > +	adv748x_write_check(state, page, 0xf0, 0x00, &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);
> 
> Where does the 0x20 come from? I don't see it in the register
> description in the HW manual..

Isn't it the EN_AUTOCALC_DPHY_PARAMS bit ?

> > +	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 n-lane MIPI */
> 
> The comment is wrong here: this write disables the CSI TX interface.
> 
> > +	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);
> 
> Re-looking at power up/down sequences, this should actually be 0xda = 0x00,
> as specified in seciont 9.5.2 of the HW manual.
> 
> I know all of these come from tables in the previous version and has not
> been introduced by this patch, but while at there it might be worth
> fixing them.
> 
> And actually, I don't like much the comments for registers pll_en and
> dphy_pdn registers, and they might be improved, since you're rewriting
> this sequence anyhow.
> 
> I had a patch pending, before I realized you could change this in your
> next v4. In case you want to have a look:
> https://paste.debian.net/1052965/

I would prefer fixes to be made on top of this patch, to separate the 
refactoring from the functional changes as much as possible.

> > +
> > +	/* 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;
> >  	
> >  	if (!is_tx_enabled(tx))
> > @@ -321,14 +329,7 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool
> > on)
> >  	WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
> >  			"Enabling with unknown bit set");
> > 
> > -	if (on)
> > -		reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
> > -				       adv748x_power_up_txb_1lane;
> > -	else
> > -		reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
> > -				       adv748x_power_down_txb_1lane;
> > -
> > -	return adv748x_write_regs(state, reglist);
> > +	return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
> >  }
> >  
> >  /* ----------------------------------------------------------------------

-- 
Regards,

Laurent Pinchart

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

[jacopo mondi]

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

Hi Laurent,

On Fri, Nov 23, 2018 at 04:43:18PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Friday, 23 November 2018 16:01:54 EET jacopo mondi wrote:
> > On Fri, Nov 02, 2018 at 05:00:09PM +0100, Niklas Söderlund wrote:
> > > 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 v2
> > > - Fix typos in comments.
> > > - Remove unneeded boiler plait code in adv748x_tx_power() as suggested
> > >
> > >   by Jacopo and Laurent.
> > >
> > > - Take into account the two different register used when powering up TXA
> > >
> > >   and TXB due to an earlier patch in this series aligns the power
> > >   sequence with the manual.
> > >
> > > * 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 | 157 ++++++++++++-----------
> > >  1 file changed, 79 insertions(+), 78 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > > b/drivers/media/i2c/adv748x/adv748x-core.c index
> > > 9d80d7f3062b16bc..d94c63cb6a2efdba 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,79 +241,77 @@ 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, 0xdb, 0x10},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_TXA, 0xd6, 0x07},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_TXA, 0xc4, 0x0a},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_TXA, 0x71, 0x33},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_TXA, 0x72, 0x11},	/* ADI Required Write */
> > > -	{ADV748X_PAGE_TXA, 0xf0, 0x00},	/* i2c_dphy_pwdn - 1'b0 */
> > > -
> > > -	{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, 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, 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 n-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 Write */
> > > +	if (is_txa(tx)) {
> > > +		adv748x_write_check(state, page, 0xdb, 0x10, &ret);
> > > +		adv748x_write_check(state, page, 0xd6, 0x07, &ret);
> > > +	} else {
> > > +		adv748x_write_check(state, page, 0xd2, 0x40, &ret);
> > > +	}
> > > +
> > > +	adv748x_write_check(state, page, 0xc4, 0x0a, &ret);
> > > +	adv748x_write_check(state, page, 0x71, 0x33, &ret);
> > > +	adv748x_write_check(state, page, 0x72, 0x11, &ret);
> > > +
> > > +	/* i2c_dphy_pwdn - 1'b0 */
> > > +	adv748x_write_check(state, page, 0xf0, 0x00, &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);
> >
> > Where does the 0x20 come from? I don't see it in the register
> > description in the HW manual..
>
> Isn't it the EN_AUTOCALC_DPHY_PARAMS bit ?
>

Right, I missed that :)

> > > +	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 n-lane MIPI */
> >
> > The comment is wrong here: this write disables the CSI TX interface.
> >
> > > +	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);
> >
> > Re-looking at power up/down sequences, this should actually be 0xda = 0x00,
> > as specified in seciont 9.5.2 of the HW manual.
> >
> > I know all of these come from tables in the previous version and has not
> > been introduced by this patch, but while at there it might be worth
> > fixing them.
> >
> > And actually, I don't like much the comments for registers pll_en and
> > dphy_pdn registers, and they might be improved, since you're rewriting
> > this sequence anyhow.
> >
> > I had a patch pending, before I realized you could change this in your
> > next v4. In case you want to have a look:
> > https://paste.debian.net/1052965/
>
> I would prefer fixes to be made on top of this patch, to separate the
> refactoring from the functional changes as much as possible.
>

Fine. I will send them based on next iteration of this series from
Niklas.

Thanks
   j

> > > +
> > > +	/* 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;
> > >
> > >  	if (!is_tx_enabled(tx))
> > > @@ -321,14 +329,7 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool
> > > on)
> > >  	WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
> > >  			"Enabling with unknown bit set");
> > >
> > > -	if (on)
> > > -		reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
> > > -				       adv748x_power_up_txb_1lane;
> > > -	else
> > > -		reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
> > > -				       adv748x_power_down_txb_1lane;
> > > -
> > > -	return adv748x_write_regs(state, reglist);
> > > +	return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
> > >  }
> > >
> > >  /* ----------------------------------------------------------------------
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

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

Re: [PATCH v3 0/4] i2c: adv748x: add support for CSI-2 TXA to work

[jacopo mondi]

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

Hi Niklas,

On Fri, Nov 02, 2018 at 05:00:05PM +0100, Niklas Söderlund wrote:
> 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/4 lists the 'data-lanes' DT property as mandatory for endpoints
> describing the CSI-2 transmitters. Patch 2/4 refactors the
> initialization sequence of the adv748x to be able to reuse more code.
> Patch 3/4 adds the DT parsing and storing of the number of lanes. Patch
> 4/4 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.

I have now tested v3 on Ebisu E3 which has only 2 data lanes
connected.

Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j


>
> Niklas Söderlund (4):
>   dt-bindings: adv748x: make data-lanes property mandatory for CSI-2
>     endpoints
>   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 |   4 +-
>  drivers/media/i2c/adv748x/adv748x-core.c      | 235 ++++++++++--------
>  drivers/media/i2c/adv748x/adv748x.h           |   1 +
>  3 files changed, 135 insertions(+), 105 deletions(-)
>
> --
> 2.19.1
>

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