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

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

[Jacopo Mondi]

Hello,
   third iteration for adv748x dynamic selection of video source.

Please refer to v1 and v2 cover letter for more details:
https://patchwork.kernel.org/cover/10723971/
https://patchwork.kernel.org/cover/10749535/

This version includes comments from Laurent and Kieran, on patches 1/6 and
mostly on 6/6, which resulted in a better implementation. Thanks.

I have tested this on Ebisu E3 where I can capture HDMI and CVBS inputs from
TXA, and on Salvator-x M3-W where I can capture HDMI from TXA, and CVBS from
both TXA and TXB, but only when setting the number of CSI-2 data lanes to 2 in
DTS.

It remains unresolved how to dynamically negotiate the number of CSI-2 data
lanes to use between the video source and the CSI-2 receiver, which prevents
using AFE->TXA routing on all Gen3 SoC which use 4 CSI-2 data lanes.

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

Branch available for testing at:
git://jmondi.org/linux adv748x_dynamic-routing_v3

Thanks
  j

Jacopo Mondi (6):
  media: adv748x: Add is_txb()
  media: adv748x: Rename reset procedures
  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 TX link_setup callback

 drivers/media/i2c/adv748x/adv748x-afe.c  |  2 +-
 drivers/media/i2c/adv748x/adv748x-core.c | 74 +++++++++++++++++++-----
 drivers/media/i2c/adv748x/adv748x-csi2.c | 64 +++++++++++++-------
 drivers/media/i2c/adv748x/adv748x-hdmi.c |  2 +-
 drivers/media/i2c/adv748x/adv748x.h      | 10 ++++
 5 files changed, 112 insertions(+), 40 deletions(-)

--
2.20.1

[PATCH v3 1/6] media: adv748x: Add is_txb()

[Jacopo Mondi]

Add small is_txb() macro to the existing is_txa() and use it where
appropriate.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 2 +-
 drivers/media/i2c/adv748x/adv748x.h      | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 6ce21542ed48..b6b5d8c7ea7c 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -82,7 +82,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
 		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_txb(tx) && is_afe_enabled(state))
 		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
 						  &state->afe.sd,
 						  ADV748X_AFE_SOURCE);
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index b482c7fe6957..ab0c84adbea9 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -89,8 +89,11 @@ struct adv748x_csi2 {
 
 #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
 #define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd)
+
 #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
 #define is_txa(_tx) ((_tx) == &(_tx)->state->txa)
+#define is_txb(_tx) ((_tx) == &(_tx)->state->txb)
+
 #define is_afe_enabled(_state)					\
 	((_state)->endpoints[ADV748X_PORT_AIN0] != NULL ||	\
 	 (_state)->endpoints[ADV748X_PORT_AIN1] != NULL ||	\
-- 
2.20.1

[PATCH v3 2/6] media: adv748x: Rename reset procedures

[Jacopo Mondi]

Rename the chip reset procedure as they configure the CP (HDMI) and SD
(AFE) cores.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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..ad4e6424753a 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 with RGB888 format. */
+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 with YUV8 format. */
+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.20.1

[PATCH v3 3/6] 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.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 44 +++++++++++++-----------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index b6b5d8c7ea7c..8c3714495e11 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
+ * @enable: Link enabled flag
  *
  * 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,
+				      bool enable)
 {
-	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);
+				     enable ? MEDIA_LNK_FL_ENABLED : 0);
 }
 
 /* -----------------------------------------------------------------------------
@@ -68,25 +63,32 @@ 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 AFE and HDMI, 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 is the AFE->TXB one.
 	 */
-	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_txb(tx) && is_afe_enabled(state))
-		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
-						  &state->afe.sd,
-						  ADV748X_AFE_SOURCE);
-	return 0;
+	if (is_afe_enabled(state)) {
+		ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
+						 &state->afe.sd,
+						 ADV748X_AFE_SOURCE,
+						 is_txb(tx));
+		if (ret)
+			return ret;
+	}
+
+	/* Register link to HDMI for TXA only. */
+	if (is_txb(tx) || !is_hdmi_enabled(state))
+		return 0;
+
+	return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
+					  ADV748X_HDMI_SOURCE, true);
 }
 
 static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
-- 
2.20.1

[PATCH v3 4/6] 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.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-core.c |  2 +-
 drivers/media/i2c/adv748x/adv748x-csi2.c | 13 ++++++++++---
 drivers/media/i2c/adv748x/adv748x.h      |  1 +
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index ad4e6424753a..200e00f93546 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->src == &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 8c3714495e11..353b6b9bf6a7 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -46,9 +46,16 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
 			return ret;
 	}
 
-	return media_create_pad_link(&src->entity, src_pad,
-				     &tx->sd.entity, ADV748X_CSI2_SINK,
-				     enable ? MEDIA_LNK_FL_ENABLED : 0);
+	ret = media_create_pad_link(&src->entity, src_pad,
+				    &tx->sd.entity, ADV748X_CSI2_SINK,
+				    enable ? MEDIA_LNK_FL_ENABLED : 0);
+	if (ret)
+		return ret;
+
+	if (enable)
+		tx->src = src;
+
+	return 0;
 }
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index ab0c84adbea9..d22270f5e2c1 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -84,6 +84,7 @@ struct adv748x_csi2 {
 	struct media_pad pads[ADV748X_CSI2_NR_PADS];
 	struct v4l2_ctrl_handler ctrl_hdl;
 	struct v4l2_ctrl *pixel_rate;
+	struct v4l2_subdev *src;
 	struct v4l2_subdev sd;
 };
 
-- 
2.20.1

[PATCH v3 5/6] 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.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-afe.c  |  2 +-
 drivers/media/i2c/adv748x/adv748x-csi2.c | 15 +++++++++++++--
 drivers/media/i2c/adv748x/adv748x-hdmi.c |  2 +-
 drivers/media/i2c/adv748x/adv748x.h      |  4 ++++
 4 files changed, 19 insertions(+), 4 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 353b6b9bf6a7..2091cda50935 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -88,14 +88,25 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
 						 is_txb(tx));
 		if (ret)
 			return ret;
+
+		/* TXB can output AFE signals only. */
+		if (is_txb(tx))
+			state->afe.tx = tx;
 	}
 
 	/* Register link to HDMI for TXA only. */
 	if (is_txb(tx) || !is_hdmi_enabled(state))
 		return 0;
 
-	return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
-					  ADV748X_HDMI_SOURCE, true);
+	ret = adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
+					 ADV748X_HDMI_SOURCE, true);
+	if (ret)
+		return ret;
+
+	/* The default HDMI output is TXA. */
+	state->hdmi.tx = tx;
+
+	return 0;
 }
 
 static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
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 d22270f5e2c1..934a9d9a75c8 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -121,6 +121,8 @@ struct adv748x_hdmi {
 	struct v4l2_dv_timings timings;
 	struct v4l2_fract aspect_ratio;
 
+	struct adv748x_csi2 *tx;
+
 	struct {
 		u8 edid[512];
 		u32 present;
@@ -151,6 +153,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.20.1

[PATCH v3 6/6] media: adv748x: Implement TX 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 | 48 +++++++++++++++++++++++-
 drivers/media/i2c/adv748x/adv748x.h      |  2 +
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 200e00f93546..ea7e5ca48f1a 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -335,6 +335,51 @@ 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 = adv748x_sd_to_csi2(sd);
+	bool enable = flags & MEDIA_LNK_FL_ENABLED;
+	u8 io10_mask = ADV748X_IO_10_CSI1_EN |
+		       ADV748X_IO_10_CSI4_EN |
+		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
+	u8 io10 = 0;
+
+	/* Refuse to enable multiple links to the same TX at the same time. */
+	if (enable && tx->src)
+		return -EINVAL;
+
+	/* Set or clear the source (HDMI or AFE) and the current TX. */
+	if (rsd == &state->afe.sd)
+		state->afe.tx = enable ? tx : NULL;
+	else
+		state->hdmi.tx = enable ? tx : NULL;
+
+	tx->src = enable ? rsd : NULL;
+
+	if (state->afe.tx) {
+		/* AFE Requires TXA enabled, even when output to TXB */
+		io10 |= ADV748X_IO_10_CSI4_EN;
+		if (is_txa(tx))
+			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
+		else
+			io10 |= ADV748X_IO_10_CSI1_EN;
+	}
+
+	if (state->hdmi.tx)
+		io10 |= ADV748X_IO_10_CSI4_EN;
+
+	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
+}
+
+static const struct media_entity_operations adv748x_tx_media_ops = {
+	.link_setup	= adv748x_link_setup,
+	.link_validate	= v4l2_subdev_link_validate,
+};
 
 static const struct media_entity_operations adv748x_media_ops = {
 	.link_validate = v4l2_subdev_link_validate,
@@ -516,7 +561,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, struct adv748x_state *state,
 		state->client->addr, ident);
 
 	sd->entity.function = function;
-	sd->entity.ops = &adv748x_media_ops;
+	sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ?
+			 &adv748x_tx_media_ops : &adv748x_media_ops;
 }
 
 static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 934a9d9a75c8..b00c1995efb0 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -94,6 +94,7 @@ struct adv748x_csi2 {
 #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
 #define is_txa(_tx) ((_tx) == &(_tx)->state->txa)
 #define is_txb(_tx) ((_tx) == &(_tx)->state->txb)
+#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx))
 
 #define is_afe_enabled(_state)					\
 	((_state)->endpoints[ADV748X_PORT_AIN0] != NULL ||	\
@@ -223,6 +224,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	BIT(3)
 
 #define ADV748X_IO_CHIP_REV_ID_1	0xdf
 #define ADV748X_IO_CHIP_REV_ID_2	0xe0
-- 
2.20.1

Re: [PATCH v3 6/6] media: adv748x: Implement TX link_setup callback

[Kieran Bingham]

Hi Jacopo,

Thank you for the update,

On 10/01/2019 14:02, 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.

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

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 48 +++++++++++++++++++++++-
>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
>  2 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 200e00f93546..ea7e5ca48f1a 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -335,6 +335,51 @@ 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 = adv748x_sd_to_csi2(sd);
> +	bool enable = flags & MEDIA_LNK_FL_ENABLED;
> +	u8 io10_mask = ADV748X_IO_10_CSI1_EN |
> +		       ADV748X_IO_10_CSI4_EN |
> +		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +	u8 io10 = 0;
> +
> +	/* Refuse to enable multiple links to the same TX at the same time. */
> +	if (enable && tx->src)
> +		return -EINVAL;
> +
> +	/* Set or clear the source (HDMI or AFE) and the current TX. */
> +	if (rsd == &state->afe.sd)
> +		state->afe.tx = enable ? tx : NULL;
> +	else
> +		state->hdmi.tx = enable ? tx : NULL;
> +
> +	tx->src = enable ? rsd : NULL;
> +
> +	if (state->afe.tx) {
> +		/* AFE Requires TXA enabled, even when output to TXB */
> +		io10 |= ADV748X_IO_10_CSI4_EN;
> +		if (is_txa(tx))
> +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +		else
> +			io10 |= ADV748X_IO_10_CSI1_EN;
> +	}
> +
> +	if (state->hdmi.tx)
> +		io10 |= ADV748X_IO_10_CSI4_EN;
> +
> +	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
> +}
> +
> +static const struct media_entity_operations adv748x_tx_media_ops = {
> +	.link_setup	= adv748x_link_setup,
> +	.link_validate	= v4l2_subdev_link_validate,
> +};
>  
>  static const struct media_entity_operations adv748x_media_ops = {
>  	.link_validate = v4l2_subdev_link_validate,
> @@ -516,7 +561,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, struct adv748x_state *state,
>  		state->client->addr, ident);
>  
>  	sd->entity.function = function;
> -	sd->entity.ops = &adv748x_media_ops;
> +	sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ?
> +			 &adv748x_tx_media_ops : &adv748x_media_ops;
>  }
>  
>  static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 934a9d9a75c8..b00c1995efb0 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -94,6 +94,7 @@ struct adv748x_csi2 {
>  #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
>  #define is_txa(_tx) ((_tx) == &(_tx)->state->txa)
>  #define is_txb(_tx) ((_tx) == &(_tx)->state->txb)
> +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx))
>  
>  #define is_afe_enabled(_state)					\
>  	((_state)->endpoints[ADV748X_PORT_AIN0] != NULL ||	\
> @@ -223,6 +224,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	BIT(3)
>  
>  #define ADV748X_IO_CHIP_REV_ID_1	0xdf
>  #define ADV748X_IO_CHIP_REV_ID_2	0xe0
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 0/6] media: adv748x: Implement dynamic routing support

[Kieran Bingham]

Hi Jacopo,

for this series:

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

For anyone else who wants to use this series, please remember that if
you reset links on the media controller you will now have to set up the
links between the HDMI/AFE and TXA/TXB with this series in place.

The driver will default to the current configuration at startup.

--
Kieran


On 10/01/2019 14:02, Jacopo Mondi wrote:
> Hello,
>    third iteration for adv748x dynamic selection of video source.
> 
> Please refer to v1 and v2 cover letter for more details:
> https://patchwork.kernel.org/cover/10723971/
> https://patchwork.kernel.org/cover/10749535/
> 
> This version includes comments from Laurent and Kieran, on patches 1/6 and
> mostly on 6/6, which resulted in a better implementation. Thanks.
> 
> I have tested this on Ebisu E3 where I can capture HDMI and CVBS inputs from
> TXA, and on Salvator-x M3-W where I can capture HDMI from TXA, and CVBS from
> both TXA and TXB, but only when setting the number of CSI-2 data lanes to 2 in
> DTS.
> 
> It remains unresolved how to dynamically negotiate the number of CSI-2 data
> lanes to use between the video source and the CSI-2 receiver, which prevents
> using AFE->TXA routing on all Gen3 SoC which use 4 CSI-2 data lanes.
> 
> 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
> 
> Branch available for testing at:
> git://jmondi.org/linux adv748x_dynamic-routing_v3
> 
> Thanks
>   j
> 
> Jacopo Mondi (6):
>   media: adv748x: Add is_txb()
>   media: adv748x: Rename reset procedures
>   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 TX link_setup callback
> 
>  drivers/media/i2c/adv748x/adv748x-afe.c  |  2 +-
>  drivers/media/i2c/adv748x/adv748x-core.c | 74 +++++++++++++++++++-----
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 64 +++++++++++++-------
>  drivers/media/i2c/adv748x/adv748x-hdmi.c |  2 +-
>  drivers/media/i2c/adv748x/adv748x.h      | 10 ++++
>  5 files changed, 112 insertions(+), 40 deletions(-)
> 
> --
> 2.20.1
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 1/6] media: adv748x: Add is_txb()

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2019-01-10 15:02:08 +0100, Jacopo Mondi wrote:
> Add small is_txb() macro to the existing is_txa() and use it where
> appropriate.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 2 +-
>  drivers/media/i2c/adv748x/adv748x.h      | 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 6ce21542ed48..b6b5d8c7ea7c 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -82,7 +82,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
>  		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_txb(tx) && is_afe_enabled(state))
>  		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
>  						  &state->afe.sd,
>  						  ADV748X_AFE_SOURCE);
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index b482c7fe6957..ab0c84adbea9 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -89,8 +89,11 @@ struct adv748x_csi2 {
>  
>  #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
>  #define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd)
> +
>  #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
>  #define is_txa(_tx) ((_tx) == &(_tx)->state->txa)
> +#define is_txb(_tx) ((_tx) == &(_tx)->state->txb)
> +
>  #define is_afe_enabled(_state)					\
>  	((_state)->endpoints[ADV748X_PORT_AIN0] != NULL ||	\
>  	 (_state)->endpoints[ADV748X_PORT_AIN1] != NULL ||	\
> -- 
> 2.20.1
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 2/6] media: adv748x: Rename reset procedures

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2019-01-10 15:02:09 +0100, Jacopo Mondi wrote:
> Rename the chip reset procedure as they configure the CP (HDMI) and SD
> (AFE) cores.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

> ---
>  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..ad4e6424753a 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 with RGB888 format. */
> +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 with YUV8 format. */
> +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.20.1
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 3/6] media: adv748x: csi2: Link AFE with TXA and TXB

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2019-01-10 15:02:10 +0100, 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.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 44 +++++++++++++-----------
>  1 file changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index b6b5d8c7ea7c..8c3714495e11 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
> + * @enable: Link enabled flag
>   *
>   * 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,
> +				      bool enable)
>  {
> -	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);
> +				     enable ? MEDIA_LNK_FL_ENABLED : 0);
>  }
>  
>  /* -----------------------------------------------------------------------------
> @@ -68,25 +63,32 @@ 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 AFE and HDMI, 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 is the AFE->TXB one.
>  	 */
> -	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_txb(tx) && is_afe_enabled(state))
> -		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> -						  &state->afe.sd,
> -						  ADV748X_AFE_SOURCE);
> -	return 0;
> +	if (is_afe_enabled(state)) {
> +		ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> +						 &state->afe.sd,
> +						 ADV748X_AFE_SOURCE,
> +						 is_txb(tx));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Register link to HDMI for TXA only. */
> +	if (is_txb(tx) || !is_hdmi_enabled(state))

Small nit, I would s/is_txb(tx)/!is_txa(tx)/ here as to me it becomes 
easier to read. With or without this change,

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

> +		return 0;
> +
> +	return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
> +					  ADV748X_HDMI_SOURCE, true);
>  }
>  
>  static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
> -- 
> 2.20.1
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 3/6] media: adv748x: csi2: Link AFE with TXA and TXB

[Jacopo Mondi]

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

Hi Niklas,

On Mon, Jan 14, 2019 at 03:55:33PM +0100, Niklas Söderlund wrote:

[snip]

> > +	/* Register link to HDMI for TXA only. */
> > +	if (is_txb(tx) || !is_hdmi_enabled(state))
>
> Small nit, I would s/is_txb(tx)/!is_txa(tx)/ here as to me it becomes
> easier to read. With or without this change,
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>

Would you want me to resend for this or can this series be collected?

Thanks
  j

> > +		return 0;
> > +
> > +	return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
> > +					  ADV748X_HDMI_SOURCE, true);
> >  }
> >
> >  static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
> > --
> > 2.20.1
> >
>
> --
> Regards,
> Niklas Söderlund

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

Re: [PATCH v3 3/6] media: adv748x: csi2: Link AFE with TXA and TXB

[Niklas Söderlund]

Hi Jacopo,

On 2019-01-16 10:10:49 +0100, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Mon, Jan 14, 2019 at 03:55:33PM +0100, Niklas Söderlund wrote:
> 
> [snip]
> 
> > > +	/* Register link to HDMI for TXA only. */
> > > +	if (is_txb(tx) || !is_hdmi_enabled(state))
> >
> > Small nit, I would s/is_txb(tx)/!is_txa(tx)/ here as to me it becomes
> > easier to read. With or without this change,
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >
> 
> Would you want me to resend for this or can this series be collected?

I plan to resume review of the last patches in this series today. After 
that I leave it to you and Kieran to discuss whether or not this needs a 
resend :-)

> 
> Thanks
>   j
> 
> > > +		return 0;
> > > +
> > > +	return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
> > > +					  ADV748X_HDMI_SOURCE, true);
> > >  }
> > >
> > >  static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
> > > --
> > > 2.20.1
> > >
> >
> > --
> > Regards,
> > Niklas Söderlund



-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 4/6] media: adv748x: Store the source subdevice in TX

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2019-01-10 15:02:11 +0100, 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.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

> ---
>  drivers/media/i2c/adv748x/adv748x-core.c |  2 +-
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 13 ++++++++++---
>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index ad4e6424753a..200e00f93546 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->src == &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 8c3714495e11..353b6b9bf6a7 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -46,9 +46,16 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
>  			return ret;
>  	}
>  
> -	return media_create_pad_link(&src->entity, src_pad,
> -				     &tx->sd.entity, ADV748X_CSI2_SINK,
> -				     enable ? MEDIA_LNK_FL_ENABLED : 0);
> +	ret = media_create_pad_link(&src->entity, src_pad,
> +				    &tx->sd.entity, ADV748X_CSI2_SINK,
> +				    enable ? MEDIA_LNK_FL_ENABLED : 0);
> +	if (ret)
> +		return ret;
> +
> +	if (enable)
> +		tx->src = src;
> +
> +	return 0;
>  }
>  
>  /* -----------------------------------------------------------------------------
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index ab0c84adbea9..d22270f5e2c1 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -84,6 +84,7 @@ struct adv748x_csi2 {
>  	struct media_pad pads[ADV748X_CSI2_NR_PADS];
>  	struct v4l2_ctrl_handler ctrl_hdl;
>  	struct v4l2_ctrl *pixel_rate;
> +	struct v4l2_subdev *src;
>  	struct v4l2_subdev sd;
>  };
>  
> -- 
> 2.20.1
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 3/6] media: adv748x: csi2: Link AFE with TXA and TXB

[Niklas Söderlund]

Hi (again) Jacopo,

I found something else in this patch unfortunately :-(

On 2019-01-14 15:55:33 +0100, Niklas Söderlund wrote:
> Hi Jacopo,
> 
> Thanks for your patch.
> 
> On 2019-01-10 15:02:10 +0100, 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.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-csi2.c | 44 +++++++++++++-----------
> >  1 file changed, 23 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index b6b5d8c7ea7c..8c3714495e11 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
> > + * @enable: Link enabled flag
> >   *
> >   * 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,
> > +				      bool enable)
> >  {
> > -	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);
> > +				     enable ? MEDIA_LNK_FL_ENABLED : 0);
> >  }
> >  
> >  /* -----------------------------------------------------------------------------
> > @@ -68,25 +63,32 @@ 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 AFE and HDMI, 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 is the AFE->TXB one.
> >  	 */
> > -	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_txb(tx) && is_afe_enabled(state))
> > -		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> > -						  &state->afe.sd,
> > -						  ADV748X_AFE_SOURCE);
> > -	return 0;
> > +	if (is_afe_enabled(state)) {
> > +		ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> > +						 &state->afe.sd,
> > +						 ADV748X_AFE_SOURCE,
> > +						 is_txb(tx));
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	/* Register link to HDMI for TXA only. */
> > +	if (is_txb(tx) || !is_hdmi_enabled(state))
> 
> Small nit, I would s/is_txb(tx)/!is_txa(tx)/ here as to me it becomes 
> easier to read. With or without this change,
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> > +		return 0;
> > +
> > +	return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
> > +					  ADV748X_HDMI_SOURCE, true);

If the call to adv748x_csi2_register_link() fails should not the 
(possible) link to the AFE be removed?

> >  }
> >  
> >  static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
> > -- 
> > 2.20.1
> > 
> 
> -- 
> Regards,
> Niklas Söderlund

-- 
Regards,
Niklas Söderlund

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

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2019-01-10 15:02:12 +0100, 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.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

> ---
>  drivers/media/i2c/adv748x/adv748x-afe.c  |  2 +-
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 15 +++++++++++++--
>  drivers/media/i2c/adv748x/adv748x-hdmi.c |  2 +-
>  drivers/media/i2c/adv748x/adv748x.h      |  4 ++++
>  4 files changed, 19 insertions(+), 4 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 353b6b9bf6a7..2091cda50935 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -88,14 +88,25 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
>  						 is_txb(tx));
>  		if (ret)
>  			return ret;
> +
> +		/* TXB can output AFE signals only. */
> +		if (is_txb(tx))
> +			state->afe.tx = tx;
>  	}
>  
>  	/* Register link to HDMI for TXA only. */
>  	if (is_txb(tx) || !is_hdmi_enabled(state))
>  		return 0;
>  
> -	return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
> -					  ADV748X_HDMI_SOURCE, true);
> +	ret = adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
> +					 ADV748X_HDMI_SOURCE, true);
> +	if (ret)
> +		return ret;
> +
> +	/* The default HDMI output is TXA. */
> +	state->hdmi.tx = tx;
> +
> +	return 0;
>  }
>  
>  static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
> 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 d22270f5e2c1..934a9d9a75c8 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -121,6 +121,8 @@ struct adv748x_hdmi {
>  	struct v4l2_dv_timings timings;
>  	struct v4l2_fract aspect_ratio;
>  
> +	struct adv748x_csi2 *tx;
> +
>  	struct {
>  		u8 edid[512];
>  		u32 present;
> @@ -151,6 +153,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.20.1
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 6/6] media: adv748x: Implement TX link_setup callback

[Niklas Söderlund]

Hi Jacopo,

Thanks for your effort.

On 2019-01-10 15:02:13 +0100, 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>

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

> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 48 +++++++++++++++++++++++-
>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
>  2 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 200e00f93546..ea7e5ca48f1a 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -335,6 +335,51 @@ 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 = adv748x_sd_to_csi2(sd);
> +	bool enable = flags & MEDIA_LNK_FL_ENABLED;
> +	u8 io10_mask = ADV748X_IO_10_CSI1_EN |
> +		       ADV748X_IO_10_CSI4_EN |
> +		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +	u8 io10 = 0;
> +
> +	/* Refuse to enable multiple links to the same TX at the same time. */
> +	if (enable && tx->src)
> +		return -EINVAL;
> +
> +	/* Set or clear the source (HDMI or AFE) and the current TX. */
> +	if (rsd == &state->afe.sd)
> +		state->afe.tx = enable ? tx : NULL;
> +	else
> +		state->hdmi.tx = enable ? tx : NULL;
> +
> +	tx->src = enable ? rsd : NULL;
> +
> +	if (state->afe.tx) {
> +		/* AFE Requires TXA enabled, even when output to TXB */
> +		io10 |= ADV748X_IO_10_CSI4_EN;
> +		if (is_txa(tx))
> +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +		else
> +			io10 |= ADV748X_IO_10_CSI1_EN;
> +	}
> +
> +	if (state->hdmi.tx)
> +		io10 |= ADV748X_IO_10_CSI4_EN;
> +
> +	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
> +}
> +
> +static const struct media_entity_operations adv748x_tx_media_ops = {
> +	.link_setup	= adv748x_link_setup,
> +	.link_validate	= v4l2_subdev_link_validate,
> +};
>  
>  static const struct media_entity_operations adv748x_media_ops = {
>  	.link_validate = v4l2_subdev_link_validate,
> @@ -516,7 +561,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, struct adv748x_state *state,
>  		state->client->addr, ident);
>  
>  	sd->entity.function = function;
> -	sd->entity.ops = &adv748x_media_ops;
> +	sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ?
> +			 &adv748x_tx_media_ops : &adv748x_media_ops;
>  }
>  
>  static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 934a9d9a75c8..b00c1995efb0 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -94,6 +94,7 @@ struct adv748x_csi2 {
>  #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
>  #define is_txa(_tx) ((_tx) == &(_tx)->state->txa)
>  #define is_txb(_tx) ((_tx) == &(_tx)->state->txb)
> +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx))
>  
>  #define is_afe_enabled(_state)					\
>  	((_state)->endpoints[ADV748X_PORT_AIN0] != NULL ||	\
> @@ -223,6 +224,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	BIT(3)
>  
>  #define ADV748X_IO_CHIP_REV_ID_1	0xdf
>  #define ADV748X_IO_CHIP_REV_ID_2	0xe0
> -- 
> 2.20.1
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 3/6] media: adv748x: csi2: Link AFE with TXA and TXB

[Jacopo Mondi]

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

Hi Niklas,
   sorry for replying late

On Wed, Jan 16, 2019 at 02:44:25PM +0100, Niklas Söderlund wrote:
> Hi (again) Jacopo,
>
> I found something else in this patch unfortunately :-(
>
> On 2019-01-14 15:55:33 +0100, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your patch.
> >
> > On 2019-01-10 15:02:10 +0100, 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.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/media/i2c/adv748x/adv748x-csi2.c | 44 +++++++++++++-----------
> > >  1 file changed, 23 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > index b6b5d8c7ea7c..8c3714495e11 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
> > > + * @enable: Link enabled flag
> > >   *
> > >   * 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,
> > > +				      bool enable)
> > >  {
> > > -	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);
> > > +				     enable ? MEDIA_LNK_FL_ENABLED : 0);
> > >  }
> > >
> > >  /* -----------------------------------------------------------------------------
> > > @@ -68,25 +63,32 @@ 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 AFE and HDMI, 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 is the AFE->TXB one.
> > >  	 */
> > > -	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_txb(tx) && is_afe_enabled(state))
> > > -		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> > > -						  &state->afe.sd,
> > > -						  ADV748X_AFE_SOURCE);
> > > -	return 0;
> > > +	if (is_afe_enabled(state)) {
> > > +		ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> > > +						 &state->afe.sd,
> > > +						 ADV748X_AFE_SOURCE,
> > > +						 is_txb(tx));
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	/* Register link to HDMI for TXA only. */
> > > +	if (is_txb(tx) || !is_hdmi_enabled(state))
> >
> > Small nit, I would s/is_txb(tx)/!is_txa(tx)/ here as to me it becomes
> > easier to read. With or without this change,
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > > +		return 0;
> > > +
> > > +	return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
> > > +					  ADV748X_HDMI_SOURCE, true);
>
> If the call to adv748x_csi2_register_link() fails should not the
> (possible) link to the AFE be removed?
>

The .register() callback is called from v4l2-device.c in
v4l2_device_register_subdev(). If the callback returns an error, the
subdev gets not registered at all and the media entity cleaned up, so it
won't appear in the media graph.

I think we're safe and the patch series is good to go. What's your
opinion?

Thanks
  j


> > >  }
> > >
> > >  static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
> > > --
> > > 2.20.1
> > >
> >
> > --
> > Regards,
> > Niklas Söderlund
>
> --
> Regards,
> Niklas Söderlund

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

Re: [PATCH v3 3/6] media: adv748x: csi2: Link AFE with TXA and TXB

[Niklas Söderlund]

On 2019-01-28 15:47:37 +0100, Jacopo Mondi wrote:
> Hi Niklas,
>    sorry for replying late
> 
> On Wed, Jan 16, 2019 at 02:44:25PM +0100, Niklas Söderlund wrote:
> > Hi (again) Jacopo,
> >
> > I found something else in this patch unfortunately :-(
> >
> > On 2019-01-14 15:55:33 +0100, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > Thanks for your patch.
> > >
> > > On 2019-01-10 15:02:10 +0100, 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.
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  drivers/media/i2c/adv748x/adv748x-csi2.c | 44 +++++++++++++-----------
> > > >  1 file changed, 23 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > index b6b5d8c7ea7c..8c3714495e11 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
> > > > + * @enable: Link enabled flag
> > > >   *
> > > >   * 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,
> > > > +				      bool enable)
> > > >  {
> > > > -	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);
> > > > +				     enable ? MEDIA_LNK_FL_ENABLED : 0);
> > > >  }
> > > >
> > > >  /* -----------------------------------------------------------------------------
> > > > @@ -68,25 +63,32 @@ 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 AFE and HDMI, 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 is the AFE->TXB one.
> > > >  	 */
> > > > -	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_txb(tx) && is_afe_enabled(state))
> > > > -		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> > > > -						  &state->afe.sd,
> > > > -						  ADV748X_AFE_SOURCE);
> > > > -	return 0;
> > > > +	if (is_afe_enabled(state)) {
> > > > +		ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> > > > +						 &state->afe.sd,
> > > > +						 ADV748X_AFE_SOURCE,
> > > > +						 is_txb(tx));
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	/* Register link to HDMI for TXA only. */
> > > > +	if (is_txb(tx) || !is_hdmi_enabled(state))
> > >
> > > Small nit, I would s/is_txb(tx)/!is_txa(tx)/ here as to me it becomes
> > > easier to read. With or without this change,
> > >
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >
> > > > +		return 0;
> > > > +
> > > > +	return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
> > > > +					  ADV748X_HDMI_SOURCE, true);
> >
> > If the call to adv748x_csi2_register_link() fails should not the
> > (possible) link to the AFE be removed?
> >
> 
> The .register() callback is called from v4l2-device.c in
> v4l2_device_register_subdev(). If the callback returns an error, the
> subdev gets not registered at all and the media entity cleaned up, so it
> won't appear in the media graph.
> 
> I think we're safe and the patch series is good to go. What's your
> opinion?

As long as no stray links are left in the graph I'm happy :-)

> 
> Thanks
>   j
> 
> 
> > > >  }
> > > >
> > > >  static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
> > > > --
> > > > 2.20.1
> > > >
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
> >
> > --
> > Regards,
> > Niklas Söderlund



-- 
Regards,
Niklas Söderlund