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

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

[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/3 adds the DT parsing and storing of the number of lanes. Patch 
2/3 adds functionality for intercepting and injecting the requested 
number of lanes when writing the register for NUM_LANES for the TXA 
register 0x00. Lastly patch 3/3 fixes a type related to lane settings 
for TXB which is confusing (at lest to me) when reviewing the result of 
this series.

Patch 1/3 and 2/3 could be squashed together but as the method in 2/3 is 
less then obvious since it intercepts the long tables of register writes 
I thought splitting them could ease review.

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 (3):
  i2c: adv748x: store number of CSI-2 lanes described in device tree
  i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter
  i2c: adv748x: fix typo in comment for TXB CSI-2 transmitter power down

 drivers/media/i2c/adv748x/adv748x-core.c | 89 +++++++++++++++++++++---
 drivers/media/i2c/adv748x/adv748x.h      |  1 +
 2 files changed, 81 insertions(+), 9 deletions(-)

-- 
2.18.0

[PATCH 1/3] 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
lines to transmit data on. In order to be able 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>
---
 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 85c027bdcd56748d..a93f8ea89a228474 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"
@@ -561,11 +562,54 @@ 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 (%d) of lanes\n",
+				num_lanes);
+			return -EINVAL;
+		}
+
+		state->txa.num_lanes = num_lanes;
+		adv_dbg(state, "TXA: using %d lanes\n", state->txa.num_lanes);
+	}
+
+	if (vep.base.port == ADV748X_PORT_TXB) {
+		if (num_lanes != 1) {
+			adv_err(state, "TXB: Invalid number (%d) of lanes\n",
+				num_lanes);
+			return -EINVAL;
+		}
+
+		state->txb.num_lanes = num_lanes;
+		adv_dbg(state, "TXB: using %d 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 found = false;
+	int ret;
 
 	for_each_endpoint_of_node(state->dev->of_node, ep_np) {
 		of_graph_parse_endpoint(ep_np, &ep);
@@ -589,6 +633,11 @@ static int adv748x_parse_dt(struct adv748x_state *state)
 		state->endpoints[ep.port] = ep_np;
 
 		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 found ? 0 : -ENODEV;
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index c9016acaba34aff2..88ad06a3045c5427 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -78,6 +78,7 @@ struct adv748x_csi2 {
 	struct adv748x_state *state;
 	struct v4l2_mbus_framefmt format;
 	unsigned int page;
+	unsigned int num_lanes;
 
 	struct media_pad pads[ADV748X_CSI2_NR_PADS];
 	struct v4l2_ctrl_handler ctrl_hdl;
-- 
2.18.0

[PATCH 2/3] 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
configuring the hardware, some writing to undocumented registers.
Instead of creating 3 sets of the register tables for the different
modes catch when the register containing NUM_LANES[2:0] is written to
and inject the correct number of lanes.

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

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index a93f8ea89a228474..9a82cdf301bccb41 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -207,13 +207,23 @@ static int adv748x_write_regs(struct adv748x_state *state,
 			      const struct adv748x_reg_value *regs)
 {
 	int ret;
+	u8 value;
 
 	while (regs->page != ADV748X_PAGE_EOR) {
 		if (regs->page == ADV748X_PAGE_WAIT) {
 			msleep(regs->value);
 		} else {
+			value = regs->value;
+
+			/*
+			 * Register 0x00 in TXA needs to bei injected with
+			 * the number of CSI-2 lanes used to transmitt.
+			 */
+			if (regs->page == ADV748X_PAGE_TXA && regs->reg == 0x00)
+				value = (value & ~7) | state->txa.num_lanes;
+
 			ret = adv748x_write(state, regs->page, regs->reg,
-				      regs->value);
+					    value);
 			if (ret < 0) {
 				adv_err(state,
 					"Error regs page: 0x%02x reg: 0x%02x\n",
@@ -233,14 +243,18 @@ static int adv748x_write_regs(struct adv748x_state *state,
 
 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 */
+	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
+	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
+	{ADV748X_PAGE_TXA, 0x00, 0xa0},	/* 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 */
+
+	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
+	{ADV748X_PAGE_TXA, 0x00, 0x20 },/* 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 */
@@ -253,7 +267,10 @@ 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 */
+
+	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
+	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
+
 	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
 	{ADV748X_PAGE_TXA, 0xc1, 0x3b},	/* ADI Required Write */
 
@@ -399,8 +416,10 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
 	/* Outputs Enabled */
 	{ADV748X_PAGE_IO, 0x10, 0xa0},	/* Enable 4-lane CSI Tx & Pixel Port */
 
-	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
-	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
+	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
+	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
+	{ADV748X_PAGE_TXA, 0x00, 0xa0},	/* 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 */
@@ -412,7 +431,10 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
 	{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 */
+
+	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
+	{ADV748X_PAGE_TXA, 0x00, 0x20 },/* 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 */
-- 
2.18.0

[PATCH 3/3] i2c: adv748x: fix typo in comment for TXB CSI-2 transmitter power down

[Niklas Söderlund]

Fix copy-and-past error in comment for TXB CSI-2 transmitter power down
sequence.

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

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 9a82cdf301bccb41..86cb38f4d7cc11c6 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -299,7 +299,7 @@ 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 4-lane MIPI */
+	{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 */
 
-- 
2.18.0

Re: [PATCH 3/3] i2c: adv748x: fix typo in comment for TXB CSI-2 transmitter power down

[Sergei Shtylyov]

On 9/18/2018 4:45 AM, Niklas Söderlund wrote:

> Fix copy-and-past error in comment for TXB CSI-2 transmitter power down

    -paste. :-)

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

MBR, Sergei

Re: [PATCH 3/3] i2c: adv748x: fix typo in comment for TXB CSI-2 transmitter power down

[Kieran Bingham]

Hi Niklas,

Thank you for the patch,

I don't think this conflicts with Jacopo's series at all does it ?

Perhaps with the amount of adv748x churn currently I should create an
integration/for-next branch :-)

On 18/09/18 02:45, Niklas Söderlund wrote:
> Fix copy-and-past error in comment for TXB CSI-2 transmitter power down
> sequence.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

This looks good and useful to me.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 9a82cdf301bccb41..86cb38f4d7cc11c6 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -299,7 +299,7 @@ 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 4-lane MIPI */
> +	{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 */



-- 
Regards
--
Kieran

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

[Kieran Bingham]

Hi Niklas,

Thank you for the patch,

On 18/09/18 02:45, 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
> configuring the hardware, some writing to undocumented registers.
> Instead of creating 3 sets of the register tables for the different
> modes catch when the register containing NUM_LANES[2:0] is written to
> and inject the correct number of lanes.
> 

Aye aye aye.

Neat solution to avoid adding more tables - but is it necessary? And I
can't find it easy to overlook the hack value in checking every register
write against a specific value :-(


Couldn't we create a function called "adv748x_configure_tx_lanes(...)"
or such and just write this value as appropriate after the tables are
written?

That will then hopefully take us a step towards not needing (as much of)
these tables.


However - *I fully understand ordering may be important here* so
actually it looks like we can't write this after writing the table.

But it does look conveniently early in the tables, so we could split the
tables out and start functionalising them with the information we do know.

I.e. We could have our init function enable the lanes, and handle the
auto DPHY, then write the rest through the tables.


> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 38 +++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index a93f8ea89a228474..9a82cdf301bccb41 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -207,13 +207,23 @@ static int adv748x_write_regs(struct adv748x_state *state,
>  			      const struct adv748x_reg_value *regs)
>  {
>  	int ret;
> +	u8 value;
>  
>  	while (regs->page != ADV748X_PAGE_EOR) {
>  		if (regs->page == ADV748X_PAGE_WAIT) {
>  			msleep(regs->value);
>  		} else {
> +			value = regs->value;
> +
> +			/*
> +			 * Register 0x00 in TXA needs to bei injected with

s/bei/be/

> +			 * the number of CSI-2 lanes used to transmitt.

s/transmitt/transmit/

> +			 */
> +			if (regs->page == ADV748X_PAGE_TXA && regs->reg == 0x00)
> +				value = (value & ~7) | state->txa.num_lanes;
> +
>  			ret = adv748x_write(state, regs->page, regs->reg,
> -				      regs->value);
> +					    value);
>  			if (ret < 0) {
>  				adv_err(state,
>  					"Error regs page: 0x%02x reg: 0x%02x\n",
> @@ -233,14 +243,18 @@ static int adv748x_write_regs(struct adv748x_state *state,
>  
>  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 */
> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
> +	{ADV748X_PAGE_TXA, 0x00, 0xa0},	/* 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 */
> +
> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> +	{ADV748X_PAGE_TXA, 0x00, 0x20 },/* 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 */
> @@ -253,7 +267,10 @@ 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 */
> +
> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */

If we're in power down - shouldn't this be "Disable n-lane MIPI */ ??

> +
>  	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
>  	{ADV748X_PAGE_TXA, 0xc1, 0x3b},	/* ADI Required Write */
>  
> @@ -399,8 +416,10 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
>  	/* Outputs Enabled */
>  	{ADV748X_PAGE_IO, 0x10, 0xa0},	/* Enable 4-lane CSI Tx & Pixel Port */
>  
> -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
> +	{ADV748X_PAGE_TXA, 0x00, 0xa0},	/* 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 */
> @@ -412,7 +431,10 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
>  	{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 */
> +
> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> +	{ADV748X_PAGE_TXA, 0x00, 0x20 },/* 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 */
> 

-- 
Regards
--
Kieran

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday, 18 September 2018 04:45:07 EEST Niklas Söderlund wrote:
> The adv748x CSI-2 transmitters TXA and TXB can use different number of
> lines to transmit data on. In order to be able configure the device

s/lines/lanes/
s/data on/data/
s/able configure/able to configure/

> 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>
> ---
>  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
> 85c027bdcd56748d..a93f8ea89a228474 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"
> @@ -561,11 +562,54 @@ 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 (%d) of lanes\n",

%u for unsigned int (and below as well).

> +				num_lanes);
> +			return -EINVAL;
> +		}
> +
> +		state->txa.num_lanes = num_lanes;
> +		adv_dbg(state, "TXA: using %d lanes\n", state->txa.num_lanes);

I wonder if the debug message is really needed.

Apart from that,

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

> +	}
> +
> +	if (vep.base.port == ADV748X_PORT_TXB) {
> +		if (num_lanes != 1) {
> +			adv_err(state, "TXB: Invalid number (%d) of lanes\n",
> +				num_lanes);
> +			return -EINVAL;
> +		}
> +
> +		state->txb.num_lanes = num_lanes;
> +		adv_dbg(state, "TXB: using %d 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 found = false;
> +	int ret;
> 
>  	for_each_endpoint_of_node(state->dev->of_node, ep_np) {
>  		of_graph_parse_endpoint(ep_np, &ep);
> @@ -589,6 +633,11 @@ static int adv748x_parse_dt(struct adv748x_state
> *state) state->endpoints[ep.port] = ep_np;
> 
>  		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 found ? 0 : -ENODEV;
> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h index
> c9016acaba34aff2..88ad06a3045c5427 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -78,6 +78,7 @@ struct adv748x_csi2 {
>  	struct adv748x_state *state;
>  	struct v4l2_mbus_framefmt format;
>  	unsigned int page;
> +	unsigned int num_lanes;
> 
>  	struct media_pad pads[ADV748X_CSI2_NR_PADS];
>  	struct v4l2_ctrl_handler ctrl_hdl;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] i2c: adv748x: fix typo in comment for TXB CSI-2 transmitter power down

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday, 18 September 2018 04:45:09 EEST Niklas Söderlund wrote:
> Fix copy-and-past error in comment for TXB CSI-2 transmitter power down
> sequence.

Apart from the s/past/paste/ typo that Sergei pointed out, I see no other 
issue.

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

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 9a82cdf301bccb41..86cb38f4d7cc11c6 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -299,7 +299,7 @@ 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 4-lane MIPI */
> +	{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 */


-- 
Regards,

Laurent Pinchart

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

[Kieran Bingham]

Hi Niklas,

Thank you for the patch,

On 18/09/18 02:45, Niklas Söderlund wrote:
> The adv748x CSI-2 transmitters TXA and TXB can use different number of
> lines to transmit data on. In order to be able 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.
> 

Am I right in assuming that it is the CSI device which specifies the
number of lanes in their DT?

Could we make this clear in the commit log (and possibly an extra
comment in the code). At first I was assuming we would have to declare
the number of lanes in the ADV748x TX DT node, but I don't think that's
the case.

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  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 85c027bdcd56748d..a93f8ea89a228474 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"
> @@ -561,11 +562,54 @@ 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 I'm not mistaken we are parsing /someone elses/ DT node here (the CSI
receiver or such).

Is it now guaranteed on the mipi_csi2 bus to have the (correct) lanes
defined?

Do we need to fall back to some safe defaults at all (1 lane?) ?
Actually - perhaps there is no safe default. I guess if the lanes aren't
configured correctly we're not going to get a good signal at the other end.

> +	if (vep.base.port == ADV748X_PORT_TXA) {
> +		if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
> +			adv_err(state, "TXA: Invalid number (%d) of lanes\n",
> +				num_lanes);
> +			return -EINVAL;
> +		}
> +
> +		state->txa.num_lanes = num_lanes;
> +		adv_dbg(state, "TXA: using %d lanes\n", state->txa.num_lanes);
> +	}
> +
> +	if (vep.base.port == ADV748X_PORT_TXB) {
> +		if (num_lanes != 1) {
> +			adv_err(state, "TXB: Invalid number (%d) of lanes\n",
> +				num_lanes);
> +			return -EINVAL;
> +		}
> +
> +		state->txb.num_lanes = num_lanes;
> +		adv_dbg(state, "TXB: using %d 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 found = false;
> +	int ret;
>  
>  	for_each_endpoint_of_node(state->dev->of_node, ep_np) {
>  		of_graph_parse_endpoint(ep_np, &ep);
> @@ -589,6 +633,11 @@ static int adv748x_parse_dt(struct adv748x_state *state)
>  		state->endpoints[ep.port] = ep_np;
>  
>  		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 found ? 0 : -ENODEV;
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index c9016acaba34aff2..88ad06a3045c5427 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -78,6 +78,7 @@ struct adv748x_csi2 {
>  	struct adv748x_state *state;
>  	struct v4l2_mbus_framefmt format;
>  	unsigned int page;
> +	unsigned int num_lanes;
>  
>  	struct media_pad pads[ADV748X_CSI2_NR_PADS];
>  	struct v4l2_ctrl_handler ctrl_hdl;
> 

-- 
Regards
--
Kieran

Re: [PATCH 3/3] i2c: adv748x: fix typo in comment for TXB CSI-2 transmitter power down

[Kieran Bingham]

Hi Niklas,

On 18/09/18 10:54, Kieran Bingham wrote:
> Hi Niklas,
> 
> Thank you for the patch,
> 
> I don't think this conflicts with Jacopo's series at all does it ?
> 
> Perhaps with the amount of adv748x churn currently I should create an
> integration/for-next branch :-)
> 
> On 18/09/18 02:45, Niklas Söderlund wrote:
>> Fix copy-and-past error in comment for TXB CSI-2 transmitter power down
>> sequence.
>>
>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> This looks good and useful to me.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
>> ---
>>  drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
>> index 9a82cdf301bccb41..86cb38f4d7cc11c6 100644
>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
>> @@ -299,7 +299,7 @@ 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 4-lane MIPI */
>> +	{ADV748X_PAGE_TXB, 0x00, 0x81},	/* Enable 1-lane MIPI */

I should just go look at the rest of the code - but it stands out in
this hunk that we are enabling the lane in our power-down sequence?.

Perhaps we power it down further in the table which isn't provided by
the diff.

>>  	{ADV748X_PAGE_TXB, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
>>  	{ADV748X_PAGE_TXB, 0xc1, 0x3b},	/* ADI Required Write */
> 
> 
> 

-- 
Regards
--
Kieran

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

[Laurent Pinchart]

Hi Kieran,

On Tuesday, 18 September 2018 13:19:39 EEST Kieran Bingham wrote:
> On 18/09/18 02:45, Niklas Söderlund wrote:
> > The adv748x CSI-2 transmitters TXA and TXB can use different number of
> > lines to transmit data on. In order to be able 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.
> 
> Am I right in assuming that it is the CSI device which specifies the
> number of lanes in their DT?

Do you mean the CSI-2 receiver ? Both the receiver and the transmitter should 
specify the data lanes in their DT node.

> Could we make this clear in the commit log (and possibly an extra
> comment in the code). At first I was assuming we would have to declare
> the number of lanes in the ADV748x TX DT node, but I don't think that's
> the case.
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> > 
> >  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
> > 85c027bdcd56748d..a93f8ea89a228474 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"
> > @@ -561,11 +562,54 @@ 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 I'm not mistaken we are parsing /someone elses/ DT node here (the CSI
> receiver or such).

Aren't we parsing our own endpoint ? The ep argument comes from ep_np in 
adv748x_parse_dt(), and that's the endpoint iterator used with

	for_each_endpoint_of_node(state->dev->of_node, ep_np)

> Is it now guaranteed on the mipi_csi2 bus to have the (correct) lanes
> defined?
> 
> Do we need to fall back to some safe defaults at all (1 lane?) ?
> Actually - perhaps there is no safe default. I guess if the lanes aren't
> configured correctly we're not going to get a good signal at the other end.

The endpoints should contain a data-lanes property. That's the case in the 
mainline DT sources, but it's not explicitly stated as a mandatory property. I 
think we should update the bindings.

> > +	if (vep.base.port == ADV748X_PORT_TXA) {
> > +		if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
> > +			adv_err(state, "TXA: Invalid number (%d) of lanes\n",
> > +				num_lanes);
> > +			return -EINVAL;
> > +		}
> > +
> > +		state->txa.num_lanes = num_lanes;
> > +		adv_dbg(state, "TXA: using %d lanes\n", state->txa.num_lanes);
> > +	}
> > +
> > +	if (vep.base.port == ADV748X_PORT_TXB) {
> > +		if (num_lanes != 1) {
> > +			adv_err(state, "TXB: Invalid number (%d) of lanes\n",
> > +				num_lanes);
> > +			return -EINVAL;
> > +		}
> > +
> > +		state->txb.num_lanes = num_lanes;
> > +		adv_dbg(state, "TXB: using %d 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 found = false;
> > +	int ret;
> > 
> >  	for_each_endpoint_of_node(state->dev->of_node, ep_np) {
> >  		of_graph_parse_endpoint(ep_np, &ep);
> > @@ -589,6 +633,11 @@ static int adv748x_parse_dt(struct adv748x_state
> > *state)
> >  		state->endpoints[ep.port] = ep_np;
> >  		
> >  		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 found ? 0 : -ENODEV;
> > diff --git a/drivers/media/i2c/adv748x/adv748x.h
> > b/drivers/media/i2c/adv748x/adv748x.h index
> > c9016acaba34aff2..88ad06a3045c5427 100644
> > --- a/drivers/media/i2c/adv748x/adv748x.h
> > +++ b/drivers/media/i2c/adv748x/adv748x.h
> > @@ -78,6 +78,7 @@ struct adv748x_csi2 {
> >  	struct adv748x_state *state;
> >  	struct v4l2_mbus_framefmt format;
> >  	unsigned int page;
> > +	unsigned int num_lanes;
> > 
> >  	struct media_pad pads[ADV748X_CSI2_NR_PADS];
> >  	struct v4l2_ctrl_handler ctrl_hdl;

-- 
Regards,

Laurent Pinchart

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

[Kieran Bingham]

Hi Laurent, Niklas,

On 18/09/18 11:28, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Tuesday, 18 September 2018 13:19:39 EEST Kieran Bingham wrote:
>> On 18/09/18 02:45, Niklas Söderlund wrote:
>>> The adv748x CSI-2 transmitters TXA and TXB can use different number of
>>> lines to transmit data on. In order to be able 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.
>>
>> Am I right in assuming that it is the CSI device which specifies the
>> number of lanes in their DT?
> 
> Do you mean the CSI-2 receiver ? Both the receiver and the transmitter should 
> specify the data lanes in their DT node.

Yes, I should have said CSI-2 receiver.

Aha - so *both* sides of the link have to specify the lanes and
presumably match with each other?

> 
>> Could we make this clear in the commit log (and possibly an extra
>> comment in the code). At first I was assuming we would have to declare
>> the number of lanes in the ADV748x TX DT node, but I don't think that's
>> the case.
>>
>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>> ---
>>>
>>>  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
>>> 85c027bdcd56748d..a93f8ea89a228474 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"
>>> @@ -561,11 +562,54 @@ 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 I'm not mistaken we are parsing /someone elses/ DT node here (the CSI
>> receiver or such).
> 
> Aren't we parsing our own endpoint ? The ep argument comes from ep_np in 
> adv748x_parse_dt(), and that's the endpoint iterator used with
> 
> 	for_each_endpoint_of_node(state->dev->of_node, ep_np)

Bah - my head was polluted with the async subdevice stuff where we were
getting the endpoint of the other device, but of course that's
completely unrelated here.


> 
>> Is it now guaranteed on the mipi_csi2 bus to have the (correct) lanes
>> defined?
>>
>> Do we need to fall back to some safe defaults at all (1 lane?) ?
>> Actually - perhaps there is no safe default. I guess if the lanes aren't
>> configured correctly we're not going to get a good signal at the other end.
> 
> The endpoints should contain a data-lanes property. That's the case in the 
> mainline DT sources, but it's not explicitly stated as a mandatory property. I 
> think we should update the bindings.

Yes, - as this code change is making the property mandatory - we should
certainly state that in the bindings, unless we can fall back to a
sensible default (perhaps the max supported on that component?)


> 
>>> +	if (vep.base.port == ADV748X_PORT_TXA) {
>>> +		if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
>>> +			adv_err(state, "TXA: Invalid number (%d) of lanes\n",
>>> +				num_lanes);
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		state->txa.num_lanes = num_lanes;
>>> +		adv_dbg(state, "TXA: using %d lanes\n", state->txa.num_lanes);
>>> +	}
>>> +
>>> +	if (vep.base.port == ADV748X_PORT_TXB) {
>>> +		if (num_lanes != 1) {
>>> +			adv_err(state, "TXB: Invalid number (%d) of lanes\n",
>>> +				num_lanes);
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		state->txb.num_lanes = num_lanes;
>>> +		adv_dbg(state, "TXB: using %d 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 found = false;
>>> +	int ret;
>>>
>>>  	for_each_endpoint_of_node(state->dev->of_node, ep_np) {
>>>  		of_graph_parse_endpoint(ep_np, &ep);
>>> @@ -589,6 +633,11 @@ static int adv748x_parse_dt(struct adv748x_state
>>> *state)
>>>  		state->endpoints[ep.port] = ep_np;
>>>  		
>>>  		found = true;
>>> +
>>> +		/* Store number of CSI-2 lanes used for TXA and TXB. */

Potentially : s/Store/Identify the/ ?

>>> +		ret = adv748x_parse_csi2_lanes(state, ep.port, ep_np);
>>> +		if (ret)
>>> +			return ret;
>>>  	}
>>>  	
>>>  	return found ? 0 : -ENODEV;
>>> diff --git a/drivers/media/i2c/adv748x/adv748x.h
>>> b/drivers/media/i2c/adv748x/adv748x.h index
>>> c9016acaba34aff2..88ad06a3045c5427 100644
>>> --- a/drivers/media/i2c/adv748x/adv748x.h
>>> +++ b/drivers/media/i2c/adv748x/adv748x.h
>>> @@ -78,6 +78,7 @@ struct adv748x_csi2 {
>>>  	struct adv748x_state *state;
>>>  	struct v4l2_mbus_framefmt format;
>>>  	unsigned int page;
>>> +	unsigned int num_lanes;
>>>
>>>  	struct media_pad pads[ADV748X_CSI2_NR_PADS];
>>>  	struct v4l2_ctrl_handler ctrl_hdl;
> 

-- 
Regards
--
Kieran

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

[Laurent Pinchart]

Hi Kieran,

On Tuesday, 18 September 2018 13:37:55 EEST Kieran Bingham wrote:
> On 18/09/18 11:28, Laurent Pinchart wrote:
> > On Tuesday, 18 September 2018 13:19:39 EEST Kieran Bingham wrote:
> >> On 18/09/18 02:45, Niklas Söderlund wrote:
> >>> The adv748x CSI-2 transmitters TXA and TXB can use different number of
> >>> lines to transmit data on. In order to be able 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.
> >> 
> >> Am I right in assuming that it is the CSI device which specifies the
> >> number of lanes in their DT?
> > 
> > Do you mean the CSI-2 receiver ? Both the receiver and the transmitter
> > should specify the data lanes in their DT node.
> 
> Yes, I should have said CSI-2 receiver.
> 
> Aha - so *both* sides of the link have to specify the lanes and
> presumably match with each other?

Yes, they should certainly match :-)

> >> Could we make this clear in the commit log (and possibly an extra
> >> comment in the code). At first I was assuming we would have to declare
> >> the number of lanes in the ADV748x TX DT node, but I don't think that's
> >> the case.
> >> 
> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>> ---
> >>> 
> >>>  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
> >>> 85c027bdcd56748d..a93f8ea89a228474 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c

[snip]

> >>> +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 I'm not mistaken we are parsing /someone elses/ DT node here (the CSI
> >> receiver or such).
> > 
> > Aren't we parsing our own endpoint ? The ep argument comes from ep_np in
> > adv748x_parse_dt(), and that's the endpoint iterator used with
> > 
> > 	for_each_endpoint_of_node(state->dev->of_node, ep_np)
> 
> Bah - my head was polluted with the async subdevice stuff where we were
> getting the endpoint of the other device, but of course that's
> completely unrelated here.
> 
> >> Is it now guaranteed on the mipi_csi2 bus to have the (correct) lanes
> >> defined?
> >> 
> >> Do we need to fall back to some safe defaults at all (1 lane?) ?
> >> Actually - perhaps there is no safe default. I guess if the lanes aren't
> >> configured correctly we're not going to get a good signal at the other
> >> end.
> > 
> > The endpoints should contain a data-lanes property. That's the case in the
> > mainline DT sources, but it's not explicitly stated as a mandatory
> > property. I think we should update the bindings.
> 
> Yes, - as this code change is making the property mandatory - we should
> certainly state that in the bindings, unless we can fall back to a
> sensible default (perhaps the max supported on that component?)

I'm not sure there's a sensible default, I'd rather specify it explicitly. 
Note that the data-lanes property doesn't just specify the number of lanes, 
but also how they are remapped, when that feature is supported by the CSI-2 
transmitter or receiver.

> >>> +	if (vep.base.port == ADV748X_PORT_TXA) {
> >>> +		if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
> >>> +			adv_err(state, "TXA: Invalid number (%d) of lanes\n",
> >>> +				num_lanes);
> >>> +			return -EINVAL;
> >>> +		}
> >>> +
> >>> +		state->txa.num_lanes = num_lanes;
> >>> +		adv_dbg(state, "TXA: using %d lanes\n", state->txa.num_lanes);
> >>> +	}
> >>> +
> >>> +	if (vep.base.port == ADV748X_PORT_TXB) {
> >>> +		if (num_lanes != 1) {
> >>> +			adv_err(state, "TXB: Invalid number (%d) of lanes\n",
> >>> +				num_lanes);
> >>> +			return -EINVAL;
> >>> +		}
> >>> +
> >>> +		state->txb.num_lanes = num_lanes;
> >>> +		adv_dbg(state, "TXB: using %d lanes\n", state->txb.num_lanes);
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}

[snip]

-- 
Regards,

Laurent Pinchart

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

[Kieran Bingham]

Hi Laurent,

On 18/09/18 11:46, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Tuesday, 18 September 2018 13:37:55 EEST Kieran Bingham wrote:
>> On 18/09/18 11:28, Laurent Pinchart wrote:
>>> On Tuesday, 18 September 2018 13:19:39 EEST Kieran Bingham wrote:
>>>> On 18/09/18 02:45, Niklas Söderlund wrote:
>>>>> The adv748x CSI-2 transmitters TXA and TXB can use different number of
>>>>> lines to transmit data on. In order to be able 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.
>>>>
>>>> Am I right in assuming that it is the CSI device which specifies the
>>>> number of lanes in their DT?
>>>
>>> Do you mean the CSI-2 receiver ? Both the receiver and the transmitter
>>> should specify the data lanes in their DT node.
>>
>> Yes, I should have said CSI-2 receiver.
>>
>> Aha - so *both* sides of the link have to specify the lanes and
>> presumably match with each other?
> 
> Yes, they should certainly match :-)

I assumed so :) - do we need to validate that at a framework level?
(or perhaps it already is, all I've only looked at this morning is
e-mails :D )

> 
>>>> Could we make this clear in the commit log (and possibly an extra
>>>> comment in the code). At first I was assuming we would have to declare
>>>> the number of lanes in the ADV748x TX DT node, but I don't think that's
>>>> the case.
>>>>
>>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>>> ---
>>>>>
>>>>>  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
>>>>> 85c027bdcd56748d..a93f8ea89a228474 100644
>>>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
>>>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> 
> [snip]
> 
>>>>> +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 I'm not mistaken we are parsing /someone elses/ DT node here (the CSI
>>>> receiver or such).
>>>
>>> Aren't we parsing our own endpoint ? The ep argument comes from ep_np in
>>> adv748x_parse_dt(), and that's the endpoint iterator used with
>>>
>>> 	for_each_endpoint_of_node(state->dev->of_node, ep_np)
>>
>> Bah - my head was polluted with the async subdevice stuff where we were
>> getting the endpoint of the other device, but of course that's
>> completely unrelated here.
>>
>>>> Is it now guaranteed on the mipi_csi2 bus to have the (correct) lanes
>>>> defined?
>>>>
>>>> Do we need to fall back to some safe defaults at all (1 lane?) ?
>>>> Actually - perhaps there is no safe default. I guess if the lanes aren't
>>>> configured correctly we're not going to get a good signal at the other
>>>> end.
>>>
>>> The endpoints should contain a data-lanes property. That's the case in the
>>> mainline DT sources, but it's not explicitly stated as a mandatory
>>> property. I think we should update the bindings.
>>
>> Yes, - as this code change is making the property mandatory - we should
>> certainly state that in the bindings, unless we can fall back to a
>> sensible default (perhaps the max supported on that component?)
> 
> I'm not sure there's a sensible default, I'd rather specify it explicitly. 
> Note that the data-lanes property doesn't just specify the number of lanes, 
> but also how they are remapped, when that feature is supported by the CSI-2 
> transmitter or receiver.


Ok understood. As I feared - we can't really default - because it has to
match and be defined.

So making the DT property mandatory really is the way to go then.



>>>>> +	if (vep.base.port == ADV748X_PORT_TXA) {
>>>>> +		if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
>>>>> +			adv_err(state, "TXA: Invalid number (%d) of lanes\n",
>>>>> +				num_lanes);
>>>>> +			return -EINVAL;
>>>>> +		}
>>>>> +
>>>>> +		state->txa.num_lanes = num_lanes;
>>>>> +		adv_dbg(state, "TXA: using %d lanes\n", state->txa.num_lanes);
>>>>> +	}
>>>>> +
>>>>> +	if (vep.base.port == ADV748X_PORT_TXB) {
>>>>> +		if (num_lanes != 1) {
>>>>> +			adv_err(state, "TXB: Invalid number (%d) of lanes\n",
>>>>> +				num_lanes);
>>>>> +			return -EINVAL;
>>>>> +		}
>>>>> +
>>>>> +		state->txb.num_lanes = num_lanes;
>>>>> +		adv_dbg(state, "TXB: using %d lanes\n", state->txb.num_lanes);
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
> 
> [snip]
> 

-- 
Regards
--
Kieran

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

[Laurent Pinchart]

Hi Kieran,

On Tuesday, 18 September 2018 13:51:34 EEST Kieran Bingham wrote:
> On 18/09/18 11:46, Laurent Pinchart wrote:
> > On Tuesday, 18 September 2018 13:37:55 EEST Kieran Bingham wrote:
> >> On 18/09/18 11:28, Laurent Pinchart wrote:
> >>> On Tuesday, 18 September 2018 13:19:39 EEST Kieran Bingham wrote:
> >>>> On 18/09/18 02:45, Niklas Söderlund wrote:
> >>>>> The adv748x CSI-2 transmitters TXA and TXB can use different number of
> >>>>> lines to transmit data on. In order to be able 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.
> >>>> 
> >>>> Am I right in assuming that it is the CSI device which specifies the
> >>>> number of lanes in their DT?
> >>> 
> >>> Do you mean the CSI-2 receiver ? Both the receiver and the transmitter
> >>> should specify the data lanes in their DT node.
> >> 
> >> Yes, I should have said CSI-2 receiver.
> >> 
> >> Aha - so *both* sides of the link have to specify the lanes and
> >> presumably match with each other?
> > 
> > Yes, they should certainly match :-)
> 
> I assumed so :) - do we need to validate that at a framework level?
> (or perhaps it already is, all I've only looked at this morning is
> e-mails :D )

It's not done yet as far as I know. CC'ing Sakari who may have a comment 
regarding whether this should be added.

> >>>> Could we make this clear in the commit log (and possibly an extra
> >>>> comment in the code). At first I was assuming we would have to declare
> >>>> the number of lanes in the ADV748x TX DT node, but I don't think that's
> >>>> the case.
> >>>> 
> >>>>> Signed-off-by: Niklas Söderlund
> >>>>> <niklas.soderlund+renesas@ragnatech.se>
> >>>>> ---
> >>>>> 
> >>>>>  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
> >>>>> 85c027bdcd56748d..a93f8ea89a228474 100644
> >>>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> >>>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > 
> > [snip]
> > 
> >>>>> +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 I'm not mistaken we are parsing /someone elses/ DT node here (the
> >>>> CSI receiver or such).
> >>> 
> >>> Aren't we parsing our own endpoint ? The ep argument comes from ep_np in
> >>> adv748x_parse_dt(), and that's the endpoint iterator used with
> >>> 
> >>> 	for_each_endpoint_of_node(state->dev->of_node, ep_np)
> >> 
> >> Bah - my head was polluted with the async subdevice stuff where we were
> >> getting the endpoint of the other device, but of course that's
> >> completely unrelated here.
> >> 
> >>>> Is it now guaranteed on the mipi_csi2 bus to have the (correct) lanes
> >>>> defined?
> >>>> 
> >>>> Do we need to fall back to some safe defaults at all (1 lane?) ?
> >>>> Actually - perhaps there is no safe default. I guess if the lanes
> >>>> aren't configured correctly we're not going to get a good signal at the
> >>>> other end.
> >>> 
> >>> The endpoints should contain a data-lanes property. That's the case in
> >>> the mainline DT sources, but it's not explicitly stated as a mandatory
> >>> property. I think we should update the bindings.
> >> 
> >> Yes, - as this code change is making the property mandatory - we should
> >> certainly state that in the bindings, unless we can fall back to a
> >> sensible default (perhaps the max supported on that component?)
> > 
> > I'm not sure there's a sensible default, I'd rather specify it explicitly.
> > Note that the data-lanes property doesn't just specify the number of
> > lanes, but also how they are remapped, when that feature is supported by
> > the CSI-2 transmitter or receiver.
> 
> Ok understood. As I feared - we can't really default - because it has to
> match and be defined.
> 
> So making the DT property mandatory really is the way to go then.
> 
> >>>>> +	if (vep.base.port == ADV748X_PORT_TXA) {
> >>>>> +		if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
> >>>>> +			adv_err(state, "TXA: Invalid number (%d) of lanes\n",
> >>>>> +				num_lanes);
> >>>>> +			return -EINVAL;
> >>>>> +		}
> >>>>> +
> >>>>> +		state->txa.num_lanes = num_lanes;
> >>>>> +		adv_dbg(state, "TXA: using %d lanes\n", state->txa.num_lanes);
> >>>>> +	}
> >>>>> +
> >>>>> +	if (vep.base.port == ADV748X_PORT_TXB) {
> >>>>> +		if (num_lanes != 1) {
> >>>>> +			adv_err(state, "TXB: Invalid number (%d) of lanes\n",
> >>>>> +				num_lanes);
> >>>>> +			return -EINVAL;
> >>>>> +		}
> >>>>> +
> >>>>> +		state->txb.num_lanes = num_lanes;
> >>>>> +		adv_dbg(state, "TXB: using %d lanes\n", state->txb.num_lanes);
> >>>>> +	}
> >>>>> +
> >>>>> +	return 0;
> >>>>> +}
> > 
> > [snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] i2c: adv748x: fix typo in comment for TXB CSI-2 transmitter power down

[jacopo mondi]

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

Hi Kieran,

On Tue, Sep 18, 2018 at 10:54:44AM +0100, Kieran Bingham wrote:
> Hi Niklas,
>
> Thank you for the patch,
>
> I don't think this conflicts with Jacopo's series at all does it ?

It does, and I think this series should have been (re)based, or the
other way around, but all these changes should probably go together,
don't they?

>
> Perhaps with the amount of adv748x churn currently I should create an
> integration/for-next branch :-)
>

Also, but we may be able to handle this a single series, once we have
Ebisu working.

Thanks
   j

> On 18/09/18 02:45, Niklas Söderlund wrote:
> > Fix copy-and-past error in comment for TXB CSI-2 transmitter power down
> > sequence.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> This looks good and useful to me.
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index 9a82cdf301bccb41..86cb38f4d7cc11c6 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -299,7 +299,7 @@ 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 4-lane MIPI */
> > +	{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 */
>
>
>
> --
> Regards
> --
> Kieran

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

Re: [PATCH 3/3] i2c: adv748x: fix typo in comment for TXB CSI-2 transmitter power down

[Kieran Bingham]

Hi Jacopo,

On 18/09/18 13:34, jacopo mondi wrote:
> Hi Kieran,
> 
> On Tue, Sep 18, 2018 at 10:54:44AM +0100, Kieran Bingham wrote:
>> Hi Niklas,
>>
>> Thank you for the patch,
>>
>> I don't think this conflicts with Jacopo's series at all does it ?
> 
> It does, and I think this series should have been (re)based, or the
> other way around, but all these changes should probably go together,
> don't they?

I think when I wrote that comment, I actually meant *just this patch* :-)

I think your series is more mature and closer to integration, so it
might be that this series should be on top.


>>
>> Perhaps with the amount of adv748x churn currently I should create an
>> integration/for-next branch :-)
>>
> 
> Also, but we may be able to handle this a single series, once we have
> Ebisu working.
> 
> Thanks
>    j
> 
>> On 18/09/18 02:45, Niklas Söderlund wrote:
>>> Fix copy-and-past error in comment for TXB CSI-2 transmitter power down
>>> sequence.
>>>
>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>
>> This looks good and useful to me.
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>>> ---
>>>  drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
>>> index 9a82cdf301bccb41..86cb38f4d7cc11c6 100644
>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
>>> @@ -299,7 +299,7 @@ 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 4-lane MIPI */
>>> +	{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 */
>>
>>
>>
>> --
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran

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

[Niklas Söderlund]

Hi Kieran and Laurent,

Thanks for all your comments!

On 2018-09-18 11:51:34 +0100, Kieran Bingham wrote:

[snip]

> > 
> > I'm not sure there's a sensible default, I'd rather specify it explicitly. 
> > Note that the data-lanes property doesn't just specify the number of lanes, 
> > but also how they are remapped, when that feature is supported by the CSI-2 
> > transmitter or receiver.
> 
> 
> Ok understood. As I feared - we can't really default - because it has to
> match and be defined.
> 
> So making the DT property mandatory really is the way to go then.

I will add a patch making the property mandatory.

-- 
Regards,
Niklas Söderlund

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

[Niklas Söderlund]

Hi Kieran and Laurent,

Thanks for all your comments!

On 2018-09-18 11:51:34 +0100, Kieran Bingham wrote:

[snip]

> > 
> > I'm not sure there's a sensible default, I'd rather specify it explicitly. 
> > Note that the data-lanes property doesn't just specify the number of lanes, 
> > but also how they are remapped, when that feature is supported by the CSI-2 
> > transmitter or receiver.
> 
> 
> Ok understood. As I feared - we can't really default - because it has to
> match and be defined.
> 
> So making the DT property mandatory really is the way to go then.

I will add a patch making the property mandatory.

-- 
Regards,
Niklas S�derlund

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

[Niklas Söderlund]

Hi Kieran,

Thanks for your comments.

On 2018-09-18 11:13:45 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> Thank you for the patch,
> 
> On 18/09/18 02:45, 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
> > configuring the hardware, some writing to undocumented registers.
> > Instead of creating 3 sets of the register tables for the different
> > modes catch when the register containing NUM_LANES[2:0] is written to
> > and inject the correct number of lanes.
> > 
> 
> Aye aye aye.
> 
> Neat solution to avoid adding more tables - but is it necessary? And I
> can't find it easy to overlook the hack value in checking every register
> write against a specific value :-(

I agree it's not the most obvious nice solution.

> 
> 
> Couldn't we create a function called "adv748x_configure_tx_lanes(...)"
> or such and just write this value as appropriate after the tables are
> written?
> 
> That will then hopefully take us a step towards not needing (as much of)
> these tables.

This was my starting point :-) But the register is referenced multiple 
times in the tables and converting the tables to individual register 
writes turned out such a mess I judged this solution to be less of a 
maintenance burden. I'm however open to your views on how you would 
prefer this to be handled. Keep in mind that each row in the register 
tables needs to be turned into a:

    /* Write registerx */
    ret = adv748x_write(...)
    if (ret)
        return ret;

So the overview of what happens become much harder to read. Other 
options such as creating copies of the tables and injecting the NUM_LANE 
value at probe time I feel just hides the behavior even more.

Another option I tried was to splice the tables whenever the register in 
question was referenced. This became hard to read but less lines of 
code.

> However - *I fully understand ordering may be important here* so
> actually it looks like we can't write this after writing the table.
> 
> But it does look conveniently early in the tables, so we could split the
> tables out and start functionalising them with the information we do know.

I have not tested if ordering is important or not, the documentation we 
have is just a sequential list of register writes, The register is used 
in multiple places in the tables making things even more ugly.

adv748x_power_up_txa_4lane: 3 times, beginning and middle
adv748x_power_down_txa_4lane: 1 time, middle
adv748x_init_txa_4lane: 3 times, middle and end

> 
> I.e. We could have our init function enable the lanes, and handle the
> auto DPHY, then write the rest through the tables.

If only that where possible :-)

I hold off posting v2 until I know how you wish to handle this. To help 
you make a decision the number of register writes in the tables involved 
:-)

adv748x_power_up_txa_4lane: 11
adv748x_power_down_txa_4lane: 5
adv748x_init_txa_4lane: ~50

> 
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 38 +++++++++++++++++++-----
> >  1 file changed, 30 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index a93f8ea89a228474..9a82cdf301bccb41 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -207,13 +207,23 @@ static int adv748x_write_regs(struct adv748x_state *state,
> >  			      const struct adv748x_reg_value *regs)
> >  {
> >  	int ret;
> > +	u8 value;
> >  
> >  	while (regs->page != ADV748X_PAGE_EOR) {
> >  		if (regs->page == ADV748X_PAGE_WAIT) {
> >  			msleep(regs->value);
> >  		} else {
> > +			value = regs->value;
> > +
> > +			/*
> > +			 * Register 0x00 in TXA needs to bei injected with
> 
> s/bei/be/
> 
> > +			 * the number of CSI-2 lanes used to transmitt.
> 
> s/transmitt/transmit/
> 
> > +			 */
> > +			if (regs->page == ADV748X_PAGE_TXA && regs->reg == 0x00)
> > +				value = (value & ~7) | state->txa.num_lanes;
> > +
> >  			ret = adv748x_write(state, regs->page, regs->reg,
> > -				      regs->value);
> > +					    value);
> >  			if (ret < 0) {
> >  				adv_err(state,
> >  					"Error regs page: 0x%02x reg: 0x%02x\n",
> > @@ -233,14 +243,18 @@ static int adv748x_write_regs(struct adv748x_state *state,
> >  
> >  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 */
> > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> > +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
> > +	{ADV748X_PAGE_TXA, 0x00, 0xa0},	/* 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 */
> > +
> > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> > +	{ADV748X_PAGE_TXA, 0x00, 0x20 },/* 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 */
> > @@ -253,7 +267,10 @@ 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 */
> > +
> > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> > +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
> 
> If we're in power down - shouldn't this be "Disable n-lane MIPI */ ??

Well the register write enables the lanes. IIRC the comments here come 
from the register tables text files found in the "documentation".

> 
> > +
> >  	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> >  	{ADV748X_PAGE_TXA, 0xc1, 0x3b},	/* ADI Required Write */
> >  
> > @@ -399,8 +416,10 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
> >  	/* Outputs Enabled */
> >  	{ADV748X_PAGE_IO, 0x10, 0xa0},	/* Enable 4-lane CSI Tx & Pixel Port */
> >  
> > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> > +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
> > +	{ADV748X_PAGE_TXA, 0x00, 0xa0},	/* 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 */
> > @@ -412,7 +431,10 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
> >  	{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 */
> > +
> > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> > +	{ADV748X_PAGE_TXA, 0x00, 0x20 },/* 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 */
> > 
> 
> -- 
> Regards
> --
> Kieran

-- 
Regards,
Niklas Söderlund

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

[Niklas Söderlund]

Hi Kieran,

Thanks for your comments.

On 2018-09-18 11:13:45 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> Thank you for the patch,
> 
> On 18/09/18 02:45, 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
> > configuring the hardware, some writing to undocumented registers.
> > Instead of creating 3 sets of the register tables for the different
> > modes catch when the register containing NUM_LANES[2:0] is written to
> > and inject the correct number of lanes.
> > 
> 
> Aye aye aye.
> 
> Neat solution to avoid adding more tables - but is it necessary? And I
> can't find it easy to overlook the hack value in checking every register
> write against a specific value :-(

I agree it's not the most obvious nice solution.

> 
> 
> Couldn't we create a function called "adv748x_configure_tx_lanes(...)"
> or such and just write this value as appropriate after the tables are
> written?
> 
> That will then hopefully take us a step towards not needing (as much of)
> these tables.

This was my starting point :-) But the register is referenced multiple 
times in the tables and converting the tables to individual register 
writes turned out such a mess I judged this solution to be less of a 
maintenance burden. I'm however open to your views on how you would 
prefer this to be handled. Keep in mind that each row in the register 
tables needs to be turned into a:

    /* Write registerx */
    ret = adv748x_write(...)
    if (ret)
        return ret;

So the overview of what happens become much harder to read. Other 
options such as creating copies of the tables and injecting the NUM_LANE 
value at probe time I feel just hides the behavior even more.

Another option I tried was to splice the tables whenever the register in 
question was referenced. This became hard to read but less lines of 
code.

> However - *I fully understand ordering may be important here* so
> actually it looks like we can't write this after writing the table.
> 
> But it does look conveniently early in the tables, so we could split the
> tables out and start functionalising them with the information we do know.

I have not tested if ordering is important or not, the documentation we 
have is just a sequential list of register writes, The register is used 
in multiple places in the tables making things even more ugly.

adv748x_power_up_txa_4lane: 3 times, beginning and middle
adv748x_power_down_txa_4lane: 1 time, middle
adv748x_init_txa_4lane: 3 times, middle and end

> 
> I.e. We could have our init function enable the lanes, and handle the
> auto DPHY, then write the rest through the tables.

If only that where possible :-)

I hold off posting v2 until I know how you wish to handle this. To help 
you make a decision the number of register writes in the tables involved 
:-)

adv748x_power_up_txa_4lane: 11
adv748x_power_down_txa_4lane: 5
adv748x_init_txa_4lane: ~50

> 
> 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 38 +++++++++++++++++++-----
> >  1 file changed, 30 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index a93f8ea89a228474..9a82cdf301bccb41 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -207,13 +207,23 @@ static int adv748x_write_regs(struct adv748x_state *state,
> >  			      const struct adv748x_reg_value *regs)
> >  {
> >  	int ret;
> > +	u8 value;
> >  
> >  	while (regs->page != ADV748X_PAGE_EOR) {
> >  		if (regs->page == ADV748X_PAGE_WAIT) {
> >  			msleep(regs->value);
> >  		} else {
> > +			value = regs->value;
> > +
> > +			/*
> > +			 * Register 0x00 in TXA needs to bei injected with
> 
> s/bei/be/
> 
> > +			 * the number of CSI-2 lanes used to transmitt.
> 
> s/transmitt/transmit/
> 
> > +			 */
> > +			if (regs->page == ADV748X_PAGE_TXA && regs->reg == 0x00)
> > +				value = (value & ~7) | state->txa.num_lanes;
> > +
> >  			ret = adv748x_write(state, regs->page, regs->reg,
> > -				      regs->value);
> > +					    value);
> >  			if (ret < 0) {
> >  				adv_err(state,
> >  					"Error regs page: 0x%02x reg: 0x%02x\n",
> > @@ -233,14 +243,18 @@ static int adv748x_write_regs(struct adv748x_state *state,
> >  
> >  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 */
> > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> > +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
> > +	{ADV748X_PAGE_TXA, 0x00, 0xa0},	/* 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 */
> > +
> > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> > +	{ADV748X_PAGE_TXA, 0x00, 0x20 },/* 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 */
> > @@ -253,7 +267,10 @@ 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 */
> > +
> > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> > +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
> 
> If we're in power down - shouldn't this be "Disable n-lane MIPI */ ??

Well the register write enables the lanes. IIRC the comments here come 
from the register tables text files found in the "documentation".

> 
> > +
> >  	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> >  	{ADV748X_PAGE_TXA, 0xc1, 0x3b},	/* ADI Required Write */
> >  
> > @@ -399,8 +416,10 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
> >  	/* Outputs Enabled */
> >  	{ADV748X_PAGE_IO, 0x10, 0xa0},	/* Enable 4-lane CSI Tx & Pixel Port */
> >  
> > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> > +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
> > +	{ADV748X_PAGE_TXA, 0x00, 0xa0},	/* 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 */
> > @@ -412,7 +431,10 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
> >  	{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 */
> > +
> > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
> > +	{ADV748X_PAGE_TXA, 0x00, 0x20 },/* 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 */
> > 
> 
> -- 
> Regards
> --
> Kieran

-- 
Regards,
Niklas S�derlund

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

[Kieran Bingham]

Hi Niklas,

On 18/09/18 20:29, Niklas Söderlund wrote:
> Hi Kieran,
> 
> Thanks for your comments.
> 
> On 2018-09-18 11:13:45 +0100, Kieran Bingham wrote:
>> Hi Niklas,
>>
>> Thank you for the patch,
>>
>> On 18/09/18 02:45, 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
>>> configuring the hardware, some writing to undocumented registers.
>>> Instead of creating 3 sets of the register tables for the different
>>> modes catch when the register containing NUM_LANES[2:0] is written to
>>> and inject the correct number of lanes.
>>>
>>
>> Aye aye aye.
>>
>> Neat solution to avoid adding more tables - but is it necessary? And I
>> can't find it easy to overlook the hack value in checking every register
>> write against a specific value :-(
> 
> I agree it's not the most obvious nice solution.
> 
>>
>>
>> Couldn't we create a function called "adv748x_configure_tx_lanes(...)"
>> or such and just write this value as appropriate after the tables are
>> written?
>>
>> That will then hopefully take us a step towards not needing (as much of)
>> these tables.
> 
> This was my starting point :-) But the register is referenced multiple 
> times in the tables and converting the tables to individual register 
> writes turned out such a mess I judged this solution to be less of a 
> maintenance burden. I'm however open to your views on how you would 
> prefer this to be handled. Keep in mind that each row in the register 
> tables needs to be turned into a:
> 
>     /* Write registerx */
>     ret = adv748x_write(...)
>     if (ret)
>         return ret;

Yes, that construct for each register is certainly painful, compared to
a table...

I wonder if we can be 'clever/naughty' with macros, or perhaps just do a
best effort set of writes and catch if any fail.. (this also might be a
bit ugly)

  ret |= adv748x_write(A, a);
  ret |= adv748x_write(B, b);
  ret |= adv748x_write(C, c);
  ret |= adv748x_write(D, d);

  if (ret)
	return -EIO;


Or - we could programmatically create the tables of registers to write
in an array, and easily turn the table generation into code which we can
manipulate without checking errors after each value.

Then the whole table would get written in a single batch...

(Sort of like how we treat the display lists in the VSP1)

I'm not sure how much code that would take yet, or if it will be more or
less readable :) Needs some thought....

I wonder what Laurent's take on this is ....


> So the overview of what happens become much harder to read. Other 
> options such as creating copies of the tables and injecting the NUM_LANE 
> value at probe time I feel just hides the behavior even more.
> 
> Another option I tried was to splice the tables whenever the register in 
> question was referenced. This became hard to read but less lines of 
> code.
> 
>> However - *I fully understand ordering may be important here* so
>> actually it looks like we can't write this after writing the table.
>>
>> But it does look conveniently early in the tables, so we could split the
>> tables out and start functionalising them with the information we do know.
> 
> I have not tested if ordering is important or not, the documentation we 
> have is just a sequential list of register writes, The register is used 
> in multiple places in the tables making things even more ugly.

yeouch.

> 
> adv748x_power_up_txa_4lane: 3 times, beginning and middle
> adv748x_power_down_txa_4lane: 1 time, middle
> adv748x_init_txa_4lane: 3 times, middle and end
> 
>>
>> I.e. We could have our init function enable the lanes, and handle the
>> auto DPHY, then write the rest through the tables.
> 
> If only that where possible :-)
> 
> I hold off posting v2 until I know how you wish to handle this. To help 
> you make a decision the number of register writes in the tables involved 
> :-)
> 
> adv748x_power_up_txa_4lane: 11
> adv748x_power_down_txa_4lane: 5

I feel like the powerup and down, are good targets for converting to
functions, and merging to support both TXA and TXB (I appreciate this is
not a five minute job, but something on my wish-list)

> adv748x_init_txa_4lane: ~50

Yes, 50 writes is probably a lot more painful ...


>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>> ---
>>>  drivers/media/i2c/adv748x/adv748x-core.c | 38 +++++++++++++++++++-----
>>>  1 file changed, 30 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
>>> index a93f8ea89a228474..9a82cdf301bccb41 100644
>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
>>> @@ -207,13 +207,23 @@ static int adv748x_write_regs(struct adv748x_state *state,
>>>  			      const struct adv748x_reg_value *regs)
>>>  {
>>>  	int ret;
>>> +	u8 value;
>>>  
>>>  	while (regs->page != ADV748X_PAGE_EOR) {
>>>  		if (regs->page == ADV748X_PAGE_WAIT) {
>>>  			msleep(regs->value);
>>>  		} else {
>>> +			value = regs->value;
>>> +
>>> +			/*
>>> +			 * Register 0x00 in TXA needs to bei injected with
>>
>> s/bei/be/
>>
>>> +			 * the number of CSI-2 lanes used to transmitt.
>>
>> s/transmitt/transmit/
>>
>>> +			 */
>>> +			if (regs->page == ADV748X_PAGE_TXA && regs->reg == 0x00)
>>> +				value = (value & ~7) | state->txa.num_lanes;
>>> +
>>>  			ret = adv748x_write(state, regs->page, regs->reg,
>>> -				      regs->value);
>>> +					    value);
>>>  			if (ret < 0) {
>>>  				adv_err(state,
>>>  					"Error regs page: 0x%02x reg: 0x%02x\n",
>>> @@ -233,14 +243,18 @@ static int adv748x_write_regs(struct adv748x_state *state,
>>>  
>>>  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 */
>>> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
>>> +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
>>> +	{ADV748X_PAGE_TXA, 0x00, 0xa0},	/* 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 */
>>> +
>>> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
>>> +	{ADV748X_PAGE_TXA, 0x00, 0x20 },/* 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 */
>>> @@ -253,7 +267,10 @@ 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 */
>>> +
>>> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
>>> +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
>>
>> If we're in power down - shouldn't this be "Disable n-lane MIPI */ ??
> 
> Well the register write enables the lanes. IIRC the comments here come 
> from the register tables text files found in the "documentation".

Isn't the documentation fabulous :-)

	"Write these values, don't ask any questions..."

>>
>>> +
>>>  	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
>>>  	{ADV748X_PAGE_TXA, 0xc1, 0x3b},	/* ADI Required Write */
>>>  
>>> @@ -399,8 +416,10 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
>>>  	/* Outputs Enabled */
>>>  	{ADV748X_PAGE_IO, 0x10, 0xa0},	/* Enable 4-lane CSI Tx & Pixel Port */
>>>  
>>> -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
>>> -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
>>> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
>>> +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
>>> +	{ADV748X_PAGE_TXA, 0x00, 0xa0},	/* 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 */
>>> @@ -412,7 +431,10 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
>>>  	{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 */
>>> +
>>> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. */
>>> +	{ADV748X_PAGE_TXA, 0x00, 0x20 },/* 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 */
>>>
>>
>> -- 
>> Regards
>> --
>> Kieran
> 

-- 
Regards
--
Kieran

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

[Laurent Pinchart]

Hi Niklas,

On Tuesday, 18 September 2018 22:29:32 EEST Niklas Söderlund wrote:
> On 2018-09-18 11:13:45 +0100, Kieran Bingham wrote:
> > On 18/09/18 02:45, 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
> >> configuring the hardware, some writing to undocumented registers.
> >> Instead of creating 3 sets of the register tables for the different
> >> modes catch when the register containing NUM_LANES[2:0] is written to
> >> and inject the correct number of lanes.
> > 
> > Aye aye aye.
> > 
> > Neat solution to avoid adding more tables - but is it necessary? And I
> > can't find it easy to overlook the hack value in checking every register
> > write against a specific value :-(
> 
> I agree it's not the most obvious nice solution.
> 
> > Couldn't we create a function called "adv748x_configure_tx_lanes(...)"
> > or such and just write this value as appropriate after the tables are
> > written?
> > 
> > That will then hopefully take us a step towards not needing (as much of)
> > these tables.
> 
> This was my starting point :-) But the register is referenced multiple
> times in the tables and converting the tables to individual register
> writes turned out such a mess I judged this solution to be less of a
> maintenance burden. I'm however open to your views on how you would
> prefer this to be handled. Keep in mind that each row in the register
> tables needs to be turned into a:
> 
>     /* Write registerx */
>     ret = adv748x_write(...)
>     if (ret)
>         return ret;
> 
> So the overview of what happens become much harder to read.

Error checking may indeed make the code more difficult to read. One option 
would be, in pseudo-code, something like the following.


struct adv748x_state {
	int write_error;
};

static int adv748x_error_check(struct adv748x_state *state)
{
	int error = state->write_error;

	state->write_error = 0;
	return error;
}

static int adv748x_write(struct adv748x_state *state, ...)
{
	int ret;

	if (state->write_error)
		return write_error;

	ret = write(...);
	state->write_error = ret;
	return ret;
}

...
	adv748x_write(state, ...);
	adv748x_write(state, ...);
	adv748x_write(state, ...);
	adv748x_write(state, ...);

	ret = adv748x_error_check(state);
	if (ret)
		return ret;
...

> Other options such as creating copies of the tables and injecting the
> NUM_LANE value at probe time I feel just hides the behavior even more.
> 
> Another option I tried was to splice the tables whenever the register in
> question was referenced. This became hard to read but less lines of
> code.
> 
> > However - *I fully understand ordering may be important here* so
> > actually it looks like we can't write this after writing the table.
> > 
> > But it does look conveniently early in the tables, so we could split the
> > tables out and start functionalising them with the information we do know.
> 
> I have not tested if ordering is important or not, the documentation we
> have is just a sequential list of register writes, The register is used
> in multiple places in the tables making things even more ugly.

I would at least try reordering writes where we think it would make sense.

> adv748x_power_up_txa_4lane: 3 times, beginning and middle
> adv748x_power_down_txa_4lane: 1 time, middle
> adv748x_init_txa_4lane: 3 times, middle and end
> 
> > I.e. We could have our init function enable the lanes, and handle the
> > auto DPHY, then write the rest through the tables.
> 
> If only that where possible :-)
> 
> I hold off posting v2 until I know how you wish to handle this. To help
> you make a decision the number of register writes in the tables involved
> 
> :-)
> 
> adv748x_power_up_txa_4lane: 11
> adv748x_power_down_txa_4lane: 5
> adv748x_init_txa_4lane: ~50
> 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > > 
> > >  drivers/media/i2c/adv748x/adv748x-core.c | 38 +++++++++++++++++++-----
> > >  1 file changed, 30 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > > b/drivers/media/i2c/adv748x/adv748x-core.c index
> > > a93f8ea89a228474..9a82cdf301bccb41 100644
> > > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > > @@ -207,13 +207,23 @@ static int adv748x_write_regs(struct adv748x_state
> > > *state,> > 
> > >  			      const struct adv748x_reg_value *regs)
> > >  
> > >  {
> > >  
> > >  	int ret;
> > > 
> > > +	u8 value;
> > > 
> > >  	while (regs->page != ADV748X_PAGE_EOR) {
> > >  	
> > >  		if (regs->page == ADV748X_PAGE_WAIT) {
> > >  		
> > >  			msleep(regs->value);
> > >  		
> > >  		} else {
> > > 
> > > +			value = regs->value;
> > > +
> > > +			/*
> > > +			 * Register 0x00 in TXA needs to bei injected with
> > 
> > s/bei/be/
> > 
> > > +			 * the number of CSI-2 lanes used to transmitt.
> > 
> > s/transmitt/transmit/
> > 
> > > +			 */
> > > +			if (regs->page == ADV748X_PAGE_TXA && regs->reg == 0x00)
> > > +				value = (value & ~7) | state->txa.num_lanes;
> > > +
> > > 
> > >  			ret = adv748x_write(state, regs->page, regs->reg,
> > > 
> > > -				      regs->value);
> > > +					    value);
> > > 
> > >  			if (ret < 0) {
> > >  			
> > >  				adv_err(state,
> > >  				
> > >  					"Error regs page: 0x%02x reg: 0x%02x\n",
> > > 
> > > @@ -233,14 +243,18 @@ static int adv748x_write_regs(struct adv748x_state
> > > *state,> > 
> > >  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 */
> > > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. 
*/
> > > +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
> > > +	{ADV748X_PAGE_TXA, 0x00, 0xa0},	/* 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 */
> > > +
> > > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. 
*/
> > > +	{ADV748X_PAGE_TXA, 0x00, 0x20 },/* 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 */
> > > 
> > > @@ -253,7 +267,10 @@ 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 */
> > > +
> > > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. 
*/
> > > +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
> > 
> > If we're in power down - shouldn't this be "Disable n-lane MIPI */ ??
> 
> Well the register write enables the lanes. IIRC the comments here come
> from the register tables text files found in the "documentation".
> 
> > > +
> > > 
> > >  	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> > >  	{ADV748X_PAGE_TXA, 0xc1, 0x3b},	/* ADI Required Write */
> > > 
> > > @@ -399,8 +416,10 @@ static const struct adv748x_reg_value
> > > adv748x_init_txa_4lane[] = {> > 
> > >  	/* Outputs Enabled */
> > >  	{ADV748X_PAGE_IO, 0x10, 0xa0},	/* Enable 4-lane CSI Tx & Pixel Port 
*/
> > > 
> > > -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> > > -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> > > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. 
*/
> > > +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
> > > +	{ADV748X_PAGE_TXA, 0x00, 0xa0},	/* 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 */
> > > 
> > > @@ -412,7 +431,10 @@ static const struct adv748x_reg_value
> > > adv748x_init_txa_4lane[] = {> > 
> > >  	{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 */
> > > +
> > > +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. 
*/
> > > +	{ADV748X_PAGE_TXA, 0x00, 0x20 },/* 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 */

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Kieran,

On Tuesday, 18 September 2018 23:35:16 EEST Kieran Bingham wrote:
> On 18/09/18 20:29, Niklas Söderlund wrote:
> > On 2018-09-18 11:13:45 +0100, Kieran Bingham wrote:
> >> On 18/09/18 02:45, 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
> >>> configuring the hardware, some writing to undocumented registers.
> >>> Instead of creating 3 sets of the register tables for the different
> >>> modes catch when the register containing NUM_LANES[2:0] is written to
> >>> and inject the correct number of lanes.
> >> 
> >> Aye aye aye.
> >> 
> >> Neat solution to avoid adding more tables - but is it necessary? And I
> >> can't find it easy to overlook the hack value in checking every register
> >> write against a specific value :-(
> > 
> > I agree it's not the most obvious nice solution.
> > 
> >> Couldn't we create a function called "adv748x_configure_tx_lanes(...)"
> >> or such and just write this value as appropriate after the tables are
> >> written?
> >> 
> >> That will then hopefully take us a step towards not needing (as much of)
> >> these tables.
> > 
> > This was my starting point :-) But the register is referenced multiple
> > times in the tables and converting the tables to individual register
> > writes turned out such a mess I judged this solution to be less of a
> > maintenance burden. I'm however open to your views on how you would
> > prefer this to be handled. Keep in mind that each row in the register
> > tables needs to be turned into a:
> > 
> >     /* Write registerx */
> >     ret = adv748x_write(...)
> >     if (ret)
> >     
> >         return ret;
> 
> Yes, that construct for each register is certainly painful, compared to
> a table...
> 
> I wonder if we can be 'clever/naughty' with macros, or perhaps just do a
> best effort set of writes and catch if any fail.. (this also might be a
> bit ugly)
> 
>   ret |= adv748x_write(A, a);
>   ret |= adv748x_write(B, b);
>   ret |= adv748x_write(C, c);
>   ret |= adv748x_write(D, d);
>   if (ret)
> 	return -EIO;

I'm not very fond of such constructs as it can hide the original error code. I 
proposed an alternative in this mail thread. If that ends up adding too much 
overhead the above construct could be considered.

> Or - we could programmatically create the tables of registers to write
> in an array, and easily turn the table generation into code which we can
> manipulate without checking errors after each value.
> 
> Then the whole table would get written in a single batch...
> 
> (Sort of like how we treat the display lists in the VSP1)

We could do that, but my gut feeling is that it would be too complex and over-
engineered.

> I'm not sure how much code that would take yet, or if it will be more or
> less readable :) Needs some thought....
> 
> I wonder what Laurent's take on this is ....

Hopefully you're not wondering anymore :-)

> > So the overview of what happens become much harder to read. Other
> > options such as creating copies of the tables and injecting the NUM_LANE
> > value at probe time I feel just hides the behavior even more.
> > 
> > Another option I tried was to splice the tables whenever the register in
> > question was referenced. This became hard to read but less lines of
> > code.
> > 
> >> However - *I fully understand ordering may be important here* so
> >> actually it looks like we can't write this after writing the table.
> >> 
> >> But it does look conveniently early in the tables, so we could split the
> >> tables out and start functionalising them with the information we do
> >> know.
> > 
> > I have not tested if ordering is important or not, the documentation we
> > have is just a sequential list of register writes, The register is used
> > in multiple places in the tables making things even more ugly.
> 
> yeouch.
> 
> > adv748x_power_up_txa_4lane: 3 times, beginning and middle
> > adv748x_power_down_txa_4lane: 1 time, middle
> > adv748x_init_txa_4lane: 3 times, middle and end
> > 
> >> I.e. We could have our init function enable the lanes, and handle the
> >> auto DPHY, then write the rest through the tables.
> > 
> > If only that where possible :-)
> > 
> > I hold off posting v2 until I know how you wish to handle this. To help
> > you make a decision the number of register writes in the tables involved
> > 
> > :-)
> > 
> > adv748x_power_up_txa_4lane: 11
> > adv748x_power_down_txa_4lane: 5
> 
> I feel like the powerup and down, are good targets for converting to
> functions, and merging to support both TXA and TXB (I appreciate this is
> not a five minute job, but something on my wish-list)
> 
> > adv748x_init_txa_4lane: ~50
> 
> Yes, 50 writes is probably a lot more painful ...
> 
> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>> ---
> >>> 
> >>>  drivers/media/i2c/adv748x/adv748x-core.c | 38 +++++++++++++++++++-----
> >>>  1 file changed, 30 insertions(+), 8 deletions(-)
> >>> 
> >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> b/drivers/media/i2c/adv748x/adv748x-core.c index
> >>> a93f8ea89a228474..9a82cdf301bccb41 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> >>> @@ -207,13 +207,23 @@ static int adv748x_write_regs(struct adv748x_state
> >>> *state,>>> 
> >>>  			      const struct adv748x_reg_value *regs)
> >>>  
> >>>  {
> >>>  
> >>>  	int ret;
> >>> 
> >>> +	u8 value;
> >>> 
> >>>  	while (regs->page != ADV748X_PAGE_EOR) {
> >>>  	
> >>>  		if (regs->page == ADV748X_PAGE_WAIT) {
> >>>  		
> >>>  			msleep(regs->value);
> >>>  		
> >>>  		} else {
> >>> 
> >>> +			value = regs->value;
> >>> +
> >>> +			/*
> >>> +			 * Register 0x00 in TXA needs to bei injected with
> >> 
> >> s/bei/be/
> >> 
> >>> +			 * the number of CSI-2 lanes used to transmitt.
> >> 
> >> s/transmitt/transmit/
> >> 
> >>> +			 */
> >>> +			if (regs->page == ADV748X_PAGE_TXA && regs->reg == 0x00)
> >>> +				value = (value & ~7) | state->txa.num_lanes;
> >>> +
> >>> 
> >>>  			ret = adv748x_write(state, regs->page, regs->reg,
> >>> 
> >>> -				      regs->value);
> >>> +					    value);
> >>> 
> >>>  			if (ret < 0) {
> >>>  			
> >>>  				adv_err(state,
> >>>  				
> >>>  					"Error regs page: 0x%02x reg: 0x%02x\n",
> >>> 
> >>> @@ -233,14 +243,18 @@ static int adv748x_write_regs(struct adv748x_state
> >>> *state,>>> 
> >>>  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 */
> >>> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. 
*/
> >>> +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
> >>> +	{ADV748X_PAGE_TXA, 0x00, 0xa0},	/* 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 */
> >>> +
> >>> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. 
*/
> >>> +	{ADV748X_PAGE_TXA, 0x00, 0x20 },/* 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 */
> >>> 
> >>> @@ -253,7 +267,10 @@ 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 */
> >>> +
> >>> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. 
*/
> >>> +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
> >> 
> >> If we're in power down - shouldn't this be "Disable n-lane MIPI */ ??
> > 
> > Well the register write enables the lanes. IIRC the comments here come
> > from the register tables text files found in the "documentation".
> 
> Isn't the documentation fabulous :-)
> 
> 	"Write these values, don't ask any questions..."
> 
> >>> +
> >>> 
> >>>  	{ADV748X_PAGE_TXA, 0xda, 0x01},	/* i2c_mipi_pll_en - 1'b1 */
> >>>  	{ADV748X_PAGE_TXA, 0xc1, 0x3b},	/* ADI Required Write */
> >>> 
> >>> @@ -399,8 +416,10 @@ static const struct adv748x_reg_value
> >>> adv748x_init_txa_4lane[] = {>>> 
> >>>  	/* Outputs Enabled */
> >>>  	{ADV748X_PAGE_IO, 0x10, 0xa0},	/* Enable 4-lane CSI Tx & Pixel Port 
*/
> >>> 
> >>> -	{ADV748X_PAGE_TXA, 0x00, 0x84},	/* Enable 4-lane MIPI */
> >>> -	{ADV748X_PAGE_TXA, 0x00, 0xa4},	/* Set Auto DPHY Timing */
> >>> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. 
*/
> >>> +	{ADV748X_PAGE_TXA, 0x00, 0x80},	/* Enable n-lane MIPI */
> >>> +	{ADV748X_PAGE_TXA, 0x00, 0xa0},	/* 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 */
> >>> 
> >>> @@ -412,7 +431,10 @@ static const struct adv748x_reg_value
> >>> adv748x_init_txa_4lane[] = {>>> 
> >>>  	{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 */
> >>> +
> >>> +	/* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write. 
*/
> >>> +	{ADV748X_PAGE_TXA, 0x00, 0x20 },/* 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 */


-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent, Kieran,

On Tue, Sep 18, 2018 at 02:13:29PM +0300, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Tuesday, 18 September 2018 13:51:34 EEST Kieran Bingham wrote:
> > On 18/09/18 11:46, Laurent Pinchart wrote:
> > > On Tuesday, 18 September 2018 13:37:55 EEST Kieran Bingham wrote:
> > >> On 18/09/18 11:28, Laurent Pinchart wrote:
> > >>> On Tuesday, 18 September 2018 13:19:39 EEST Kieran Bingham wrote:
> > >>>> On 18/09/18 02:45, Niklas Söderlund wrote:
> > >>>>> The adv748x CSI-2 transmitters TXA and TXB can use different number of
> > >>>>> lines to transmit data on. In order to be able 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.
> > >>>> 
> > >>>> Am I right in assuming that it is the CSI device which specifies the
> > >>>> number of lanes in their DT?
> > >>> 
> > >>> Do you mean the CSI-2 receiver ? Both the receiver and the transmitter
> > >>> should specify the data lanes in their DT node.
> > >> 
> > >> Yes, I should have said CSI-2 receiver.
> > >> 
> > >> Aha - so *both* sides of the link have to specify the lanes and
> > >> presumably match with each other?
> > > 
> > > Yes, they should certainly match :-)
> > 
> > I assumed so :) - do we need to validate that at a framework level?
> > (or perhaps it already is, all I've only looked at this morning is
> > e-mails :D )
> 
> It's not done yet as far as I know. CC'ing Sakari who may have a comment 
> regarding whether this should be added.

There's no validation done currently. The endpoints are parsed separately
at the moment, initiated by the respective device driver.

The latest fwnode set brings a concept of default configuration that also
allows support for setting the default for the number of lanes. This is
known by the driver, but could not be known by the framework checking the
configuration across the endpoints. I guess this could be done at the time
both endpoints have been parsed, as in stored to the async sub-device.

Some checks could indeed be done, but what to do when those checks fail?

That said, I don't think this has really been an issue so far --- if you
get this wrong the devices just won't work. The last fwnode set greatly
improves the quality of the debug information printed, so debugging should
be easier when things go wrong already. And this is already more than we've
had so far.

> 
> > >>>> Could we make this clear in the commit log (and possibly an extra
> > >>>> comment in the code). At first I was assuming we would have to declare
> > >>>> the number of lanes in the ADV748x TX DT node, but I don't think that's
> > >>>> the case.
> > >>>> 
> > >>>>> Signed-off-by: Niklas Söderlund
> > >>>>> <niklas.soderlund+renesas@ragnatech.se>
> > >>>>> ---
> > >>>>> 
> > >>>>>  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
> > >>>>> 85c027bdcd56748d..a93f8ea89a228474 100644
> > >>>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > >>>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > > 
> > > [snip]
> > > 
> > >>>>> +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 I'm not mistaken we are parsing /someone elses/ DT node here (the
> > >>>> CSI receiver or such).
> > >>> 
> > >>> Aren't we parsing our own endpoint ? The ep argument comes from ep_np in
> > >>> adv748x_parse_dt(), and that's the endpoint iterator used with
> > >>> 
> > >>> 	for_each_endpoint_of_node(state->dev->of_node, ep_np)
> > >> 
> > >> Bah - my head was polluted with the async subdevice stuff where we were
> > >> getting the endpoint of the other device, but of course that's
> > >> completely unrelated here.
> > >> 
> > >>>> Is it now guaranteed on the mipi_csi2 bus to have the (correct) lanes
> > >>>> defined?
> > >>>> 
> > >>>> Do we need to fall back to some safe defaults at all (1 lane?) ?
> > >>>> Actually - perhaps there is no safe default. I guess if the lanes
> > >>>> aren't configured correctly we're not going to get a good signal at the
> > >>>> other end.
> > >>> 
> > >>> The endpoints should contain a data-lanes property. That's the case in
> > >>> the mainline DT sources, but it's not explicitly stated as a mandatory
> > >>> property. I think we should update the bindings.

Many devices have just a single lane. Do you think this should be mandatory
in that case as well?

Lane mapping is not a very common feature nowadays; to be fair, I don't
know other hardware than omap3isp that supports it. The numbers are still
needed as many devices nowadays support choosing how the lanes are
distributed across the PHYs.

> > >> 
> > >> Yes, - as this code change is making the property mandatory - we should
> > >> certainly state that in the bindings, unless we can fall back to a
> > >> sensible default (perhaps the max supported on that component?)
> > > 
> > > I'm not sure there's a sensible default, I'd rather specify it explicitly.
> > > Note that the data-lanes property doesn't just specify the number of
> > > lanes, but also how they are remapped, when that feature is supported by
> > > the CSI-2 transmitter or receiver.
> > 
> > Ok understood. As I feared - we can't really default - because it has to
> > match and be defined.
> > 
> > So making the DT property mandatory really is the way to go then.

I certainly have no objections to making this mandatory for some devices as
long as it makes sense --- and for devices with just a single data lane
without remapping with the clock lane it does not.

In that case, the driver would just set the number of lanes in the default
configuration to zero, and check the configuration it gets back is valid
--- as usual.

For what it's worth, quite a few parallel interface devices explicitly
state default configurations in their DT bindings. I admit the data-lanes
property is not a great candidate for setting a default for (if a device
has more than a single data lane).

> > 
> > >>>>> +	if (vep.base.port == ADV748X_PORT_TXA) {
> > >>>>> +		if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
> > >>>>> +			adv_err(state, "TXA: Invalid number (%d) of lanes\n",
> > >>>>> +				num_lanes);
> > >>>>> +			return -EINVAL;
> > >>>>> +		}
> > >>>>> +
> > >>>>> +		state->txa.num_lanes = num_lanes;
> > >>>>> +		adv_dbg(state, "TXA: using %d lanes\n", state->txa.num_lanes);
> > >>>>> +	}
> > >>>>> +
> > >>>>> +	if (vep.base.port == ADV748X_PORT_TXB) {
> > >>>>> +		if (num_lanes != 1) {
> > >>>>> +			adv_err(state, "TXB: Invalid number (%d) of lanes\n",
> > >>>>> +				num_lanes);
> > >>>>> +			return -EINVAL;
> > >>>>> +		}
> > >>>>> +
> > >>>>> +		state->txb.num_lanes = num_lanes;
> > >>>>> +		adv_dbg(state, "TXB: using %d lanes\n", state->txb.num_lanes);
> > >>>>> +	}
> > >>>>> +
> > >>>>> +	return 0;
> > >>>>> +}
> > > 
> > > [snip]
> 

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

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

[Sakari Ailus]

Hi Laurent, Kieran,

On Tue, Sep 18, 2018 at 02:13:29PM +0300, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Tuesday, 18 September 2018 13:51:34 EEST Kieran Bingham wrote:
> > On 18/09/18 11:46, Laurent Pinchart wrote:
> > > On Tuesday, 18 September 2018 13:37:55 EEST Kieran Bingham wrote:
> > >> On 18/09/18 11:28, Laurent Pinchart wrote:
> > >>> On Tuesday, 18 September 2018 13:19:39 EEST Kieran Bingham wrote:
> > >>>> On 18/09/18 02:45, Niklas S�derlund wrote:
> > >>>>> The adv748x CSI-2 transmitters TXA and TXB can use different number of
> > >>>>> lines to transmit data on. In order to be able 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.
> > >>>> 
> > >>>> Am I right in assuming that it is the CSI device which specifies the
> > >>>> number of lanes in their DT?
> > >>> 
> > >>> Do you mean the CSI-2 receiver ? Both the receiver and the transmitter
> > >>> should specify the data lanes in their DT node.
> > >> 
> > >> Yes, I should have said CSI-2 receiver.
> > >> 
> > >> Aha - so *both* sides of the link have to specify the lanes and
> > >> presumably match with each other?
> > > 
> > > Yes, they should certainly match :-)
> > 
> > I assumed so :) - do we need to validate that at a framework level?
> > (or perhaps it already is, all I've only looked at this morning is
> > e-mails :D )
> 
> It's not done yet as far as I know. CC'ing Sakari who may have a comment 
> regarding whether this should be added.

There's no validation done currently. The endpoints are parsed separately
at the moment, initiated by the respective device driver.

The latest fwnode set brings a concept of default configuration that also
allows support for setting the default for the number of lanes. This is
known by the driver, but could not be known by the framework checking the
configuration across the endpoints. I guess this could be done at the time
both endpoints have been parsed, as in stored to the async sub-device.

Some checks could indeed be done, but what to do when those checks fail?

That said, I don't think this has really been an issue so far --- if you
get this wrong the devices just won't work. The last fwnode set greatly
improves the quality of the debug information printed, so debugging should
be easier when things go wrong already. And this is already more than we've
had so far.

> 
> > >>>> Could we make this clear in the commit log (and possibly an extra
> > >>>> comment in the code). At first I was assuming we would have to declare
> > >>>> the number of lanes in the ADV748x TX DT node, but I don't think that's
> > >>>> the case.
> > >>>> 
> > >>>>> Signed-off-by: Niklas S�derlund
> > >>>>> <niklas.soderlund+renesas@ragnatech.se>
> > >>>>> ---
> > >>>>> 
> > >>>>>  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
> > >>>>> 85c027bdcd56748d..a93f8ea89a228474 100644
> > >>>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > >>>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > > 
> > > [snip]
> > > 
> > >>>>> +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 I'm not mistaken we are parsing /someone elses/ DT node here (the
> > >>>> CSI receiver or such).
> > >>> 
> > >>> Aren't we parsing our own endpoint ? The ep argument comes from ep_np in
> > >>> adv748x_parse_dt(), and that's the endpoint iterator used with
> > >>> 
> > >>> 	for_each_endpoint_of_node(state->dev->of_node, ep_np)
> > >> 
> > >> Bah - my head was polluted with the async subdevice stuff where we were
> > >> getting the endpoint of the other device, but of course that's
> > >> completely unrelated here.
> > >> 
> > >>>> Is it now guaranteed on the mipi_csi2 bus to have the (correct) lanes
> > >>>> defined?
> > >>>> 
> > >>>> Do we need to fall back to some safe defaults at all (1 lane?) ?
> > >>>> Actually - perhaps there is no safe default. I guess if the lanes
> > >>>> aren't configured correctly we're not going to get a good signal at the
> > >>>> other end.
> > >>> 
> > >>> The endpoints should contain a data-lanes property. That's the case in
> > >>> the mainline DT sources, but it's not explicitly stated as a mandatory
> > >>> property. I think we should update the bindings.

Many devices have just a single lane. Do you think this should be mandatory
in that case as well?

Lane mapping is not a very common feature nowadays; to be fair, I don't
know other hardware than omap3isp that supports it. The numbers are still
needed as many devices nowadays support choosing how the lanes are
distributed across the PHYs.

> > >> 
> > >> Yes, - as this code change is making the property mandatory - we should
> > >> certainly state that in the bindings, unless we can fall back to a
> > >> sensible default (perhaps the max supported on that component?)
> > > 
> > > I'm not sure there's a sensible default, I'd rather specify it explicitly.
> > > Note that the data-lanes property doesn't just specify the number of
> > > lanes, but also how they are remapped, when that feature is supported by
> > > the CSI-2 transmitter or receiver.
> > 
> > Ok understood. As I feared - we can't really default - because it has to
> > match and be defined.
> > 
> > So making the DT property mandatory really is the way to go then.

I certainly have no objections to making this mandatory for some devices as
long as it makes sense --- and for devices with just a single data lane
without remapping with the clock lane it does not.

In that case, the driver would just set the number of lanes in the default
configuration to zero, and check the configuration it gets back is valid
--- as usual.

For what it's worth, quite a few parallel interface devices explicitly
state default configurations in their DT bindings. I admit the data-lanes
property is not a great candidate for setting a default for (if a device
has more than a single data lane).

> > 
> > >>>>> +	if (vep.base.port == ADV748X_PORT_TXA) {
> > >>>>> +		if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
> > >>>>> +			adv_err(state, "TXA: Invalid number (%d) of lanes\n",
> > >>>>> +				num_lanes);
> > >>>>> +			return -EINVAL;
> > >>>>> +		}
> > >>>>> +
> > >>>>> +		state->txa.num_lanes = num_lanes;
> > >>>>> +		adv_dbg(state, "TXA: using %d lanes\n", state->txa.num_lanes);
> > >>>>> +	}
> > >>>>> +
> > >>>>> +	if (vep.base.port == ADV748X_PORT_TXB) {
> > >>>>> +		if (num_lanes != 1) {
> > >>>>> +			adv_err(state, "TXB: Invalid number (%d) of lanes\n",
> > >>>>> +				num_lanes);
> > >>>>> +			return -EINVAL;
> > >>>>> +		}
> > >>>>> +
> > >>>>> +		state->txb.num_lanes = num_lanes;
> > >>>>> +		adv_dbg(state, "TXB: using %d lanes\n", state->txb.num_lanes);
> > >>>>> +	}
> > >>>>> +
> > >>>>> +	return 0;
> > >>>>> +}
> > > 
> > > [snip]
> 

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

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

[Dave Stevenson]

Hi All,

On Tue, 18 Sep 2018 at 12:13, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Kieran,
>
> On Tuesday, 18 September 2018 13:51:34 EEST Kieran Bingham wrote:
> > On 18/09/18 11:46, Laurent Pinchart wrote:
> > > On Tuesday, 18 September 2018 13:37:55 EEST Kieran Bingham wrote:
> > >> On 18/09/18 11:28, Laurent Pinchart wrote:
> > >>> On Tuesday, 18 September 2018 13:19:39 EEST Kieran Bingham wrote:
> > >>>> On 18/09/18 02:45, Niklas Söderlund wrote:
> > >>>>> The adv748x CSI-2 transmitters TXA and TXB can use different number of
> > >>>>> lines to transmit data on. In order to be able 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.
> > >>>>
> > >>>> Am I right in assuming that it is the CSI device which specifies the
> > >>>> number of lanes in their DT?
> > >>>
> > >>> Do you mean the CSI-2 receiver ? Both the receiver and the transmitter
> > >>> should specify the data lanes in their DT node.
> > >>
> > >> Yes, I should have said CSI-2 receiver.
> > >>
> > >> Aha - so *both* sides of the link have to specify the lanes and
> > >> presumably match with each other?
> > >
> > > Yes, they should certainly match :-)
> >
> > I assumed so :) - do we need to validate that at a framework level?
> > (or perhaps it already is, all I've only looked at this morning is
> > e-mails :D )
>
> It's not done yet as far as I know. CC'ing Sakari who may have a comment
> regarding whether this should be added.

(Interested party here due to CSI2 interfacing on the Pi, and I'd like
to try and get adv748x working on the Pi once I can get some hardware)

Do they need to match? DT is supposedly describing the hardware. Where
are you drawing the boundary between the two devices from the
hardware/devicetree perspective?

As an example, the CSI2 receiver can support 4 lanes, whilst the
source only needs 2, or only has 2 connected. As long as the two ends
agree on the value in use (which will typically match the source),
then there is no issue.
It does require the CSI2 receiver driver to validate the remote
endpoint settings to ensure that the source doesn't try to use more
lanes than the receiver can cope with. That isn't a big deal as you
already have the link to the remote end-point. Presumably you're
already checking it to determine settings such as if the source is
using continuous clocks or not, unless you expect that to be
duplicated on both endpoints or your hardware doesn't care.

I'm genuinely interested on views here. On the Pi there will be a
variety of CSI2 devices connected, generally configured using
dtoverlays, and they will have varying requirements on number of
lanes. Standard Pis only have 2 CSI-2 lanes exposed out of a possible
4 for the peripheral. The Compute Module is the exception where one
CSI2 interface has all 4 lanes brought out, the other only supports 2
lanes anyway.
I'm expecting the CSI2 receiver endpoint data-lanes property to match
that exposed by the Pi board, whilst the source endpoint data-lanes
property defines what the source uses. That allows easy validation by
the driver that the configuration can work. Otherwise an overlay would
have to write the number of lanes used on both the CSI endpoints and
potentially configuring it to use more lanes than can be supported.


There is also the oddball one of the TC358743 which dynamically
switches the number of lanes in use based on the data rate required.
That's probably a separate discussion, but is currently dealt with via
g_mbus_config as amended back in Sept 2017 [1].

Cheers,
  Dave

[1] Discussion https://www.spinics.net/lists/linux-media/msg122287.html
and patch https://www.spinics.net/lists/linux-media/msg122435.html

> > >>>> Could we make this clear in the commit log (and possibly an extra
> > >>>> comment in the code). At first I was assuming we would have to declare
> > >>>> the number of lanes in the ADV748x TX DT node, but I don't think that's
> > >>>> the case.
> > >>>>
> > >>>>> Signed-off-by: Niklas Söderlund
> > >>>>> <niklas.soderlund+renesas@ragnatech.se>
> > >>>>> ---
> > >>>>>
> > >>>>>  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
> > >>>>> 85c027bdcd56748d..a93f8ea89a228474 100644
> > >>>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > >>>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > >
> > > [snip]
> > >
> > >>>>> +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 I'm not mistaken we are parsing /someone elses/ DT node here (the
> > >>>> CSI receiver or such).
> > >>>
> > >>> Aren't we parsing our own endpoint ? The ep argument comes from ep_np in
> > >>> adv748x_parse_dt(), and that's the endpoint iterator used with
> > >>>
> > >>>   for_each_endpoint_of_node(state->dev->of_node, ep_np)
> > >>
> > >> Bah - my head was polluted with the async subdevice stuff where we were
> > >> getting the endpoint of the other device, but of course that's
> > >> completely unrelated here.
> > >>
> > >>>> Is it now guaranteed on the mipi_csi2 bus to have the (correct) lanes
> > >>>> defined?
> > >>>>
> > >>>> Do we need to fall back to some safe defaults at all (1 lane?) ?
> > >>>> Actually - perhaps there is no safe default. I guess if the lanes
> > >>>> aren't configured correctly we're not going to get a good signal at the
> > >>>> other end.
> > >>>
> > >>> The endpoints should contain a data-lanes property. That's the case in
> > >>> the mainline DT sources, but it's not explicitly stated as a mandatory
> > >>> property. I think we should update the bindings.
> > >>
> > >> Yes, - as this code change is making the property mandatory - we should
> > >> certainly state that in the bindings, unless we can fall back to a
> > >> sensible default (perhaps the max supported on that component?)
> > >
> > > I'm not sure there's a sensible default, I'd rather specify it explicitly.
> > > Note that the data-lanes property doesn't just specify the number of
> > > lanes, but also how they are remapped, when that feature is supported by
> > > the CSI-2 transmitter or receiver.
> >
> > Ok understood. As I feared - we can't really default - because it has to
> > match and be defined.
> >
> > So making the DT property mandatory really is the way to go then.
> >
> > >>>>> +       if (vep.base.port == ADV748X_PORT_TXA) {
> > >>>>> +               if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
> > >>>>> +                       adv_err(state, "TXA: Invalid number (%d) of lanes\n",
> > >>>>> +                               num_lanes);
> > >>>>> +                       return -EINVAL;
> > >>>>> +               }
> > >>>>> +
> > >>>>> +               state->txa.num_lanes = num_lanes;
> > >>>>> +               adv_dbg(state, "TXA: using %d lanes\n", state->txa.num_lanes);
> > >>>>> +       }
> > >>>>> +
> > >>>>> +       if (vep.base.port == ADV748X_PORT_TXB) {
> > >>>>> +               if (num_lanes != 1) {
> > >>>>> +                       adv_err(state, "TXB: Invalid number (%d) of lanes\n",
> > >>>>> +                               num_lanes);
> > >>>>> +                       return -EINVAL;
> > >>>>> +               }
> > >>>>> +
> > >>>>> +               state->txb.num_lanes = num_lanes;
> > >>>>> +               adv_dbg(state, "TXB: using %d lanes\n", state->txb.num_lanes);
> > >>>>> +       }
> > >>>>> +
> > >>>>> +       return 0;
> > >>>>> +}
> > >
> > > [snip]
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

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

[Laurent Pinchart]

Hi Dave,

On Friday, 21 September 2018 12:33:39 EEST Dave Stevenson wrote:
> On Tue, 18 Sep 2018 at 12:13, Laurent Pinchart wrote:
> > On Tuesday, 18 September 2018 13:51:34 EEST Kieran Bingham wrote:
> >> On 18/09/18 11:46, Laurent Pinchart wrote:
> >>> On Tuesday, 18 September 2018 13:37:55 EEST Kieran Bingham wrote:
> >>>> On 18/09/18 11:28, Laurent Pinchart wrote:
> >>>>> On Tuesday, 18 September 2018 13:19:39 EEST Kieran Bingham wrote:
> >>>>>> On 18/09/18 02:45, Niklas Söderlund wrote:
> >>>>>>> The adv748x CSI-2 transmitters TXA and TXB can use different
> >>>>>>> number of lines to transmit data on. In order to be able 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.
> >>>>>> 
> >>>>>> Am I right in assuming that it is the CSI device which specifies
> >>>>>> the number of lanes in their DT?
> >>>>> 
> >>>>> Do you mean the CSI-2 receiver ? Both the receiver and the
> >>>>> transmitter should specify the data lanes in their DT node.
> >>>> 
> >>>> Yes, I should have said CSI-2 receiver.
> >>>> 
> >>>> Aha - so *both* sides of the link have to specify the lanes and
> >>>> presumably match with each other?
> >>> 
> >>> Yes, they should certainly match :-)
> >> 
> >> I assumed so :) - do we need to validate that at a framework level?
> >> (or perhaps it already is, all I've only looked at this morning is
> >> e-mails :D )
> > 
> > It's not done yet as far as I know. CC'ing Sakari who may have a comment
> > regarding whether this should be added.
> 
> (Interested party here due to CSI2 interfacing on the Pi, and I'd like
> to try and get adv748x working on the Pi once I can get some hardware)
> 
> Do they need to match? DT is supposedly describing the hardware. Where
> are you drawing the boundary between the two devices from the
> hardware/devicetree perspective?
> 
> As an example, the CSI2 receiver can support 4 lanes, whilst the
> source only needs 2, or only has 2 connected. As long as the two ends
> agree on the value in use (which will typically match the source),
> then there is no issue.
> It does require the CSI2 receiver driver to validate the remote
> endpoint settings to ensure that the source doesn't try to use more
> lanes than the receiver can cope with. That isn't a big deal as you
> already have the link to the remote end-point. Presumably you're
> already checking it to determine settings such as if the source is
> using continuous clocks or not, unless you expect that to be
> duplicated on both endpoints or your hardware doesn't care.

At least for the data-lanes property, the value is meant to describe how lanes 
are physically connected on the board from the transmitter to the receiver. It 
does not tell the maximum of lanes that the device supports, which is 
intrinsic information known to the driver already.

Regarding other parameters, such as the clock mode you mentioned, they should 
usually be queried and negotiated at runtime, especially given that both the 
transmitting and receiving devices may very well support multiple options. We 
usually don't involve DT in those cases. Parsing DT properties of a remote 
node is frowned upon, because those properties are qualified by the compatible 
string of the node they sit in. A driver matching "foo,abc" knows what 
properties are defined for the corresponding DT node, but when that driver 
crosses links to DT node "bar,xyz", it has no a priori knowledge of what to 
expect there.

Following the OF graph through endpoints and ports is still fine, as if a DT 
node uses the OF graph bindings, the nodes it is connected to are assumed to 
use the same bindings. No assumption can usually be made on other properties 
though, including the ones describing the bus configuration. For that reason 
the properties for the CSI-2 source should be parsed by the CSI-2 source 
driver, and the configuration of the source should be queried by the CSI-2 
receiver at runtime using the v4l2_subdev API (which is routinely extended 
with additional configuration information when the need arises).

> I'm genuinely interested on views here. On the Pi there will be a
> variety of CSI2 devices connected, generally configured using
> dtoverlays, and they will have varying requirements on number of
> lanes. Standard Pis only have 2 CSI-2 lanes exposed out of a possible
> 4 for the peripheral. The Compute Module is the exception where one
> CSI2 interface has all 4 lanes brought out, the other only supports 2
> lanes anyway.
> I'm expecting the CSI2 receiver endpoint data-lanes property to match
> that exposed by the Pi board, whilst the source endpoint data-lanes
> property defines what the source uses. That allows easy validation by
> the driver that the configuration can work. Otherwise an overlay would
> have to write the number of lanes used on both the CSI endpoints and
> potentially configuring it to use more lanes than can be supported.

As explained above, we currently expect the latter, with the overlay modifying 
the data-lanes property of the receiver as well. This is partly due to the 
fact that receivers can support data lanes remapping, so the data-lanes 
property of the receiver needs not only to specify the number of lanes, but 
also how they are connected.

If you want to validate the data-lanes property to ensure the overlay doesn't 
try to use more lanes than available, you can do so either against a hardcoded 
value in the receiver driver (when the maximum is fixed for a given compatible 
string), or against a value read from DT (when the maximum depends on the 
board).

> There is also the oddball one of the TC358743 which dynamically
> switches the number of lanes in use based on the data rate required.
> That's probably a separate discussion, but is currently dealt with via
> g_mbus_config as amended back in Sept 2017 [1].

This falls into the case of dynamic configuration discovery and negotiation I 
mentioned above, and we clearly need to make sure the v4l2_subdev API supports 
this use case.

> [1] Discussion https://www.spinics.net/lists/linux-media/msg122287.html
> and patch https://www.spinics.net/lists/linux-media/msg122435.html
> 
> >>>>>> Could we make this clear in the commit log (and possibly an extra
> >>>>>> comment in the code). At first I was assuming we would have to
> >>>>>> declare the number of lanes in the ADV748x TX DT node, but I don't
> >>>>>> think that's the case.
> >>>>>> 
> >>>>>>> Signed-off-by: Niklas Söderlund
> >>>>>>> <niklas.soderlund+renesas@ragnatech.se>
> >>>>>>> ---
> >>>>>>> 
> >>>>>>>  drivers/media/i2c/adv748x/adv748x-core.c | 49 +++++++++++++++++++++
> >>>>>>>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
> >>>>>>>  2 files changed, 50 insertions(+)

[snip]

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

On Fri, Sep 21, 2018 at 01:01:09PM +0300, Laurent Pinchart wrote:
...
> > There is also the oddball one of the TC358743 which dynamically
> > switches the number of lanes in use based on the data rate required.
> > That's probably a separate discussion, but is currently dealt with via
> > g_mbus_config as amended back in Sept 2017 [1].
> 
> This falls into the case of dynamic configuration discovery and negotiation I 
> mentioned above, and we clearly need to make sure the v4l2_subdev API supports 
> this use case.

This could be added to struct v4l2_mbus_frame_desc; Niklas has driver that
uses the framework support here, so this would likely end up merged soon:

<URL:https://git.linuxtv.org/sailus/media_tree.git/tree/include/media/v4l2-subdev.h?h=vc&id=0cbd2b25b37ef5b2e6a14340dbca6d2d2d5af98e>

The CSI-2 bus parameters are missing there currently but nothing prevents
adding them. The semantics of set_frame_desc() needs to be probably defined
better than it currently is.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

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

[Dave Stevenson]

Hi Laurent,

Thanks for the response.

On Fri, 21 Sep 2018 at 11:00, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dave,
>
> On Friday, 21 September 2018 12:33:39 EEST Dave Stevenson wrote:
> > On Tue, 18 Sep 2018 at 12:13, Laurent Pinchart wrote:
> > > On Tuesday, 18 September 2018 13:51:34 EEST Kieran Bingham wrote:
> > >> On 18/09/18 11:46, Laurent Pinchart wrote:
> > >>> On Tuesday, 18 September 2018 13:37:55 EEST Kieran Bingham wrote:
> > >>>> On 18/09/18 11:28, Laurent Pinchart wrote:
> > >>>>> On Tuesday, 18 September 2018 13:19:39 EEST Kieran Bingham wrote:
> > >>>>>> On 18/09/18 02:45, Niklas Söderlund wrote:
> > >>>>>>> The adv748x CSI-2 transmitters TXA and TXB can use different
> > >>>>>>> number of lines to transmit data on. In order to be able 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.
> > >>>>>>
> > >>>>>> Am I right in assuming that it is the CSI device which specifies
> > >>>>>> the number of lanes in their DT?
> > >>>>>
> > >>>>> Do you mean the CSI-2 receiver ? Both the receiver and the
> > >>>>> transmitter should specify the data lanes in their DT node.
> > >>>>
> > >>>> Yes, I should have said CSI-2 receiver.
> > >>>>
> > >>>> Aha - so *both* sides of the link have to specify the lanes and
> > >>>> presumably match with each other?
> > >>>
> > >>> Yes, they should certainly match :-)
> > >>
> > >> I assumed so :) - do we need to validate that at a framework level?
> > >> (or perhaps it already is, all I've only looked at this morning is
> > >> e-mails :D )
> > >
> > > It's not done yet as far as I know. CC'ing Sakari who may have a comment
> > > regarding whether this should be added.
> >
> > (Interested party here due to CSI2 interfacing on the Pi, and I'd like
> > to try and get adv748x working on the Pi once I can get some hardware)
> >
> > Do they need to match? DT is supposedly describing the hardware. Where
> > are you drawing the boundary between the two devices from the
> > hardware/devicetree perspective?
> >
> > As an example, the CSI2 receiver can support 4 lanes, whilst the
> > source only needs 2, or only has 2 connected. As long as the two ends
> > agree on the value in use (which will typically match the source),
> > then there is no issue.
> > It does require the CSI2 receiver driver to validate the remote
> > endpoint settings to ensure that the source doesn't try to use more
> > lanes than the receiver can cope with. That isn't a big deal as you
> > already have the link to the remote end-point. Presumably you're
> > already checking it to determine settings such as if the source is
> > using continuous clocks or not, unless you expect that to be
> > duplicated on both endpoints or your hardware doesn't care.
>
> At least for the data-lanes property, the value is meant to describe how lanes
> are physically connected on the board from the transmitter to the receiver. It
> does not tell the maximum of lanes that the device supports, which is
> intrinsic information known to the driver already.

Where is the driver getting that information from if not DT?
On the Pi we have two instances of the same peripheral, but it is
parameterised such that one instance only has 2 lanes implemented, and
the other has 4 lanes. Also we have some boards with both the 4-lane
instance where only 2 are wired out to the camera connector.

When trying to get the Pi DT bindings accepted I got blocked from
adding a DT parameter that specified the number of lanes implemented
by the peripheral and told to use data-lanes [1]. Now you say the
driver is meant to intrinsically know. I'm confused.

> Regarding other parameters, such as the clock mode you mentioned, they should
> usually be queried and negotiated at runtime, especially given that both the
> transmitting and receiving devices may very well support multiple options. We
> usually don't involve DT in those cases. Parsing DT properties of a remote
> node is frowned upon, because those properties are qualified by the compatible
> string of the node they sit in. A driver matching "foo,abc" knows what
> properties are defined for the corresponding DT node, but when that driver
> crosses links to DT node "bar,xyz", it has no a priori knowledge of what to
> expect there.
>
> Following the OF graph through endpoints and ports is still fine, as if a DT
> node uses the OF graph bindings, the nodes it is connected to are assumed to
> use the same bindings. No assumption can usually be made on other properties
> though, including the ones describing the bus configuration. For that reason
> the properties for the CSI-2 source should be parsed by the CSI-2 source
> driver, and the configuration of the source should be queried by the CSI-2
> receiver at runtime using the v4l2_subdev API (which is routinely extended
> with additional configuration information when the need arises).

You've lost me as to what you are saying is and isn't frowned upon.
data-lanes is in the endpoint, so we're following the OF graph through
the remote-endpoint. Are you saying that it is, or isn't, valid to
assume anything about data-lanes in remote-endpoint?

Do we have a subdev API call that provides this information at
runtime? There are the relatively new fwnode calls, but that is
parsing the endpoint, and you you're saying we're not meant to look at
the remote-endpoint.
Or are you saying that it should be done through the subdev API, but
isn't at the moment? I'd just like to know for definite.

>From what you say it appears I need to update my example DT binding
[2] as it should duplicate "clock-noncontinuous;" in both endpoints.
Is that right?

> > I'm genuinely interested on views here. On the Pi there will be a
> > variety of CSI2 devices connected, generally configured using
> > dtoverlays, and they will have varying requirements on number of
> > lanes. Standard Pis only have 2 CSI-2 lanes exposed out of a possible
> > 4 for the peripheral. The Compute Module is the exception where one
> > CSI2 interface has all 4 lanes brought out, the other only supports 2
> > lanes anyway.
> > I'm expecting the CSI2 receiver endpoint data-lanes property to match
> > that exposed by the Pi board, whilst the source endpoint data-lanes
> > property defines what the source uses. That allows easy validation by
> > the driver that the configuration can work. Otherwise an overlay would
> > have to write the number of lanes used on both the CSI endpoints and
> > potentially configuring it to use more lanes than can be supported.
>
> As explained above, we currently expect the latter, with the overlay modifying
> the data-lanes property of the receiver as well. This is partly due to the
> fact that receivers can support data lanes remapping, so the data-lanes
> property of the receiver needs not only to specify the number of lanes, but
> also how they are connected.

So data-lanes can't be used as any form of sanity checking, and
multiple parameters get duplicated between the endpoints. Ack.

> If you want to validate the data-lanes property to ensure the overlay doesn't
> try to use more lanes than available, you can do so either against a hardcoded
> value in the receiver driver (when the maximum is fixed for a given compatible
> string), or against a value read from DT (when the maximum depends on the
> board).

As above, I was told by Sakari to use data-lanes [1].
His comment:
"Please use "data-lanes" endpoint property instead. This is the number of
connected physical lanes and specific to the hardware."

I'd read that as It is the number of physical lanes connected to the
camera connector (not necessarily the camera itself) on that
particular board, otherwise it isn't a max-lanes parameter just a
lanes.
You're saying it should be specific to the board and sensor combination.

> > There is also the oddball one of the TC358743 which dynamically
> > switches the number of lanes in use based on the data rate required.
> > That's probably a separate discussion, but is currently dealt with via
> > g_mbus_config as amended back in Sept 2017 [1].
>
> This falls into the case of dynamic configuration discovery and negotiation I
> mentioned above, and we clearly need to make sure the v4l2_subdev API supports
> this use case.

So it doesn't support it at the moment?
We're relying 100% on both DT entries being matched and consistent,
and can't cope with dynamic reconfig (I see Phillipp's patch for the
workaround with g_mbus_config never got merged).

I thought I'd got a handle on this DT stuff, but obviously not :-(

Thanks,
  Dave

[1] https://www.spinics.net/lists/linux-media/msg117080.html
[2] https://www.spinics.net/lists/linux-media/msg122299.html

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

[Dave Stevenson]

Hi Sakari

On Fri, 21 Sep 2018 at 13:03, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Laurent,
>
> On Fri, Sep 21, 2018 at 01:01:09PM +0300, Laurent Pinchart wrote:
> ...
> > > There is also the oddball one of the TC358743 which dynamically
> > > switches the number of lanes in use based on the data rate required.
> > > That's probably a separate discussion, but is currently dealt with via
> > > g_mbus_config as amended back in Sept 2017 [1].
> >
> > This falls into the case of dynamic configuration discovery and negotiation I
> > mentioned above, and we clearly need to make sure the v4l2_subdev API supports
> > this use case.
>
> This could be added to struct v4l2_mbus_frame_desc; Niklas has driver that
> uses the framework support here, so this would likely end up merged soon:
>
> <URL:https://git.linuxtv.org/sailus/media_tree.git/tree/include/media/v4l2-subdev.h?h=vc&id=0cbd2b25b37ef5b2e6a14340dbca6d2d2d5af98e>
>
> The CSI-2 bus parameters are missing there currently but nothing prevents
> adding them. The semantics of set_frame_desc() needs to be probably defined
> better than it currently is.

So which parameters are you thinking of putting in there? Just the
number of lanes, or clocking modes and all other parameters for the
CSI interface?
It sounds like this should take over from the receiver's DT
completely, other than for lane reordering.

Of course the $1million question is rough timescales? The last commit
on there appears to be March 2017.
I've had to backburner my CSI2 receiver driver due to other time
pressures, so it sounds like I may as well leave it there until this
all settles down, or start looking at Niklas' driver and what changes
infers.

Thanks,
  Dave

Re: [PATCH 3/3] i2c: adv748x: fix typo in comment for TXB CSI-2 transmitter power down

[Kieran Bingham]

Hi Niklas,

On 18/09/18 02:45, Niklas Söderlund wrote:
> Fix copy-and-past error in comment for TXB CSI-2 transmitter power down
> sequence.
> 

I have collected this patch into my adv748x/for-next branch (and fixed
the typo in "copy-and-past" and submitted as a pull request for Hans and
Mauro.

--
Regards

Kieran


> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 9a82cdf301bccb41..86cb38f4d7cc11c6 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -299,7 +299,7 @@ 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 4-lane MIPI */
> +	{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 */
>  
> 

Re: [PATCH 3/3] i2c: adv748x: fix typo in comment for TXB CSI-2 transmitter power down

[Niklas Söderlund]

Hi Kieran,

On 2018-09-26 14:49:22 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 18/09/18 02:45, Niklas Söderlund wrote:
> > Fix copy-and-past error in comment for TXB CSI-2 transmitter power down
> > sequence.
> > 
> 
> I have collected this patch into my adv748x/for-next branch (and fixed
> the typo in "copy-and-past" and submitted as a pull request for Hans and
> Mauro.

Thanks I will drop this patch for my v2.

> 
> --
> Regards
> 
> Kieran
> 
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index 9a82cdf301bccb41..86cb38f4d7cc11c6 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -299,7 +299,7 @@ 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 4-lane MIPI */
> > +	{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 */
> >  
> > 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 3/3] i2c: adv748x: fix typo in comment for TXB CSI-2 transmitter power down

[Niklas Söderlund]

Hi Kieran,

On 2018-09-26 14:49:22 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 18/09/18 02:45, Niklas S�derlund wrote:
> > Fix copy-and-past error in comment for TXB CSI-2 transmitter power down
> > sequence.
> > 
> 
> I have collected this patch into my adv748x/for-next branch (and fixed
> the typo in "copy-and-past" and submitted as a pull request for Hans and
> Mauro.

Thanks I will drop this patch for my v2.

> 
> --
> Regards
> 
> Kieran
> 
> 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index 9a82cdf301bccb41..86cb38f4d7cc11c6 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -299,7 +299,7 @@ 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 4-lane MIPI */
> > +	{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 */
> >  
> > 

-- 
Regards,
Niklas S�derlund

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

[Sakari Ailus]

Hi Dave,

Apologies for the delay.

On Fri, Sep 21, 2018 at 02:46:23PM +0100, Dave Stevenson wrote:
> Hi Sakari
> 
> On Fri, 21 Sep 2018 at 13:03, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > Hi Laurent,
> >
> > On Fri, Sep 21, 2018 at 01:01:09PM +0300, Laurent Pinchart wrote:
> > ...
> > > > There is also the oddball one of the TC358743 which dynamically
> > > > switches the number of lanes in use based on the data rate required.
> > > > That's probably a separate discussion, but is currently dealt with via
> > > > g_mbus_config as amended back in Sept 2017 [1].
> > >
> > > This falls into the case of dynamic configuration discovery and negotiation I
> > > mentioned above, and we clearly need to make sure the v4l2_subdev API supports
> > > this use case.
> >
> > This could be added to struct v4l2_mbus_frame_desc; Niklas has driver that
> > uses the framework support here, so this would likely end up merged soon:
> >
> > <URL:https://git.linuxtv.org/sailus/media_tree.git/tree/include/media/v4l2-subdev.h?h=vc&id=0cbd2b25b37ef5b2e6a14340dbca6d2d2d5af98e>
> >
> > The CSI-2 bus parameters are missing there currently but nothing prevents
> > adding them. The semantics of set_frame_desc() needs to be probably defined
> > better than it currently is.
> 
> So which parameters are you thinking of putting in there? Just the
> number of lanes, or clocking modes and all other parameters for the
> CSI interface?

I think it could be the number of active lanes, I don't think other
parameters need to change.

> It sounds like this should take over from the receiver's DT
> completely, other than for lane reordering.

Hmm. Right now I don't have an opinion either way. But I'd like to know
what others think.

The endpoint configuration is currently local to the endpoint only. On
other busses than CSI-2 there are more parameters that may be different on
each side of the endpoint. If the parameters are moved to the frame
descriptor entirely, there's no way to e.g. validate them in probe. At
least one would need to show that this is not an issue, or address it
somehow.

> 
> Of course the $1million question is rough timescales? The last commit
> on there appears to be March 2017.
> I've had to backburner my CSI2 receiver driver due to other time
> pressures, so it sounds like I may as well leave it there until this
> all settles down, or start looking at Niklas' driver and what changes
> infers.

Yes; if you write patches to this, please do that on top of Niklas' set.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi