[PATCH 0/5] media: adv748x: Implement dynamic routing support

[PATCH 0/5] media: adv748x: Implement dynamic routing support

[Jacopo Mondi]

Hello,
   this series implements dynamic selection of video source for the
adv748x video/HDMI decoder.

The series aims to have CVBS capture work on R-Car E3 Ebisu board,
where the HDMI and CVBS input share a single CSI-2 TX output.

In all other R-Car Gen3 boards the CVBS input is statically routed to TXB,
while HDMI goes to TXA. On E3 both CVBS and HDMI are routed to TXA, so that
to have CVBS video captured the video routes have to be modified at
run-time.

In order to do so this series implement the 'link_setup()' callback for the
adv748x, that gets called when a link from AFE or HDMI to a TX is created in
the media graph.

Unfortunately this series is not enough to have CVBS properly working
when routed to TXA on any board but E3. The reason is that the number of
CSI-2 data lanes needs to be re-negotiated between the adv748x's CSI-2 TX
and the rcar-csi2 CSI-2 RX when the video input is set to CVBS, and currently
the v4l2 framework does not implement any operation to do so.

I'm hesitant to have the full series merged, as it would allow configuring
a non-working CVBS->TXA link on Gen3 boards, while said configuration is not
yet working properly. At the same time, in order to have CVBS captured on E3
this series is required. I expect some discussions on how to proceed forward,
possibly delaying this series inclusion after a proper lane reconfiguration
operations is added to V4L2.

The series is based on media tree master with the following series from
Niklas applied on top:
[PATCH v4 0/4] i2c: adv748x: add support for CSI-2 TXA to work in 1-, 2- and 4-lane mode

Tested on E3:
HDMI->TXA works as usual
CVBS->TXA captures images (although I have image quality issues)

Tested on Salvator-X M3-W with number of CSI-2 data lanes forced to 2:
HDMI->TXA works but images are not of same quality as in 4 lanes configuration
CVBS->TXA works (with same image quality issues seen on E3)
CVBS->TXB works ((with same image quality issues seen on E3)

When testing on Salvator-X M3-W without forcing the number of data lanes to 2,
capturing CVBS input from TXA hangs (no frames are returned).

Feedbacks on how to proceed forward are welcome. As I've said I'm fine
postponing inclusion of this series after an operation to support CSI-2 data
lane negotiation is implemented in V4L2.

Thanks
   j

Jacopo Mondi (5):
  media: adv748x: Rework reset procedure
  media: adv748x: csi2: Link AFE with TXA and TXB
  media: adv748x: Store the source subdevice in TX
  media: adv748x: Store the TX sink in HDMI/AFE
  media: adv748x: Implement link_setup callback

 drivers/media/i2c/adv748x/adv748x-afe.c  |  2 +-
 drivers/media/i2c/adv748x/adv748x-core.c | 89 ++++++++++++++++++++++++++------
 drivers/media/i2c/adv748x/adv748x-csi2.c | 64 ++++++++++++++++-------
 drivers/media/i2c/adv748x/adv748x-hdmi.c |  2 +-
 drivers/media/i2c/adv748x/adv748x.h      |  6 +++
 5 files changed, 125 insertions(+), 38 deletions(-)

--
2.7.4

[PATCH 1/5] media: adv748x: Rework reset procedure

[Jacopo Mondi]

Re-work the chip reset procedure to configure the CP (HDMI) and SD (AFE) cores
before resetting the MIPI CSI-2 TXs.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index d94c63cb6a2e..5495dc7891e8 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -353,9 +353,8 @@ static const struct adv748x_reg_value adv748x_sw_reset[] = {
 	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
 };

-/* Supported Formats For Script Below */
-/* - 01-29 HDMI to MIPI TxA CSI 4-Lane - RGB888: */
-static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
+/* Initialize CP Core. */
+static const struct adv748x_reg_value adv748x_init_hdmi[] = {
 	/* Disable chip powerdown & Enable HDMI Rx block */
 	{ADV748X_PAGE_IO, 0x00, 0x40},

@@ -399,10 +398,8 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
 	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
 };

-/* 02-01 Analog CVBS to MIPI TX-B CSI 1-Lane - */
-/* Autodetect CVBS Single Ended In Ain 1 - MIPI Out */
-static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
-
+/* Initialize AFE core. */
+static const struct adv748x_reg_value adv748x_init_afe[] = {
 	{ADV748X_PAGE_IO, 0x00, 0x30},	/* Disable chip powerdown Rx */
 	{ADV748X_PAGE_IO, 0xf2, 0x01},	/* Enable I2C Read Auto-Increment */

@@ -445,19 +442,18 @@ static int adv748x_reset(struct adv748x_state *state)
 	if (ret < 0)
 		return ret;

-	/* Init and power down TXA */
-	ret = adv748x_write_regs(state, adv748x_init_txa_4lane);
+	/* Initialize CP and AFE cores. */
+	ret = adv748x_write_regs(state, adv748x_init_hdmi);
 	if (ret)
 		return ret;

-	adv748x_tx_power(&state->txa, 1);
-	adv748x_tx_power(&state->txa, 0);
-
-	/* Init and power down TXB */
-	ret = adv748x_write_regs(state, adv748x_init_txb_1lane);
+	ret = adv748x_write_regs(state, adv748x_init_afe);
 	if (ret)
 		return ret;

+	/* Reset TXA and TXB */
+	adv748x_tx_power(&state->txa, 1);
+	adv748x_tx_power(&state->txa, 0);
 	adv748x_tx_power(&state->txb, 1);
 	adv748x_tx_power(&state->txb, 0);

--
2.7.4

[PATCH 2/5] media: adv748x: csi2: Link AFE with TXA and TXB

[Jacopo Mondi]

The ADV748x chip supports routing AFE output to either TXA or TXB.
In order to support run-time configuration of video stream path, create an
additional (not enabled) "AFE:8->TXA:0" link, and remove the IMMUTABLE flag
from existing ones.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 48 ++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 6ce21542ed48..4d1aefc2c8d0 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -27,6 +27,7 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
  * @v4l2_dev: Video registration device
  * @src: Source subdevice to establish link
  * @src_pad: Pad number of source to link to this @tx
+ * @flags: Flags for the newly created link
  *
  * Ensure that the subdevice is registered against the v4l2_device, and link the
  * source pad to the sink pad of the CSI2 bus entity.
@@ -34,17 +35,11 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
 static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
 				      struct v4l2_device *v4l2_dev,
 				      struct v4l2_subdev *src,
-				      unsigned int src_pad)
+				      unsigned int src_pad,
+				      unsigned int flags)
 {
-	int enabled = MEDIA_LNK_FL_ENABLED;
 	int ret;

-	/*
-	 * Dynamic linking of the AFE is not supported.
-	 * Register the links as immutable.
-	 */
-	enabled |= MEDIA_LNK_FL_IMMUTABLE;
-
 	if (!src->v4l2_dev) {
 		ret = v4l2_device_register_subdev(v4l2_dev, src);
 		if (ret)
@@ -53,7 +48,7 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,

 	return media_create_pad_link(&src->entity, src_pad,
 				     &tx->sd.entity, ADV748X_CSI2_SINK,
-				     enabled);
+				     flags);
 }

 /* -----------------------------------------------------------------------------
@@ -68,24 +63,41 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
 {
 	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
 	struct adv748x_state *state = tx->state;
+	int ret;

 	adv_dbg(state, "Registered %s (%s)", is_txa(tx) ? "TXA":"TXB",
 			sd->name);

 	/*
-	 * The adv748x hardware allows the AFE to route through the TXA, however
-	 * this is not currently supported in this driver.
+	 * Link TXA to HDMI and AFE, and TXB to AFE only as TXB cannot output
+	 * HDMI.
 	 *
-	 * Link HDMI->TXA, and AFE->TXB directly.
+	 * The HDMI->TXA link is enabled by default, as the AFE->TXB is.
 	 */
-	if (is_txa(tx) && is_hdmi_enabled(state))
-		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
-						  &state->hdmi.sd,
-						  ADV748X_HDMI_SOURCE);
-	if (!is_txa(tx) && is_afe_enabled(state))
+	if (is_txa(tx)) {
+		if (is_hdmi_enabled(state)) {
+			ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
+							 &state->hdmi.sd,
+							 ADV748X_HDMI_SOURCE,
+							 MEDIA_LNK_FL_ENABLED);
+			if (ret)
+				return ret;
+		}
+
+		if (is_afe_enabled(state)) {
+			ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
+							 &state->afe.sd,
+							 ADV748X_AFE_SOURCE,
+							 0);
+			if (ret)
+				return ret;
+		}
+	} else if (is_afe_enabled(state))
 		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
 						  &state->afe.sd,
-						  ADV748X_AFE_SOURCE);
+						  ADV748X_AFE_SOURCE,
+						  MEDIA_LNK_FL_ENABLED);
+
 	return 0;
 }

--
2.7.4

[PATCH 3/5] media: adv748x: Store the source subdevice in TX

[Jacopo Mondi]

The power_up_tx() procedure needs to set a few registers conditionally to
the selected video source, but it currently checks for the provided tx to
be either TXA or TXB.

With the introduction of dynamic routing between HDMI and AFE entities to
TXA, checking which TX the function is operating on is not meaningful anymore.

To fix this, store the subdevice of the source providing video data to the
CSI-2 TX in the 'struct adv748x_csi2' representing the TX and check on it.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
 drivers/media/i2c/adv748x/adv748x-csi2.c | 3 +++
 drivers/media/i2c/adv748x/adv748x.h      | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 5495dc7891e8..f3aabbccdfb5 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -254,7 +254,7 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
 	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
 
 	/* ADI Required Write */
-	if (is_txa(tx)) {
+	if (tx->rsd == &state->hdmi.sd) {
 		adv748x_write_check(state, page, 0xdb, 0x10, &ret);
 		adv748x_write_check(state, page, 0xd6, 0x07, &ret);
 	} else {
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 4d1aefc2c8d0..307966f4c736 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -46,6 +46,9 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
 			return ret;
 	}
 
+	if (flags & MEDIA_LNK_FL_ENABLED)
+		tx->rsd = src;
+
 	return media_create_pad_link(&src->entity, src_pad,
 				     &tx->sd.entity, ADV748X_CSI2_SINK,
 				     flags);
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index b482c7fe6957..387002d6da65 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -85,6 +85,7 @@ struct adv748x_csi2 {
 	struct v4l2_ctrl_handler ctrl_hdl;
 	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_subdev sd;
+	struct v4l2_subdev *rsd;
 };
 
 #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
-- 
2.7.4

[PATCH 4/5] media: adv748x: Store the TX sink in HDMI/AFE

[Jacopo Mondi]

Both the AFE and HDMI s_stream routines (adv748x_afe_s_stream() and
adv748x_hdmi_s_stream()) have to enable the CSI-2 TX they are streaming video
data to.

With the introduction of dynamic routing between HDMI and AFE entities to
TXA, the video stream sink needs to be set at run time, and not statically
selected as the s_stream functions are currently doing.

To fix this, store a reference to the active CSI-2 TX sink for both HDMI and
AFE sources, and operate on it when starting/stopping the stream.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-afe.c  |  2 +-
 drivers/media/i2c/adv748x/adv748x-csi2.c | 19 ++++++++++++++-----
 drivers/media/i2c/adv748x/adv748x-hdmi.c |  2 +-
 drivers/media/i2c/adv748x/adv748x.h      |  4 ++++
 4 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
index 71714634efb0..dbbb1e4d6363 100644
--- a/drivers/media/i2c/adv748x/adv748x-afe.c
+++ b/drivers/media/i2c/adv748x/adv748x-afe.c
@@ -282,7 +282,7 @@ static int adv748x_afe_s_stream(struct v4l2_subdev *sd, int enable)
 			goto unlock;
 	}
 
-	ret = adv748x_tx_power(&state->txb, enable);
+	ret = adv748x_tx_power(afe->tx, enable);
 	if (ret)
 		goto unlock;
 
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 307966f4c736..0d6344a51795 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -85,6 +85,9 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
 							 MEDIA_LNK_FL_ENABLED);
 			if (ret)
 				return ret;
+
+			/* The default HDMI output is TXA. */
+			state->hdmi.tx = tx;
 		}
 
 		if (is_afe_enabled(state)) {
@@ -95,11 +98,17 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
 			if (ret)
 				return ret;
 		}
-	} else if (is_afe_enabled(state))
-		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
-						  &state->afe.sd,
-						  ADV748X_AFE_SOURCE,
-						  MEDIA_LNK_FL_ENABLED);
+	} else if (is_afe_enabled(state)) {
+		ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
+						 &state->afe.sd,
+						 ADV748X_AFE_SOURCE,
+						 MEDIA_LNK_FL_ENABLED);
+		if (ret)
+			return ret;
+
+		/* The default AFE output is TXB. */
+		state->afe.tx = tx;
+	}
 
 	return 0;
 }
diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c b/drivers/media/i2c/adv748x/adv748x-hdmi.c
index 35d027941482..c557f8fdf11a 100644
--- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
+++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
@@ -358,7 +358,7 @@ static int adv748x_hdmi_s_stream(struct v4l2_subdev *sd, int enable)
 
 	mutex_lock(&state->mutex);
 
-	ret = adv748x_tx_power(&state->txa, enable);
+	ret = adv748x_tx_power(hdmi->tx, enable);
 	if (ret)
 		goto done;
 
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 387002d6da65..0ee3b8d5c795 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -118,6 +118,8 @@ struct adv748x_hdmi {
 	struct v4l2_dv_timings timings;
 	struct v4l2_fract aspect_ratio;
 
+	struct adv748x_csi2 *tx;
+
 	struct {
 		u8 edid[512];
 		u32 present;
@@ -148,6 +150,8 @@ struct adv748x_afe {
 	struct v4l2_subdev sd;
 	struct v4l2_mbus_framefmt format;
 
+	struct adv748x_csi2 *tx;
+
 	bool streaming;
 	v4l2_std_id curr_norm;
 	unsigned int input;
-- 
2.7.4

[PATCH 5/5] media: adv748x: Implement link_setup callback

[Jacopo Mondi]

When the adv748x driver is informed about a link being created from HDMI or
AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
sure to implement proper routing management at link setup time, to route
the selected video stream to the desired TX output.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 63 +++++++++++++++++++++++++++++++-
 drivers/media/i2c/adv748x/adv748x.h      |  1 +
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index f3aabbccdfb5..08dc0e89b053 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -335,9 +335,70 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
 /* -----------------------------------------------------------------------------
  * Media Operations
  */
+static int adv748x_link_setup(struct media_entity *entity,
+			      const struct media_pad *local,
+			      const struct media_pad *remote, u32 flags)
+{
+	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
+	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+	struct adv748x_state *state = v4l2_get_subdevdata(sd);
+	struct adv748x_csi2 *tx;
+	struct media_link *link;
+	u8 io10;
+
+	/*
+	 * For each link setup from [HDMI|AFE] to TX we receive two
+	 * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]".
+	 *
+	 * Use the second notification form to make sure we're linking
+	 * to a TX and find out from where, to set up routing properly.
+	 */
+	if ((sd != &state->txa.sd && sd != &state->txb.sd) ||
+	    !(flags & MEDIA_LNK_FL_ENABLED))
+		return 0;
+	tx = adv748x_sd_to_csi2(sd);
+
+	/*
+	 * Now that we're sure we're operating on one of the two TXs,
+	 * make sure there are no enabled links ending there from
+	 * either HDMI or AFE (this can only happens for TXA though).
+	 */
+	if (is_txa(tx))
+		list_for_each_entry(link, &entity->links, list)
+			if (link->sink->entity == entity &&
+			    link->flags & MEDIA_LNK_FL_ENABLED)
+				return -EINVAL;
+
+	/* Change video stream routing, according to the newly created link. */
+	io10 = io_read(state, ADV748X_IO_10);
+	if (rsd == &state->afe.sd) {
+		state->afe.tx = tx;
+
+		/*
+		 * If AFE is routed to TXA, make sure TXB is off;
+		 * If AFE goes to TXB, we need TXA powered on.
+		 */
+		if (is_txa(tx)) {
+			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
+			io10 &= ~ADV748X_IO_10_CSI1_EN;
+		} else {
+			io10 |= ADV748X_IO_10_CSI4_EN |
+				ADV748X_IO_10_CSI1_EN;
+		}
+	} else {
+		state->hdmi.tx = tx;
+		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
+	}
+	io_write(state, ADV748X_IO_10, io10);
+
+	tx->rsd = rsd;
+
+	return 0;
+}
 
 static const struct media_entity_operations adv748x_media_ops = {
-	.link_validate = v4l2_subdev_link_validate,
+	.link_setup	= adv748x_link_setup,
+	.link_validate	= v4l2_subdev_link_validate,
 };
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 0ee3b8d5c795..63a17c31c169 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -220,6 +220,7 @@ struct adv748x_state {
 #define ADV748X_IO_10_CSI4_EN		BIT(7)
 #define ADV748X_IO_10_CSI1_EN		BIT(6)
 #define ADV748X_IO_10_PIX_OUT_EN	BIT(5)
+#define ADV748X_IO_10_CSI4_IN_SEL_AFE	0x08
 
 #define ADV748X_IO_CHIP_REV_ID_1	0xdf
 #define ADV748X_IO_CHIP_REV_ID_2	0xe0
-- 
2.7.4

Re: [PATCH 2/5] media: adv748x: csi2: Link AFE with TXA and TXB

[Kieran Bingham]

Hi Jacopo,

Thank you for the patch,

On 11/12/2018 15:16, Jacopo Mondi wrote:
> The ADV748x chip supports routing AFE output to either TXA or TXB.
> In order to support run-time configuration of video stream path, create an
> additional (not enabled) "AFE:8->TXA:0" link, and remove the IMMUTABLE flag
> from existing ones.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 48 ++++++++++++++++++++------------
>  1 file changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 6ce21542ed48..4d1aefc2c8d0 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -27,6 +27,7 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
>   * @v4l2_dev: Video registration device
>   * @src: Source subdevice to establish link
>   * @src_pad: Pad number of source to link to this @tx
> + * @flags: Flags for the newly created link
>   *
>   * Ensure that the subdevice is registered against the v4l2_device, and link the
>   * source pad to the sink pad of the CSI2 bus entity.
> @@ -34,17 +35,11 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
>  static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
>  				      struct v4l2_device *v4l2_dev,
>  				      struct v4l2_subdev *src,
> -				      unsigned int src_pad)
> +				      unsigned int src_pad,
> +				      unsigned int flags)
>  {
> -	int enabled = MEDIA_LNK_FL_ENABLED;
>  	int ret;
> 
> -	/*
> -	 * Dynamic linking of the AFE is not supported.
> -	 * Register the links as immutable.
> -	 */
> -	enabled |= MEDIA_LNK_FL_IMMUTABLE;
> -

Yup - that part certainly needs to go ...

>  	if (!src->v4l2_dev) {
>  		ret = v4l2_device_register_subdev(v4l2_dev, src);
>  		if (ret)
> @@ -53,7 +48,7 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
> 
>  	return media_create_pad_link(&src->entity, src_pad,
>  				     &tx->sd.entity, ADV748X_CSI2_SINK,
> -				     enabled);
> +				     flags);
>  }
> 
>  /* -----------------------------------------------------------------------------
> @@ -68,24 +63,41 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
>  {
>  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
>  	struct adv748x_state *state = tx->state;
> +	int ret;
> 
>  	adv_dbg(state, "Registered %s (%s)", is_txa(tx) ? "TXA":"TXB",
>  			sd->name);
> 
>  	/*
> -	 * The adv748x hardware allows the AFE to route through the TXA, however
> -	 * this is not currently supported in this driver.
> +	 * Link TXA to HDMI and AFE, and TXB to AFE only as TXB cannot output
> +	 * HDMI.
>  	 *
> -	 * Link HDMI->TXA, and AFE->TXB directly.
> +	 * The HDMI->TXA link is enabled by default, as the AFE->TXB is.
>  	 */
> -	if (is_txa(tx) && is_hdmi_enabled(state))
> -		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> -						  &state->hdmi.sd,
> -						  ADV748X_HDMI_SOURCE);
> -	if (!is_txa(tx) && is_afe_enabled(state))
> +	if (is_txa(tx)) {
> +		if (is_hdmi_enabled(state)) {
> +			ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> +							 &state->hdmi.sd,
> +							 ADV748X_HDMI_SOURCE,
> +							 MEDIA_LNK_FL_ENABLED);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		if (is_afe_enabled(state)) {
> +			ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> +							 &state->afe.sd,
> +							 ADV748X_AFE_SOURCE,
> +							 0);
> +			if (ret)
> +				return ret;
> +		}


> +	} else if (is_afe_enabled(state))

I believe when adding braces to one side of an if statement, we are
supposed to add to the else clauses too ?

>  		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
>  						  &state->afe.sd,
> -						  ADV748X_AFE_SOURCE);
> +						  ADV748X_AFE_SOURCE,
> +						  MEDIA_LNK_FL_ENABLED);

Won't this enable the AFE link for both TXA and TXB ?
Which one will win? Just the last one ? the first one ?
Does it error?

(It might not be a problem ... I can't recall what the behaviour is)


> +

There are a lot of nested if's above, and I think we can simplify
greatly if we move the logic for the flags inside
adv748x_csi2_register_link(), and adjust the checks on is_xxx_enabled()

What do you think about the following pseudo code?:


int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
  				      struct v4l2_device *v4l2_dev,
  				      struct v4l2_subdev *src,
				      unsigned int src_pad,
				      bool enable)
{

  int flags = 0;
  int ret;

  if (!src->v4l2_dev) {
	ret = v4l2_device_register_subdev(v4l2_dev, src)
	if (ret) return ret;
  }

  if (enable)
	flags = MEDIA_LNK_FL_ENABLED;

   return media_create_pad_link(&src->entity, src_pad,
 			        &tx->sd.entity, ADV748X_CSI2_SINK,
 			        flags);
}

int adv748x_csi2_registered(struct v4l2_subdev *sd)
{
  int ret;

  if (is_afe_enabled(state) {
      ret = adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->afe.sd,
				   ADV748X_AFE_SOURCE, !is_txa(tx));
      if (ret)
	  return ret;
  }

  /* TX-B only supports AFE */
  if (!is_txa(tx) || !(is_hdmi_enabled(state))
	return 0;

  return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
				    ADV748X_HDMI_SOURCE, true);
}


The above will for TXA:
	register_link(..., AFE_SOURCE, enable = false );
	register_link(..., HDMI_SOURCE, enable = true );

then TXB:
	register_link(..., AFE_SOURCE, enable = true );

Does that meet our needs?




>  	return 0;
>  }
> 
> --
> 2.7.4
> 

-- 
Regards
--
Kieran

Re: [PATCH 3/5] media: adv748x: Store the source subdevice in TX

[Kieran Bingham]

Hi Jacopo,

On 11/12/2018 15:16, Jacopo Mondi wrote:
> The power_up_tx() procedure needs to set a few registers conditionally to
> the selected video source, but it currently checks for the provided tx to
> be either TXA or TXB.
> 
> With the introduction of dynamic routing between HDMI and AFE entities to
> TXA, checking which TX the function is operating on is not meaningful anymore.
> 
> To fix this, store the subdevice of the source providing video data to the
> CSI-2 TX in the 'struct adv748x_csi2' representing the TX and check on it.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 3 +++
>  drivers/media/i2c/adv748x/adv748x.h      | 1 +
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 5495dc7891e8..f3aabbccdfb5 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -254,7 +254,7 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
>  	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
>  
>  	/* ADI Required Write */
> -	if (is_txa(tx)) {
> +	if (tx->rsd == &state->hdmi.sd) {

rsd? I presume this means 'remote subdevice' here?

I feel like 'src' is more meaningful here ... perhaps this could read
  (tx->src == &state->hdmi.sd)

Is that more understandable?


>  		adv748x_write_check(state, page, 0xdb, 0x10, &ret);
>  		adv748x_write_check(state, page, 0xd6, 0x07, &ret);
>  	} else {
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 4d1aefc2c8d0..307966f4c736 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -46,6 +46,9 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
>  			return ret;
>  	}
>  
> +	if (flags & MEDIA_LNK_FL_ENABLED)
> +		tx->rsd = src;

Is this easy to clear when the link changes?

(I'll find out in a later patch I guess)

> +
>  	return media_create_pad_link(&src->entity, src_pad,
>  				     &tx->sd.entity, ADV748X_CSI2_SINK,
>  				     flags);
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index b482c7fe6957..387002d6da65 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -85,6 +85,7 @@ struct adv748x_csi2 {
>  	struct v4l2_ctrl_handler ctrl_hdl;
>  	struct v4l2_ctrl *pixel_rate;
>  	struct v4l2_subdev sd;
> +	struct v4l2_subdev *rsd;
>  };
>  
>  #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
> 

-- 
Regards
--
Kieran

Re: [PATCH 4/5] media: adv748x: Store the TX sink in HDMI/AFE

[Kieran Bingham]

Hi Jacopo,

On 11/12/2018 15:16, Jacopo Mondi wrote:
> Both the AFE and HDMI s_stream routines (adv748x_afe_s_stream() and
> adv748x_hdmi_s_stream()) have to enable the CSI-2 TX they are streaming video
> data to.
> 
> With the introduction of dynamic routing between HDMI and AFE entities to
> TXA, the video stream sink needs to be set at run time, and not statically
> selected as the s_stream functions are currently doing.
> 
> To fix this, store a reference to the active CSI-2 TX sink for both HDMI and
> AFE sources, and operate on it when starting/stopping the stream.

Keeping these link references certainly feels like a useful thing to do.

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-afe.c  |  2 +-
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 19 ++++++++++++++-----
>  drivers/media/i2c/adv748x/adv748x-hdmi.c |  2 +-
>  drivers/media/i2c/adv748x/adv748x.h      |  4 ++++
>  4 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
> index 71714634efb0..dbbb1e4d6363 100644
> --- a/drivers/media/i2c/adv748x/adv748x-afe.c
> +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
> @@ -282,7 +282,7 @@ static int adv748x_afe_s_stream(struct v4l2_subdev *sd, int enable)
>  			goto unlock;
>  	}
>  
> -	ret = adv748x_tx_power(&state->txb, enable);
> +	ret = adv748x_tx_power(afe->tx, enable);

And makes that much better...

>  	if (ret)
>  		goto unlock;
>  
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 307966f4c736..0d6344a51795 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -85,6 +85,9 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
>  							 MEDIA_LNK_FL_ENABLED);
>  			if (ret)
>  				return ret;
> +
> +			/* The default HDMI output is TXA. */
> +			state->hdmi.tx = tx;

It's a shame that we haven't got a 'base class' for the AFE/HDMI so that
this could be set independently...


>  		}
>  
>  		if (is_afe_enabled(state)) {
> @@ -95,11 +98,17 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
>  			if (ret)
>  				return ret;
>  		}
> -	} else if (is_afe_enabled(state))
> -		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> -						  &state->afe.sd,
> -						  ADV748X_AFE_SOURCE,
> -						  MEDIA_LNK_FL_ENABLED);
> +	} else if (is_afe_enabled(state)) {
> +		ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> +						 &state->afe.sd,
> +						 ADV748X_AFE_SOURCE,
> +						 MEDIA_LNK_FL_ENABLED);
> +		if (ret)
> +			return ret;
> +
> +		/* The default AFE output is TXB. */
> +		state->afe.tx = tx;

Also - Can this be simplified into the pseudo code I suggested earlier.

I haven't given that much thought here as it's late - and I don't know
if you'll accept my pseudo code change yet :)


> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c b/drivers/media/i2c/adv748x/adv748x-hdmi.c
> index 35d027941482..c557f8fdf11a 100644
> --- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
> +++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
> @@ -358,7 +358,7 @@ static int adv748x_hdmi_s_stream(struct v4l2_subdev *sd, int enable)
>  
>  	mutex_lock(&state->mutex);
>  
> -	ret = adv748x_tx_power(&state->txa, enable);
> +	ret = adv748x_tx_power(hdmi->tx, enable);

Certainly beneficial here :)

>  	if (ret)
>  		goto done;
>  
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 387002d6da65..0ee3b8d5c795 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -118,6 +118,8 @@ struct adv748x_hdmi {
>  	struct v4l2_dv_timings timings;
>  	struct v4l2_fract aspect_ratio;
>  
> +	struct adv748x_csi2 *tx;
> +
>  	struct {
>  		u8 edid[512];
>  		u32 present;
> @@ -148,6 +150,8 @@ struct adv748x_afe {
>  	struct v4l2_subdev sd;
>  	struct v4l2_mbus_framefmt format;
>  
> +	struct adv748x_csi2 *tx;
> +
>  	bool streaming;
>  	v4l2_std_id curr_norm;
>  	unsigned int input;
> 

-- 
Regards
--
Kieran

Re: [PATCH 5/5] media: adv748x: Implement link_setup callback

[Kieran Bingham]

Hi Jacopo,

On 11/12/2018 15:16, Jacopo Mondi wrote:
> When the adv748x driver is informed about a link being created from HDMI or
> AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
> sure to implement proper routing management at link setup time, to route
> the selected video stream to the desired TX output.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 63 +++++++++++++++++++++++++++++++-
>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
>  2 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index f3aabbccdfb5..08dc0e89b053 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -335,9 +335,70 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>  /* -----------------------------------------------------------------------------
>   * Media Operations
>   */
> +static int adv748x_link_setup(struct media_entity *entity,
> +			      const struct media_pad *local,
> +			      const struct media_pad *remote, u32 flags)
> +{
> +	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> +	struct adv748x_state *state = v4l2_get_subdevdata(sd);
> +	struct adv748x_csi2 *tx;
> +	struct media_link *link;
> +	u8 io10;
> +
> +	/*
> +	 * For each link setup from [HDMI|AFE] to TX we receive two
> +	 * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]".
> +	 *
> +	 * Use the second notification form to make sure we're linking
> +	 * to a TX and find out from where, to set up routing properly.
> +	 */


> +	if ((sd != &state->txa.sd && sd != &state->txb.sd) ||

I'm starting to think an 'is_txb(tx)' would help clean up some code ...
Then we could do the assignment of tx above, and then this line would read

  if ( (!(is_txa(tx) && !(is_txb(tx)))
     || !(flags & MEDIA_LNK_FL_ENABLED) )


It shouldn't matter that the adv748x_sd_to_csi2(sd) could be called on
non-TX SD's as they will then simply fail to match the above is_txa/is_txb.



> +	    !(flags & MEDIA_LNK_FL_ENABLED))
> +		return 0;

Don't we need to clear some local references when disabling links?

(or actually perhaps it doesn't matter if we keep stale references in a
disabled object, because it's disabled)

> +	tx = adv748x_sd_to_csi2(sd);


> +
> +	/*
> +	 * Now that we're sure we're operating on one of the two TXs,
> +	 * make sure there are no enabled links ending there from
> +	 * either HDMI or AFE (this can only happens for TXA though).
> +	 */
> +	if (is_txa(tx))
> +		list_for_each_entry(link, &entity->links, list)
> +			if (link->sink->entity == entity &&
> +			    link->flags & MEDIA_LNK_FL_ENABLED)
> +				return -EINVAL;
> +

What does this protect?

Doesn't this code read as:

  if (is TXA)
	for each entity
		Check all links - and if any are enabled, -EINVAL

Don't we ever want a link to be enabled on TXA?

(I must surely be mis-reading this - and it's nearly mid-night - so I'm
going to say I'm confused and it's time for me to stop and go to bed :D)


> +	/* Change video stream routing, according to the newly created link. */
> +	io10 = io_read(state, ADV748X_IO_10);
> +	if (rsd == &state->afe.sd) {
> +		state->afe.tx = tx;
> +
> +		/*
> +		 * If AFE is routed to TXA, make sure TXB is off;
> +		 * If AFE goes to TXB, we need TXA powered on.
> +		 */
> +		if (is_txa(tx)) {
> +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +			io10 &= ~ADV748X_IO_10_CSI1_EN;
> +		} else {
> +			io10 |= ADV748X_IO_10_CSI4_EN |
> +				ADV748X_IO_10_CSI1_EN;
> +		}
> +	} else {
> +		state->hdmi.tx = tx;
> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +	}
> +	io_write(state, ADV748X_IO_10, io10);
> +
> +	tx->rsd = rsd;
> +
> +	return 0;
> +}
>  
>  static const struct media_entity_operations adv748x_media_ops = {
> -	.link_validate = v4l2_subdev_link_validate,
> +	.link_setup	= adv748x_link_setup,
> +	.link_validate	= v4l2_subdev_link_validate,
>  };
>  
>  /* -----------------------------------------------------------------------------
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 0ee3b8d5c795..63a17c31c169 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -220,6 +220,7 @@ struct adv748x_state {
>  #define ADV748X_IO_10_CSI4_EN		BIT(7)
>  #define ADV748X_IO_10_CSI1_EN		BIT(6)
>  #define ADV748X_IO_10_PIX_OUT_EN	BIT(5)
> +#define ADV748X_IO_10_CSI4_IN_SEL_AFE	0x08

Should this be BIT(3)?

>  
>  #define ADV748X_IO_CHIP_REV_ID_1	0xdf
>  #define ADV748X_IO_CHIP_REV_ID_2	0xe0
> 

-- 
Regards
--
Kieran

Re: [PATCH 1/5] media: adv748x: Rework reset procedure

[Kieran Bingham]

Hi Jacopo,

On 11/12/2018 15:16, Jacopo Mondi wrote:
> Re-work the chip reset procedure to configure the CP (HDMI) and SD (AFE) cores
> before resetting the MIPI CSI-2 TXs.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index d94c63cb6a2e..5495dc7891e8 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -353,9 +353,8 @@ static const struct adv748x_reg_value adv748x_sw_reset[] = {
>  	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
>  };
> 
> -/* Supported Formats For Script Below */
> -/* - 01-29 HDMI to MIPI TxA CSI 4-Lane - RGB888: */

Is this information redundant ? (CSI-4Lane, RGB888 configuration?)

> -static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
> +/* Initialize CP Core. */
> +static const struct adv748x_reg_value adv748x_init_hdmi[] = {

While we're here - is there much scope - or value in changing these
tables to functions with parameters using Niklas' adv748x_write_check() ?

The suggestion only has value if there are parameters that we would need
to configure. So it might be reasonable to leave these tables.

A general Ack on renaming to the function instead of the
TX/configuration though - as that makes the purpose clearer.


>  	/* Disable chip powerdown & Enable HDMI Rx block */
>  	{ADV748X_PAGE_IO, 0x00, 0x40},
> 
> @@ -399,10 +398,8 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
>  	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
>  };
> 
> -/* 02-01 Analog CVBS to MIPI TX-B CSI 1-Lane - */
> -/* Autodetect CVBS Single Ended In Ain 1 - MIPI Out */
> -static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
> -

Same comments as above really :)

> +/* Initialize AFE core. */
> +static const struct adv748x_reg_value adv748x_init_afe[] = {
>  	{ADV748X_PAGE_IO, 0x00, 0x30},	/* Disable chip powerdown Rx */
>  	{ADV748X_PAGE_IO, 0xf2, 0x01},	/* Enable I2C Read Auto-Increment */
> 
> @@ -445,19 +442,18 @@ static int adv748x_reset(struct adv748x_state *state)
>  	if (ret < 0)
>  		return ret;
> 
> -	/* Init and power down TXA */
> -	ret = adv748x_write_regs(state, adv748x_init_txa_4lane);
> +	/* Initialize CP and AFE cores. */
> +	ret = adv748x_write_regs(state, adv748x_init_hdmi);
>  	if (ret)
>  		return ret;
> 
> -	adv748x_tx_power(&state->txa, 1);
> -	adv748x_tx_power(&state->txa, 0);
> -
> -	/* Init and power down TXB */
> -	ret = adv748x_write_regs(state, adv748x_init_txb_1lane);
> +	ret = adv748x_write_regs(state, adv748x_init_afe);
>  	if (ret)
>  		return ret;
> 
> +	/* Reset TXA and TXB */
> +	adv748x_tx_power(&state->txa, 1);
> +	adv748x_tx_power(&state->txa, 0);
>  	adv748x_tx_power(&state->txb, 1);
>  	adv748x_tx_power(&state->txb, 0);
> 
> --
> 2.7.4
> 

-- 
Regards
--
Kieran

Re: [PATCH 1/5] media: adv748x: Rework reset procedure

[jacopo mondi]

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

Hi Kieran,
   thanks for review

On Tue, Dec 11, 2018 at 11:52:03PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 11/12/2018 15:16, Jacopo Mondi wrote:
> > Re-work the chip reset procedure to configure the CP (HDMI) and SD (AFE) cores
> > before resetting the MIPI CSI-2 TXs.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 24 ++++++++++--------------
> >  1 file changed, 10 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index d94c63cb6a2e..5495dc7891e8 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -353,9 +353,8 @@ static const struct adv748x_reg_value adv748x_sw_reset[] = {
> >  	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> >  };
> >
> > -/* Supported Formats For Script Below */
> > -/* - 01-29 HDMI to MIPI TxA CSI 4-Lane - RGB888: */
>
> Is this information redundant ? (CSI-4Lane, RGB888 configuration?)
>

The CSI-2 data lane configuration has been break out from this table
by Niklas' patches. I've tried also moving the format configuration
out of this, but I haven't sent that change. The HDMI video direction
is now handled at link setup time, so I guess the only relevant
information is about the RGB888 format configured on the CP backend.
I'll keep that.

> > -static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
> > +/* Initialize CP Core. */
> > +static const struct adv748x_reg_value adv748x_init_hdmi[] = {
>
> While we're here - is there much scope - or value in changing these
> tables to functions with parameters using Niklas' adv748x_write_check() ?
>
> The suggestion only has value if there are parameters that we would need
> to configure. So it might be reasonable to leave these tables.
>

Right now I don't see much value in that. I would prefer breaking out
the format configuration from this static tables, but that's for
later.

> A general Ack on renaming to the function instead of the
> TX/configuration though - as that makes the purpose clearer.
>
>
> >  	/* Disable chip powerdown & Enable HDMI Rx block */
> >  	{ADV748X_PAGE_IO, 0x00, 0x40},
> >
> > @@ -399,10 +398,8 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
> >  	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> >  };
> >
> > -/* 02-01 Analog CVBS to MIPI TX-B CSI 1-Lane - */
> > -/* Autodetect CVBS Single Ended In Ain 1 - MIPI Out */
> > -static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
> > -
>
> Same comments as above really :)
>

I'll see what I can keep.

Thanks
  j

> > +/* Initialize AFE core. */
> > +static const struct adv748x_reg_value adv748x_init_afe[] = {
> >  	{ADV748X_PAGE_IO, 0x00, 0x30},	/* Disable chip powerdown Rx */
> >  	{ADV748X_PAGE_IO, 0xf2, 0x01},	/* Enable I2C Read Auto-Increment */
> >
> > @@ -445,19 +442,18 @@ static int adv748x_reset(struct adv748x_state *state)
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	/* Init and power down TXA */
> > -	ret = adv748x_write_regs(state, adv748x_init_txa_4lane);
> > +	/* Initialize CP and AFE cores. */
> > +	ret = adv748x_write_regs(state, adv748x_init_hdmi);
> >  	if (ret)
> >  		return ret;
> >
> > -	adv748x_tx_power(&state->txa, 1);
> > -	adv748x_tx_power(&state->txa, 0);
> > -
> > -	/* Init and power down TXB */
> > -	ret = adv748x_write_regs(state, adv748x_init_txb_1lane);
> > +	ret = adv748x_write_regs(state, adv748x_init_afe);
> >  	if (ret)
> >  		return ret;
> >
> > +	/* Reset TXA and TXB */
> > +	adv748x_tx_power(&state->txa, 1);
> > +	adv748x_tx_power(&state->txa, 0);
> >  	adv748x_tx_power(&state->txb, 1);
> >  	adv748x_tx_power(&state->txb, 0);
> >
> > --
> > 2.7.4
> >
>
> --
> Regards
> --
> Kieran

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

Re: [PATCH 2/5] media: adv748x: csi2: Link AFE with TXA and TXB

[jacopo mondi]

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

Hi Kieran,

On Tue, Dec 11, 2018 at 11:07:09PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> Thank you for the patch,
>
> On 11/12/2018 15:16, Jacopo Mondi wrote:
> > The ADV748x chip supports routing AFE output to either TXA or TXB.
> > In order to support run-time configuration of video stream path, create an
> > additional (not enabled) "AFE:8->TXA:0" link, and remove the IMMUTABLE flag
> > from existing ones.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-csi2.c | 48 ++++++++++++++++++++------------
> >  1 file changed, 30 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index 6ce21542ed48..4d1aefc2c8d0 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -27,6 +27,7 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
> >   * @v4l2_dev: Video registration device
> >   * @src: Source subdevice to establish link
> >   * @src_pad: Pad number of source to link to this @tx
> > + * @flags: Flags for the newly created link
> >   *
> >   * Ensure that the subdevice is registered against the v4l2_device, and link the
> >   * source pad to the sink pad of the CSI2 bus entity.
> > @@ -34,17 +35,11 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
> >  static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
> >  				      struct v4l2_device *v4l2_dev,
> >  				      struct v4l2_subdev *src,
> > -				      unsigned int src_pad)
> > +				      unsigned int src_pad,
> > +				      unsigned int flags)
> >  {
> > -	int enabled = MEDIA_LNK_FL_ENABLED;
> >  	int ret;
> >
> > -	/*
> > -	 * Dynamic linking of the AFE is not supported.
> > -	 * Register the links as immutable.
> > -	 */
> > -	enabled |= MEDIA_LNK_FL_IMMUTABLE;
> > -
>
> Yup - that part certainly needs to go ...
>
> >  	if (!src->v4l2_dev) {
> >  		ret = v4l2_device_register_subdev(v4l2_dev, src);
> >  		if (ret)
> > @@ -53,7 +48,7 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
> >
> >  	return media_create_pad_link(&src->entity, src_pad,
> >  				     &tx->sd.entity, ADV748X_CSI2_SINK,
> > -				     enabled);
> > +				     flags);
> >  }
> >
> >  /* -----------------------------------------------------------------------------
> > @@ -68,24 +63,41 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
> >  {
> >  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> >  	struct adv748x_state *state = tx->state;
> > +	int ret;
> >
> >  	adv_dbg(state, "Registered %s (%s)", is_txa(tx) ? "TXA":"TXB",
> >  			sd->name);
> >
> >  	/*
> > -	 * The adv748x hardware allows the AFE to route through the TXA, however
> > -	 * this is not currently supported in this driver.
> > +	 * Link TXA to HDMI and AFE, and TXB to AFE only as TXB cannot output
> > +	 * HDMI.
> >  	 *
> > -	 * Link HDMI->TXA, and AFE->TXB directly.
> > +	 * The HDMI->TXA link is enabled by default, as the AFE->TXB is.
> >  	 */
> > -	if (is_txa(tx) && is_hdmi_enabled(state))
> > -		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> > -						  &state->hdmi.sd,
> > -						  ADV748X_HDMI_SOURCE);
> > -	if (!is_txa(tx) && is_afe_enabled(state))
> > +	if (is_txa(tx)) {
> > +		if (is_hdmi_enabled(state)) {
> > +			ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> > +							 &state->hdmi.sd,
> > +							 ADV748X_HDMI_SOURCE,
> > +							 MEDIA_LNK_FL_ENABLED);
> > +			if (ret)
> > +				return ret;
> > +		}
> > +
> > +		if (is_afe_enabled(state)) {
> > +			ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> > +							 &state->afe.sd,
> > +							 ADV748X_AFE_SOURCE,
> > +							 0);
> > +			if (ret)
> > +				return ret;
> > +		}
>
>
> > +	} else if (is_afe_enabled(state))
>
> I believe when adding braces to one side of an if statement, we are
> supposed to add to the else clauses too ?

Correct

>
> >  		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> >  						  &state->afe.sd,
> > -						  ADV748X_AFE_SOURCE);
> > +						  ADV748X_AFE_SOURCE,
> > +						  MEDIA_LNK_FL_ENABLED);
>
> Won't this enable the AFE link for both TXA and TXB ?
> Which one will win? Just the last one ? the first one ?
> Does it error?
>
> (It might not be a problem ... I can't recall what the behaviour is)
>
>

The AFE->TXA link is created as not enabled (see the 0 as last
argument in the adv748x_csi2_register_link() call above here, in the
'is_txa(tx)' case

> > +
>
> There are a lot of nested if's above, and I think we can simplify
> greatly if we move the logic for the flags inside
> adv748x_csi2_register_link(), and adjust the checks on is_xxx_enabled()
>
> What do you think about the following pseudo code?:
>
>
> int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
>   				      struct v4l2_device *v4l2_dev,
>   				      struct v4l2_subdev *src,
> 				      unsigned int src_pad,
> 				      bool enable)
> {
>
>   int flags = 0;
>   int ret;
>
>   if (!src->v4l2_dev) {
> 	ret = v4l2_device_register_subdev(v4l2_dev, src)
> 	if (ret) return ret;
>   }
>
>   if (enable)
> 	flags = MEDIA_LNK_FL_ENABLED;
>
>    return media_create_pad_link(&src->entity, src_pad,
>  			        &tx->sd.entity, ADV748X_CSI2_SINK,
>  			        flags);
> }
>
> int adv748x_csi2_registered(struct v4l2_subdev *sd)
> {
>   int ret;
>
>   if (is_afe_enabled(state) {
>       ret = adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->afe.sd,
> 				   ADV748X_AFE_SOURCE, !is_txa(tx));
>       if (ret)
> 	  return ret;
>   }
>
>   /* TX-B only supports AFE */
>   if (!is_txa(tx) || !(is_hdmi_enabled(state))
> 	return 0;
>
>   return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
> 				    ADV748X_HDMI_SOURCE, true);
> }
>
>
> The above will for TXA:
> 	register_link(..., AFE_SOURCE, enable = false );
> 	register_link(..., HDMI_SOURCE, enable = true );
>
> then TXB:
> 	register_link(..., AFE_SOURCE, enable = true );
>
> Does that meet our needs?
>

Yes it does, and it looks better. Thanks!

>
>
>
> >  	return 0;
> >  }
> >
> > --
> > 2.7.4
> >
>
> --
> Regards
> --
> Kieran

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

Re: [PATCH 5/5] media: adv748x: Implement link_setup callback

[jacopo mondi]

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

Hi Kieran,

On Tue, Dec 11, 2018 at 11:43:08PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 11/12/2018 15:16, Jacopo Mondi wrote:
> > When the adv748x driver is informed about a link being created from HDMI or
> > AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
> > sure to implement proper routing management at link setup time, to route
> > the selected video stream to the desired TX output.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 63 +++++++++++++++++++++++++++++++-
> >  drivers/media/i2c/adv748x/adv748x.h      |  1 +
> >  2 files changed, 63 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index f3aabbccdfb5..08dc0e89b053 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -335,9 +335,70 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> >  /* -----------------------------------------------------------------------------
> >   * Media Operations
> >   */
> > +static int adv748x_link_setup(struct media_entity *entity,
> > +			      const struct media_pad *local,
> > +			      const struct media_pad *remote, u32 flags)
> > +{
> > +	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
> > +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> > +	struct adv748x_state *state = v4l2_get_subdevdata(sd);
> > +	struct adv748x_csi2 *tx;
> > +	struct media_link *link;
> > +	u8 io10;
> > +
> > +	/*
> > +	 * For each link setup from [HDMI|AFE] to TX we receive two
> > +	 * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]".
> > +	 *
> > +	 * Use the second notification form to make sure we're linking
> > +	 * to a TX and find out from where, to set up routing properly.
> > +	 */
>
>
> > +	if ((sd != &state->txa.sd && sd != &state->txb.sd) ||
>
> I'm starting to think an 'is_txb(tx)' would help clean up some code ...
> Then we could do the assignment of tx above, and then this line would read
>
>   if ( (!(is_txa(tx) && !(is_txb(tx)))
>      || !(flags & MEDIA_LNK_FL_ENABLED) )
>
>
> It shouldn't matter that the adv748x_sd_to_csi2(sd) could be called on
> non-TX SD's as they will then simply fail to match the above is_txa/is_txb.
>

Checking for is_txa() and is_txb() would require to call
'adv_sd_to_csi2(sd)' before having made sure the 'sd' actually
represent a csi2_tx. I would keep it as it is.
>
>
> > +	    !(flags & MEDIA_LNK_FL_ENABLED))
> > +		return 0;
>
> Don't we need to clear some local references when disabling links?
>

I don't think so, if the link is disabled the pipeline would never
start and s_stream() (where the reference to the connected tx is used)
will never be called the AFE or HDMI backends.


> (or actually perhaps it doesn't matter if we keep stale references in a
> disabled object, because it's disabled)

Yes. Even if both HDMI and AFE have 'TXA' as their connected TX, only one
of them has an actually enabled link, and to enable that link, the
previously existing one has to be disabled first, otherwise this
function fails (see the -EINVAL a few lines below)

>
> > +	tx = adv748x_sd_to_csi2(sd);
>
>
> > +
> > +	/*
> > +	 * Now that we're sure we're operating on one of the two TXs,
> > +	 * make sure there are no enabled links ending there from
> > +	 * either HDMI or AFE (this can only happens for TXA though).
> > +	 */
> > +	if (is_txa(tx))
> > +		list_for_each_entry(link, &entity->links, list)
> > +			if (link->sink->entity == entity &&
> > +			    link->flags & MEDIA_LNK_FL_ENABLED)
> > +				return -EINVAL;
> > +
>
> What does this protect?
>
> Doesn't this code read as:
>
>   if (is TXA)
> 	for each entity
> 		Check all links - and if any are enabled, -EINVAL
>
> Don't we ever want a link to be enabled on TXA?

Not if we are enabling another one. One should first disable the
existing link, then create a new one.

>
> (I must surely be mis-reading this - and it's nearly mid-night - so I'm
> going to say I'm confused and it's time for me to stop and go to bed :D)
>
>
> > +	/* Change video stream routing, according to the newly created link. */
> > +	io10 = io_read(state, ADV748X_IO_10);
> > +	if (rsd == &state->afe.sd) {
> > +		state->afe.tx = tx;
> > +
> > +		/*
> > +		 * If AFE is routed to TXA, make sure TXB is off;
> > +		 * If AFE goes to TXB, we need TXA powered on.
> > +		 */
> > +		if (is_txa(tx)) {
> > +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +			io10 &= ~ADV748X_IO_10_CSI1_EN;
> > +		} else {
> > +			io10 |= ADV748X_IO_10_CSI4_EN |
> > +				ADV748X_IO_10_CSI1_EN;
> > +		}
> > +	} else {
> > +		state->hdmi.tx = tx;
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +	}
> > +	io_write(state, ADV748X_IO_10, io10);
> > +
> > +	tx->rsd = rsd;
> > +
> > +	return 0;
> > +}
> >
> >  static const struct media_entity_operations adv748x_media_ops = {
> > -	.link_validate = v4l2_subdev_link_validate,
> > +	.link_setup	= adv748x_link_setup,
> > +	.link_validate	= v4l2_subdev_link_validate,
> >  };
> >
> >  /* -----------------------------------------------------------------------------
> > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> > index 0ee3b8d5c795..63a17c31c169 100644
> > --- a/drivers/media/i2c/adv748x/adv748x.h
> > +++ b/drivers/media/i2c/adv748x/adv748x.h
> > @@ -220,6 +220,7 @@ struct adv748x_state {
> >  #define ADV748X_IO_10_CSI4_EN		BIT(7)
> >  #define ADV748X_IO_10_CSI1_EN		BIT(6)
> >  #define ADV748X_IO_10_PIX_OUT_EN	BIT(5)
> > +#define ADV748X_IO_10_CSI4_IN_SEL_AFE	0x08
>
> Should this be BIT(3)?
>

It surely read better. See, you were not that sleepy as you said,
after all :p

Thanks for review, I'll wait some more time to receive more comments
and will resend.

Thanks
  j

> >
> >  #define ADV748X_IO_CHIP_REV_ID_1	0xdf
> >  #define ADV748X_IO_CHIP_REV_ID_2	0xe0
> >
>
> --
> Regards
> --
> Kieran

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

Re: [PATCH 1/5] media: adv748x: Rework reset procedure

[Kieran Bingham]

Heya

On 12/12/2018 08:16, jacopo mondi wrote:
> Hi Kieran,
>    thanks for review
> 
> On Tue, Dec 11, 2018 at 11:52:03PM +0000, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 11/12/2018 15:16, Jacopo Mondi wrote:
>>> Re-work the chip reset procedure to configure the CP (HDMI) and SD (AFE) cores
>>> before resetting the MIPI CSI-2 TXs.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  drivers/media/i2c/adv748x/adv748x-core.c | 24 ++++++++++--------------
>>>  1 file changed, 10 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
>>> index d94c63cb6a2e..5495dc7891e8 100644
>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
>>> @@ -353,9 +353,8 @@ static const struct adv748x_reg_value adv748x_sw_reset[] = {
>>>  	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
>>>  };
>>>
>>> -/* Supported Formats For Script Below */
>>> -/* - 01-29 HDMI to MIPI TxA CSI 4-Lane - RGB888: */
>>
>> Is this information redundant ? (CSI-4Lane, RGB888 configuration?)
>>
> 
> The CSI-2 data lane configuration has been break out from this table
> by Niklas' patches. I've tried also moving the format configuration
> out of this, but I haven't sent that change. The HDMI video direction
> is now handled at link setup time, so I guess the only relevant
> information is about the RGB888 format configured on the CP backend.
> I'll keep that.
> 

Thanks for the clarification.

>>> -static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
>>> +/* Initialize CP Core. */
>>> +static const struct adv748x_reg_value adv748x_init_hdmi[] = {
>>
>> While we're here - is there much scope - or value in changing these
>> tables to functions with parameters using Niklas' adv748x_write_check() ?
>>
>> The suggestion only has value if there are parameters that we would need
>> to configure. So it might be reasonable to leave these tables.
>>
> 
> Right now I don't see much value in that. I would prefer breaking out
> the format configuration from this static tables, but that's for
> later.

Perfect - I agree - doesn't need to happen in this patch.

If the format configuration can be broken out from the table later then
that's great news.



>> A general Ack on renaming to the function instead of the
>> TX/configuration though - as that makes the purpose clearer.
>>
>>
>>>  	/* Disable chip powerdown & Enable HDMI Rx block */
>>>  	{ADV748X_PAGE_IO, 0x00, 0x40},
>>>
>>> @@ -399,10 +398,8 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
>>>  	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
>>>  };
>>>
>>> -/* 02-01 Analog CVBS to MIPI TX-B CSI 1-Lane - */
>>> -/* Autodetect CVBS Single Ended In Ain 1 - MIPI Out */
>>> -static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
>>> -
>>
>> Same comments as above really :)
>>
> 
> I'll see what I can keep.
> 
> Thanks
>   j
> 
>>> +/* Initialize AFE core. */
>>> +static const struct adv748x_reg_value adv748x_init_afe[] = {
>>>  	{ADV748X_PAGE_IO, 0x00, 0x30},	/* Disable chip powerdown Rx */
>>>  	{ADV748X_PAGE_IO, 0xf2, 0x01},	/* Enable I2C Read Auto-Increment */
>>>
>>> @@ -445,19 +442,18 @@ static int adv748x_reset(struct adv748x_state *state)
>>>  	if (ret < 0)
>>>  		return ret;
>>>
>>> -	/* Init and power down TXA */
>>> -	ret = adv748x_write_regs(state, adv748x_init_txa_4lane);
>>> +	/* Initialize CP and AFE cores. */
>>> +	ret = adv748x_write_regs(state, adv748x_init_hdmi);
>>>  	if (ret)
>>>  		return ret;
>>>
>>> -	adv748x_tx_power(&state->txa, 1);
>>> -	adv748x_tx_power(&state->txa, 0);
>>> -
>>> -	/* Init and power down TXB */
>>> -	ret = adv748x_write_regs(state, adv748x_init_txb_1lane);
>>> +	ret = adv748x_write_regs(state, adv748x_init_afe);
>>>  	if (ret)
>>>  		return ret;
>>>
>>> +	/* Reset TXA and TXB */
>>> +	adv748x_tx_power(&state->txa, 1);
>>> +	adv748x_tx_power(&state->txa, 0);
>>>  	adv748x_tx_power(&state->txb, 1);
>>>  	adv748x_tx_power(&state->txb, 0);
>>>
>>> --
>>> 2.7.4
>>>
>>
>> --
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran

Re: [PATCH 2/5] media: adv748x: csi2: Link AFE with TXA and TXB

[Kieran Bingham]

Hi Jacopo,

On 12/12/2018 08:21, jacopo mondi wrote:
> Hi Kieran,
> 
> On Tue, Dec 11, 2018 at 11:07:09PM +0000, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> Thank you for the patch,
>>
>> On 11/12/2018 15:16, Jacopo Mondi wrote:
>>> The ADV748x chip supports routing AFE output to either TXA or TXB.
>>> In order to support run-time configuration of video stream path, create an
>>> additional (not enabled) "AFE:8->TXA:0" link, and remove the IMMUTABLE flag
>>> from existing ones.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  drivers/media/i2c/adv748x/adv748x-csi2.c | 48 ++++++++++++++++++++------------
>>>  1 file changed, 30 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
>>> index 6ce21542ed48..4d1aefc2c8d0 100644
>>> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
>>> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
>>> @@ -27,6 +27,7 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
>>>   * @v4l2_dev: Video registration device
>>>   * @src: Source subdevice to establish link
>>>   * @src_pad: Pad number of source to link to this @tx
>>> + * @flags: Flags for the newly created link
>>>   *
>>>   * Ensure that the subdevice is registered against the v4l2_device, and link the
>>>   * source pad to the sink pad of the CSI2 bus entity.
>>> @@ -34,17 +35,11 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
>>>  static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
>>>  				      struct v4l2_device *v4l2_dev,
>>>  				      struct v4l2_subdev *src,
>>> -				      unsigned int src_pad)
>>> +				      unsigned int src_pad,
>>> +				      unsigned int flags)
>>>  {
>>> -	int enabled = MEDIA_LNK_FL_ENABLED;
>>>  	int ret;
>>>
>>> -	/*
>>> -	 * Dynamic linking of the AFE is not supported.
>>> -	 * Register the links as immutable.
>>> -	 */
>>> -	enabled |= MEDIA_LNK_FL_IMMUTABLE;
>>> -
>>
>> Yup - that part certainly needs to go ...
>>
>>>  	if (!src->v4l2_dev) {
>>>  		ret = v4l2_device_register_subdev(v4l2_dev, src);
>>>  		if (ret)
>>> @@ -53,7 +48,7 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
>>>
>>>  	return media_create_pad_link(&src->entity, src_pad,
>>>  				     &tx->sd.entity, ADV748X_CSI2_SINK,
>>> -				     enabled);
>>> +				     flags);
>>>  }
>>>
>>>  /* -----------------------------------------------------------------------------
>>> @@ -68,24 +63,41 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
>>>  {
>>>  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
>>>  	struct adv748x_state *state = tx->state;
>>> +	int ret;
>>>
>>>  	adv_dbg(state, "Registered %s (%s)", is_txa(tx) ? "TXA":"TXB",
>>>  			sd->name);
>>>
>>>  	/*
>>> -	 * The adv748x hardware allows the AFE to route through the TXA, however
>>> -	 * this is not currently supported in this driver.
>>> +	 * Link TXA to HDMI and AFE, and TXB to AFE only as TXB cannot output
>>> +	 * HDMI.
>>>  	 *
>>> -	 * Link HDMI->TXA, and AFE->TXB directly.
>>> +	 * The HDMI->TXA link is enabled by default, as the AFE->TXB is.
>>>  	 */
>>> -	if (is_txa(tx) && is_hdmi_enabled(state))
>>> -		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
>>> -						  &state->hdmi.sd,
>>> -						  ADV748X_HDMI_SOURCE);
>>> -	if (!is_txa(tx) && is_afe_enabled(state))
>>> +	if (is_txa(tx)) {
>>> +		if (is_hdmi_enabled(state)) {
>>> +			ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
>>> +							 &state->hdmi.sd,
>>> +							 ADV748X_HDMI_SOURCE,
>>> +							 MEDIA_LNK_FL_ENABLED);
>>> +			if (ret)
>>> +				return ret;
>>> +		}
>>> +
>>> +		if (is_afe_enabled(state)) {
>>> +			ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
>>> +							 &state->afe.sd,
>>> +							 ADV748X_AFE_SOURCE,
>>> +							 0);
>>> +			if (ret)
>>> +				return ret;
>>> +		}
>>
>>
>>> +	} else if (is_afe_enabled(state))
>>
>> I believe when adding braces to one side of an if statement, we are
>> supposed to add to the else clauses too ?
> 
> Correct
> 
>>
>>>  		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
>>>  						  &state->afe.sd,
>>> -						  ADV748X_AFE_SOURCE);
>>> +						  ADV748X_AFE_SOURCE,
>>> +						  MEDIA_LNK_FL_ENABLED);
>>
>> Won't this enable the AFE link for both TXA and TXB ?
>> Which one will win? Just the last one ? the first one ?
>> Does it error?
>>
>> (It might not be a problem ... I can't recall what the behaviour is)
>>
>>
> 
> The AFE->TXA link is created as not enabled (see the 0 as last
> argument in the adv748x_csi2_register_link() call above here, in the
> 'is_txa(tx)' case

Ugh - of course it is ... I was clearly way too tired last night. Oops :D


> 
>>> +
>>
>> There are a lot of nested if's above, and I think we can simplify
>> greatly if we move the logic for the flags inside
>> adv748x_csi2_register_link(), and adjust the checks on is_xxx_enabled()
>>
>> What do you think about the following pseudo code?:
>>
>>
>> int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
>>   				      struct v4l2_device *v4l2_dev,
>>   				      struct v4l2_subdev *src,
>> 				      unsigned int src_pad,
>> 				      bool enable)
>> {
>>
>>   int flags = 0;
>>   int ret;
>>
>>   if (!src->v4l2_dev) {
>> 	ret = v4l2_device_register_subdev(v4l2_dev, src)
>> 	if (ret) return ret;
>>   }
>>
>>   if (enable)
>> 	flags = MEDIA_LNK_FL_ENABLED;
>>
>>    return media_create_pad_link(&src->entity, src_pad,
>>  			        &tx->sd.entity, ADV748X_CSI2_SINK,
>>  			        flags);
>> }
>>
>> int adv748x_csi2_registered(struct v4l2_subdev *sd)
>> {
>>   int ret;
>>
>>   if (is_afe_enabled(state) {
>>       ret = adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->afe.sd,
>> 				   ADV748X_AFE_SOURCE, !is_txa(tx));
>>       if (ret)
>> 	  return ret;
>>   }
>>
>>   /* TX-B only supports AFE */
>>   if (!is_txa(tx) || !(is_hdmi_enabled(state))
>> 	return 0;
>>
>>   return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
>> 				    ADV748X_HDMI_SOURCE, true);
>> }
>>
>>
>> The above will for TXA:
>> 	register_link(..., AFE_SOURCE, enable = false );
>> 	register_link(..., HDMI_SOURCE, enable = true );
>>
>> then TXB:
>> 	register_link(..., AFE_SOURCE, enable = true );
>>
>> Does that meet our needs?
>>
> 
> Yes it does, and it looks better. Thanks!

Great - something useful came out of my non-sleeping sleepiness :)




>>>  	return 0;
>>>  }
>>>
>>> --
>>> 2.7.4
>>>
>>
>> --
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran

Re: [PATCH 5/5] media: adv748x: Implement link_setup callback

[Kieran Bingham]

Hi Jacopo,

On 12/12/2018 08:27, jacopo mondi wrote:
> Hi Kieran,
> 
> On Tue, Dec 11, 2018 at 11:43:08PM +0000, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 11/12/2018 15:16, Jacopo Mondi wrote:
>>> When the adv748x driver is informed about a link being created from HDMI or
>>> AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
>>> sure to implement proper routing management at link setup time, to route
>>> the selected video stream to the desired TX output.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  drivers/media/i2c/adv748x/adv748x-core.c | 63 +++++++++++++++++++++++++++++++-
>>>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
>>>  2 files changed, 63 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
>>> index f3aabbccdfb5..08dc0e89b053 100644
>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
>>> @@ -335,9 +335,70 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>>>  /* -----------------------------------------------------------------------------
>>>   * Media Operations
>>>   */
>>> +static int adv748x_link_setup(struct media_entity *entity,
>>> +			      const struct media_pad *local,
>>> +			      const struct media_pad *remote, u32 flags)
>>> +{
>>> +	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
>>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
>>> +	struct adv748x_state *state = v4l2_get_subdevdata(sd);
>>> +	struct adv748x_csi2 *tx;
>>> +	struct media_link *link;
>>> +	u8 io10;
>>> +
>>> +	/*
>>> +	 * For each link setup from [HDMI|AFE] to TX we receive two
>>> +	 * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]".
>>> +	 *
>>> +	 * Use the second notification form to make sure we're linking
>>> +	 * to a TX and find out from where, to set up routing properly.
>>> +	 */
>>
>>
>>> +	if ((sd != &state->txa.sd && sd != &state->txb.sd) ||
>>
>> I'm starting to think an 'is_txb(tx)' would help clean up some code ...
>> Then we could do the assignment of tx above, and then this line would read
>>
>>   if ( (!(is_txa(tx) && !(is_txb(tx)))
>>      || !(flags & MEDIA_LNK_FL_ENABLED) )
>>
>>
>> It shouldn't matter that the adv748x_sd_to_csi2(sd) could be called on
>> non-TX SD's as they will then simply fail to match the above is_txa/is_txb.
>>
> 
> Checking for is_txa() and is_txb() would require to call
> 'adv_sd_to_csi2(sd)' before having made sure the 'sd' actually
> represent a csi2_tx. I would keep it as it is.

Indeed - but the is_txa() / is_txb() would then guard against their usage.

We only use !is_txa(tx) once currently. If we add more - then it might
be worth a separate patch to add in the is_txb() anyway if you feel like
adding to your patch count  ;-)

>>
>>
>>> +	    !(flags & MEDIA_LNK_FL_ENABLED))
>>> +		return 0;
>>
>> Don't we need to clear some local references when disabling links?
>>
> 
> I don't think so, if the link is disabled the pipeline would never
> start and s_stream() (where the reference to the connected tx is used)
> will never be called the AFE or HDMI backends.


Ok - that's fine then.


>> (or actually perhaps it doesn't matter if we keep stale references in a
>> disabled object, because it's disabled)
> 
> Yes. Even if both HDMI and AFE have 'TXA' as their connected TX, only one
> of them has an actually enabled link, and to enable that link, the
> previously existing one has to be disabled first, otherwise this
> function fails (see the -EINVAL a few lines below)


Good.


> 
>>
>>> +	tx = adv748x_sd_to_csi2(sd);
>>
>>
>>> +
>>> +	/*
>>> +	 * Now that we're sure we're operating on one of the two TXs,
>>> +	 * make sure there are no enabled links ending there from
>>> +	 * either HDMI or AFE (this can only happens for TXA though).
>>> +	 */
>>> +	if (is_txa(tx))
>>> +		list_for_each_entry(link, &entity->links, list)
>>> +			if (link->sink->entity == entity &&
>>> +			    link->flags & MEDIA_LNK_FL_ENABLED)
>>> +				return -EINVAL;
>>> +
>>
>> What does this protect?
>>
>> Doesn't this code read as:
>>
>>   if (is TXA)
>> 	for each entity
>> 		Check all links - and if any are enabled, -EINVAL
>>
>> Don't we ever want a link to be enabled on TXA?
> 
> Not if we are enabling another one. One should first disable the
> existing link, then create a new one.

Ah - I read the code correctly - but mis-interpreted where the links
were coming from. I incorrectly thought they were 'new' links - not
checking the existing links (which is obvious from the whole
'entity->links' parameter in the for_each() - but ... well :)


I (probably incorrectly) /assume/ then that we could drop the
if(is_txa(tx)) conditional and indent here? As for TXB we will know that
it's links are not enabled - and will pass ?

I'm not sure if that would make for cleaner code (reduced indent) or
less obvious intent (not acting on TXA) though - so ... up to you :)


>>
>> (I must surely be mis-reading this - and it's nearly mid-night - so I'm
>> going to say I'm confused and it's time for me to stop and go to bed :D)

To: Me - "You were. Good job you went to bed :)"

>>
>>
>>> +	/* Change video stream routing, according to the newly created link. */
>>> +	io10 = io_read(state, ADV748X_IO_10);
>>> +	if (rsd == &state->afe.sd) {
>>> +		state->afe.tx = tx;
>>> +
>>> +		/*
>>> +		 * If AFE is routed to TXA, make sure TXB is off;
>>> +		 * If AFE goes to TXB, we need TXA powered on.
>>> +		 */
>>> +		if (is_txa(tx)) {
>>> +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>> +			io10 &= ~ADV748X_IO_10_CSI1_EN;
>>> +		} else {
>>> +			io10 |= ADV748X_IO_10_CSI4_EN |
>>> +				ADV748X_IO_10_CSI1_EN;
>>> +		}
>>> +	} else {
>>> +		state->hdmi.tx = tx;
>>> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>> +	}
>>> +	io_write(state, ADV748X_IO_10, io10);
>>> +
>>> +	tx->rsd = rsd;
>>> +
>>> +	return 0;
>>> +}
>>>
>>>  static const struct media_entity_operations adv748x_media_ops = {
>>> -	.link_validate = v4l2_subdev_link_validate,
>>> +	.link_setup	= adv748x_link_setup,
>>> +	.link_validate	= v4l2_subdev_link_validate,
>>>  };
>>>
>>>  /* -----------------------------------------------------------------------------
>>> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
>>> index 0ee3b8d5c795..63a17c31c169 100644
>>> --- a/drivers/media/i2c/adv748x/adv748x.h
>>> +++ b/drivers/media/i2c/adv748x/adv748x.h
>>> @@ -220,6 +220,7 @@ struct adv748x_state {
>>>  #define ADV748X_IO_10_CSI4_EN		BIT(7)
>>>  #define ADV748X_IO_10_CSI1_EN		BIT(6)
>>>  #define ADV748X_IO_10_PIX_OUT_EN	BIT(5)
>>> +#define ADV748X_IO_10_CSI4_IN_SEL_AFE	0x08
>>
>> Should this be BIT(3)?
>>
> 
> It surely read better. See, you were not that sleepy as you said,
> after all :p
> 
> Thanks for review, I'll wait some more time to receive more comments
> and will resend.
> 
> Thanks
>   j
> 
>>>
>>>  #define ADV748X_IO_CHIP_REV_ID_1	0xdf
>>>  #define ADV748X_IO_CHIP_REV_ID_2	0xe0
>>>
>>
>> --
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran

Re: [PATCH 1/5] media: adv748x: Rework reset procedure

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wednesday, 12 December 2018 12:13:44 EET Kieran Bingham wrote:
> On 12/12/2018 08:16, jacopo mondi wrote:
> > On Tue, Dec 11, 2018 at 11:52:03PM +0000, Kieran Bingham wrote:
> >> On 11/12/2018 15:16, Jacopo Mondi wrote:
> >>> Re-work the chip reset procedure to configure the CP (HDMI) and SD (AFE)
> >>> cores before resetting the MIPI CSI-2 TXs.
> >>> 
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>> 
> >>>  drivers/media/i2c/adv748x/adv748x-core.c | 24 ++++++++++--------------
> >>>  1 file changed, 10 insertions(+), 14 deletions(-)
> >>> 
> >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> b/drivers/media/i2c/adv748x/adv748x-core.c index
> >>> d94c63cb6a2e..5495dc7891e8 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> >>> @@ -353,9 +353,8 @@ static const struct adv748x_reg_value
> >>> adv748x_sw_reset[] = {>>> 
> >>>  	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> >>>  
> >>>  };
> >>> 
> >>> -/* Supported Formats For Script Below */
> >>> -/* - 01-29 HDMI to MIPI TxA CSI 4-Lane - RGB888: */
> >> 
> >> Is this information redundant ? (CSI-4Lane, RGB888 configuration?)
> > 
> > The CSI-2 data lane configuration has been break out from this table
> > by Niklas' patches. I've tried also moving the format configuration
> > out of this, but I haven't sent that change. The HDMI video direction
> > is now handled at link setup time, so I guess the only relevant
> > information is about the RGB888 format configured on the CP backend.
> > I'll keep that.
> 
> Thanks for the clarification.

Sounds good to me. With this change,

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

> >>> -static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
> >>> +/* Initialize CP Core. */
> >>> +static const struct adv748x_reg_value adv748x_init_hdmi[] = {
> >> 
> >> While we're here - is there much scope - or value in changing these
> >> tables to functions with parameters using Niklas' adv748x_write_check() ?
> >> 
> >> The suggestion only has value if there are parameters that we would need
> >> to configure. So it might be reasonable to leave these tables.
> > 
> > Right now I don't see much value in that. I would prefer breaking out
> > the format configuration from this static tables, but that's for
> > later.
> 
> Perfect - I agree - doesn't need to happen in this patch.
> 
> If the format configuration can be broken out from the table later then
> that's great news.

I think it will make sense to do so, yes.

> >> A general Ack on renaming to the function instead of the
> >> TX/configuration though - as that makes the purpose clearer.
> >> 
> >>>  	/* Disable chip powerdown & Enable HDMI Rx block */
> >>>  	{ADV748X_PAGE_IO, 0x00, 0x40},
> >>> 
> >>> @@ -399,10 +398,8 @@ static const struct adv748x_reg_value
> >>> adv748x_init_txa_4lane[] = {>>> 
> >>>  	{ADV748X_PAGE_EOR, 0xff, 0xff}	/* End of register table */
> >>>  
> >>>  };
> >>> 
> >>> -/* 02-01 Analog CVBS to MIPI TX-B CSI 1-Lane - */
> >>> -/* Autodetect CVBS Single Ended In Ain 1 - MIPI Out */
> >>> -static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
> >>> -
> >> 
> >> Same comments as above really :)
> > 
> > I'll see what I can keep.
> > 
> > Thanks
> > 
> >   j
> >   
> >>> +/* Initialize AFE core. */
> >>> +static const struct adv748x_reg_value adv748x_init_afe[] = {
> >>> 
> >>>  	{ADV748X_PAGE_IO, 0x00, 0x30},	/* Disable chip powerdown Rx */
> >>>  	{ADV748X_PAGE_IO, 0xf2, 0x01},	/* Enable I2C Read Auto-Increment */
> >>> 
> >>> @@ -445,19 +442,18 @@ static int adv748x_reset(struct adv748x_state
> >>> *state)
> >>> 
> >>>  	if (ret < 0)
> >>>  	
> >>>  		return ret;
> >>> 
> >>> -	/* Init and power down TXA */
> >>> -	ret = adv748x_write_regs(state, adv748x_init_txa_4lane);
> >>> +	/* Initialize CP and AFE cores. */
> >>> +	ret = adv748x_write_regs(state, adv748x_init_hdmi);
> >>> 
> >>>  	if (ret)
> >>>  	
> >>>  		return ret;
> >>> 
> >>> -	adv748x_tx_power(&state->txa, 1);
> >>> -	adv748x_tx_power(&state->txa, 0);
> >>> -
> >>> -	/* Init and power down TXB */
> >>> -	ret = adv748x_write_regs(state, adv748x_init_txb_1lane);
> >>> +	ret = adv748x_write_regs(state, adv748x_init_afe);
> >>> 
> >>>  	if (ret)
> >>>  	
> >>>  		return ret;
> >>> 
> >>> +	/* Reset TXA and TXB */
> >>> +	adv748x_tx_power(&state->txa, 1);
> >>> +	adv748x_tx_power(&state->txa, 0);
> >>> 
> >>>  	adv748x_tx_power(&state->txb, 1);
> >>>  	adv748x_tx_power(&state->txb, 0);
> >>> 
> >>> --
> >>> 2.7.4
> >> 
> >> --
> >> Regards
> >> --
> >> Kieran


-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] media: adv748x: csi2: Link AFE with TXA and TXB

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wednesday, 12 December 2018 10:21:01 EET jacopo mondi wrote:
> On Tue, Dec 11, 2018 at 11:07:09PM +0000, Kieran Bingham wrote:
> > On 11/12/2018 15:16, Jacopo Mondi wrote:
> >> The ADV748x chip supports routing AFE output to either TXA or TXB.
> >> In order to support run-time configuration of video stream path, create
> >> an additional (not enabled) "AFE:8->TXA:0" link, and remove the
> >> IMMUTABLE flag from existing ones.
> >> 
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> ---
> >> 
> >>  drivers/media/i2c/adv748x/adv748x-csi2.c | 48 ++++++++++++++++---------
> >>  1 file changed, 30 insertions(+), 18 deletions(-)
> >> 
> >> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c
> >> b/drivers/media/i2c/adv748x/adv748x-csi2.c index
> >> 6ce21542ed48..4d1aefc2c8d0 100644
> >> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> >> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> >> @@ -27,6 +27,7 @@ static int adv748x_csi2_set_virtual_channel(struct
> >> adv748x_csi2 *tx,> > 
> >>   * @v4l2_dev: Video registration device
> >>   * @src: Source subdevice to establish link
> >>   * @src_pad: Pad number of source to link to this @tx
> >> + * @flags: Flags for the newly created link

You may want to name this link_flags, up to you.

> >>   *
> >>   * Ensure that the subdevice is registered against the v4l2_device, and
> >>   link the
> >>   * source pad to the sink pad of the CSI2 bus entity.

[snip]

> >> @@ -68,24 +63,41 @@ static int adv748x_csi2_registered(struct
> >> v4l2_subdev *sd)
> >>  {
> >>  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> >>  	struct adv748x_state *state = tx->state;
> >> +	int ret;
> >> 
> >>  	adv_dbg(state, "Registered %s (%s)", is_txa(tx) ? "TXA":"TXB",
> >>  			sd->name);
> >>  	
> >>  	/*
> >> -	 * The adv748x hardware allows the AFE to route through the TXA,
> >> however
> >> -	 * this is not currently supported in this driver.
> >> +	 * Link TXA to HDMI and AFE, and TXB to AFE only as TXB cannot output
> >> +	 * HDMI.
> >>  	 *
> >> -	 * Link HDMI->TXA, and AFE->TXB directly.
> >> +	 * The HDMI->TXA link is enabled by default, as the AFE->TXB is.
> >>  	 */
> >> -	if (is_txa(tx) && is_hdmi_enabled(state))
> >> -		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> >> -						  &state->hdmi.sd,
> >> -						  ADV748X_HDMI_SOURCE);
> >> -	if (!is_txa(tx) && is_afe_enabled(state))
> >> +	if (is_txa(tx)) {
> >> +		if (is_hdmi_enabled(state)) {
> >> +			ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> >> +							 &state->hdmi.sd,
> >> +							 ADV748X_HDMI_SOURCE,
> >> +							 MEDIA_LNK_FL_ENABLED);
> >> +			if (ret)
> >> +				return ret;
> >> +		}
> >> +
> >> +		if (is_afe_enabled(state)) {
> >> +			ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> >> +							 &state->afe.sd,
> >> +							 ADV748X_AFE_SOURCE,
> >> +							 0);
> >> +			if (ret)
> >> +				return ret;
> >> +		}

I wonder whether in the case where both HDMI and TXB are disabled we shouldn't 
create this link as immutable and enabled ? This would require a big 
refactoring, possibly including deep changes in the v4l2-async and MC code, so 
it's likely out of scope for this patch (unless you see an easy way to do so).

> >> +	} else if (is_afe_enabled(state))
> > 
> > I believe when adding braces to one side of an if statement, we are
> > supposed to add to the else clauses too ?
> 
> Correct

And I would write this

	} else {
		if (is_afe_enabled(state))
			...
	}

To more clearly visualize the TXA and TXB code.

> >>  		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> >>  						  &state->afe.sd,
> >> -						  ADV748X_AFE_SOURCE);
> >> +						  ADV748X_AFE_SOURCE,
> >> +						  MEDIA_LNK_FL_ENABLED);
> > 
> > Won't this enable the AFE link for both TXA and TXB ?
> > Which one will win? Just the last one ? the first one ?
> > Does it error?
> > 
> > (It might not be a problem ... I can't recall what the behaviour is)
> 
> The AFE->TXA link is created as not enabled (see the 0 as last
> argument in the adv748x_csi2_register_link() call above here, in the
> 'is_txa(tx)' case
> 
> >> +
> > 
> > There are a lot of nested if's above, and I think we can simplify
> > greatly if we move the logic for the flags inside
> > adv748x_csi2_register_link(), and adjust the checks on is_xxx_enabled()
> > 
> > What do you think about the following pseudo code?:
> > 
> > int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
> >   				      struct v4l2_device *v4l2_dev,
> >   				      struct v4l2_subdev *src,
> > 				      unsigned int src_pad,
> > 				      bool enable)
> > {
> >   int flags = 0;
> >   int ret;
> >   
> >   if (!src->v4l2_dev) {
> > 	ret = v4l2_device_register_subdev(v4l2_dev, src)
> > 	if (ret) return ret;
> >   }
> >   
> >   if (enable)
> > 	flags = MEDIA_LNK_FL_ENABLED;
> > 	
> >    return media_create_pad_link(&src->entity, src_pad,
> >  			        &tx->sd.entity, ADV748X_CSI2_SINK,
> >  			        flags);
> > }
> > 
> > int adv748x_csi2_registered(struct v4l2_subdev *sd)
> > {
> >   int ret;
> >   

I would add

	/* The AFE can be routed to both TXA and TXB. */

> >   if (is_afe_enabled(state) {
> >       ret = adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->afe.sd,
> > 				   ADV748X_AFE_SOURCE, !is_txa(tx));
> >       if (ret)
> > 	  return ret;
> >   }
> >   
> >   /* TX-B only supports AFE */
> >   if (!is_txa(tx) || !(is_hdmi_enabled(state))
> > 	return 0;
> > 	
> >   return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
> > 				    ADV748X_HDMI_SOURCE, true);
> > }

Looks better than my above proposal :-)

> > The above will for TXA:
> > 	register_link(..., AFE_SOURCE, enable = false );
> > 	register_link(..., HDMI_SOURCE, enable = true );
> > 
> > then TXB:
> > 	register_link(..., AFE_SOURCE, enable = true );
> > 
> > Does that meet our needs?
> 
> Yes it does, and it looks better. Thanks!
> 
> >>  	return 0;
> >>  }

With Kieran's proposed change,

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] media: adv748x: Store the source subdevice in TX

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tuesday, 11 December 2018 17:16:11 EET Jacopo Mondi wrote:
> The power_up_tx() procedure needs to set a few registers conditionally to
> the selected video source, but it currently checks for the provided tx to
> be either TXA or TXB.
> 
> With the introduction of dynamic routing between HDMI and AFE entities to
> TXA, checking which TX the function is operating on is not meaningful
> anymore.
> 
> To fix this, store the subdevice of the source providing video data to the
> CSI-2 TX in the 'struct adv748x_csi2' representing the TX and check on it.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 3 +++
>  drivers/media/i2c/adv748x/adv748x.h      | 1 +
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index 5495dc7891e8..f3aabbccdfb5
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -254,7 +254,7 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
>  	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
> 
>  	/* ADI Required Write */
> -	if (is_txa(tx)) {
> +	if (tx->rsd == &state->hdmi.sd) {
>  		adv748x_write_check(state, page, 0xdb, 0x10, &ret);
>  		adv748x_write_check(state, page, 0xd6, 0x07, &ret);
>  	} else {
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c
> b/drivers/media/i2c/adv748x/adv748x-csi2.c index 4d1aefc2c8d0..307966f4c736
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -46,6 +46,9 @@ static int adv748x_csi2_register_link(struct adv748x_csi2
> *tx, return ret;
>  	}
> 
> +	if (flags & MEDIA_LNK_FL_ENABLED)
> +		tx->rsd = src;
> +
>  	return media_create_pad_link(&src->entity, src_pad,
>  				     &tx->sd.entity, ADV748X_CSI2_SINK,
>  				     flags);
> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h index b482c7fe6957..387002d6da65
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -85,6 +85,7 @@ struct adv748x_csi2 {
>  	struct v4l2_ctrl_handler ctrl_hdl;
>  	struct v4l2_ctrl *pixel_rate;
>  	struct v4l2_subdev sd;
> +	struct v4l2_subdev *rsd;

How about naming this source instead of rsd ? rsd is a bit cryptic.

With that change,

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

>  };
> 
>  #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/5] media: adv748x: Store the TX sink in HDMI/AFE

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tuesday, 11 December 2018 17:16:12 EET Jacopo Mondi wrote:
> Both the AFE and HDMI s_stream routines (adv748x_afe_s_stream() and
> adv748x_hdmi_s_stream()) have to enable the CSI-2 TX they are streaming
> video data to.
> 
> With the introduction of dynamic routing between HDMI and AFE entities to
> TXA, the video stream sink needs to be set at run time, and not statically
> selected as the s_stream functions are currently doing.
> 
> To fix this, store a reference to the active CSI-2 TX sink for both HDMI and
> AFE sources, and operate on it when starting/stopping the stream.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-afe.c  |  2 +-
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 19 ++++++++++++++-----
>  drivers/media/i2c/adv748x/adv748x-hdmi.c |  2 +-
>  drivers/media/i2c/adv748x/adv748x.h      |  4 ++++
>  4 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c
> b/drivers/media/i2c/adv748x/adv748x-afe.c index 71714634efb0..dbbb1e4d6363
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-afe.c
> +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
> @@ -282,7 +282,7 @@ static int adv748x_afe_s_stream(struct v4l2_subdev *sd,
> int enable) goto unlock;
>  	}
> 
> -	ret = adv748x_tx_power(&state->txb, enable);
> +	ret = adv748x_tx_power(afe->tx, enable);
>  	if (ret)
>  		goto unlock;
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c
> b/drivers/media/i2c/adv748x/adv748x-csi2.c index 307966f4c736..0d6344a51795
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -85,6 +85,9 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
> MEDIA_LNK_FL_ENABLED);
>  			if (ret)
>  				return ret;
> +
> +			/* The default HDMI output is TXA. */
> +			state->hdmi.tx = tx;
>  		}
> 
>  		if (is_afe_enabled(state)) {
> @@ -95,11 +98,17 @@ static int adv748x_csi2_registered(struct v4l2_subdev
> *sd) if (ret)
>  				return ret;
>  		}
> -	} else if (is_afe_enabled(state))
> -		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> -						  &state->afe.sd,
> -						  ADV748X_AFE_SOURCE,
> -						  MEDIA_LNK_FL_ENABLED);
> +	} else if (is_afe_enabled(state)) {
> +		ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> +						 &state->afe.sd,
> +						 ADV748X_AFE_SOURCE,
> +						 MEDIA_LNK_FL_ENABLED);
> +		if (ret)
> +			return ret;
> +
> +		/* The default AFE output is TXB. */
> +		state->afe.tx = tx;
> +	}
> 
>  	return 0;
>  }
> diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c
> b/drivers/media/i2c/adv748x/adv748x-hdmi.c index 35d027941482..c557f8fdf11a
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
> +++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
> @@ -358,7 +358,7 @@ static int adv748x_hdmi_s_stream(struct v4l2_subdev *sd,
> int enable)
> 
>  	mutex_lock(&state->mutex);
> 
> -	ret = adv748x_tx_power(&state->txa, enable);
> +	ret = adv748x_tx_power(hdmi->tx, enable);
>  	if (ret)
>  		goto done;
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h index 387002d6da65..0ee3b8d5c795
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -118,6 +118,8 @@ struct adv748x_hdmi {
>  	struct v4l2_dv_timings timings;
>  	struct v4l2_fract aspect_ratio;
> 
> +	struct adv748x_csi2 *tx;
> +
>  	struct {
>  		u8 edid[512];
>  		u32 present;
> @@ -148,6 +150,8 @@ struct adv748x_afe {
>  	struct v4l2_subdev sd;
>  	struct v4l2_mbus_framefmt format;
> 
> +	struct adv748x_csi2 *tx;
> +
>  	bool streaming;
>  	v4l2_std_id curr_norm;
>  	unsigned int input;

This may call for defining a common structure to store the common fields of 
adv748x_hdmi and adv748x_afe. Out of scope for this patch, but please keep it 
in mind.

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/5] media: adv748x: Implement link_setup callback

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tuesday, 11 December 2018 17:16:13 EET Jacopo Mondi wrote:
> When the adv748x driver is informed about a link being created from HDMI or
> AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
> sure to implement proper routing management at link setup time, to route
> the selected video stream to the desired TX output.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 63 ++++++++++++++++++++++++++++-
>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
>  2 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index f3aabbccdfb5..08dc0e89b053
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -335,9 +335,70 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>  /* ------------------------------------------------------------------------
>   * Media Operations
>   */
> +static int adv748x_link_setup(struct media_entity *entity,
> +			      const struct media_pad *local,
> +			      const struct media_pad *remote, u32 flags)
> +{
> +	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> +	struct adv748x_state *state = v4l2_get_subdevdata(sd);
> +	struct adv748x_csi2 *tx;
> +	struct media_link *link;
> +	u8 io10;
> +
> +	/*
> +	 * For each link setup from [HDMI|AFE] to TX we receive two
> +	 * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]".
> +	 *
> +	 * Use the second notification form to make sure we're linking
> +	 * to a TX and find out from where, to set up routing properly.
> +	 */

Why don't you implement the link handler just for the TX entities then ?

> +	if ((sd != &state->txa.sd && sd != &state->txb.sd) ||
> +	    !(flags & MEDIA_LNK_FL_ENABLED))

When disabling the link you should reset the ->source and ->tx pointers.

> +		return 0;
> +	tx = adv748x_sd_to_csi2(sd);
> +
> +	/*
> +	 * Now that we're sure we're operating on one of the two TXs,
> +	 * make sure there are no enabled links ending there from
> +	 * either HDMI or AFE (this can only happens for TXA though).
> +	 */
> +	if (is_txa(tx))
> +		list_for_each_entry(link, &entity->links, list)
> +			if (link->sink->entity == entity &&
> +			    link->flags & MEDIA_LNK_FL_ENABLED)
> +				return -EINVAL;

You can simplify this by checking if tx->source == NULL (after resetting tx-
>source when disabling the link of course).

> +	/* Change video stream routing, according to the newly created link. */
> +	io10 = io_read(state, ADV748X_IO_10);
> +	if (rsd == &state->afe.sd) {
> +		state->afe.tx = tx;
> +
> +		/*
> +		 * If AFE is routed to TXA, make sure TXB is off;
> +		 * If AFE goes to TXB, we need TXA powered on.
> +		 */
> +		if (is_txa(tx)) {
> +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +			io10 &= ~ADV748X_IO_10_CSI1_EN;
> +		} else {
> +			io10 |= ADV748X_IO_10_CSI4_EN |
> +				ADV748X_IO_10_CSI1_EN;
> +		}
> +	} else {
> +		state->hdmi.tx = tx;
> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +	}
> +	io_write(state, ADV748X_IO_10, io10);

Is it guaranteed that the chip will be powered on at this point ? How about 
writing the register at stream on time instead ?

> +	tx->rsd = rsd;
> +
> +	return 0;
> +}
> 
>  static const struct media_entity_operations adv748x_media_ops = {
> -	.link_validate = v4l2_subdev_link_validate,
> +	.link_setup	= adv748x_link_setup,
> +	.link_validate	= v4l2_subdev_link_validate,
>  };
> 
>  /* ------------------------------------------------------------------------
> -- diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h index 0ee3b8d5c795..63a17c31c169
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -220,6 +220,7 @@ struct adv748x_state {
>  #define ADV748X_IO_10_CSI4_EN		BIT(7)
>  #define ADV748X_IO_10_CSI1_EN		BIT(6)
>  #define ADV748X_IO_10_PIX_OUT_EN	BIT(5)
> +#define ADV748X_IO_10_CSI4_IN_SEL_AFE	0x08
> 
>  #define ADV748X_IO_CHIP_REV_ID_1	0xdf
>  #define ADV748X_IO_CHIP_REV_ID_2	0xe0

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/5] media: adv748x: Implement link_setup callback

[Jacopo Mondi]

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

Hi Laurent, Kieran,
  thanks for the review

On Thu, Dec 13, 2018 at 11:40:00AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tuesday, 11 December 2018 17:16:13 EET Jacopo Mondi wrote:
> > When the adv748x driver is informed about a link being created from HDMI or
> > AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
> > sure to implement proper routing management at link setup time, to route
> > the selected video stream to the desired TX output.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 63 ++++++++++++++++++++++++++++-
> >  drivers/media/i2c/adv748x/adv748x.h      |  1 +
> >  2 files changed, 63 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > b/drivers/media/i2c/adv748x/adv748x-core.c index f3aabbccdfb5..08dc0e89b053
> > 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -335,9 +335,70 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> >  /* ------------------------------------------------------------------------
> >   * Media Operations
> >   */
> > +static int adv748x_link_setup(struct media_entity *entity,
> > +			      const struct media_pad *local,
> > +			      const struct media_pad *remote, u32 flags)
> > +{
> > +	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
> > +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> > +	struct adv748x_state *state = v4l2_get_subdevdata(sd);
> > +	struct adv748x_csi2 *tx;
> > +	struct media_link *link;
> > +	u8 io10;
> > +
> > +	/*
> > +	 * For each link setup from [HDMI|AFE] to TX we receive two
> > +	 * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]".
> > +	 *
> > +	 * Use the second notification form to make sure we're linking
> > +	 * to a TX and find out from where, to set up routing properly.
> > +	 */
>
> Why don't you implement the link handler just for the TX entities then ?
>

Done. Much better

> > +	if ((sd != &state->txa.sd && sd != &state->txb.sd) ||
> > +	    !(flags & MEDIA_LNK_FL_ENABLED))
>
> When disabling the link you should reset the ->source and ->tx pointers.
>

right

> > +		return 0;
> > +	tx = adv748x_sd_to_csi2(sd);
> > +
> > +	/*
> > +	 * Now that we're sure we're operating on one of the two TXs,
> > +	 * make sure there are no enabled links ending there from
> > +	 * either HDMI or AFE (this can only happens for TXA though).
> > +	 */
> > +	if (is_txa(tx))
> > +		list_for_each_entry(link, &entity->links, list)
> > +			if (link->sink->entity == entity &&
> > +			    link->flags & MEDIA_LNK_FL_ENABLED)
> > +				return -EINVAL;
>
> You can simplify this by checking if tx->source == NULL (after resetting tx-
> >source when disabling the link of course).
>
> > +	/* Change video stream routing, according to the newly created link. */
> > +	io10 = io_read(state, ADV748X_IO_10);
> > +	if (rsd == &state->afe.sd) {
> > +		state->afe.tx = tx;
> > +
> > +		/*
> > +		 * If AFE is routed to TXA, make sure TXB is off;
> > +		 * If AFE goes to TXB, we need TXA powered on.
> > +		 */
> > +		if (is_txa(tx)) {
> > +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +			io10 &= ~ADV748X_IO_10_CSI1_EN;
> > +		} else {
> > +			io10 |= ADV748X_IO_10_CSI4_EN |
> > +				ADV748X_IO_10_CSI1_EN;
> > +		}
> > +	} else {
> > +		state->hdmi.tx = tx;
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +	}
> > +	io_write(state, ADV748X_IO_10, io10);
>
> Is it guaranteed that the chip will be powered on at this point ? How about
> writing the register at stream on time instead ?

You know, I struggle to find a better place where to do so...
- Right now the register gets written at reset time only
- s_stream: Each subdev (afe, hdmi, txa and txb) has its own s_stream
  so it's hard to find a centralized place where to write this
  register that impacts both TXes
- afe/hdmi s_stream() ops call tx_power_up, but again this is per-TX
- There doesn't seems to be much concern in the driver in
  synchronizing register writes with power states already actually. Not
  that's an excuse, but when I played around and tried to power off
  the chip's PLLs for real I got into LP-11 state detection issues
  (again :) I would leave it here if that's fine with you :)

Thanks
   j

>
> > +	tx->rsd = rsd;
> > +
> > +	return 0;
> > +}
> >
> >  static const struct media_entity_operations adv748x_media_ops = {
> > -	.link_validate = v4l2_subdev_link_validate,
> > +	.link_setup	= adv748x_link_setup,
> > +	.link_validate	= v4l2_subdev_link_validate,
> >  };
> >
> >  /* ------------------------------------------------------------------------
> > -- diff --git a/drivers/media/i2c/adv748x/adv748x.h
> > b/drivers/media/i2c/adv748x/adv748x.h index 0ee3b8d5c795..63a17c31c169
> > 100644
> > --- a/drivers/media/i2c/adv748x/adv748x.h
> > +++ b/drivers/media/i2c/adv748x/adv748x.h
> > @@ -220,6 +220,7 @@ struct adv748x_state {
> >  #define ADV748X_IO_10_CSI4_EN		BIT(7)
> >  #define ADV748X_IO_10_CSI1_EN		BIT(6)
> >  #define ADV748X_IO_10_PIX_OUT_EN	BIT(5)
> > +#define ADV748X_IO_10_CSI4_IN_SEL_AFE	0x08
> >
> >  #define ADV748X_IO_CHIP_REV_ID_1	0xdf
> >  #define ADV748X_IO_CHIP_REV_ID_2	0xe0
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

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