[PATCH v2 0/4] media: v4l: fwnode: Add support for CSI-2 C-PHY line orders

[PATCH v2 0/4] media: v4l: fwnode: Add support for CSI-2 C-PHY line orders

[Niklas Söderlund]

Hello,

This series aims to extend the video interface bindings with a new
property to describe the CSI-2 C-PHY line orders. In comparison with
CSI-2 D-PHY where each data-lane is constructed from two lines making up
a differential pair the C-PHY uses three phase encoding constructed from
three lines to create a single data lane.

The three lines making up the C-PHY data lane are labeled A, B and C in
the specification and can be ordered in any combination (ABC, ACB, BAC,
BCA, CAB and CBA). This can be compared to the D-PHY model where the two
lines in the differential pair can be "normal" or "inverted" to do the
same thing.

For the D-PHY uses-case the exists a property 'lane-polarities' can be
used to describe this line wiring. However there are no property to
describe this ordering of the C-PHY lines in the video interfaces
definition nor in the V4L2 fwnode structures or parser.

Patch 1/4 extends the video bindings with a new property 'line-orders'
which can describe this property of the C-PHY. The property name and the
const values used for different line configurations are taken from the
MIPI Discovery and Configuration (DisCo) Specification for Imaging
document.

Patch 2/4 extends the V4L2 fwnode data structure and parser to consume
and exposes this property to drivers.

While patch 3/4 and 4/4 adds an example use of the property both in the
bindings and in the driver using the R-Car CSI-2 receiver driver on V4H.

A note on the changes to the R-Car driver not relevant to the core V4L2
or bindings work. The V4H WhiteHawk development platform is the only
model where the CSI-2 bus is used in a C-PHY configuration. Early
datasheets where used to add support for it and at that time the line
order registers where not documented so magic values where used as-is.
This have been addressed in later versions of the datasheet and this can
now be done properly.

The magic values used however configured one of the data lanes used in a
BCA configuration, which is required for proper operation on that
development platform. Thus the change in patch 4/4 breaks proper
operation with older DTS files lacking the new line-orders property.

I think this is fine as the only known use-case for this platform is
together with the MAX96712 CSI-2 transmitter and for this we only have a
staging driver capable of generating test patterns. To extend this to a
capture pipeline capable of capturing frames from a real source DTS
changes are needed to describe the video source, so an updated DTS are
need anyhow.

See individual patches for changelog.

Niklas Söderlund (4):
  media: dt-bindings: Add property to describe CSI-2 C-PHY line orders
  media: v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders
  arm64: dts: renesas: white-hawk-csi-dsi: Define CSI-2 data line orders
  media: rcar-csi2: Allow specifying C-PHY line order

 .../bindings/media/video-interfaces.yaml      | 21 ++++++
 .../boot/dts/renesas/white-hawk-csi-dsi.dtsi  |  6 ++
 drivers/media/platform/renesas/rcar-csi2.c    | 74 +++++++++++++++++--
 drivers/media/v4l2-core/v4l2-fwnode.c         | 43 ++++++++++-
 include/dt-bindings/media/video-interfaces.h  |  7 ++
 include/media/v4l2-mediabus.h                 | 21 ++++++
 6 files changed, 164 insertions(+), 8 deletions(-)

-- 
2.47.0

[PATCH v2 1/4] media: dt-bindings: Add property to describe CSI-2 C-PHY line orders

[Niklas Söderlund]

Each data lane on a CSI-2 C-PHY bus uses three phase encoding and is
constructed from three physical wires. The wires are referred to as A, B
and C and their default order is ABC. However to ease hardware design
the specification allows for the wires to be switched in any order.

Add a vendor neutral property to describe the line order used. The
property name 'line-orders', the possible values it can be assigned and
there names are taken from the MIPI Discovery and Configuration (DisCo)
Specification for Imaging.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v1
- Add missing 'items' node.
- Improve usage of should and must it the property description as
  suggested by Sakari, thanks!
---
 .../bindings/media/video-interfaces.yaml      | 21 +++++++++++++++++++
 include/dt-bindings/media/video-interfaces.h  |  7 +++++++
 2 files changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.yaml b/Documentation/devicetree/bindings/media/video-interfaces.yaml
index 26e3e7d7c67b..038e85b45bef 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.yaml
+++ b/Documentation/devicetree/bindings/media/video-interfaces.yaml
@@ -210,6 +210,27 @@ properties:
       lane-polarities property is omitted, the value must be interpreted as 0
       (normal). This property is valid for serial busses only.
 
+  line-orders:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 8
+    items:
+      enum:
+        - 0 # ABC
+        - 1 # ACB
+        - 2 # BAC
+        - 3 # BCA
+        - 4 # CAB
+        - 5 # CBA
+    description:
+      An array of line orders of the CSI-2 C-PHY data lanes. The order of the
+      lanes are the same as in data-lanes property. Valid values are 0-5 as
+      defined in the MIPI Discovery and Configuration (DisCo) Specification for
+      Imaging. The length of the array must be the same length as the
+      data-lanes property. If the line-orders property is omitted, the value
+      shall be interpreted as 0 (ABC). This property is valid for CSI-2 C-PHY
+      busses only.
+
   strobe:
     $ref: /schemas/types.yaml#/definitions/uint32
     enum: [ 0, 1 ]
diff --git a/include/dt-bindings/media/video-interfaces.h b/include/dt-bindings/media/video-interfaces.h
index 68ac4e05e37f..88b9d05d8075 100644
--- a/include/dt-bindings/media/video-interfaces.h
+++ b/include/dt-bindings/media/video-interfaces.h
@@ -13,4 +13,11 @@
 #define MEDIA_BUS_TYPE_PARALLEL			5
 #define MEDIA_BUS_TYPE_BT656			6
 
+#define MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC	0
+#define MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ACB	1
+#define MEDIA_BUS_CSI2_CPHY_LINE_ORDER_BAC	2
+#define MEDIA_BUS_CSI2_CPHY_LINE_ORDER_BCA	3
+#define MEDIA_BUS_CSI2_CPHY_LINE_ORDER_CAB	4
+#define MEDIA_BUS_CSI2_CPHY_LINE_ORDER_CBA	5
+
 #endif /* __DT_BINDINGS_MEDIA_VIDEO_INTERFACES_H__ */
-- 
2.47.0

[PATCH v2 2/4] media: v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders

[Niklas Söderlund]

Extend the fwnode parsing to validate and fill in the CSI-2 C-PHY
line-orders order properties as defined in MIPI Discovery and
Configuration (DisCo) Specification for Imaging.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v1
- Use array instead of switch to get printable line order string for
  debug output.
- Wrap lines harder for 80 chars instead of 100, but keep string formats
  on same line even if they break the 80 chars.
---
 drivers/media/v4l2-core/v4l2-fwnode.c | 43 ++++++++++++++++++++++++++-
 include/media/v4l2-mediabus.h         | 21 +++++++++++++
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index f19c8adf2c61..bb5ea3e00414 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -127,7 +127,7 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
 {
 	struct v4l2_mbus_config_mipi_csi2 *bus = &vep->bus.mipi_csi2;
 	bool have_clk_lane = false, have_data_lanes = false,
-		have_lane_polarities = false;
+		have_lane_polarities = false, have_line_orders = false;
 	unsigned int flags = 0, lanes_used = 0;
 	u32 array[1 + V4L2_MBUS_CSI2_MAX_DATA_LANES];
 	u32 clock_lane = 0;
@@ -197,6 +197,17 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
 		have_lane_polarities = true;
 	}
 
+	rval = fwnode_property_count_u32(fwnode, "line-orders");
+	if (rval > 0) {
+		if (rval != num_data_lanes) {
+			pr_warn("invalid number of line-orders entries (need %u, got %u)\n",
+				num_data_lanes, rval);
+			return -EINVAL;
+		}
+
+		have_line_orders = true;
+	}
+
 	if (!fwnode_property_read_u32(fwnode, "clock-lanes", &v)) {
 		clock_lane = v;
 		pr_debug("clock lane position %u\n", v);
@@ -250,6 +261,36 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
 		} else {
 			pr_debug("no lane polarities defined, assuming not inverted\n");
 		}
+
+		if (have_line_orders) {
+			fwnode_property_read_u32_array(fwnode,
+						       "line-orders", array,
+						       num_data_lanes);
+
+			for (i = 0; i < num_data_lanes; i++) {
+				static const char * const orders[] = {
+					"ABC", "ACB", "BAC", "BCA", "CAB", "CBA"
+				};
+
+				if (array[i] > 5) {
+					pr_warn("lane %u invalid line-order assuming ABC (got %u)\n",
+						i, array[i]);
+					bus->line_orders[i] =
+						V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC;
+					continue;
+				}
+
+				bus->line_orders[i] = array[i];
+				pr_debug("lane %u line order %s", i,
+					 orders[array[i]]);
+			}
+		} else {
+			for (i = 0; i < num_data_lanes; i++)
+				bus->line_orders[i] =
+					V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC;
+
+			pr_debug("no line orders defined, assuming ABC\n");
+		}
 	}
 
 	return 0;
diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 5bce6e423e94..e7f019f68c8d 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -73,6 +73,24 @@
 
 #define V4L2_MBUS_CSI2_MAX_DATA_LANES		8
 
+/**
+ * enum v4l2_mbus_csi2_cphy_line_orders_type - CSI-2 C-PHY line order
+ * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC: C-PHY line order ABC (default)
+ * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB: C-PHY line order ACB
+ * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BAC: C-PHY line order BAC
+ * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BCA: C-PHY line order BCA
+ * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CAB: C-PHY line order CAB
+ * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CBA: C-PHY line order CBA
+ */
+enum v4l2_mbus_csi2_cphy_line_orders_type {
+	V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC,
+	V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB,
+	V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BAC,
+	V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BCA,
+	V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CAB,
+	V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CBA,
+};
+
 /**
  * struct v4l2_mbus_config_mipi_csi2 - MIPI CSI-2 data bus configuration
  * @flags: media bus (V4L2_MBUS_*) flags
@@ -81,6 +99,8 @@
  * @num_data_lanes: number of data lanes
  * @lane_polarities: polarity of the lanes. The order is the same of
  *		   the physical lanes.
+ * @line_orders: line order of the data lanes. The order is the same of the
+ *		   physical lanes.
  */
 struct v4l2_mbus_config_mipi_csi2 {
 	unsigned int flags;
@@ -88,6 +108,7 @@ struct v4l2_mbus_config_mipi_csi2 {
 	unsigned char clock_lane;
 	unsigned char num_data_lanes;
 	bool lane_polarities[1 + V4L2_MBUS_CSI2_MAX_DATA_LANES];
+	enum v4l2_mbus_csi2_cphy_line_orders_type line_orders[V4L2_MBUS_CSI2_MAX_DATA_LANES];
 };
 
 /**
-- 
2.47.0

[PATCH v2 3/4] arm64: dts: renesas: white-hawk-csi-dsi: Define CSI-2 data line orders

[Niklas Söderlund]

The second CSI-2 C-PHY data-lane have a different line order (BCA) then
the two other data-lanes (ABC) for both connected CSI-2 receivers,
describe this in the device tree.

This have worked in the past as the R-Car CSI-2 driver did not have
documentation for the line order configuration and a magic value was
written to the register for this specific setup. Now the registers
involved are documented and the hardware description as well as the
driver needs to be corrected.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 arch/arm64/boot/dts/renesas/white-hawk-csi-dsi.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/white-hawk-csi-dsi.dtsi b/arch/arm64/boot/dts/renesas/white-hawk-csi-dsi.dtsi
index 3006b0a64f41..a5d1c1008e7e 100644
--- a/arch/arm64/boot/dts/renesas/white-hawk-csi-dsi.dtsi
+++ b/arch/arm64/boot/dts/renesas/white-hawk-csi-dsi.dtsi
@@ -21,6 +21,9 @@ csi40_in: endpoint {
 				bus-type = <MEDIA_BUS_TYPE_CSI2_CPHY>;
 				clock-lanes = <0>;
 				data-lanes = <1 2 3>;
+				line-orders = <MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC
+					       MEDIA_BUS_CSI2_CPHY_LINE_ORDER_BCA
+					       MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC>;
 				remote-endpoint = <&max96712_out0>;
 			};
 		};
@@ -41,6 +44,9 @@ csi41_in: endpoint {
 				bus-type = <MEDIA_BUS_TYPE_CSI2_CPHY>;
 				clock-lanes = <0>;
 				data-lanes = <1 2 3>;
+				line-orders = <MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC
+					       MEDIA_BUS_CSI2_CPHY_LINE_ORDER_BCA
+					       MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC>;
 				remote-endpoint = <&max96712_out1>;
 			};
 		};
-- 
2.47.0

[PATCH v2 4/4] media: rcar-csi2: Allow specifying C-PHY line order

[Niklas Söderlund]

Later versions of the V4H datasheet adds documentation for the line
order register needed to support all possible configurations. Extend the
driver to take the line order for each data line into account when
configuring the device.

Unfortunately not all registers initially thought to be involved in line
order configuration where directly related. One magic value is still in
the driver and left as-is, but it is not related to line order as that
procedure have now been documented.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/renesas/rcar-csi2.c | 74 ++++++++++++++++++++--
 1 file changed, 67 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
index 27ffdd28cbf7..8a0b6a68e194 100644
--- a/drivers/media/platform/renesas/rcar-csi2.c
+++ b/drivers/media/platform/renesas/rcar-csi2.c
@@ -183,17 +183,19 @@ struct rcar_csi2;
 #define V4H_CORE_DIG_IOCTRL_RW_AFE_CB_CTRL_2_REG(n)	(0x23840 + ((n) * 2)) /* n = 0 - 11 */
 #define V4H_CORE_DIG_RW_COMMON_REG(n)			(0x23880 + ((n) * 2)) /* n = 0 - 15 */
 #define V4H_CORE_DIG_ANACTRL_RW_COMMON_ANACTRL_REG(n)	(0x239e0 + ((n) * 2)) /* n = 0 - 3 */
-#define V4H_CORE_DIG_CLANE_1_RW_CFG_0_REG		0x2a400
 #define V4H_CORE_DIG_CLANE_1_RW_HS_TX_6_REG		0x2a60c
 
 /* V4H C-PHY */
 #define V4H_CORE_DIG_RW_TRIO0_REG(n)			(0x22100 + ((n) * 2)) /* n = 0 - 3 */
 #define V4H_CORE_DIG_RW_TRIO1_REG(n)			(0x22500 + ((n) * 2)) /* n = 0 - 3 */
 #define V4H_CORE_DIG_RW_TRIO2_REG(n)			(0x22900 + ((n) * 2)) /* n = 0 - 3 */
+#define V4H_CORE_DIG_CLANE_0_RW_CFG_0_REG		0x2a000
 #define V4H_CORE_DIG_CLANE_0_RW_LP_0_REG		0x2a080
 #define V4H_CORE_DIG_CLANE_0_RW_HS_RX_REG(n)		(0x2a100 + ((n) * 2)) /* n = 0 - 6 */
+#define V4H_CORE_DIG_CLANE_1_RW_CFG_0_REG		0x2a400
 #define V4H_CORE_DIG_CLANE_1_RW_LP_0_REG		0x2a480
 #define V4H_CORE_DIG_CLANE_1_RW_HS_RX_REG(n)		(0x2a500 + ((n) * 2)) /* n = 0 - 6 */
+#define V4H_CORE_DIG_CLANE_2_RW_CFG_0_REG		0x2a800
 #define V4H_CORE_DIG_CLANE_2_RW_LP_0_REG		0x2a880
 #define V4H_CORE_DIG_CLANE_2_RW_HS_RX_REG(n)		(0x2a900 + ((n) * 2)) /* n = 0 - 6 */
 
@@ -672,6 +674,21 @@ static const struct rcar_csi2_format *rcsi2_code_to_fmt(unsigned int code)
 	return NULL;
 }
 
+struct rcsi2_cphy_line_order {
+	enum v4l2_mbus_csi2_cphy_line_orders_type order;
+	u16 cfg;
+	u16 ctrl29;
+};
+
+static const struct rcsi2_cphy_line_order rcsi2_cphy_line_orders[] = {
+	{ .order = V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC, .cfg = 0x0, .ctrl29 = 0x0 },
+	{ .order = V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB, .cfg = 0xa, .ctrl29 = 0x1 },
+	{ .order = V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BAC, .cfg = 0xc, .ctrl29 = 0x1 },
+	{ .order = V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BCA, .cfg = 0x5, .ctrl29 = 0x0 },
+	{ .order = V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CAB, .cfg = 0x3, .ctrl29 = 0x0 },
+	{ .order = V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CBA, .cfg = 0x9, .ctrl29 = 0x1 }
+};
+
 enum rcar_csi2_pads {
 	RCAR_CSI2_SINK,
 	RCAR_CSI2_SOURCE_VC0,
@@ -722,6 +739,7 @@ struct rcar_csi2 {
 	bool cphy;
 	unsigned short lanes;
 	unsigned char lane_swap[4];
+	enum v4l2_mbus_csi2_cphy_line_orders_type line_orders[3];
 };
 
 static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
@@ -754,11 +772,24 @@ static void rcsi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data)
 	iowrite32(data, priv->base + reg);
 }
 
+static u16 rcsi2_read16(struct rcar_csi2 *priv, unsigned int reg)
+{
+	return ioread16(priv->base + reg);
+}
+
 static void rcsi2_write16(struct rcar_csi2 *priv, unsigned int reg, u16 data)
 {
 	iowrite16(data, priv->base + reg);
 }
 
+static void rcsi2_modify16(struct rcar_csi2 *priv, unsigned int reg, u16 data, u16 mask)
+{
+	u16 val;
+
+	val = rcsi2_read16(priv, reg) & ~mask;
+	rcsi2_write16(priv, reg, val | data);
+}
+
 static int rcsi2_phtw_write(struct rcar_csi2 *priv, u8 data, u8 code)
 {
 	unsigned int timeout;
@@ -1112,6 +1143,26 @@ static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv,
 	return 0;
 }
 
+static void rsci2_set_line_order(struct rcar_csi2 *priv,
+				 enum v4l2_mbus_csi2_cphy_line_orders_type order,
+				 unsigned int cfgreg, unsigned int ctrlreg)
+{
+	const struct rcsi2_cphy_line_order *info = NULL;
+
+	for (unsigned int i = 0; i < ARRAY_SIZE(rcsi2_cphy_line_orders); i++) {
+		if (rcsi2_cphy_line_orders[i].order == order) {
+			info = &rcsi2_cphy_line_orders[i];
+			break;
+		}
+	}
+
+	if (!info)
+		return;
+
+	rcsi2_modify16(priv, cfgreg, info->cfg, 0x000f);
+	rcsi2_modify16(priv, ctrlreg, info->ctrl29, 0x0100);
+}
+
 static int rcsi2_wait_phy_start_v4h(struct rcar_csi2 *priv, u32 match)
 {
 	unsigned int timeout;
@@ -1189,12 +1240,18 @@ static int rcsi2_c_phy_setting_v4h(struct rcar_csi2 *priv, int msps)
 	rcsi2_write16(priv, V4H_CORE_DIG_RW_TRIO1_REG(1), conf->trio1);
 	rcsi2_write16(priv, V4H_CORE_DIG_RW_TRIO2_REG(1), conf->trio1);
 
-	/*
-	 * Configure pin-swap.
-	 * TODO: This registers is not documented yet, the values should depend
-	 * on the 'clock-lanes' and 'data-lanes' devicetree properties.
-	 */
-	rcsi2_write16(priv, V4H_CORE_DIG_CLANE_1_RW_CFG_0_REG, 0xf5);
+	/* Configure data line order. */
+	rsci2_set_line_order(priv, priv->line_orders[0],
+			     V4H_CORE_DIG_CLANE_0_RW_CFG_0_REG,
+			     V4H_CORE_DIG_IOCTRL_RW_AFE_LANE0_CTRL_2_REG(9));
+	rsci2_set_line_order(priv, priv->line_orders[1],
+			     V4H_CORE_DIG_CLANE_1_RW_CFG_0_REG,
+			     V4H_CORE_DIG_IOCTRL_RW_AFE_LANE1_CTRL_2_REG(9));
+	rsci2_set_line_order(priv, priv->line_orders[2],
+			     V4H_CORE_DIG_CLANE_2_RW_CFG_0_REG,
+			     V4H_CORE_DIG_IOCTRL_RW_AFE_LANE2_CTRL_2_REG(9));
+
+	/* TODO: This registers is not documented. */
 	rcsi2_write16(priv, V4H_CORE_DIG_CLANE_1_RW_HS_TX_6_REG, 0x5000);
 
 	/* Leave Shutdown mode */
@@ -1732,6 +1789,9 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
 		}
 	}
 
+	for (i = 0; i < ARRAY_SIZE(priv->line_orders); i++)
+		priv->line_orders[i] = vep->bus.mipi_csi2.line_orders[i];
+
 	return 0;
 }
 
-- 
2.47.0

Re: [PATCH v2 1/4] media: dt-bindings: Add property to describe CSI-2 C-PHY line orders

[Rob Herring (Arm)]

On Thu, 21 Nov 2024 14:41:05 +0100, Niklas Söderlund wrote:
> Each data lane on a CSI-2 C-PHY bus uses three phase encoding and is
> constructed from three physical wires. The wires are referred to as A, B
> and C and their default order is ABC. However to ease hardware design
> the specification allows for the wires to be switched in any order.
> 
> Add a vendor neutral property to describe the line order used. The
> property name 'line-orders', the possible values it can be assigned and
> there names are taken from the MIPI Discovery and Configuration (DisCo)
> Specification for Imaging.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v1
> - Add missing 'items' node.
> - Improve usage of should and must it the property description as
>   suggested by Sakari, thanks!
> ---
>  .../bindings/media/video-interfaces.yaml      | 21 +++++++++++++++++++
>  include/dt-bindings/media/video-interfaces.h  |  7 +++++++
>  2 files changed, 28 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v2 2/4] media: v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders

[Sakari Ailus]

Hej Niklas,

On Thu, Nov 21, 2024 at 02:41:06PM +0100, Niklas Söderlund wrote:
> Extend the fwnode parsing to validate and fill in the CSI-2 C-PHY
> line-orders order properties as defined in MIPI Discovery and
> Configuration (DisCo) Specification for Imaging.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v1
> - Use array instead of switch to get printable line order string for
>   debug output.
> - Wrap lines harder for 80 chars instead of 100, but keep string formats
>   on same line even if they break the 80 chars.
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 43 ++++++++++++++++++++++++++-
>  include/media/v4l2-mediabus.h         | 21 +++++++++++++
>  2 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index f19c8adf2c61..bb5ea3e00414 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -127,7 +127,7 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
>  {
>  	struct v4l2_mbus_config_mipi_csi2 *bus = &vep->bus.mipi_csi2;
>  	bool have_clk_lane = false, have_data_lanes = false,
> -		have_lane_polarities = false;
> +		have_lane_polarities = false, have_line_orders = false;
>  	unsigned int flags = 0, lanes_used = 0;
>  	u32 array[1 + V4L2_MBUS_CSI2_MAX_DATA_LANES];
>  	u32 clock_lane = 0;
> @@ -197,6 +197,17 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
>  		have_lane_polarities = true;
>  	}
>  
> +	rval = fwnode_property_count_u32(fwnode, "line-orders");
> +	if (rval > 0) {
> +		if (rval != num_data_lanes) {
> +			pr_warn("invalid number of line-orders entries (need %u, got %u)\n",
> +				num_data_lanes, rval);
> +			return -EINVAL;
> +		}
> +
> +		have_line_orders = true;
> +	}
> +
>  	if (!fwnode_property_read_u32(fwnode, "clock-lanes", &v)) {
>  		clock_lane = v;
>  		pr_debug("clock lane position %u\n", v);
> @@ -250,6 +261,36 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
>  		} else {
>  			pr_debug("no lane polarities defined, assuming not inverted\n");
>  		}
> +
> +		if (have_line_orders) {
> +			fwnode_property_read_u32_array(fwnode,
> +						       "line-orders", array,
> +						       num_data_lanes);
> +
> +			for (i = 0; i < num_data_lanes; i++) {
> +				static const char * const orders[] = {
> +					"ABC", "ACB", "BAC", "BCA", "CAB", "CBA"
> +				};
> +
> +				if (array[i] > 5) {


I'd use:

				if (... >= ARRAY_SIZE(order)) {

I can do the change while applying...

> +					pr_warn("lane %u invalid line-order assuming ABC (got %u)\n",
> +						i, array[i]);
> +					bus->line_orders[i] =
> +						V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC;
> +					continue;
> +				}
> +
> +				bus->line_orders[i] = array[i];
> +				pr_debug("lane %u line order %s", i,
> +					 orders[array[i]]);
> +			}
> +		} else {
> +			for (i = 0; i < num_data_lanes; i++)
> +				bus->line_orders[i] =
> +					V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC;
> +
> +			pr_debug("no line orders defined, assuming ABC\n");
> +		}
>  	}
>  
>  	return 0;
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 5bce6e423e94..e7f019f68c8d 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -73,6 +73,24 @@
>  
>  #define V4L2_MBUS_CSI2_MAX_DATA_LANES		8
>  
> +/**
> + * enum v4l2_mbus_csi2_cphy_line_orders_type - CSI-2 C-PHY line order
> + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC: C-PHY line order ABC (default)
> + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB: C-PHY line order ACB
> + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BAC: C-PHY line order BAC
> + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BCA: C-PHY line order BCA
> + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CAB: C-PHY line order CAB
> + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CBA: C-PHY line order CBA
> + */
> +enum v4l2_mbus_csi2_cphy_line_orders_type {
> +	V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC,
> +	V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB,
> +	V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BAC,
> +	V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BCA,
> +	V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CAB,
> +	V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CBA,
> +};
> +
>  /**
>   * struct v4l2_mbus_config_mipi_csi2 - MIPI CSI-2 data bus configuration
>   * @flags: media bus (V4L2_MBUS_*) flags
> @@ -81,6 +99,8 @@
>   * @num_data_lanes: number of data lanes
>   * @lane_polarities: polarity of the lanes. The order is the same of
>   *		   the physical lanes.
> + * @line_orders: line order of the data lanes. The order is the same of the
> + *		   physical lanes.
>   */
>  struct v4l2_mbus_config_mipi_csi2 {
>  	unsigned int flags;
> @@ -88,6 +108,7 @@ struct v4l2_mbus_config_mipi_csi2 {
>  	unsigned char clock_lane;
>  	unsigned char num_data_lanes;
>  	bool lane_polarities[1 + V4L2_MBUS_CSI2_MAX_DATA_LANES];
> +	enum v4l2_mbus_csi2_cphy_line_orders_type line_orders[V4L2_MBUS_CSI2_MAX_DATA_LANES];
>  };
>  
>  /**

-- 
Med vänliga hälsingar,

Sakari Ailus

Re: [PATCH v2 2/4] media: v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders

[Niklas Söderlund]

Hej Sakari,

Tack för att du tog dig tid och tittade på detta.

On 2024-12-12 07:25:38 +0000, Sakari Ailus wrote:
> > @@ -250,6 +261,36 @@ static int 
> > v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> >  		} else {
> >  			pr_debug("no lane polarities defined, assuming not inverted\n");
> >  		}
> > +
> > +		if (have_line_orders) {
> > +			fwnode_property_read_u32_array(fwnode,
> > +						       "line-orders", array,
> > +						       num_data_lanes);
> > +
> > +			for (i = 0; i < num_data_lanes; i++) {
> > +				static const char * const orders[] = {
> > +					"ABC", "ACB", "BAC", "BCA", "CAB", "CBA"
> > +				};
> > +
> > +				if (array[i] > 5) {
> 
> 
> I'd use:
> 
> 				if (... >= ARRAY_SIZE(order)) {
> 
> I can do the change while applying...

Thanks and pleas do.

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2 3/4] arm64: dts: renesas: white-hawk-csi-dsi: Define CSI-2 data line orders

[Geert Uytterhoeven]

Hi Niklas, Sakari, Mauro,

On Thu, Nov 21, 2024 at 2:41 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> The second CSI-2 C-PHY data-lane have a different line order (BCA) then
> the two other data-lanes (ABC) for both connected CSI-2 receivers,
> describe this in the device tree.
>
> This have worked in the past as the R-Car CSI-2 driver did not have

has

> documentation for the line order configuration and a magic value was
> written to the register for this specific setup. Now the registers
> involved are documented and the hardware description as well as the
> driver needs to be corrected.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/arch/arm64/boot/dts/renesas/white-hawk-csi-dsi.dtsi
> +++ b/arch/arm64/boot/dts/renesas/white-hawk-csi-dsi.dtsi
> @@ -21,6 +21,9 @@ csi40_in: endpoint {
>                                 bus-type = <MEDIA_BUS_TYPE_CSI2_CPHY>;
>                                 clock-lanes = <0>;
>                                 data-lanes = <1 2 3>;
> +                               line-orders = <MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC
> +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_BCA
> +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC>;
>                                 remote-endpoint = <&max96712_out0>;
>                         };
>                 };
> @@ -41,6 +44,9 @@ csi41_in: endpoint {
>                                 bus-type = <MEDIA_BUS_TYPE_CSI2_CPHY>;
>                                 clock-lanes = <0>;
>                                 data-lanes = <1 2 3>;
> +                               line-orders = <MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC
> +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_BCA
> +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC>;
>                                 remote-endpoint = <&max96712_out1>;
>                         };
>                 };

Using the MEDIA_BUS_CSI2_CPHY_LINE_ORDER_* definitions has a hard
dependency on commit 91a7088096a49eb4 ("media: dt-bindings: Add property
to describe CSI-2 C-PHY line orders") in media/master, hence I cannot
take this patch in renesas-devel until that dependency is resolved.

However, according to the cover letter, commit 573b4adddbd22baf ("media:
v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders") in media/master
causes a regression in the absence of the line-orders properties
(which I had missed before, unfortunately).
So I think it is best if this patch goes in through the media tree,
which already has the prerequisites and the regression:
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>

Alternatively, I can:
  1. Cherry-pick commit 91a7088096a49eb4 first,
  2. Replace the MEDIA_BUS_CSI2_CPHY_LINE_ORDER_* definitions by
     their numerical values.

Please let me know if you prefer option 1 or 2.
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/4] media: v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders

[Geert Uytterhoeven]

Hi Niklas,

On Thu, Nov 21, 2024 at 2:41 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Extend the fwnode parsing to validate and fill in the CSI-2 C-PHY
> line-orders order properties as defined in MIPI Discovery and
> Configuration (DisCo) Specification for Imaging.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v1
> - Use array instead of switch to get printable line order string for
>   debug output.
> - Wrap lines harder for 80 chars instead of 100, but keep string formats
>   on same line even if they break the 80 chars.

Thanks for your patch, which is now commit 573b4adddbd22baf
("media: v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders") in
media/master.

> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -250,6 +261,36 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
>                 } else {
>                         pr_debug("no lane polarities defined, assuming not inverted\n");
>                 }
> +
> +               if (have_line_orders) {
> +                       fwnode_property_read_u32_array(fwnode,
> +                                                      "line-orders", array,
> +                                                      num_data_lanes);
> +
> +                       for (i = 0; i < num_data_lanes; i++) {
> +                               static const char * const orders[] = {
> +                                       "ABC", "ACB", "BAC", "BCA", "CAB", "CBA"
> +                               };
> +
> +                               if (array[i] > 5) {
> +                                       pr_warn("lane %u invalid line-order assuming ABC (got %u)\n",
> +                                               i, array[i]);
> +                                       bus->line_orders[i] =
> +                                               V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC;
> +                                       continue;
> +                               }
> +
> +                               bus->line_orders[i] = array[i];

This does not do any translation (unlike v4l2_fwnode_bus_type_to_mbus()
to translate from MEDIA_BUS_TYPE_* to V4L2_MBUS_* definitions) ...

> +                               pr_debug("lane %u line order %s", i,
> +                                        orders[array[i]]);
> +                       }
> +               } else {
> +                       for (i = 0; i < num_data_lanes; i++)
> +                               bus->line_orders[i] =
> +                                       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC;
> +
> +                       pr_debug("no line orders defined, assuming ABC\n");
> +               }
>         }
>
>         return 0;
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 5bce6e423e94..e7f019f68c8d 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -73,6 +73,24 @@
>
>  #define V4L2_MBUS_CSI2_MAX_DATA_LANES          8
>
> +/**
> + * enum v4l2_mbus_csi2_cphy_line_orders_type - CSI-2 C-PHY line order
> + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC: C-PHY line order ABC (default)
> + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB: C-PHY line order ACB
> + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BAC: C-PHY line order BAC
> + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BCA: C-PHY line order BCA
> + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CAB: C-PHY line order CAB
> + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CBA: C-PHY line order CBA
> + */
> +enum v4l2_mbus_csi2_cphy_line_orders_type {
> +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC,
> +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB,
> +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BAC,
> +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BCA,
> +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CAB,
> +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CBA,
> +};

... hence these values must be identical to the corresponding
MEDIA_BUS_CSI2_CPHY_LINE_ORDER_* in
include/dt-bindings/media/video-interfaces.h.
So please provide a safe-guard to make sure they do not become out
of sync.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/4] arm64: dts: renesas: white-hawk-csi-dsi: Define CSI-2 data line orders

[Niklas Söderlund]

Hi Geert,

Thanks for your feedback.

On 2024-12-27 14:22:31 +0100, Geert Uytterhoeven wrote:
> Hi Niklas, Sakari, Mauro,
> 
> On Thu, Nov 21, 2024 at 2:41 PM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > The second CSI-2 C-PHY data-lane have a different line order (BCA) then
> > the two other data-lanes (ABC) for both connected CSI-2 receivers,
> > describe this in the device tree.
> >
> > This have worked in the past as the R-Car CSI-2 driver did not have
> 
> has
> 
> > documentation for the line order configuration and a magic value was
> > written to the register for this specific setup. Now the registers
> > involved are documented and the hardware description as well as the
> > driver needs to be corrected.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Thanks for your patch!
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> > --- a/arch/arm64/boot/dts/renesas/white-hawk-csi-dsi.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/white-hawk-csi-dsi.dtsi
> > @@ -21,6 +21,9 @@ csi40_in: endpoint {
> >                                 bus-type = <MEDIA_BUS_TYPE_CSI2_CPHY>;
> >                                 clock-lanes = <0>;
> >                                 data-lanes = <1 2 3>;
> > +                               line-orders = <MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC
> > +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_BCA
> > +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC>;
> >                                 remote-endpoint = <&max96712_out0>;
> >                         };
> >                 };
> > @@ -41,6 +44,9 @@ csi41_in: endpoint {
> >                                 bus-type = <MEDIA_BUS_TYPE_CSI2_CPHY>;
> >                                 clock-lanes = <0>;
> >                                 data-lanes = <1 2 3>;
> > +                               line-orders = <MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC
> > +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_BCA
> > +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC>;
> >                                 remote-endpoint = <&max96712_out1>;
> >                         };
> >                 };
> 
> Using the MEDIA_BUS_CSI2_CPHY_LINE_ORDER_* definitions has a hard
> dependency on commit 91a7088096a49eb4 ("media: dt-bindings: Add property
> to describe CSI-2 C-PHY line orders") in media/master, hence I cannot
> take this patch in renesas-devel until that dependency is resolved.
> 
> However, according to the cover letter, commit 573b4adddbd22baf ("media:
> v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders") in media/master
> causes a regression in the absence of the line-orders properties
> (which I had missed before, unfortunately).
> So I think it is best if this patch goes in through the media tree,
> which already has the prerequisites and the regression:
> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Alternatively, I can:
>   1. Cherry-pick commit 91a7088096a49eb4 first,
>   2. Replace the MEDIA_BUS_CSI2_CPHY_LINE_ORDER_* definitions by
>      their numerical values.
> 
> Please let me know if you prefer option 1 or 2.
> Thanks!

My preference would be for this patch to go thru the media tree with 
your tags to create the least churn, if Sakari is OK with that ofc.

If not I leave it up to Sakari which option is most preferable to him, 
I'm OK with both alternatives.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2 2/4] media: v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders

[Niklas Söderlund]

Hi Geert,

Thanks for your review.

On 2024-12-27 14:31:11 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Thu, Nov 21, 2024 at 2:41 PM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > Extend the fwnode parsing to validate and fill in the CSI-2 C-PHY
> > line-orders order properties as defined in MIPI Discovery and
> > Configuration (DisCo) Specification for Imaging.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> > * Changes since v1
> > - Use array instead of switch to get printable line order string for
> >   debug output.
> > - Wrap lines harder for 80 chars instead of 100, but keep string formats
> >   on same line even if they break the 80 chars.
> 
> Thanks for your patch, which is now commit 573b4adddbd22baf
> ("media: v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders") in
> media/master.
> 
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -250,6 +261,36 @@ static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> >                 } else {
> >                         pr_debug("no lane polarities defined, assuming not inverted\n");
> >                 }
> > +
> > +               if (have_line_orders) {
> > +                       fwnode_property_read_u32_array(fwnode,
> > +                                                      "line-orders", array,
> > +                                                      num_data_lanes);
> > +
> > +                       for (i = 0; i < num_data_lanes; i++) {
> > +                               static const char * const orders[] = {
> > +                                       "ABC", "ACB", "BAC", "BCA", "CAB", "CBA"
> > +                               };
> > +
> > +                               if (array[i] > 5) {
> > +                                       pr_warn("lane %u invalid line-order assuming ABC (got %u)\n",
> > +                                               i, array[i]);
> > +                                       bus->line_orders[i] =
> > +                                               V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC;
> > +                                       continue;
> > +                               }
> > +
> > +                               bus->line_orders[i] = array[i];
> 
> This does not do any translation (unlike v4l2_fwnode_bus_type_to_mbus()
> to translate from MEDIA_BUS_TYPE_* to V4L2_MBUS_* definitions) ...
> 
> > +                               pr_debug("lane %u line order %s", i,
> > +                                        orders[array[i]]);
> > +                       }
> > +               } else {
> > +                       for (i = 0; i < num_data_lanes; i++)
> > +                               bus->line_orders[i] =
> > +                                       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC;
> > +
> > +                       pr_debug("no line orders defined, assuming ABC\n");
> > +               }
> >         }
> >
> >         return 0;
> > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> > index 5bce6e423e94..e7f019f68c8d 100644
> > --- a/include/media/v4l2-mediabus.h
> > +++ b/include/media/v4l2-mediabus.h
> > @@ -73,6 +73,24 @@
> >
> >  #define V4L2_MBUS_CSI2_MAX_DATA_LANES          8
> >
> > +/**
> > + * enum v4l2_mbus_csi2_cphy_line_orders_type - CSI-2 C-PHY line order
> > + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC: C-PHY line order ABC (default)
> > + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB: C-PHY line order ACB
> > + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BAC: C-PHY line order BAC
> > + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BCA: C-PHY line order BCA
> > + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CAB: C-PHY line order CAB
> > + * @V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CBA: C-PHY line order CBA
> > + */
> > +enum v4l2_mbus_csi2_cphy_line_orders_type {
> > +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ABC,
> > +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_ACB,
> > +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BAC,
> > +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_BCA,
> > +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CAB,
> > +       V4L2_MBUS_CSI2_CPHY_LINE_ORDER_CBA,
> > +};
> 
> ... hence these values must be identical to the corresponding
> MEDIA_BUS_CSI2_CPHY_LINE_ORDER_* in
> include/dt-bindings/media/video-interfaces.h.
> So please provide a safe-guard to make sure they do not become out
> of sync.

This is a good point, I will create a follow up patch to address this.  
Thanks for spotting it!

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2 3/4] arm64: dts: renesas: white-hawk-csi-dsi: Define CSI-2 data line orders

[Geert Uytterhoeven]

Hi Niklas,

On Sat, Jan 4, 2025 at 1:17 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> On 2024-12-27 14:22:31 +0100, Geert Uytterhoeven wrote:
> > On Thu, Nov 21, 2024 at 2:41 PM Niklas Söderlund
> > <niklas.soderlund+renesas@ragnatech.se> wrote:
> > > The second CSI-2 C-PHY data-lane have a different line order (BCA) then
> > > the two other data-lanes (ABC) for both connected CSI-2 receivers,
> > > describe this in the device tree.
> > >
> > > This have worked in the past as the R-Car CSI-2 driver did not have
> >
> > has
> >
> > > documentation for the line order configuration and a magic value was
> > > written to the register for this specific setup. Now the registers
> > > involved are documented and the hardware description as well as the
> > > driver needs to be corrected.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > Thanks for your patch!
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > > --- a/arch/arm64/boot/dts/renesas/white-hawk-csi-dsi.dtsi
> > > +++ b/arch/arm64/boot/dts/renesas/white-hawk-csi-dsi.dtsi
> > > @@ -21,6 +21,9 @@ csi40_in: endpoint {
> > >                                 bus-type = <MEDIA_BUS_TYPE_CSI2_CPHY>;
> > >                                 clock-lanes = <0>;
> > >                                 data-lanes = <1 2 3>;
> > > +                               line-orders = <MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC
> > > +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_BCA
> > > +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC>;
> > >                                 remote-endpoint = <&max96712_out0>;
> > >                         };
> > >                 };
> > > @@ -41,6 +44,9 @@ csi41_in: endpoint {
> > >                                 bus-type = <MEDIA_BUS_TYPE_CSI2_CPHY>;
> > >                                 clock-lanes = <0>;
> > >                                 data-lanes = <1 2 3>;
> > > +                               line-orders = <MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC
> > > +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_BCA
> > > +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC>;
> > >                                 remote-endpoint = <&max96712_out1>;
> > >                         };
> > >                 };
> >
> > Using the MEDIA_BUS_CSI2_CPHY_LINE_ORDER_* definitions has a hard
> > dependency on commit 91a7088096a49eb4 ("media: dt-bindings: Add property
> > to describe CSI-2 C-PHY line orders") in media/master, hence I cannot
> > take this patch in renesas-devel until that dependency is resolved.
> >
> > However, according to the cover letter, commit 573b4adddbd22baf ("media:
> > v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders") in media/master
> > causes a regression in the absence of the line-orders properties
> > (which I had missed before, unfortunately).
> > So I think it is best if this patch goes in through the media tree,
> > which already has the prerequisites and the regression:
> > Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > Alternatively, I can:
> >   1. Cherry-pick commit 91a7088096a49eb4 first,
> >   2. Replace the MEDIA_BUS_CSI2_CPHY_LINE_ORDER_* definitions by
> >      their numerical values.
> >
> > Please let me know if you prefer option 1 or 2.
> > Thanks!
>
> My preference would be for this patch to go thru the media tree with
> your tags to create the least churn, if Sakari is OK with that ofc.
>
> If not I leave it up to Sakari which option is most preferable to him,
> I'm OK with both alternatives.

Note that it's getting a bit late for the alternatives, as I plan to send
my PRs for soc today, or tomorrow the latest.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/4] arm64: dts: renesas: white-hawk-csi-dsi: Define CSI-2 data line orders

[Niklas Söderlund]

Hi Geert,

On 2025-01-06 10:45:51 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Sat, Jan 4, 2025 at 1:17 PM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > On 2024-12-27 14:22:31 +0100, Geert Uytterhoeven wrote:
> > > On Thu, Nov 21, 2024 at 2:41 PM Niklas Söderlund
> > > <niklas.soderlund+renesas@ragnatech.se> wrote:
> > > > The second CSI-2 C-PHY data-lane have a different line order (BCA) then
> > > > the two other data-lanes (ABC) for both connected CSI-2 receivers,
> > > > describe this in the device tree.
> > > >
> > > > This have worked in the past as the R-Car CSI-2 driver did not have
> > >
> > > has
> > >
> > > > documentation for the line order configuration and a magic value was
> > > > written to the register for this specific setup. Now the registers
> > > > involved are documented and the hardware description as well as the
> > > > driver needs to be corrected.
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >
> > > Thanks for your patch!
> > >
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > >
> > > > --- a/arch/arm64/boot/dts/renesas/white-hawk-csi-dsi.dtsi
> > > > +++ b/arch/arm64/boot/dts/renesas/white-hawk-csi-dsi.dtsi
> > > > @@ -21,6 +21,9 @@ csi40_in: endpoint {
> > > >                                 bus-type = <MEDIA_BUS_TYPE_CSI2_CPHY>;
> > > >                                 clock-lanes = <0>;
> > > >                                 data-lanes = <1 2 3>;
> > > > +                               line-orders = <MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC
> > > > +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_BCA
> > > > +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC>;
> > > >                                 remote-endpoint = <&max96712_out0>;
> > > >                         };
> > > >                 };
> > > > @@ -41,6 +44,9 @@ csi41_in: endpoint {
> > > >                                 bus-type = <MEDIA_BUS_TYPE_CSI2_CPHY>;
> > > >                                 clock-lanes = <0>;
> > > >                                 data-lanes = <1 2 3>;
> > > > +                               line-orders = <MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC
> > > > +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_BCA
> > > > +                                              MEDIA_BUS_CSI2_CPHY_LINE_ORDER_ABC>;
> > > >                                 remote-endpoint = <&max96712_out1>;
> > > >                         };
> > > >                 };
> > >
> > > Using the MEDIA_BUS_CSI2_CPHY_LINE_ORDER_* definitions has a hard
> > > dependency on commit 91a7088096a49eb4 ("media: dt-bindings: Add property
> > > to describe CSI-2 C-PHY line orders") in media/master, hence I cannot
> > > take this patch in renesas-devel until that dependency is resolved.
> > >
> > > However, according to the cover letter, commit 573b4adddbd22baf ("media:
> > > v4l: fwnode: Parse MiPI DisCo for C-PHY line-orders") in media/master
> > > causes a regression in the absence of the line-orders properties
> > > (which I had missed before, unfortunately).
> > > So I think it is best if this patch goes in through the media tree,
> > > which already has the prerequisites and the regression:
> > > Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > >
> > > Alternatively, I can:
> > >   1. Cherry-pick commit 91a7088096a49eb4 first,
> > >   2. Replace the MEDIA_BUS_CSI2_CPHY_LINE_ORDER_* definitions by
> > >      their numerical values.
> > >
> > > Please let me know if you prefer option 1 or 2.
> > > Thanks!
> >
> > My preference would be for this patch to go thru the media tree with
> > your tags to create the least churn, if Sakari is OK with that ofc.
> >
> > If not I leave it up to Sakari which option is most preferable to him,
> > I'm OK with both alternatives.
> 
> Note that it's getting a bit late for the alternatives, as I plan to send
> my PRs for soc today, or tomorrow the latest.

Thanks for letting us know. As we all are slowly wakening from the 
holiday season maybe the best alternative is to go with option 2, 
numerical values to avoid the issue? Then in next cycle follow up with 
using the defines?

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2 3/4] arm64: dts: renesas: white-hawk-csi-dsi: Define CSI-2 data line orders

[Niklas Söderlund]

On 2025-01-06 11:02:36 +0100, Niklas Söderlund wrote:
> > Note that it's getting a bit late for the alternatives, as I plan to 
> > send
> > my PRs for soc today, or tomorrow the latest.
> 
> Thanks for letting us know. As we all are slowly wakening from the 
> holiday season maybe the best alternative is to go with option 2, 
> numerical values to avoid the issue? Then in next cycle follow up with 
> using the defines?

I have now sent a patch for option 2 and I will send a follow up patch 
once -rc1 is out to use the defines instead of numericals. Thanks for 
your help!

-- 
Kind Regards,
Niklas Söderlund