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

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

[Jacopo Mondi]

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

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

This v2 addresses comments received from Kieran and Laurent, with the most
notable changes in patches 3/6 and 6/6.

Tested on R-Car E3 Ebisu board, where both AFE and HDMI backends share a
single CSI-2 output.

A noted on v1 cover letter, for the series to work on other R-Car SoC the
number of CSI-2 data lanes has to be negotiated, as the AFE->TXA link works
only when up to 2 data lanes are used, while most of Gen 3 SoCs (E3 excluded)
are configured to use 4 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_v2

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 | 83 +++++++++++++++++++-----
 drivers/media/i2c/adv748x/adv748x-csi2.c | 65 ++++++++++++-------
 drivers/media/i2c/adv748x/adv748x-hdmi.c |  2 +-
 drivers/media/i2c/adv748x/adv748x.h      | 13 +++-
 5 files changed, 124 insertions(+), 41 deletions(-)

--
2.20.1

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

[Jacopo Mondi]

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

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

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..bc2da1b5ce29 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -89,8 +89,12 @@ 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_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
+#define is_txa(_tx) __is_tx(_tx, a)
+#define is_txb(_tx) __is_tx(_tx, b)
+
 #define is_afe_enabled(_state)					\
 	((_state)->endpoints[ADV748X_PORT_AIN0] != NULL ||	\
 	 (_state)->endpoints[ADV748X_PORT_AIN1] != NULL ||	\
-- 
2.20.1

[PATCH v2 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: 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 v2 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.

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..9d391d6f752e 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 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_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 v2 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: 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 9d391d6f752e..de3944615e64 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 bc2da1b5ce29..d8d94053301b 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 v2 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: 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 de3944615e64..c835f6379337 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 d8d94053301b..6eb2e4a95eed 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -122,6 +122,8 @@ struct adv748x_hdmi {
 	struct v4l2_dv_timings timings;
 	struct v4l2_fract aspect_ratio;
 
+	struct adv748x_csi2 *tx;
+
 	struct {
 		u8 edid[512];
 		u32 present;
@@ -152,6 +154,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 v2 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 | 57 +++++++++++++++++++++++-
 drivers/media/i2c/adv748x/adv748x.h      |  2 +
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 200e00f93546..a586bf393558 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -335,6 +335,60 @@ 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;
+
+	/* 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 (!enable)
+		return 0;
+
+	/* Change video stream routing, according to the newly enabled link. */
+	io10 = io_read(state, ADV748X_IO_10);
+	if (rsd == &state->afe.sd) {
+		/*
+		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
+		 * if AFE goes to TXB, we need both TXA and TXB powered on.
+		 */
+		io10 &= ~ADV748X_IO_10_CSI1_EN;
+		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
+		if (is_txa(tx))
+			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
+		else
+			io10 |= ADV748X_IO_10_CSI4_EN |
+				ADV748X_IO_10_CSI1_EN;
+	} else {
+		/* Clear AFE->TXA routing and power up TXA. */
+		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
+		io10 |= ADV748X_IO_10_CSI4_EN;
+	}
+	io_write(state, ADV748X_IO_10, io10);
+
+	return 0;
+}
+
+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 +570,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 6eb2e4a95eed..eb19c6cbbb4e 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -93,6 +93,7 @@ struct adv748x_csi2 {
 
 #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
 #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
+#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx))
 #define is_txa(_tx) __is_tx(_tx, a)
 #define is_txb(_tx) __is_tx(_tx, b)
 
@@ -224,6 +225,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 v2 1/6] media: adv748x: Add is_txb()

[Kieran Bingham]

Hi Jacopo

On 06/01/2019 15:54, Jacopo Mondi wrote:
> Add small is_txb() macro to the existing is_txa() and use it where
> appropriate.

Thank you.

I think this will make the code much better to read than if (!is_txa).

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

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


> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 2 +-
>  drivers/media/i2c/adv748x/adv748x.h      | 6 +++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> 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..bc2da1b5ce29 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -89,8 +89,12 @@ 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_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
> +#define is_txa(_tx) __is_tx(_tx, a)
> +#define is_txb(_tx) __is_tx(_tx, b)

I would have just duplicated the is_txa() line here... but this is good
too :)


> +
>  #define is_afe_enabled(_state)					\
>  	((_state)->endpoints[ADV748X_PORT_AIN0] != NULL ||	\
>  	 (_state)->endpoints[ADV748X_PORT_AIN1] != NULL ||	\
> 

-- 
Regards
--
Kieran

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

[Kieran Bingham]

Hi Jacopo,

On 06/01/2019 15:54, 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>
> ---
>  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);
>  
> 

-- 
Regards
--
Kieran

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

[Jacopo Mondi]

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

Hi Kieran,

On Mon, Jan 07, 2019 at 09:49:25AM +0000, Kieran Bingham wrote:
> Hi Jacopo
>
> On 06/01/2019 15:54, Jacopo Mondi wrote:
> > Add small is_txb() macro to the existing is_txa() and use it where
> > appropriate.
>
> Thank you.
>
> I think this will make the code much better to read than if (!is_txa).
>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-csi2.c | 2 +-
> >  drivers/media/i2c/adv748x/adv748x.h      | 6 +++++-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > 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..bc2da1b5ce29 100644
> > --- a/drivers/media/i2c/adv748x/adv748x.h
> > +++ b/drivers/media/i2c/adv748x/adv748x.h
> > @@ -89,8 +89,12 @@ 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_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
> > +#define is_txa(_tx) __is_tx(_tx, a)
> > +#define is_txb(_tx) __is_tx(_tx, b)
>
> I would have just duplicated the is_txa() line here... but this is good
> too :)

I agree it might seem more complex than necessary. I initially made it
like this as I started from the 'is_tx()' macro this series adds in
6/6.

If it is easier to have an '((_tx) == &(_tx)->state->txb)' I can
change this.

Thanks
   j

>
>
> > +
> >  #define is_afe_enabled(_state)					\
> >  	((_state)->endpoints[ADV748X_PORT_AIN0] != NULL ||	\
> >  	 (_state)->endpoints[ADV748X_PORT_AIN1] != NULL ||	\
> >
>
> --
> Regards
> --
> Kieran

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

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

[Kieran Bingham]

Hi Jacopo,

On 06/01/2019 15:54, 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.

Great - this looks good to me.
Small nit below - but it's not a big deal.

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..9d391d6f752e 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 the AFE->TXB is.

I might have worded this "as is the AFE->TXB." but it's understandable.
I can update this when applying if you wish.


>  	 */
> -	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 = {
> 

-- 
Regards
--
Kieran

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

[Kieran Bingham]

Hi Jacopo,

On 07/01/2019 10:05, Jacopo Mondi wrote:
> Hi Kieran,

<snip>

>>> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
>>> index b482c7fe6957..bc2da1b5ce29 100644
>>> --- a/drivers/media/i2c/adv748x/adv748x.h
>>> +++ b/drivers/media/i2c/adv748x/adv748x.h
>>> @@ -89,8 +89,12 @@ 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_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
>>> +#define is_txa(_tx) __is_tx(_tx, a)
>>> +#define is_txb(_tx) __is_tx(_tx, b)
>>
>> I would have just duplicated the is_txa() line here... but this is good
>> too :)
> 
> I agree it might seem more complex than necessary. I initially made it
> like this as I started from the 'is_tx()' macro this series adds in
> 6/6.
> 
> If it is easier to have an '((_tx) == &(_tx)->state->txb)' I can
> change this.

It's fine for me as you've got it.

It's still clear and readable, and implements the required functionality.

<snip>

-- 
Regards
--
Kieran

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

[Kieran Bingham]

Hi Jacopo,

On 06/01/2019 15:54, 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>
> ---
>  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 9d391d6f752e..de3944615e64 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 bc2da1b5ce29..d8d94053301b 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;
>  };
>  
> 

-- 
Regards
--
Kieran

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

[Kieran Bingham]

Hi Jacopo,

On 06/01/2019 15:54, 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.
> 

Great - decouples the AFE/HDMI from a specific TX.

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 de3944615e64..c835f6379337 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 d8d94053301b..6eb2e4a95eed 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -122,6 +122,8 @@ struct adv748x_hdmi {
>  	struct v4l2_dv_timings timings;
>  	struct v4l2_fract aspect_ratio;
>  
> +	struct adv748x_csi2 *tx;
> +
>  	struct {
>  		u8 edid[512];
>  		u32 present;
> @@ -152,6 +154,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 v2 6/6] media: adv748x: Implement TX link_setup callback

[Kieran Bingham]

Hi Jacopo,

On 06/01/2019 15:54, 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.

Overall this looks like the right approach - but I feel like the
handling of the io10 register might need some consideration, because
it's value depends on the condition of both CSI2 transmitters, not just
the currently parsed link.

I had a go at some pseudo - uncompiled/untested code inline as a suggestion.

If you think it's better - feel free to rework it in ... or not as you
see fit.

Regards

Kieran




> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
>  2 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 200e00f93546..a586bf393558 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -335,6 +335,60 @@ 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;
> +
> +	/* 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 (!enable)
> +		return 0;

Don't we potentially want to take any action on disable to power down
links below ?

> +
> +	/* Change video stream routing, according to the newly enabled link. */
> +	io10 = io_read(state, ADV748X_IO_10);
> +	if (rsd == &state->afe.sd) {
> +		/*
> +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
> +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
> +		 */
> +		io10 &= ~ADV748X_IO_10_CSI1_EN;
> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +		if (is_txa(tx))
> +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;

Shouldn't the CSI4 be enabled here too? or are we assuming it's already
(/always) enabled?
		io10 |= ADV748X_IO_10_CSI4_EN;

> +		else
> +			io10 |= ADV748X_IO_10_CSI4_EN |
> +				ADV748X_IO_10_CSI1_EN;
> +	} else {
> +		/* Clear AFE->TXA routing and power up TXA. */
> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +		io10 |= ADV748X_IO_10_CSI4_EN;

But if we assume it's already enabled ... do we need this?
Perhaps it might be better to be explicit on this?

> +	}
> +	io_write(state, ADV748X_IO_10, io10);


Would it be any cleaner to use io_clrset() here?

Hrm ... also it feels like this register really should be set depending
upon the complete state of ... &state->...

So perhaps it deserves it's own function which should be called after
csi_registered() callback and any link change.

/me has a quick go at some psuedo codeishness...:

int adv74x_io_10(struct adv748x_state *state);
	u8 bits = 0;
	u8 mask = ADV748X_IO_10_CSI1_EN
		| ADV748X_IO_10_CSI4_EN
		| ADV748X_IO_10_CSI4_IN_SEL_AFE;

	if (state->afe.tx) {
		/* AFE Requires TXA enabled, even when output to TXB */
		bits |= ADV748X_IO_10_CSI4_EN;

		if (is_txa(state->afe.tx))
			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
		else
			bits |= ADV748X_IO_10_CSI1_EN;
	}

	if (state->hdmi.tx) {
		bits |= ADV748X_IO_10_CSI4_EN;
	}

	return io_clrset(state, ADV748X_IO_10, mask, bits);
}

How does that look ? (is it even correct first?)

> +
> +	return 0;
> +}
> +
> +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 +570,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;

Aha - yes that's a neat solution to ensure that only the TX links
generate link_setup calls :)



>  }
>  
>  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 6eb2e4a95eed..eb19c6cbbb4e 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -93,6 +93,7 @@ struct adv748x_csi2 {
>  
>  #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
>  #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
> +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx))
>  #define is_txa(_tx) __is_tx(_tx, a)
>  #define is_txb(_tx) __is_tx(_tx, b)
>  
> @@ -224,6 +225,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 v2 1/6] media: adv748x: Add is_txb()

[Laurent Pinchart]

Hello,

On Monday, 7 January 2019 12:38:39 EET Kieran Bingham wrote:
> On 07/01/2019 10:05, Jacopo Mondi wrote:
> > Hi Kieran,
> 
> <snip>
> 
> >>> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> >>> b/drivers/media/i2c/adv748x/adv748x.h index b482c7fe6957..bc2da1b5ce29
> >>> 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x.h
> >>> +++ b/drivers/media/i2c/adv748x/adv748x.h
> >>> @@ -89,8 +89,12 @@ 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_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
> >>> +#define is_txa(_tx) __is_tx(_tx, a)
> >>> +#define is_txb(_tx) __is_tx(_tx, b)
> >> 
> >> I would have just duplicated the is_txa() line here... but this is good
> >> too :)
> > 
> > I agree it might seem more complex than necessary. I initially made it
> > like this as I started from the 'is_tx()' macro this series adds in
> > 6/6.
> > 
> > If it is easier to have an '((_tx) == &(_tx)->state->txb)' I can
> > change this.

I would find it cleaner to write out is_txa and is_txb explicitly instead of 
hiding the implementation behind an __is_tx macro, especially given that we 
won't have to extend this in the future.

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

> It's fine for me as you've got it.
> 
> It's still clear and readable, and implements the required functionality.
> 
> <snip>

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sunday, 6 January 2019 17:54:10 EET 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 | 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..9d391d6f752e
> 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 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_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. */

I would have written "Create link", but then realized that the function is 
named adv748x_csi2_register_link(). Looking at its definition, I now see this 
means "register and link". That seems a bit of a hack, especially seeing how 
double registration is skipped in the function by checking src->v4l2_dev. 
Kieran, could this be fixed ?

> +	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 = {

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hello,

On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote:
> On 06/01/2019 15:54, 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.
> 
> Overall this looks like the right approach - but I feel like the
> handling of the io10 register might need some consideration, because
> it's value depends on the condition of both CSI2 transmitters, not just
> the currently parsed link.
> 
> I had a go at some pseudo - uncompiled/untested code inline as a suggestion.
> 
> If you think it's better - feel free to rework it in ... or not as you
> see fit.
> 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> > 
> >  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
> >  drivers/media/i2c/adv748x/adv748x.h      |  2 +
> >  2 files changed, 58 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > b/drivers/media/i2c/adv748x/adv748x-core.c index
> > 200e00f93546..a586bf393558 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -335,6 +335,60 @@ 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;
> > +
> > +	/* 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 (!enable)
> > +		return 0;
> 
> Don't we potentially want to take any action on disable to power down
> links below ?
> 
> > +
> > +	/* Change video stream routing, according to the newly enabled link. */
> > +	io10 = io_read(state, ADV748X_IO_10);
> > +	if (rsd == &state->afe.sd) {
> > +		/*
> > +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
> > +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
> > +		 */
> > +		io10 &= ~ADV748X_IO_10_CSI1_EN;
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +		if (is_txa(tx))
> > +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> 
> Shouldn't the CSI4 be enabled here too? or are we assuming it's already
> (/always) enabled?
> 		io10 |= ADV748X_IO_10_CSI4_EN;
> 
> > +		else
> > +			io10 |= ADV748X_IO_10_CSI4_EN |
> > +				ADV748X_IO_10_CSI1_EN;
> > +	} else {
> > +		/* Clear AFE->TXA routing and power up TXA. */
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +		io10 |= ADV748X_IO_10_CSI4_EN;
> 
> But if we assume it's already enabled ... do we need this?
> Perhaps it might be better to be explicit on this?
> 
> > +	}
> > +	io_write(state, ADV748X_IO_10, io10);
> 
> Would it be any cleaner to use io_clrset() here?
> 
> Hrm ... also it feels like this register really should be set depending
> upon the complete state of ... &state->...
> 
> So perhaps it deserves it's own function which should be called after
> csi_registered() callback and any link change.
> 
> /me has a quick go at some psuedo codeishness...:
> 
> int adv74x_io_10(struct adv748x_state *state);
> 	u8 bits = 0;
> 	u8 mask = ADV748X_IO_10_CSI1_EN
> 
> 		| ADV748X_IO_10_CSI4_EN
> 		| ADV748X_IO_10_CSI4_IN_SEL_AFE;
> 
> 	if (state->afe.tx) {
> 		/* AFE Requires TXA enabled, even when output to TXB */
> 		bits |= ADV748X_IO_10_CSI4_EN;
> 
> 		if (is_txa(state->afe.tx))
> 			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
> 		else
> 			bits |= ADV748X_IO_10_CSI1_EN;
> 	}
> 
> 	if (state->hdmi.tx) {
> 		bits |= ADV748X_IO_10_CSI4_EN;
> 	}
> 
> 	return io_clrset(state, ADV748X_IO_10, mask, bits);
> }
> 
> How does that look ? (is it even correct first?)
> 
> > +
> > +	return 0;
> > +}
> > +
> > +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 +570,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;
> 
> Aha - yes that's a neat solution to ensure that only the TX links
> generate link_setup calls :)

Another option would be to bail out from adv748x_link_setup() if the entity is 
not a TX*.

> >  }

[snip]

-- 
Regards,

Laurent Pinchart

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

[Kieran Bingham]

On 09/01/2019 00:15, Laurent Pinchart wrote:
> Hello,
> 
> On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote:
>> On 06/01/2019 15:54, 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.
>>
>> Overall this looks like the right approach - but I feel like the
>> handling of the io10 register might need some consideration, because
>> it's value depends on the condition of both CSI2 transmitters, not just
>> the currently parsed link.
>>
>> I had a go at some pseudo - uncompiled/untested code inline as a suggestion.
>>
>> If you think it's better - feel free to rework it in ... or not as you
>> see fit.
>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>
>>>  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
>>>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
>>>  2 files changed, 58 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
>>> b/drivers/media/i2c/adv748x/adv748x-core.c index
>>> 200e00f93546..a586bf393558 100644
>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
>>> @@ -335,6 +335,60 @@ 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;
>>> +
>>> +	/* 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 (!enable)
>>> +		return 0;
>>
>> Don't we potentially want to take any action on disable to power down
>> links below ?
>>
>>> +
>>> +	/* Change video stream routing, according to the newly enabled link. */
>>> +	io10 = io_read(state, ADV748X_IO_10);
>>> +	if (rsd == &state->afe.sd) {
>>> +		/*
>>> +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
>>> +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
>>> +		 */
>>> +		io10 &= ~ADV748X_IO_10_CSI1_EN;
>>> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>> +		if (is_txa(tx))
>>> +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>
>> Shouldn't the CSI4 be enabled here too? or are we assuming it's already
>> (/always) enabled?
>> 		io10 |= ADV748X_IO_10_CSI4_EN;
>>
>>> +		else
>>> +			io10 |= ADV748X_IO_10_CSI4_EN |
>>> +				ADV748X_IO_10_CSI1_EN;
>>> +	} else {
>>> +		/* Clear AFE->TXA routing and power up TXA. */
>>> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>> +		io10 |= ADV748X_IO_10_CSI4_EN;
>>
>> But if we assume it's already enabled ... do we need this?
>> Perhaps it might be better to be explicit on this?
>>
>>> +	}
>>> +	io_write(state, ADV748X_IO_10, io10);
>>
>> Would it be any cleaner to use io_clrset() here?
>>
>> Hrm ... also it feels like this register really should be set depending
>> upon the complete state of ... &state->...
>>
>> So perhaps it deserves it's own function which should be called after
>> csi_registered() callback and any link change.
>>
>> /me has a quick go at some psuedo codeishness...:
>>
>> int adv74x_io_10(struct adv748x_state *state);
>> 	u8 bits = 0;
>> 	u8 mask = ADV748X_IO_10_CSI1_EN
>>
>> 		| ADV748X_IO_10_CSI4_EN
>> 		| ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>
>> 	if (state->afe.tx) {
>> 		/* AFE Requires TXA enabled, even when output to TXB */
>> 		bits |= ADV748X_IO_10_CSI4_EN;
>>
>> 		if (is_txa(state->afe.tx))
>> 			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
>> 		else
>> 			bits |= ADV748X_IO_10_CSI1_EN;
>> 	}
>>
>> 	if (state->hdmi.tx) {
>> 		bits |= ADV748X_IO_10_CSI4_EN;
>> 	}
>>
>> 	return io_clrset(state, ADV748X_IO_10, mask, bits);
>> }
>>
>> How does that look ? (is it even correct first?)
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +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 +570,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;
>>
>> Aha - yes that's a neat solution to ensure that only the TX links
>> generate link_setup calls :)
> 
> Another option would be to bail out from adv748x_link_setup() if the entity is 
> not a TX*.
> 

I suggested this in v1 - but Jacopo objected with the following:

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

Now I look at the implementation here, I see this is precisely what it
is doing anyway .... still converting through adv748x_sd_to_csi2(sd) on
an unknown pointer type
 (which I still believe is a valid thing to do in this instance)

So yes, I think this would be simpler having the check at the top of the
adv748x_link_setup() call, and thus then there is no need to add a
second adv_media_ops structure.


>>>  }
> 
> [snip]
> 

-- 
Regards
--
Kieran

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

[Kieran Bingham]

Hi Jacopo,

One more comment below:

On 07/01/2019 12:36, Kieran Bingham wrote:
> Hi Jacopo,
> 
> On 06/01/2019 15:54, 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.>

<snip>

>>  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 6eb2e4a95eed..eb19c6cbbb4e 100644
>> --- a/drivers/media/i2c/adv748x/adv748x.h
>> +++ b/drivers/media/i2c/adv748x/adv748x.h
>> @@ -93,6 +93,7 @@ struct adv748x_csi2 {
>>  
>>  #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
>>  #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
>> +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx))

I'd put this /after/ is_txa/is_txb so that the use is after the
declarations.
--
KB


>>  #define is_txa(_tx) __is_tx(_tx, a)
>>  #define is_txb(_tx) __is_tx(_tx, b)
>>  
>> @@ -224,6 +225,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 v2 6/6] media: adv748x: Implement TX link_setup callback

[Jacopo Mondi]

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

Hi Laurent,

On Wed, Jan 09, 2019 at 02:15:04AM +0200, Laurent Pinchart wrote:
> Hello,
>
> On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote:
> > On 06/01/2019 15:54, 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.
> >
> > Overall this looks like the right approach - but I feel like the
> > handling of the io10 register might need some consideration, because
> > it's value depends on the condition of both CSI2 transmitters, not just
> > the currently parsed link.
> >
> > I had a go at some pseudo - uncompiled/untested code inline as a suggestion.
> >
> > If you think it's better - feel free to rework it in ... or not as you
> > see fit.
> >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >
> > >  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
> > >  drivers/media/i2c/adv748x/adv748x.h      |  2 +
> > >  2 files changed, 58 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > > b/drivers/media/i2c/adv748x/adv748x-core.c index
> > > 200e00f93546..a586bf393558 100644
> > > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > > @@ -335,6 +335,60 @@ 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;
> > > +
> > > +	/* 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 (!enable)
> > > +		return 0;
> >
> > Don't we potentially want to take any action on disable to power down
> > links below ?
> >
> > > +
> > > +	/* Change video stream routing, according to the newly enabled link. */
> > > +	io10 = io_read(state, ADV748X_IO_10);
> > > +	if (rsd == &state->afe.sd) {
> > > +		/*
> > > +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
> > > +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
> > > +		 */
> > > +		io10 &= ~ADV748X_IO_10_CSI1_EN;
> > > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > > +		if (is_txa(tx))
> > > +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >
> > Shouldn't the CSI4 be enabled here too? or are we assuming it's already
> > (/always) enabled?
> > 		io10 |= ADV748X_IO_10_CSI4_EN;
> >
> > > +		else
> > > +			io10 |= ADV748X_IO_10_CSI4_EN |
> > > +				ADV748X_IO_10_CSI1_EN;
> > > +	} else {
> > > +		/* Clear AFE->TXA routing and power up TXA. */
> > > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > > +		io10 |= ADV748X_IO_10_CSI4_EN;
> >
> > But if we assume it's already enabled ... do we need this?
> > Perhaps it might be better to be explicit on this?
> >
> > > +	}
> > > +	io_write(state, ADV748X_IO_10, io10);
> >
> > Would it be any cleaner to use io_clrset() here?
> >
> > Hrm ... also it feels like this register really should be set depending
> > upon the complete state of ... &state->...
> >
> > So perhaps it deserves it's own function which should be called after
> > csi_registered() callback and any link change.
> >
> > /me has a quick go at some psuedo codeishness...:
> >
> > int adv74x_io_10(struct adv748x_state *state);
> > 	u8 bits = 0;
> > 	u8 mask = ADV748X_IO_10_CSI1_EN
> >
> > 		| ADV748X_IO_10_CSI4_EN
> > 		| ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >
> > 	if (state->afe.tx) {
> > 		/* AFE Requires TXA enabled, even when output to TXB */
> > 		bits |= ADV748X_IO_10_CSI4_EN;
> >
> > 		if (is_txa(state->afe.tx))
> > 			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
> > 		else
> > 			bits |= ADV748X_IO_10_CSI1_EN;
> > 	}
> >
> > 	if (state->hdmi.tx) {
> > 		bits |= ADV748X_IO_10_CSI4_EN;
> > 	}
> >
> > 	return io_clrset(state, ADV748X_IO_10, mask, bits);
> > }
> >
> > How does that look ? (is it even correct first?)
> >
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +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 +570,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;
> >
> > Aha - yes that's a neat solution to ensure that only the TX links
> > generate link_setup calls :)
>
> Another option would be to bail out from adv748x_link_setup() if the entity is
> not a TX*.
>

If I'm not wrong you suggested me to register a set of operations with
the .link_setup callback only for TX entities, and I agree it is much
better, so I'm leaning to leave it as it is in this series.

Thanks
  j

> > >  }
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

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

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

[Jacopo Mondi]

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

On Wed, Jan 09, 2019 at 02:15:33PM +0000, Kieran Bingham wrote:
> On 09/01/2019 00:15, Laurent Pinchart wrote:
> > Hello,
> >
> > On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote:
> >> On 06/01/2019 15:54, 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.
> >>
> >> Overall this looks like the right approach - but I feel like the
> >> handling of the io10 register might need some consideration, because
> >> it's value depends on the condition of both CSI2 transmitters, not just
> >> the currently parsed link.
> >>
> >> I had a go at some pseudo - uncompiled/untested code inline as a suggestion.
> >>
> >> If you think it's better - feel free to rework it in ... or not as you
> >> see fit.
> >>
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>>
> >>>  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
> >>>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
> >>>  2 files changed, 58 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> b/drivers/media/i2c/adv748x/adv748x-core.c index
> >>> 200e00f93546..a586bf393558 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> >>> @@ -335,6 +335,60 @@ 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;
> >>> +
> >>> +	/* 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 (!enable)
> >>> +		return 0;
> >>
> >> Don't we potentially want to take any action on disable to power down
> >> links below ?
> >>
> >>> +
> >>> +	/* Change video stream routing, according to the newly enabled link. */
> >>> +	io10 = io_read(state, ADV748X_IO_10);
> >>> +	if (rsd == &state->afe.sd) {
> >>> +		/*
> >>> +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
> >>> +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
> >>> +		 */
> >>> +		io10 &= ~ADV748X_IO_10_CSI1_EN;
> >>> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >>> +		if (is_txa(tx))
> >>> +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >>
> >> Shouldn't the CSI4 be enabled here too? or are we assuming it's already
> >> (/always) enabled?
> >> 		io10 |= ADV748X_IO_10_CSI4_EN;
> >>
> >>> +		else
> >>> +			io10 |= ADV748X_IO_10_CSI4_EN |
> >>> +				ADV748X_IO_10_CSI1_EN;
> >>> +	} else {
> >>> +		/* Clear AFE->TXA routing and power up TXA. */
> >>> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >>> +		io10 |= ADV748X_IO_10_CSI4_EN;
> >>
> >> But if we assume it's already enabled ... do we need this?
> >> Perhaps it might be better to be explicit on this?
> >>
> >>> +	}
> >>> +	io_write(state, ADV748X_IO_10, io10);
> >>
> >> Would it be any cleaner to use io_clrset() here?
> >>
> >> Hrm ... also it feels like this register really should be set depending
> >> upon the complete state of ... &state->...
> >>
> >> So perhaps it deserves it's own function which should be called after
> >> csi_registered() callback and any link change.
> >>
> >> /me has a quick go at some psuedo codeishness...:
> >>
> >> int adv74x_io_10(struct adv748x_state *state);
> >> 	u8 bits = 0;
> >> 	u8 mask = ADV748X_IO_10_CSI1_EN
> >>
> >> 		| ADV748X_IO_10_CSI4_EN
> >> 		| ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >>
> >> 	if (state->afe.tx) {
> >> 		/* AFE Requires TXA enabled, even when output to TXB */
> >> 		bits |= ADV748X_IO_10_CSI4_EN;
> >>
> >> 		if (is_txa(state->afe.tx))
> >> 			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
> >> 		else
> >> 			bits |= ADV748X_IO_10_CSI1_EN;
> >> 	}
> >>
> >> 	if (state->hdmi.tx) {
> >> 		bits |= ADV748X_IO_10_CSI4_EN;
> >> 	}
> >>
> >> 	return io_clrset(state, ADV748X_IO_10, mask, bits);
> >> }
> >>
> >> How does that look ? (is it even correct first?)
> >>
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +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 +570,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;
> >>
> >> Aha - yes that's a neat solution to ensure that only the TX links
> >> generate link_setup calls :)
> >
> > Another option would be to bail out from adv748x_link_setup() if the entity is
> > not a TX*.
> >
>
> I suggested this in v1 - but Jacopo objected with the following:
>
> > 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.
>

That was at the time where the .link_setup() callback was called for
TXs and non-TXs. What you proposed was to call:

#define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd)

on variables that we don't have any guarantee that are of type 'struct
adv748x_csi2'. I still think it is dangerous and should be avoided and
I worked it around in v1 as:

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

> Now I look at the implementation here, I see this is precisely what it
> is doing anyway .... still converting through adv748x_sd_to_csi2(sd) on
> an unknown pointer type
>  (which I still believe is a valid thing to do in this instance)

It's not unknown, .link_setup() is only registered for TXs. If it gets
called, we know we're dealing with a TX.

>
> So yes, I think this would be simpler having the check at the top of the
> adv748x_link_setup() call, and thus then there is no need to add a
> second adv_media_ops structure.

That was what I did in v1, didn't I ?

The current implementation looks better imho, but if the both of you
prefer something similar to v1 I will consider that.

Thanks
   j
>
>
> >>>  }
> >
> > [snip]
> >
>
> --
> Regards
> --
> Kieran

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

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

[Laurent Pinchart]

Hi Jacopo,

On Thursday, 10 January 2019 10:51:00 EET Jacopo Mondi wrote:
> On Wed, Jan 09, 2019 at 02:15:04AM +0200, Laurent Pinchart wrote:
> > On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote:
> >> On 06/01/2019 15:54, 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.
> >> 
> >> Overall this looks like the right approach - but I feel like the
> >> handling of the io10 register might need some consideration, because
> >> it's value depends on the condition of both CSI2 transmitters, not just
> >> the currently parsed link.
> >> 
> >> I had a go at some pseudo - uncompiled/untested code inline as a
> >> suggestion.
> >> 
> >> If you think it's better - feel free to rework it in ... or not as you
> >> see fit.
> >> 
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>> 
> >>>  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
> >>>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
> >>>  2 files changed, 58 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> b/drivers/media/i2c/adv748x/adv748x-core.c index
> >>> 200e00f93546..a586bf393558 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c

[snip]

> >>> +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 +570,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;
> >> 
> >> Aha - yes that's a neat solution to ensure that only the TX links
> >> generate link_setup calls :)
> > 
> > Another option would be to bail out from adv748x_link_setup() if the
> > entity is not a TX*.
> 
> If I'm not wrong you suggested me to register a set of operations with
> the .link_setup callback only for TX entities, and I agree it is much
> better, so I'm leaning to leave it as it is in this series.

Sorry, I should have made it clear that this wasn't a request for a change, 
just pointing out another potential option. Your implementation is fine with 
me.

> >>>  }
> > 
> > [snip]

-- 
Regards,

Laurent Pinchart

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

[Jacopo Mondi]

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

Hi Kieran,

On Mon, Jan 07, 2019 at 12:36:28PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 06/01/2019 15:54, 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.
>
> Overall this looks like the right approach - but I feel like the
> handling of the io10 register might need some consideration, because
> it's value depends on the condition of both CSI2 transmitters, not just
> the currently parsed link.
>
> I had a go at some pseudo - uncompiled/untested code inline as a suggestion.
>
> If you think it's better - feel free to rework it in ... or not as you
> see fit.
>
> Regards
>
> Kieran
>
>
>
>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
> >  drivers/media/i2c/adv748x/adv748x.h      |  2 +
> >  2 files changed, 58 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index 200e00f93546..a586bf393558 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -335,6 +335,60 @@ 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;
> > +
> > +	/* 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 (!enable)
> > +		return 0;
>
> Don't we potentially want to take any action on disable to power down
> links below ?
>

You know, I had a thought about it, and I was not sure it was worth
it. The chip stays powered, and even if a TX stays active when it is
not linked, this is not worse than what it was before, and
considering...


> > +
> > +	/* Change video stream routing, according to the newly enabled link. */
> > +	io10 = io_read(state, ADV748X_IO_10);
> > +	if (rsd == &state->afe.sd) {
> > +		/*
> > +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
> > +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
> > +		 */

... this
I should not simply disable, say TXA, because the HDMI->TXA link has
been disabled, but I should inspect the global state everytime.

> > +		io10 &= ~ADV748X_IO_10_CSI1_EN;
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +		if (is_txa(tx))
> > +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>
> Shouldn't the CSI4 be enabled here too? or are we assuming it's already
> (/always) enabled?
> 		io10 |= ADV748X_IO_10_CSI4_EN;

Correct, I wrongly assumed it stayed on, but it could be not.

>
> > +		else
> > +			io10 |= ADV748X_IO_10_CSI4_EN |
> > +				ADV748X_IO_10_CSI1_EN;
> > +	} else {
> > +		/* Clear AFE->TXA routing and power up TXA. */
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +		io10 |= ADV748X_IO_10_CSI4_EN;
>
> But if we assume it's already enabled ... do we need this?
> Perhaps it might be better to be explicit on this?
>
> > +	}
> > +	io_write(state, ADV748X_IO_10, io10);
>
>
> Would it be any cleaner to use io_clrset() here?

Not sure if it is cleaner, but I checked if it was safe to write the
whole IO_10 register, and the only fields not touched by this are the
ones controlling output redirection to the TTL port, which should stay
zeros. Bits [1:0] are not documented though..
>
> Hrm ... also it feels like this register really should be set depending
> upon the complete state of ... &state->...
>

Possibly, that would help catching cases where a TX (or both TXs) could be
shut down, if we consider it worthy.

> So perhaps it deserves it's own function which should be called after
> csi_registered() callback and any link change.
>

Not sure if it makes sense to touch this register when a subdevice
gets registered... we create links at that time, but they're not
enabled, so the only place where this configuration changes is
actually this function. I'm not sure it is worth breaking this out,
but I agree the full state should probably be inspected, and not just
the last changed route.

I'll run some tests and see how it works.
Thanks for the comments
   j


> /me has a quick go at some psuedo codeishness...:
>
> int adv74x_io_10(struct adv748x_state *state);
> 	u8 bits = 0;
> 	u8 mask = ADV748X_IO_10_CSI1_EN
> 		| ADV748X_IO_10_CSI4_EN
> 		| ADV748X_IO_10_CSI4_IN_SEL_AFE;
>
> 	if (state->afe.tx) {
> 		/* AFE Requires TXA enabled, even when output to TXB */
> 		bits |= ADV748X_IO_10_CSI4_EN;
>
> 		if (is_txa(state->afe.tx))
> 			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
> 		else
> 			bits |= ADV748X_IO_10_CSI1_EN;
> 	}
>
> 	if (state->hdmi.tx) {
> 		bits |= ADV748X_IO_10_CSI4_EN;
> 	}
>
> 	return io_clrset(state, ADV748X_IO_10, mask, bits);
> }
>
> How does that look ? (is it even correct first?)
>
> > +
> > +	return 0;
> > +}
> > +
> > +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 +570,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;
>
> Aha - yes that's a neat solution to ensure that only the TX links
> generate link_setup calls :)
>
>
>
> >  }
> >
> >  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 6eb2e4a95eed..eb19c6cbbb4e 100644
> > --- a/drivers/media/i2c/adv748x/adv748x.h
> > +++ b/drivers/media/i2c/adv748x/adv748x.h
> > @@ -93,6 +93,7 @@ struct adv748x_csi2 {
> >
> >  #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
> >  #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
> > +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx))
> >  #define is_txa(_tx) __is_tx(_tx, a)
> >  #define is_txb(_tx) __is_tx(_tx, b)
> >
> > @@ -224,6 +225,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

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

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

[Kieran Bingham]

Hi Jacopo,

On 10/01/2019 08:58, Jacopo Mondi wrote:
> On Wed, Jan 09, 2019 at 02:15:33PM +0000, Kieran Bingham wrote:
>> On 09/01/2019 00:15, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote:
>>>> On 06/01/2019 15:54, 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.
>>>>
>>>> Overall this looks like the right approach - but I feel like the
>>>> handling of the io10 register might need some consideration, because
>>>> it's value depends on the condition of both CSI2 transmitters, not just
>>>> the currently parsed link.
>>>>
>>>> I had a go at some pseudo - uncompiled/untested code inline as a suggestion.
>>>>
>>>> If you think it's better - feel free to rework it in ... or not as you
>>>> see fit.
>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>>>> ---
>>>>>
>>>>>  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
>>>>>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
>>>>>  2 files changed, 58 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
>>>>> b/drivers/media/i2c/adv748x/adv748x-core.c index
>>>>> 200e00f93546..a586bf393558 100644
>>>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
>>>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
>>>>> @@ -335,6 +335,60 @@ 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;
>>>>> +
>>>>> +	/* 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 (!enable)
>>>>> +		return 0;
>>>>
>>>> Don't we potentially want to take any action on disable to power down
>>>> links below ?
>>>>
>>>>> +
>>>>> +	/* Change video stream routing, according to the newly enabled link. */
>>>>> +	io10 = io_read(state, ADV748X_IO_10);
>>>>> +	if (rsd == &state->afe.sd) {
>>>>> +		/*
>>>>> +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
>>>>> +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
>>>>> +		 */
>>>>> +		io10 &= ~ADV748X_IO_10_CSI1_EN;
>>>>> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>>>> +		if (is_txa(tx))
>>>>> +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>>>
>>>> Shouldn't the CSI4 be enabled here too? or are we assuming it's already
>>>> (/always) enabled?
>>>> 		io10 |= ADV748X_IO_10_CSI4_EN;
>>>>
>>>>> +		else
>>>>> +			io10 |= ADV748X_IO_10_CSI4_EN |
>>>>> +				ADV748X_IO_10_CSI1_EN;
>>>>> +	} else {
>>>>> +		/* Clear AFE->TXA routing and power up TXA. */
>>>>> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>>>> +		io10 |= ADV748X_IO_10_CSI4_EN;
>>>>
>>>> But if we assume it's already enabled ... do we need this?
>>>> Perhaps it might be better to be explicit on this?
>>>>
>>>>> +	}
>>>>> +	io_write(state, ADV748X_IO_10, io10);
>>>>
>>>> Would it be any cleaner to use io_clrset() here?
>>>>
>>>> Hrm ... also it feels like this register really should be set depending
>>>> upon the complete state of ... &state->...
>>>>
>>>> So perhaps it deserves it's own function which should be called after
>>>> csi_registered() callback and any link change.
>>>>
>>>> /me has a quick go at some psuedo codeishness...:
>>>>
>>>> int adv74x_io_10(struct adv748x_state *state);
>>>> 	u8 bits = 0;
>>>> 	u8 mask = ADV748X_IO_10_CSI1_EN
>>>>
>>>> 		| ADV748X_IO_10_CSI4_EN
>>>> 		| ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>>>
>>>> 	if (state->afe.tx) {
>>>> 		/* AFE Requires TXA enabled, even when output to TXB */
>>>> 		bits |= ADV748X_IO_10_CSI4_EN;
>>>>
>>>> 		if (is_txa(state->afe.tx))
>>>> 			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
>>>> 		else
>>>> 			bits |= ADV748X_IO_10_CSI1_EN;
>>>> 	}
>>>>
>>>> 	if (state->hdmi.tx) {
>>>> 		bits |= ADV748X_IO_10_CSI4_EN;
>>>> 	}
>>>>
>>>> 	return io_clrset(state, ADV748X_IO_10, mask, bits);
>>>> }
>>>>
>>>> How does that look ? (is it even correct first?)
>>>>
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +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 +570,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;
>>>>
>>>> Aha - yes that's a neat solution to ensure that only the TX links
>>>> generate link_setup calls :)
>>>
>>> Another option would be to bail out from adv748x_link_setup() if the entity is
>>> not a TX*.
>>>
>>
>> I suggested this in v1 - but Jacopo objected with the following:
>>
>>> 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.
>>
> 
> That was at the time where the .link_setup() callback was called for
> TXs and non-TXs. What you proposed was to call:
> 
> #define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd)
> 
> on variables that we don't have any guarantee that are of type 'struct
> adv748x_csi2'. I still think it is dangerous and should be avoided and
> I worked it around in v1 as:
> 
> +	if ((sd != &state->txa.sd && sd != &state->txb.sd) ||
> 
>> Now I look at the implementation here, I see this is precisely what it
>> is doing anyway .... still converting through adv748x_sd_to_csi2(sd) on
>> an unknown pointer type
>>  (which I still believe is a valid thing to do in this instance)
> 
> It's not unknown, .link_setup() is only registered for TXs. If it gets
> called, we know we're dealing with a TX.
> 
>>
>> So yes, I think this would be simpler having the check at the top of the
>> adv748x_link_setup() call, and thus then there is no need to add a
>> second adv_media_ops structure.
> 
> That was what I did in v1, didn't I ?
> 
> The current implementation looks better imho, but if the both of you
> prefer something similar to v1 I will consider that.

Given the extra clarification above, I'll not object to keeping it this
way. I still think it's fine to use container of and then check the
pointers for failure. The is_tx() would perform the type-validation :) -
but lets stick with the one that you prefer. Its your patch :)
--
Kieran

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

[Jacopo Mondi]

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

Hi Kieran,
On Mon, Jan 07, 2019 at 12:36:28PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 06/01/2019 15:54, 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.
>
> Overall this looks like the right approach - but I feel like the
> handling of the io10 register might need some consideration, because
> it's value depends on the condition of both CSI2 transmitters, not just
> the currently parsed link.
>
> I had a go at some pseudo - uncompiled/untested code inline as a suggestion.
>
> If you think it's better - feel free to rework it in ... or not as you
> see fit.
>
> Regards
>
> Kieran
>
>
>
>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
> >  drivers/media/i2c/adv748x/adv748x.h      |  2 +
> >  2 files changed, 58 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index 200e00f93546..a586bf393558 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -335,6 +335,60 @@ 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;
> > +
> > +	/* 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 (!enable)
> > +		return 0;
>
> Don't we potentially want to take any action on disable to power down
> links below ?
>
> > +
> > +	/* Change video stream routing, according to the newly enabled link. */
> > +	io10 = io_read(state, ADV748X_IO_10);
> > +	if (rsd == &state->afe.sd) {
> > +		/*
> > +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
> > +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
> > +		 */
> > +		io10 &= ~ADV748X_IO_10_CSI1_EN;
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +		if (is_txa(tx))
> > +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>
> Shouldn't the CSI4 be enabled here too? or are we assuming it's already
> (/always) enabled?
> 		io10 |= ADV748X_IO_10_CSI4_EN;
>
> > +		else
> > +			io10 |= ADV748X_IO_10_CSI4_EN |
> > +				ADV748X_IO_10_CSI1_EN;
> > +	} else {
> > +		/* Clear AFE->TXA routing and power up TXA. */
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +		io10 |= ADV748X_IO_10_CSI4_EN;
>
> But if we assume it's already enabled ... do we need this?
> Perhaps it might be better to be explicit on this?
>
> > +	}
> > +	io_write(state, ADV748X_IO_10, io10);
>
>
> Would it be any cleaner to use io_clrset() here?
>
> Hrm ... also it feels like this register really should be set depending
> upon the complete state of ... &state->...
>
> So perhaps it deserves it's own function which should be called after
> csi_registered() callback and any link change.
>
> /me has a quick go at some psuedo codeishness...:
>
> int adv74x_io_10(struct adv748x_state *state);
> 	u8 bits = 0;
> 	u8 mask = ADV748X_IO_10_CSI1_EN
> 		| ADV748X_IO_10_CSI4_EN
> 		| ADV748X_IO_10_CSI4_IN_SEL_AFE;
>
> 	if (state->afe.tx) {
> 		/* AFE Requires TXA enabled, even when output to TXB */
> 		bits |= ADV748X_IO_10_CSI4_EN;
>
> 		if (is_txa(state->afe.tx))
> 			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
> 		else
> 			bits |= ADV748X_IO_10_CSI1_EN;
> 	}
>
> 	if (state->hdmi.tx) {
> 		bits |= ADV748X_IO_10_CSI4_EN;
> 	}
>
> 	return io_clrset(state, ADV748X_IO_10, mask, bits);
> }
>
> How does that look ? (is it even correct first?)
>

Thanks, I've now updated the implementation to use something very
similar to what you proposed here!

Thanks
  j

> > +
> > +	return 0;
> > +}
> > +
> > +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 +570,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;
>
> Aha - yes that's a neat solution to ensure that only the TX links
> generate link_setup calls :)
>
>
>
> >  }
> >
> >  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 6eb2e4a95eed..eb19c6cbbb4e 100644
> > --- a/drivers/media/i2c/adv748x/adv748x.h
> > +++ b/drivers/media/i2c/adv748x/adv748x.h
> > @@ -93,6 +93,7 @@ struct adv748x_csi2 {
> >
> >  #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
> >  #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
> > +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx))
> >  #define is_txa(_tx) __is_tx(_tx, a)
> >  #define is_txb(_tx) __is_tx(_tx, b)
> >
> > @@ -224,6 +225,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

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