[PATCH v2 00/12] media: imx-mipi-csis: Cleanups and debugging improvements

[PATCH v2 00/12] media: imx-mipi-csis: Cleanups and debugging improvements

[Laurent Pinchart]

Hello,

This patch series bring a few miscellaneous improvements to the
imx-mipi-csis driver, and in particular improves the debugging
infrastructure.

Patch 01/12 starts by making the v4l2_get_link_freq() function accept a
const media_pad argument. This conflicts with [1], I don't mind rebasing
on top of it. Sakari, if you plan to get that patch merged soon, feel
free to take 01/12 too.

The next three patches, 02/12 to 04/12, are small cleanups. They are new
in this version of the series. Patch 05/12 then aligns the code with the
reference manual for register field names, increasing readability of the
driver when read alongside the hardware documentation. Patch 06/12 fixes
a small alignment issue in register dumps, and patch 07/12 logs per-lane
start of transmission error instead of supporting the first data lane
only, easing debugging of D-PHY issues.

The next two patches deprecate the clock-frequency DT property, which
shouldn't have been added in the first place. Patch 08/12 improves
handling of the clock frequency in the driver, and patch 09/12
deprecates the property in the DT bindings. The driver still supports
the property to ensure backward compatibility.

The last three patches introduce support for multiple output channels
and wire it up in the debugging infrastructure. The CSIS IP core
supports up to 4 output channels, with the number of instantiated
channels being a property of the SoC integration. So far, only the
i.MX8MP is known to have multiple output channels. Patch 10/12 adds a
corresponding DT property, and patch 11/12 adds initial support for that
property in the driver, and uses it to dump per-channel registers and
event counters. Finally, patch 12/12 sets the property in the i.MX8MP
DT.

[1] https://lore.kernel.org/linux-media/20250819094533.2335-3-sakari.ailus@linux.intel.com

Laurent Pinchart (12):
  media: v4l2-common: Constify media_pad argument to
    v4l2_get_link_freq()
  media: imx-mipi-csis: Simplify access to source pad
  media: imx-mipi-csis: Standardize const keyword placement
  media: imx-mipi-csis: Shorten name of subdev state variables
  media: imx-mipi-csis: Rename register macros to match reference manual
  media: imx-mipi-csis: Fix field alignment in register dump
  media: imx-mipi-csis: Log per-lane start of transmission errors
  media: imx-mipi-csis: Only set clock rate when specified in DT
  dt-bindings: media: nxp,imx-mipi-csi2: Mark clock-frequency as
    deprecated
  dt-bindings: media: nxp,imx-mipi-csi2: Add fsl,num-channels property
  media: imx-mipi-csis: Initial support for multiple output channels
  arm64: dts: imx8mp: Specify the number of channels for CSI-2 receivers

 .../bindings/media/nxp,imx-mipi-csi2.yaml     |  18 +-
 arch/arm64/boot/dts/freescale/imx8mp.dtsi     |   2 +
 drivers/media/platform/nxp/imx-mipi-csis.c    | 348 +++++++++++-------
 drivers/media/v4l2-core/v4l2-common.c         |   2 +-
 include/media/v4l2-common.h                   |   3 +-
 5 files changed, 230 insertions(+), 143 deletions(-)


base-commit: a75b8d198c55e9eb5feb6f6e155496305caba2dc
-- 
Regards,

Laurent Pinchart

[PATCH v2 01/12] media: v4l2-common: Constify media_pad argument to v4l2_get_link_freq()

[Laurent Pinchart]

The v4l2_get_link_freq() macro doesn't modify the pad argument. Make it
possible to call it with a const media_pad pointer.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-common.c | 2 +-
 include/media/v4l2-common.h           | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index a5334aa35992..4d686338e345 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -551,7 +551,7 @@ s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
 EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
 
 #ifdef CONFIG_MEDIA_CONTROLLER
-s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
+s64 __v4l2_get_link_freq_pad(const struct media_pad *pad, unsigned int mul,
 			     unsigned int div)
 {
 	struct v4l2_mbus_config mbus_config = {};
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 39dd0c78d70f..ede24e60dbec 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -580,10 +580,11 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
 #ifdef CONFIG_MEDIA_CONTROLLER
 #define v4l2_get_link_freq(pad, mul, div)				\
 	_Generic(pad,							\
+		 const struct media_pad *: __v4l2_get_link_freq_pad,	\
 		 struct media_pad *: __v4l2_get_link_freq_pad,		\
 		 struct v4l2_ctrl_handler *: __v4l2_get_link_freq_ctrl)	\
 	(pad, mul, div)
-s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
+s64 __v4l2_get_link_freq_pad(const struct media_pad *pad, unsigned int mul,
 			     unsigned int div);
 #else
 #define v4l2_get_link_freq(handler, mul, div)		\
-- 
Regards,

Laurent Pinchart

[PATCH v2 02/12] media: imx-mipi-csis: Simplify access to source pad

[Laurent Pinchart]

The mipi_csis_calculate_params() function needs to access the pad of the
connected source. The pad is already available in csis->source.pad, but
the function takes a convoluted path by getting the pad index and
indexing the source subdev's pads array. Simplify it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 2beb5f43c2c0..46f93cd677e3 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -579,13 +579,11 @@ static void __mipi_csis_set_format(struct mipi_csis_device *csis,
 static int mipi_csis_calculate_params(struct mipi_csis_device *csis,
 				      const struct csis_pix_format *csis_fmt)
 {
-	struct media_pad *src_pad =
-		&csis->source.sd->entity.pads[csis->source.pad->index];
 	s64 link_freq;
 	u32 lane_rate;
 
 	/* Calculate the line rate from the pixel rate. */
-	link_freq = v4l2_get_link_freq(src_pad, csis_fmt->width,
+	link_freq = v4l2_get_link_freq(csis->source.pad, csis_fmt->width,
 				       csis->bus.num_data_lanes * 2);
 	if (link_freq < 0) {
 		dev_err(csis->dev, "Unable to obtain link frequency: %d\n",
-- 
Regards,

Laurent Pinchart

[PATCH v2 03/12] media: imx-mipi-csis: Standardize const keyword placement

[Laurent Pinchart]

The const keyword for pointer variables is placed before the type
everywhere except in one location. Change it to improve consistency.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 46f93cd677e3..0f0863011230 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -1029,7 +1029,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
 			     struct v4l2_subdev_state *sd_state,
 			     struct v4l2_subdev_format *sdformat)
 {
-	struct csis_pix_format const *csis_fmt;
+	const struct csis_pix_format *csis_fmt;
 	struct v4l2_mbus_framefmt *fmt;
 	unsigned int align;
 
-- 
Regards,

Laurent Pinchart

[PATCH v2 04/12] media: imx-mipi-csis: Shorten name of subdev state variables

[Laurent Pinchart]

Rename subdev state parameters passed to subdev operations from sd_state
to state. This standardizes the naming of the subdev state variables
through the driver, and helps shortening lines.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 0f0863011230..894d12fef519 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -996,7 +996,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 }
 
 static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
-				    struct v4l2_subdev_state *sd_state,
+				    struct v4l2_subdev_state *state,
 				    struct v4l2_subdev_mbus_code_enum *code)
 {
 	/*
@@ -1009,7 +1009,7 @@ static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
 		if (code->index > 0)
 			return -EINVAL;
 
-		fmt = v4l2_subdev_state_get_format(sd_state, code->pad);
+		fmt = v4l2_subdev_state_get_format(state, code->pad);
 		code->code = fmt->code;
 		return 0;
 	}
@@ -1026,7 +1026,7 @@ static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
 }
 
 static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
-			     struct v4l2_subdev_state *sd_state,
+			     struct v4l2_subdev_state *state,
 			     struct v4l2_subdev_format *sdformat)
 {
 	const struct csis_pix_format *csis_fmt;
@@ -1038,7 +1038,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
 	 * modified.
 	 */
 	if (sdformat->pad == CSIS_PAD_SOURCE)
-		return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
+		return v4l2_subdev_get_fmt(sd, state, sdformat);
 
 	if (sdformat->pad != CSIS_PAD_SINK)
 		return -EINVAL;
@@ -1076,7 +1076,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
 			      &sdformat->format.height, 1,
 			      CSIS_MAX_PIX_HEIGHT, 0, 0);
 
-	fmt = v4l2_subdev_state_get_format(sd_state, sdformat->pad);
+	fmt = v4l2_subdev_state_get_format(state, sdformat->pad);
 
 	fmt->code = csis_fmt->code;
 	fmt->width = sdformat->format.width;
@@ -1090,7 +1090,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
 	sdformat->format = *fmt;
 
 	/* Propagate the format from sink to source. */
-	fmt = v4l2_subdev_state_get_format(sd_state, CSIS_PAD_SOURCE);
+	fmt = v4l2_subdev_state_get_format(state, CSIS_PAD_SOURCE);
 	*fmt = sdformat->format;
 
 	/* The format on the source pad might change due to unpacking. */
@@ -1130,7 +1130,7 @@ static int mipi_csis_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 }
 
 static int mipi_csis_init_state(struct v4l2_subdev *sd,
-				struct v4l2_subdev_state *sd_state)
+				struct v4l2_subdev_state *state)
 {
 	struct v4l2_subdev_format fmt = {
 		.pad = CSIS_PAD_SINK,
@@ -1147,7 +1147,7 @@ static int mipi_csis_init_state(struct v4l2_subdev *sd,
 		V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt.format.colorspace,
 					      fmt.format.ycbcr_enc);
 
-	return mipi_csis_set_fmt(sd, sd_state, &fmt);
+	return mipi_csis_set_fmt(sd, state, &fmt);
 }
 
 static int mipi_csis_log_status(struct v4l2_subdev *sd)
-- 
Regards,

Laurent Pinchart

[PATCH v2 05/12] media: imx-mipi-csis: Rename register macros to match reference manual

[Laurent Pinchart]

The CSIS driver uses register macro names that do not match the
reference manual of the i.MX7[DS] and i.MX8M[MNP] SoCs in which the CSIS
is integrated. Rename them to match the documentation, making the code
easier to read alongside the reference manuals.

One of the misnamed register fields is MIPI_CSIS_INT_SRC_ERR_UNKNOWN,
which led to the corresponding event being logged as "Unknown Error".
The correct register field name is MIPI_CSIS_INT_SRC_ERR_ID, documented
as "Unknown ID error". Update the event description accordingly.

While at it, also replace a few *_OFFSET macros with parametric macros
for consistency, and add the missing MIPI_CSIS_ISP_RESOL_VRESOL and
MIPI_CSIS_ISP_RESOL_HRESOL register field macros.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 69 +++++++++++-----------
 1 file changed, 36 insertions(+), 33 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 894d12fef519..1ca327e6be00 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -55,13 +55,13 @@
 /* CSIS common control */
 #define MIPI_CSIS_CMN_CTRL			0x04
 #define MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW	BIT(16)
-#define MIPI_CSIS_CMN_CTRL_INTER_MODE		BIT(10)
+#define MIPI_CSIS_CMN_CTRL_INTERLEAVE_MODE_NONE	(0 << 10)
+#define MIPI_CSIS_CMN_CTRL_INTERLEAVE_MODE_DT	(1 << 10)
+#define MIPI_CSIS_CMN_CTRL_LANE_NUMBER(n)	((n) << 8)
+#define MIPI_CSIS_CMN_CTRL_LANE_NUMBER_MASK	(3 << 8)
 #define MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW_CTRL	BIT(2)
-#define MIPI_CSIS_CMN_CTRL_RESET		BIT(1)
-#define MIPI_CSIS_CMN_CTRL_ENABLE		BIT(0)
-
-#define MIPI_CSIS_CMN_CTRL_LANE_NR_OFFSET	8
-#define MIPI_CSIS_CMN_CTRL_LANE_NR_MASK		(3 << 8)
+#define MIPI_CSIS_CMN_CTRL_SW_RESET		BIT(1)
+#define MIPI_CSIS_CMN_CTRL_CSI_EN		BIT(0)
 
 /* CSIS clock control */
 #define MIPI_CSIS_CLK_CTRL			0x08
@@ -87,7 +87,7 @@
 #define MIPI_CSIS_INT_MSK_ERR_WRONG_CFG		BIT(3)
 #define MIPI_CSIS_INT_MSK_ERR_ECC		BIT(2)
 #define MIPI_CSIS_INT_MSK_ERR_CRC		BIT(1)
-#define MIPI_CSIS_INT_MSK_ERR_UNKNOWN		BIT(0)
+#define MIPI_CSIS_INT_MSK_ERR_ID		BIT(0)
 
 /* CSIS Interrupt source */
 #define MIPI_CSIS_INT_SRC			0x14
@@ -107,7 +107,7 @@
 #define MIPI_CSIS_INT_SRC_ERR_WRONG_CFG		BIT(3)
 #define MIPI_CSIS_INT_SRC_ERR_ECC		BIT(2)
 #define MIPI_CSIS_INT_SRC_ERR_CRC		BIT(1)
-#define MIPI_CSIS_INT_SRC_ERR_UNKNOWN		BIT(0)
+#define MIPI_CSIS_INT_SRC_ERR_ID		BIT(0)
 #define MIPI_CSIS_INT_SRC_ERRORS		0xfffff
 
 /* D-PHY status control */
@@ -123,8 +123,8 @@
 #define MIPI_CSIS_DPHY_CMN_CTRL_HSSETTLE_MASK	GENMASK(31, 24)
 #define MIPI_CSIS_DPHY_CMN_CTRL_CLKSETTLE(n)	((n) << 22)
 #define MIPI_CSIS_DPHY_CMN_CTRL_CLKSETTLE_MASK	GENMASK(23, 22)
-#define MIPI_CSIS_DPHY_CMN_CTRL_DPDN_SWAP_CLK	BIT(6)
-#define MIPI_CSIS_DPHY_CMN_CTRL_DPDN_SWAP_DAT	BIT(5)
+#define MIPI_CSIS_DPHY_CMN_CTRL_S_DPDN_SWAP_CLK	BIT(6)
+#define MIPI_CSIS_DPHY_CMN_CTRL_S_DPDN_SWAP_DAT	BIT(5)
 #define MIPI_CSIS_DPHY_CMN_CTRL_ENABLE_DAT	BIT(1)
 #define MIPI_CSIS_DPHY_CMN_CTRL_ENABLE_CLK	BIT(0)
 #define MIPI_CSIS_DPHY_CMN_CTRL_ENABLE		(0x1f << 0)
@@ -179,21 +179,23 @@
 #define MIPI_CSIS_ISPCFG_PIXEL_MODE_SINGLE	(0 << 12)
 #define MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL	(1 << 12)
 #define MIPI_CSIS_ISPCFG_PIXEL_MODE_QUAD	(2 << 12)	/* i.MX8M[MNP] only */
-#define MIPI_CSIS_ISPCFG_PIXEL_MASK		(3 << 12)
-#define MIPI_CSIS_ISPCFG_ALIGN_32BIT		BIT(11)
-#define MIPI_CSIS_ISPCFG_FMT(fmt)		((fmt) << 2)
-#define MIPI_CSIS_ISPCFG_FMT_MASK		(0x3f << 2)
+#define MIPI_CSIS_ISPCFG_PIXEL_MODE_MASK	(3 << 12)
+#define MIPI_CSIS_ISPCFG_PARALLEL		BIT(11)
+#define MIPI_CSIS_ISPCFG_DATAFORMAT(fmt)	((fmt) << 2)
+#define MIPI_CSIS_ISPCFG_DATAFORMAT_MASK	(0x3f << 2)
 
 /* ISP Image Resolution register */
 #define MIPI_CSIS_ISP_RESOL_CH(n)		(0x44 + (n) * 0x10)
+#define MIPI_CSIS_ISP_RESOL_VRESOL(n)		((n) << 16)
+#define MIPI_CSIS_ISP_RESOL_HRESOL(n)		((n) << 0)
 #define CSIS_MAX_PIX_WIDTH			0xffff
 #define CSIS_MAX_PIX_HEIGHT			0xffff
 
 /* ISP SYNC register */
 #define MIPI_CSIS_ISP_SYNC_CH(n)		(0x48 + (n) * 0x10)
-#define MIPI_CSIS_ISP_SYNC_HSYNC_LINTV_OFFSET	18
-#define MIPI_CSIS_ISP_SYNC_VSYNC_SINTV_OFFSET	12
-#define MIPI_CSIS_ISP_SYNC_VSYNC_EINTV_OFFSET	0
+#define MIPI_CSIS_ISP_SYNC_HSYNC_LINTV(n)	((n) << 18)
+#define MIPI_CSIS_ISP_SYNC_VSYNC_SINTV(n)	((n) << 12)
+#define MIPI_CSIS_ISP_SYNC_VSYNC_EINTV(n)	((n) << 0)
 
 /* ISP shadow registers */
 #define MIPI_CSIS_SDW_CONFIG_CH(n)		(0x80 + (n) * 0x10)
@@ -246,7 +248,7 @@ static const struct mipi_csis_event mipi_csis_events[] = {
 	{ false, MIPI_CSIS_INT_SRC_ERR_WRONG_CFG,	"Wrong Configuration Error" },
 	{ false, MIPI_CSIS_INT_SRC_ERR_ECC,		"ECC Error" },
 	{ false, MIPI_CSIS_INT_SRC_ERR_CRC,		"CRC Error" },
-	{ false, MIPI_CSIS_INT_SRC_ERR_UNKNOWN,		"Unknown Error" },
+	{ false, MIPI_CSIS_INT_SRC_ERR_ID,		"Unknown ID Error" },
 	{ true, MIPI_CSIS_DBG_INTR_SRC_DT_NOT_SUPPORT,	"Data Type Not Supported" },
 	{ true, MIPI_CSIS_DBG_INTR_SRC_DT_IGNORE,	"Data Type Ignored" },
 	{ true, MIPI_CSIS_DBG_INTR_SRC_ERR_FRAME_SIZE,	"Frame Size Error" },
@@ -517,7 +519,7 @@ static void mipi_csis_sw_reset(struct mipi_csis_device *csis)
 	u32 val = mipi_csis_read(csis, MIPI_CSIS_CMN_CTRL);
 
 	mipi_csis_write(csis, MIPI_CSIS_CMN_CTRL,
-			val | MIPI_CSIS_CMN_CTRL_RESET);
+			val | MIPI_CSIS_CMN_CTRL_SW_RESET);
 	usleep_range(10, 20);
 }
 
@@ -527,9 +529,9 @@ static void mipi_csis_system_enable(struct mipi_csis_device *csis, int on)
 
 	val = mipi_csis_read(csis, MIPI_CSIS_CMN_CTRL);
 	if (on)
-		val |= MIPI_CSIS_CMN_CTRL_ENABLE;
+		val |= MIPI_CSIS_CMN_CTRL_CSI_EN;
 	else
-		val &= ~MIPI_CSIS_CMN_CTRL_ENABLE;
+		val &= ~MIPI_CSIS_CMN_CTRL_CSI_EN;
 	mipi_csis_write(csis, MIPI_CSIS_CMN_CTRL, val);
 
 	val = mipi_csis_read(csis, MIPI_CSIS_DPHY_CMN_CTRL);
@@ -549,8 +551,8 @@ static void __mipi_csis_set_format(struct mipi_csis_device *csis,
 
 	/* Color format */
 	val = mipi_csis_read(csis, MIPI_CSIS_ISP_CONFIG_CH(0));
-	val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK
-		| MIPI_CSIS_ISPCFG_PIXEL_MASK);
+	val &= ~(MIPI_CSIS_ISPCFG_PARALLEL | MIPI_CSIS_ISPCFG_PIXEL_MODE_MASK |
+		 MIPI_CSIS_ISPCFG_DATAFORMAT_MASK);
 
 	/*
 	 * YUV 4:2:2 can be transferred with 8 or 16 bits per clock sample
@@ -568,12 +570,13 @@ static void __mipi_csis_set_format(struct mipi_csis_device *csis,
 	if (csis_fmt->data_type == MIPI_CSI2_DT_YUV422_8B)
 		val |= MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
 
-	val |= MIPI_CSIS_ISPCFG_FMT(csis_fmt->data_type);
+	val |= MIPI_CSIS_ISPCFG_DATAFORMAT(csis_fmt->data_type);
 	mipi_csis_write(csis, MIPI_CSIS_ISP_CONFIG_CH(0), val);
 
 	/* Pixel resolution */
-	val = format->width | (format->height << 16);
-	mipi_csis_write(csis, MIPI_CSIS_ISP_RESOL_CH(0), val);
+	mipi_csis_write(csis, MIPI_CSIS_ISP_RESOL_CH(0),
+			MIPI_CSIS_ISP_RESOL_VRESOL(format->height) |
+			MIPI_CSIS_ISP_RESOL_HRESOL(format->width));
 }
 
 static int mipi_csis_calculate_params(struct mipi_csis_device *csis,
@@ -633,10 +636,10 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis,
 	u32 val;
 
 	val = mipi_csis_read(csis, MIPI_CSIS_CMN_CTRL);
-	val &= ~MIPI_CSIS_CMN_CTRL_LANE_NR_MASK;
-	val |= (lanes - 1) << MIPI_CSIS_CMN_CTRL_LANE_NR_OFFSET;
+	val &= ~MIPI_CSIS_CMN_CTRL_LANE_NUMBER_MASK;
+	val |= MIPI_CSIS_CMN_CTRL_LANE_NUMBER(lanes - 1);
 	if (csis->info->version == MIPI_CSIS_V3_3)
-		val |= MIPI_CSIS_CMN_CTRL_INTER_MODE;
+		val |= MIPI_CSIS_CMN_CTRL_INTERLEAVE_MODE_DT;
 	mipi_csis_write(csis, MIPI_CSIS_CMN_CTRL, val);
 
 	__mipi_csis_set_format(csis, format, csis_fmt);
@@ -645,10 +648,10 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis,
 			MIPI_CSIS_DPHY_CMN_CTRL_HSSETTLE(csis->hs_settle) |
 			MIPI_CSIS_DPHY_CMN_CTRL_CLKSETTLE(csis->clk_settle));
 
-	val = (0 << MIPI_CSIS_ISP_SYNC_HSYNC_LINTV_OFFSET)
-	    | (0 << MIPI_CSIS_ISP_SYNC_VSYNC_SINTV_OFFSET)
-	    | (0 << MIPI_CSIS_ISP_SYNC_VSYNC_EINTV_OFFSET);
-	mipi_csis_write(csis, MIPI_CSIS_ISP_SYNC_CH(0), val);
+	mipi_csis_write(csis, MIPI_CSIS_ISP_SYNC_CH(0),
+			MIPI_CSIS_ISP_SYNC_HSYNC_LINTV(0) |
+			MIPI_CSIS_ISP_SYNC_VSYNC_SINTV(0) |
+			MIPI_CSIS_ISP_SYNC_VSYNC_EINTV(0));
 
 	val = mipi_csis_read(csis, MIPI_CSIS_CLK_CTRL);
 	val |= MIPI_CSIS_CLK_CTRL_WCLK_SRC;
-- 
Regards,

Laurent Pinchart

[PATCH v2 06/12] media: imx-mipi-csis: Fix field alignment in register dump

[Laurent Pinchart]

Commit 95a1379004cb ("media: staging: media: imx: imx7-mipi-csis: Dump
MIPI_CSIS_FRAME_COUNTER_CH0 register") forgot to increase the maximum
register name length, resulting in misalignment of names printed in the
kernel log. Fix it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 1ca327e6be00..35c8d5309424 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -893,7 +893,7 @@ static int mipi_csis_dump_regs(struct mipi_csis_device *csis)
 
 	for (i = 0; i < ARRAY_SIZE(registers); i++) {
 		cfg = mipi_csis_read(csis, registers[i].offset);
-		dev_info(csis->dev, "%14s: 0x%08x\n", registers[i].name, cfg);
+		dev_info(csis->dev, "%17s: 0x%08x\n", registers[i].name, cfg);
 	}
 
 	pm_runtime_put(csis->dev);
-- 
Regards,

Laurent Pinchart

[PATCH v2 07/12] media: imx-mipi-csis: Log per-lane start of transmission errors

[Laurent Pinchart]

The CSIS has per-line start of transmission error interrupts. Log them
all, instead of only the first data lane.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 35c8d5309424..67da8326540b 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -100,7 +100,7 @@
 #define MIPI_CSIS_INT_SRC_NON_IMAGE_DATA	(0xf << 28)
 #define MIPI_CSIS_INT_SRC_FRAME_START		BIT(24)
 #define MIPI_CSIS_INT_SRC_FRAME_END		BIT(20)
-#define MIPI_CSIS_INT_SRC_ERR_SOT_HS		BIT(16)
+#define MIPI_CSIS_INT_SRC_ERR_SOT_HS(n)		BIT((n) + 16)
 #define MIPI_CSIS_INT_SRC_ERR_LOST_FS		BIT(12)
 #define MIPI_CSIS_INT_SRC_ERR_LOST_FE		BIT(8)
 #define MIPI_CSIS_INT_SRC_ERR_OVER		BIT(4)
@@ -241,7 +241,10 @@ struct mipi_csis_event {
 
 static const struct mipi_csis_event mipi_csis_events[] = {
 	/* Errors */
-	{ false, MIPI_CSIS_INT_SRC_ERR_SOT_HS,		"SOT Error" },
+	{ false, MIPI_CSIS_INT_SRC_ERR_SOT_HS(0),	"SOT 0 Error" },
+	{ false, MIPI_CSIS_INT_SRC_ERR_SOT_HS(1),	"SOT 1 Error" },
+	{ false, MIPI_CSIS_INT_SRC_ERR_SOT_HS(2),	"SOT 2 Error" },
+	{ false, MIPI_CSIS_INT_SRC_ERR_SOT_HS(3),	"SOT 3 Error" },
 	{ false, MIPI_CSIS_INT_SRC_ERR_LOST_FS,		"Lost Frame Start Error" },
 	{ false, MIPI_CSIS_INT_SRC_ERR_LOST_FE,		"Lost Frame End Error" },
 	{ false, MIPI_CSIS_INT_SRC_ERR_OVER,		"FIFO Overflow Error" },
-- 
Regards,

Laurent Pinchart

[PATCH v2 08/12] media: imx-mipi-csis: Only set clock rate when specified in DT

[Laurent Pinchart]

The imx-mipi-csis driver sets the rate of the wrap clock to the value
specified in the device tree's "clock-frequency" property, and defaults
to 166 MHz otherwise. This is a historical mistake, as clock rate
selection should have been left to the assigned-clock-rates property.

Honouring the clock-frequency property can't be removed without breaking
backwards compatibility, and the corresponding code isn't very
intrusive. The 166 MHz default, on the other hand, prevents
configuration of the clock rate through assigned-clock-rates, as the
driver immediately overwrites the rate. This behaviour is confusing and
has cost debugging time.

There is little value in a 166 MHz default. All mainline device tree
sources that enable the CSIS specify a clock-frequency explicitly, and
the default wrap clock configuration on supported platforms is at least
as high as 166 MHz. Drop the default, and only set the clock rate
manually when the clock-frequency property is specified.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 23 +++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 67da8326540b..83ba68a20bd1 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -230,8 +230,6 @@
 #define MIPI_CSIS_PKTDATA_EVEN			0x3000
 #define MIPI_CSIS_PKTDATA_SIZE			SZ_4K
 
-#define DEFAULT_SCLK_CSIS_FREQ			166000000UL
-
 struct mipi_csis_event {
 	bool debug;
 	u32 mask;
@@ -708,12 +706,17 @@ static int mipi_csis_clk_get(struct mipi_csis_device *csis)
 	if (ret < 0)
 		return ret;
 
-	/* Set clock rate */
-	ret = clk_set_rate(csis->clks[MIPI_CSIS_CLK_WRAP].clk,
-			   csis->clk_frequency);
-	if (ret < 0)
-		dev_err(csis->dev, "set rate=%d failed: %d\n",
-			csis->clk_frequency, ret);
+	if (csis->clk_frequency) {
+		/*
+		 * Set the clock rate. This is deprecated, for backward
+		 * compatibility with old device trees.
+		 */
+		ret = clk_set_rate(csis->clks[MIPI_CSIS_CLK_WRAP].clk,
+				   csis->clk_frequency);
+		if (ret < 0)
+			dev_err(csis->dev, "set rate=%d failed: %d\n",
+				csis->clk_frequency, ret);
+	}
 
 	return ret;
 }
@@ -1417,9 +1420,7 @@ static int mipi_csis_parse_dt(struct mipi_csis_device *csis)
 {
 	struct device_node *node = csis->dev->of_node;
 
-	if (of_property_read_u32(node, "clock-frequency",
-				 &csis->clk_frequency))
-		csis->clk_frequency = DEFAULT_SCLK_CSIS_FREQ;
+	of_property_read_u32(node, "clock-frequency", &csis->clk_frequency);
 
 	return 0;
 }
-- 
Regards,

Laurent Pinchart

[PATCH v2 09/12] dt-bindings: media: nxp,imx-mipi-csi2: Mark clock-frequency as deprecated

[Laurent Pinchart]

Usage of the clock-frequency property, which is already optional, is
discouraged in favour of using assigned-clock-rates (and
assigned-clock-parents where needed). Mark the property as deprecated,
and update the examples accordingly.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
 .../devicetree/bindings/media/nxp,imx-mipi-csi2.yaml  | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml b/Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
index 03a23a26c4f3..db4889bf881e 100644
--- a/Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
+++ b/Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
@@ -66,6 +66,7 @@ properties:
   clock-frequency:
     description: The desired external clock ("wrap") frequency, in Hz
     default: 166000000
+    deprecated: true
 
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
@@ -147,7 +148,9 @@ examples:
                  <&clks IMX7D_MIPI_CSI_ROOT_CLK>,
                  <&clks IMX7D_MIPI_DPHY_ROOT_CLK>;
         clock-names = "pclk", "wrap", "phy";
-        clock-frequency = <166000000>;
+
+        assigned-clocks = <&clks IMX7D_MIPI_CSI_ROOT_CLK>;
+        assigned-clock-rates = <166000000>;
 
         power-domains = <&pgc_mipi_phy>;
         phy-supply = <&reg_1p0d>;
@@ -185,12 +188,16 @@ examples:
         compatible = "fsl,imx8mm-mipi-csi2";
         reg = <0x32e30000 0x1000>;
         interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
-        clock-frequency = <333000000>;
+
         clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>,
                  <&clk IMX8MM_CLK_CSI1_ROOT>,
                  <&clk IMX8MM_CLK_CSI1_PHY_REF>,
                  <&clk IMX8MM_CLK_DISP_AXI_ROOT>;
         clock-names = "pclk", "wrap", "phy", "axi";
+
+        assigned-clocks = <&clk IMX8MM_CLK_CSI1_ROOT>;
+        assigned-clock-rates = <250000000>;
+
         power-domains = <&mipi_pd>;
 
         ports {
-- 
Regards,

Laurent Pinchart

[PATCH v2 10/12] dt-bindings: media: nxp,imx-mipi-csi2: Add fsl,num-channels property

[Laurent Pinchart]

The CSI-2 receiver can be instantiated with up to four output channels.
This is an integration-specific property, specify the number of
instantiated channels through a new fsl,num-channels property. The
property is optional, and defaults to 1 as only one channel is currently
supported by drivers.

Using the compatible string to infer the number of channels has been
considered, but multiple instances of the same CSIS in the same SoC
could conceptually be synthesized with a different number of channels.
An explicit property is therefore more appropriate.

The only known SoC to have more than one channel is the i.MX8MP. As the
binding examples do not cover that SoC, don't update them.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
Changes since v1:

- Expand commit message
---
 .../devicetree/bindings/media/nxp,imx-mipi-csi2.yaml       | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml b/Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
index db4889bf881e..41ad5b84eaeb 100644
--- a/Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
+++ b/Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
@@ -68,6 +68,13 @@ properties:
     default: 166000000
     deprecated: true
 
+  fsl,num-channels:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Number of output channels
+    minimum: 1
+    maximum: 4
+    default: 1
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
 
-- 
Regards,

Laurent Pinchart

[PATCH v2 11/12] media: imx-mipi-csis: Initial support for multiple output channels

[Laurent Pinchart]

Some CSIS instances feature more than one output channel. Update
register macros accordingly, parse the number of channels from the
device tree, and update register dumps and event counters to log
per-channel data.

Support for routing virtual channels and data types to output channels
through the subdev internal routing API will come later.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Update more per-channel registers
- Update commit message
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 239 +++++++++++++--------
 1 file changed, 152 insertions(+), 87 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 83ba68a20bd1..b1136336a57f 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -54,7 +54,7 @@
 
 /* CSIS common control */
 #define MIPI_CSIS_CMN_CTRL			0x04
-#define MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW	BIT(16)
+#define MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW(n)	BIT((n) + 16)
 #define MIPI_CSIS_CMN_CTRL_INTERLEAVE_MODE_NONE	(0 << 10)
 #define MIPI_CSIS_CMN_CTRL_INTERLEAVE_MODE_DT	(1 << 10)
 #define MIPI_CSIS_CMN_CTRL_LANE_NUMBER(n)	((n) << 8)
@@ -65,12 +65,9 @@
 
 /* CSIS clock control */
 #define MIPI_CSIS_CLK_CTRL			0x08
-#define MIPI_CSIS_CLK_CTRL_CLKGATE_TRAIL_CH3(x)	((x) << 28)
-#define MIPI_CSIS_CLK_CTRL_CLKGATE_TRAIL_CH2(x)	((x) << 24)
-#define MIPI_CSIS_CLK_CTRL_CLKGATE_TRAIL_CH1(x)	((x) << 20)
-#define MIPI_CSIS_CLK_CTRL_CLKGATE_TRAIL_CH0(x)	((x) << 16)
+#define MIPI_CSIS_CLK_CTRL_CLKGATE_TRAIL(n, x)	((x) << ((n) * 4 + 16))
 #define MIPI_CSIS_CLK_CTRL_CLKGATE_EN_MSK	(0xf << 4)
-#define MIPI_CSIS_CLK_CTRL_WCLK_SRC		BIT(0)
+#define MIPI_CSIS_CLK_CTRL_WCLK_SRC(n)		BIT(n)
 
 /* CSIS Interrupt mask */
 #define MIPI_CSIS_INT_MSK			0x10
@@ -98,12 +95,12 @@
 #define MIPI_CSIS_INT_SRC_ODD_AFTER		BIT(28)
 #define MIPI_CSIS_INT_SRC_ODD			(0x3 << 28)
 #define MIPI_CSIS_INT_SRC_NON_IMAGE_DATA	(0xf << 28)
-#define MIPI_CSIS_INT_SRC_FRAME_START		BIT(24)
-#define MIPI_CSIS_INT_SRC_FRAME_END		BIT(20)
+#define MIPI_CSIS_INT_SRC_FRAME_START(n)	BIT((n) + 24)
+#define MIPI_CSIS_INT_SRC_FRAME_END(n)		BIT((n) + 20)
 #define MIPI_CSIS_INT_SRC_ERR_SOT_HS(n)		BIT((n) + 16)
-#define MIPI_CSIS_INT_SRC_ERR_LOST_FS		BIT(12)
-#define MIPI_CSIS_INT_SRC_ERR_LOST_FE		BIT(8)
-#define MIPI_CSIS_INT_SRC_ERR_OVER		BIT(4)
+#define MIPI_CSIS_INT_SRC_ERR_LOST_FS(n)	BIT((n) + 12)
+#define MIPI_CSIS_INT_SRC_ERR_LOST_FE(n)	BIT((n) + 8)
+#define MIPI_CSIS_INT_SRC_ERR_OVER(n)		BIT((n) + 4)
 #define MIPI_CSIS_INT_SRC_ERR_WRONG_CFG		BIT(3)
 #define MIPI_CSIS_INT_SRC_ERR_ECC		BIT(2)
 #define MIPI_CSIS_INT_SRC_ERR_CRC		BIT(1)
@@ -205,23 +202,23 @@
 /* Debug control register */
 #define MIPI_CSIS_DBG_CTRL			0xc0
 #define MIPI_CSIS_DBG_INTR_MSK			0xc4
-#define MIPI_CSIS_DBG_INTR_MSK_DT_NOT_SUPPORT	BIT(25)
-#define MIPI_CSIS_DBG_INTR_MSK_DT_IGNORE	BIT(24)
-#define MIPI_CSIS_DBG_INTR_MSK_ERR_FRAME_SIZE	BIT(20)
-#define MIPI_CSIS_DBG_INTR_MSK_TRUNCATED_FRAME	BIT(16)
-#define MIPI_CSIS_DBG_INTR_MSK_EARLY_FE		BIT(12)
-#define MIPI_CSIS_DBG_INTR_MSK_EARLY_FS		BIT(8)
-#define MIPI_CSIS_DBG_INTR_MSK_CAM_VSYNC_FALL	BIT(4)
-#define MIPI_CSIS_DBG_INTR_MSK_CAM_VSYNC_RISE	BIT(0)
+#define MIPI_CSIS_DBG_INTR_MSK_DT_NOT_SUPPORT		BIT(25)
+#define MIPI_CSIS_DBG_INTR_MSK_DT_IGNORE		BIT(24)
+#define MIPI_CSIS_DBG_INTR_MSK_ERR_FRAME_SIZE(n)	BIT((n) + 20)
+#define MIPI_CSIS_DBG_INTR_MSK_TRUNCATED_FRAME(n)	BIT((n) + 16)
+#define MIPI_CSIS_DBG_INTR_MSK_EARLY_FE(n)		BIT((n) + 12)
+#define MIPI_CSIS_DBG_INTR_MSK_EARLY_FS(n)		BIT((n) + 8)
+#define MIPI_CSIS_DBG_INTR_MSK_CAM_VSYNC_FALL(n)	BIT((n) + 4)
+#define MIPI_CSIS_DBG_INTR_MSK_CAM_VSYNC_RISE(n)	BIT((n) + 0)
 #define MIPI_CSIS_DBG_INTR_SRC			0xc8
-#define MIPI_CSIS_DBG_INTR_SRC_DT_NOT_SUPPORT	BIT(25)
-#define MIPI_CSIS_DBG_INTR_SRC_DT_IGNORE	BIT(24)
-#define MIPI_CSIS_DBG_INTR_SRC_ERR_FRAME_SIZE	BIT(20)
-#define MIPI_CSIS_DBG_INTR_SRC_TRUNCATED_FRAME	BIT(16)
-#define MIPI_CSIS_DBG_INTR_SRC_EARLY_FE		BIT(12)
-#define MIPI_CSIS_DBG_INTR_SRC_EARLY_FS		BIT(8)
-#define MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL	BIT(4)
-#define MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE	BIT(0)
+#define MIPI_CSIS_DBG_INTR_SRC_DT_NOT_SUPPORT		BIT(25)
+#define MIPI_CSIS_DBG_INTR_SRC_DT_IGNORE		BIT(24)
+#define MIPI_CSIS_DBG_INTR_SRC_ERR_FRAME_SIZE(n)	BIT((n) + 20)
+#define MIPI_CSIS_DBG_INTR_SRC_TRUNCATED_FRAME(n)	BIT((n) + 16)
+#define MIPI_CSIS_DBG_INTR_SRC_EARLY_FE(n)		BIT((n) + 12)
+#define MIPI_CSIS_DBG_INTR_SRC_EARLY_FS(n)		BIT((n) + 8)
+#define MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(n)	BIT((n) + 4)
+#define MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(n)	BIT((n) + 0)
 
 #define MIPI_CSIS_FRAME_COUNTER_CH(n)		(0x0100 + (n) * 4)
 
@@ -230,8 +227,11 @@
 #define MIPI_CSIS_PKTDATA_EVEN			0x3000
 #define MIPI_CSIS_PKTDATA_SIZE			SZ_4K
 
+#define MIPI_CSIS_MAX_CHANNELS			4
+
 struct mipi_csis_event {
 	bool debug;
+	unsigned int channel;
 	u32 mask;
 	const char * const name;
 	unsigned int counter;
@@ -239,36 +239,70 @@ struct mipi_csis_event {
 
 static const struct mipi_csis_event mipi_csis_events[] = {
 	/* Errors */
-	{ false, MIPI_CSIS_INT_SRC_ERR_SOT_HS(0),	"SOT 0 Error" },
-	{ false, MIPI_CSIS_INT_SRC_ERR_SOT_HS(1),	"SOT 1 Error" },
-	{ false, MIPI_CSIS_INT_SRC_ERR_SOT_HS(2),	"SOT 2 Error" },
-	{ false, MIPI_CSIS_INT_SRC_ERR_SOT_HS(3),	"SOT 3 Error" },
-	{ false, MIPI_CSIS_INT_SRC_ERR_LOST_FS,		"Lost Frame Start Error" },
-	{ false, MIPI_CSIS_INT_SRC_ERR_LOST_FE,		"Lost Frame End Error" },
-	{ false, MIPI_CSIS_INT_SRC_ERR_OVER,		"FIFO Overflow Error" },
-	{ false, MIPI_CSIS_INT_SRC_ERR_WRONG_CFG,	"Wrong Configuration Error" },
-	{ false, MIPI_CSIS_INT_SRC_ERR_ECC,		"ECC Error" },
-	{ false, MIPI_CSIS_INT_SRC_ERR_CRC,		"CRC Error" },
-	{ false, MIPI_CSIS_INT_SRC_ERR_ID,		"Unknown ID Error" },
-	{ true, MIPI_CSIS_DBG_INTR_SRC_DT_NOT_SUPPORT,	"Data Type Not Supported" },
-	{ true, MIPI_CSIS_DBG_INTR_SRC_DT_IGNORE,	"Data Type Ignored" },
-	{ true, MIPI_CSIS_DBG_INTR_SRC_ERR_FRAME_SIZE,	"Frame Size Error" },
-	{ true, MIPI_CSIS_DBG_INTR_SRC_TRUNCATED_FRAME,	"Truncated Frame" },
-	{ true, MIPI_CSIS_DBG_INTR_SRC_EARLY_FE,	"Early Frame End" },
-	{ true, MIPI_CSIS_DBG_INTR_SRC_EARLY_FS,	"Early Frame Start" },
+	{ false, 0, MIPI_CSIS_INT_SRC_ERR_SOT_HS(0),		"SOT 0 Error" },
+	{ false, 0, MIPI_CSIS_INT_SRC_ERR_SOT_HS(1),		"SOT 1 Error" },
+	{ false, 0, MIPI_CSIS_INT_SRC_ERR_SOT_HS(2),		"SOT 2 Error" },
+	{ false, 0, MIPI_CSIS_INT_SRC_ERR_SOT_HS(3),		"SOT 3 Error" },
+	{ false, 0, MIPI_CSIS_INT_SRC_ERR_LOST_FS(0),		"Lost Frame Start Error 0" },
+	{ false, 1, MIPI_CSIS_INT_SRC_ERR_LOST_FS(1),		"Lost Frame Start Error 1" },
+	{ false, 2, MIPI_CSIS_INT_SRC_ERR_LOST_FS(2),		"Lost Frame Start Error 2" },
+	{ false, 3, MIPI_CSIS_INT_SRC_ERR_LOST_FS(3),		"Lost Frame Start Error 3" },
+	{ false, 0, MIPI_CSIS_INT_SRC_ERR_LOST_FE(0),		"Lost Frame End Error 0" },
+	{ false, 1, MIPI_CSIS_INT_SRC_ERR_LOST_FE(1),		"Lost Frame End Error 1" },
+	{ false, 2, MIPI_CSIS_INT_SRC_ERR_LOST_FE(2),		"Lost Frame End Error 2" },
+	{ false, 3, MIPI_CSIS_INT_SRC_ERR_LOST_FE(3),		"Lost Frame End Error 3" },
+	{ false, 0, MIPI_CSIS_INT_SRC_ERR_OVER(0),		"FIFO Overflow Error 0" },
+	{ false, 1, MIPI_CSIS_INT_SRC_ERR_OVER(1),		"FIFO Overflow Error 1" },
+	{ false, 2, MIPI_CSIS_INT_SRC_ERR_OVER(2),		"FIFO Overflow Error 2" },
+	{ false, 3, MIPI_CSIS_INT_SRC_ERR_OVER(3),		"FIFO Overflow Error 3" },
+	{ false, 0, MIPI_CSIS_INT_SRC_ERR_WRONG_CFG,		"Wrong Configuration Error" },
+	{ false, 0, MIPI_CSIS_INT_SRC_ERR_ECC,			"ECC Error" },
+	{ false, 0, MIPI_CSIS_INT_SRC_ERR_CRC,			"CRC Error" },
+	{ false, 0, MIPI_CSIS_INT_SRC_ERR_ID,			"Unknown ID Error" },
+	{ true, 0, MIPI_CSIS_DBG_INTR_SRC_DT_NOT_SUPPORT,	"Data Type Not Supported" },
+	{ true, 0, MIPI_CSIS_DBG_INTR_SRC_DT_IGNORE,		"Data Type Ignored" },
+	{ true, 0, MIPI_CSIS_DBG_INTR_SRC_ERR_FRAME_SIZE(0),	"Frame Size Error 0" },
+	{ true, 1, MIPI_CSIS_DBG_INTR_SRC_ERR_FRAME_SIZE(1),	"Frame Size Error 1" },
+	{ true, 2, MIPI_CSIS_DBG_INTR_SRC_ERR_FRAME_SIZE(2),	"Frame Size Error 2" },
+	{ true, 3, MIPI_CSIS_DBG_INTR_SRC_ERR_FRAME_SIZE(3),	"Frame Size Error 3" },
+	{ true, 0, MIPI_CSIS_DBG_INTR_SRC_TRUNCATED_FRAME(0),	"Truncated Frame 0" },
+	{ true, 1, MIPI_CSIS_DBG_INTR_SRC_TRUNCATED_FRAME(1),	"Truncated Frame 1" },
+	{ true, 2, MIPI_CSIS_DBG_INTR_SRC_TRUNCATED_FRAME(2),	"Truncated Frame 2" },
+	{ true, 3, MIPI_CSIS_DBG_INTR_SRC_TRUNCATED_FRAME(3),	"Truncated Frame 3" },
+	{ true, 0, MIPI_CSIS_DBG_INTR_SRC_EARLY_FE(0),		"Early Frame End 0" },
+	{ true, 1, MIPI_CSIS_DBG_INTR_SRC_EARLY_FE(1),		"Early Frame End 1" },
+	{ true, 2, MIPI_CSIS_DBG_INTR_SRC_EARLY_FE(2),		"Early Frame End 2" },
+	{ true, 3, MIPI_CSIS_DBG_INTR_SRC_EARLY_FE(3),		"Early Frame End 3" },
+	{ true, 0, MIPI_CSIS_DBG_INTR_SRC_EARLY_FS(0),		"Early Frame Start 0" },
+	{ true, 1, MIPI_CSIS_DBG_INTR_SRC_EARLY_FS(1),		"Early Frame Start 1" },
+	{ true, 2, MIPI_CSIS_DBG_INTR_SRC_EARLY_FS(2),		"Early Frame Start 2" },
+	{ true, 3, MIPI_CSIS_DBG_INTR_SRC_EARLY_FS(3),		"Early Frame Start 3" },
 	/* Non-image data receive events */
-	{ false, MIPI_CSIS_INT_SRC_EVEN_BEFORE,		"Non-image data before even frame" },
-	{ false, MIPI_CSIS_INT_SRC_EVEN_AFTER,		"Non-image data after even frame" },
-	{ false, MIPI_CSIS_INT_SRC_ODD_BEFORE,		"Non-image data before odd frame" },
-	{ false, MIPI_CSIS_INT_SRC_ODD_AFTER,		"Non-image data after odd frame" },
+	{ false, 0, MIPI_CSIS_INT_SRC_EVEN_BEFORE,		"Non-image data before even frame" },
+	{ false, 0, MIPI_CSIS_INT_SRC_EVEN_AFTER,		"Non-image data after even frame" },
+	{ false, 0, MIPI_CSIS_INT_SRC_ODD_BEFORE,		"Non-image data before odd frame" },
+	{ false, 0, MIPI_CSIS_INT_SRC_ODD_AFTER,		"Non-image data after odd frame" },
 	/* Frame start/end */
-	{ false, MIPI_CSIS_INT_SRC_FRAME_START,		"Frame Start" },
-	{ false, MIPI_CSIS_INT_SRC_FRAME_END,		"Frame End" },
-	{ true, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL,	"VSYNC Falling Edge" },
-	{ true, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE,	"VSYNC Rising Edge" },
+	{ false, 0, MIPI_CSIS_INT_SRC_FRAME_START(0),		"Frame Start 0" },
+	{ false, 1, MIPI_CSIS_INT_SRC_FRAME_START(1),		"Frame Start 1" },
+	{ false, 2, MIPI_CSIS_INT_SRC_FRAME_START(2),		"Frame Start 2" },
+	{ false, 3, MIPI_CSIS_INT_SRC_FRAME_START(3),		"Frame Start 3" },
+	{ false, 0, MIPI_CSIS_INT_SRC_FRAME_END(0),		"Frame End 0" },
+	{ false, 1, MIPI_CSIS_INT_SRC_FRAME_END(1),		"Frame End 1" },
+	{ false, 2, MIPI_CSIS_INT_SRC_FRAME_END(2),		"Frame End 2" },
+	{ false, 3, MIPI_CSIS_INT_SRC_FRAME_END(3),		"Frame End 3" },
+	{ true, 0, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(0),	"VSYNC Falling Edge 0" },
+	{ true, 1, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(1),	"VSYNC Falling Edge 1" },
+	{ true, 2, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(2),	"VSYNC Falling Edge 2" },
+	{ true, 3, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(3),	"VSYNC Falling Edge 3" },
+	{ true, 0, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(0),	"VSYNC Rising Edge 0" },
+	{ true, 1, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(1),	"VSYNC Rising Edge 1" },
+	{ true, 2, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(2),	"VSYNC Rising Edge 2" },
+	{ true, 3, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(3),	"VSYNC Rising Edge 3" },
 };
 
-#define MIPI_CSIS_NUM_EVENTS ARRAY_SIZE(mipi_csis_events)
+#define MIPI_CSIS_NUM_EVENTS		ARRAY_SIZE(mipi_csis_events)
+#define MIPI_CSIS_NUM_ERROR_EVENTS	(MIPI_CSIS_NUM_EVENTS - 20)
 
 enum mipi_csis_clk {
 	MIPI_CSIS_CLK_PCLK,
@@ -300,7 +334,9 @@ struct mipi_csis_device {
 	struct clk_bulk_data *clks;
 	struct reset_control *mrst;
 	struct regulator *mipi_phy_regulator;
+
 	const struct mipi_csis_info *info;
+	unsigned int num_channels;
 
 	struct v4l2_subdev sd;
 	struct media_pad pads[CSIS_PADS_NUM];
@@ -655,8 +691,8 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis,
 			MIPI_CSIS_ISP_SYNC_VSYNC_EINTV(0));
 
 	val = mipi_csis_read(csis, MIPI_CSIS_CLK_CTRL);
-	val |= MIPI_CSIS_CLK_CTRL_WCLK_SRC;
-	val |= MIPI_CSIS_CLK_CTRL_CLKGATE_TRAIL_CH0(15);
+	val |= MIPI_CSIS_CLK_CTRL_WCLK_SRC(0);
+	val |= MIPI_CSIS_CLK_CTRL_CLKGATE_TRAIL(0, 15);
 	val &= ~MIPI_CSIS_CLK_CTRL_CLKGATE_EN_MSK;
 	mipi_csis_write(csis, MIPI_CSIS_CLK_CTRL, val);
 
@@ -673,7 +709,7 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis,
 	/* Update the shadow register. */
 	val = mipi_csis_read(csis, MIPI_CSIS_CMN_CTRL);
 	mipi_csis_write(csis, MIPI_CSIS_CMN_CTRL,
-			val | MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW |
+			val | MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW(0) |
 			MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW_CTRL);
 }
 
@@ -764,16 +800,19 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
 
 	/* Update the event/error counters */
 	if ((status & MIPI_CSIS_INT_SRC_ERRORS) || csis->debug.enable) {
-		for (i = 0; i < MIPI_CSIS_NUM_EVENTS; i++) {
+		for (i = 0; i < ARRAY_SIZE(csis->events); i++) {
 			struct mipi_csis_event *event = &csis->events[i];
 
+			if (event->channel >= csis->num_channels)
+				continue;
+
 			if ((!event->debug && (status & event->mask)) ||
 			    (event->debug && (dbg_status & event->mask)))
 				event->counter++;
 		}
 	}
 
-	if (status & MIPI_CSIS_INT_SRC_FRAME_START)
+	if (status & MIPI_CSIS_INT_SRC_FRAME_START(0))
 		mipi_csis_queue_event_sof(csis);
 
 	spin_unlock_irqrestore(&csis->slock, flags);
@@ -850,7 +889,7 @@ static void mipi_csis_clear_counters(struct mipi_csis_device *csis)
 static void mipi_csis_log_counters(struct mipi_csis_device *csis, bool non_errors)
 {
 	unsigned int num_events = non_errors ? MIPI_CSIS_NUM_EVENTS
-				: MIPI_CSIS_NUM_EVENTS - 8;
+				: MIPI_CSIS_NUM_ERROR_EVENTS;
 	unsigned int counters[MIPI_CSIS_NUM_EVENTS];
 	unsigned long flags;
 	unsigned int i;
@@ -861,45 +900,67 @@ static void mipi_csis_log_counters(struct mipi_csis_device *csis, bool non_error
 	spin_unlock_irqrestore(&csis->slock, flags);
 
 	for (i = 0; i < num_events; ++i) {
+		const struct mipi_csis_event *event = &csis->events[i];
+
+		if (event->channel >= csis->num_channels)
+			continue;
+
 		if (counters[i] > 0 || csis->debug.enable)
 			dev_info(csis->dev, "%s events: %d\n",
-				 csis->events[i].name,
-				 counters[i]);
+				 event->name, counters[i]);
 	}
 }
 
+struct mipi_csis_reg_info {
+	u32 addr;
+	unsigned int offset;
+	const char * const name;
+};
+
+static void mipi_csis_dump_channel_reg(struct mipi_csis_device *csis,
+				       const struct mipi_csis_reg_info *reg,
+				       unsigned int channel)
+{
+	dev_info(csis->dev, "%16s%u: 0x%08x\n", reg->name, channel,
+		 mipi_csis_read(csis, reg->addr + channel * reg->offset));
+}
+
 static int mipi_csis_dump_regs(struct mipi_csis_device *csis)
 {
-	static const struct {
-		u32 offset;
-		const char * const name;
-	} registers[] = {
-		{ MIPI_CSIS_CMN_CTRL, "CMN_CTRL" },
-		{ MIPI_CSIS_CLK_CTRL, "CLK_CTRL" },
-		{ MIPI_CSIS_INT_MSK, "INT_MSK" },
-		{ MIPI_CSIS_DPHY_STATUS, "DPHY_STATUS" },
-		{ MIPI_CSIS_DPHY_CMN_CTRL, "DPHY_CMN_CTRL" },
-		{ MIPI_CSIS_DPHY_SCTRL_L, "DPHY_SCTRL_L" },
-		{ MIPI_CSIS_DPHY_SCTRL_H, "DPHY_SCTRL_H" },
-		{ MIPI_CSIS_ISP_CONFIG_CH(0), "ISP_CONFIG_CH0" },
-		{ MIPI_CSIS_ISP_RESOL_CH(0), "ISP_RESOL_CH0" },
-		{ MIPI_CSIS_SDW_CONFIG_CH(0), "SDW_CONFIG_CH0" },
-		{ MIPI_CSIS_SDW_RESOL_CH(0), "SDW_RESOL_CH0" },
-		{ MIPI_CSIS_DBG_CTRL, "DBG_CTRL" },
-		{ MIPI_CSIS_FRAME_COUNTER_CH(0), "FRAME_COUNTER_CH0" },
+	static const struct mipi_csis_reg_info common_registers[] = {
+		{ MIPI_CSIS_CMN_CTRL, 0, "CMN_CTRL" },
+		{ MIPI_CSIS_CLK_CTRL, 0, "CLK_CTRL" },
+		{ MIPI_CSIS_INT_MSK, 0, "INT_MSK" },
+		{ MIPI_CSIS_DPHY_STATUS, 0, "DPHY_STATUS" },
+		{ MIPI_CSIS_DPHY_CMN_CTRL, 0, "DPHY_CMN_CTRL" },
+		{ MIPI_CSIS_DPHY_SCTRL_L, 0, "DPHY_SCTRL_L" },
+		{ MIPI_CSIS_DPHY_SCTRL_H, 0, "DPHY_SCTRL_H" },
+		{ MIPI_CSIS_DBG_CTRL, 0, "DBG_CTRL" },
+	};
+	static const struct mipi_csis_reg_info channel_registers[] = {
+		{ MIPI_CSIS_ISP_CONFIG_CH(0), 0x10, "ISP_CONFIG_CH" },
+		{ MIPI_CSIS_ISP_RESOL_CH(0), 0x10, "ISP_RESOL_CH" },
+		{ MIPI_CSIS_SDW_CONFIG_CH(0), 0x10, "SDW_CONFIG_CH" },
+		{ MIPI_CSIS_SDW_RESOL_CH(0), 0x10, "SDW_RESOL_CH" },
+		{ MIPI_CSIS_FRAME_COUNTER_CH(0), 4, "FRAME_COUNTER_CH" },
 	};
-
-	unsigned int i;
-	u32 cfg;
 
 	if (!pm_runtime_get_if_in_use(csis->dev))
 		return 0;
 
 	dev_info(csis->dev, "--- REGISTERS ---\n");
 
-	for (i = 0; i < ARRAY_SIZE(registers); i++) {
-		cfg = mipi_csis_read(csis, registers[i].offset);
-		dev_info(csis->dev, "%17s: 0x%08x\n", registers[i].name, cfg);
+	for (unsigned int i = 0; i < ARRAY_SIZE(common_registers); i++) {
+		const struct mipi_csis_reg_info *reg = &common_registers[i];
+
+		dev_info(csis->dev, "%17s: 0x%08x\n", reg->name,
+			 mipi_csis_read(csis, reg->addr));
+	}
+
+	for (unsigned int chan = 0; chan < csis->num_channels; chan++) {
+		for (unsigned int i = 0; i < ARRAY_SIZE(channel_registers); ++i)
+			mipi_csis_dump_channel_reg(csis, &channel_registers[i],
+						   chan);
 	}
 
 	pm_runtime_put(csis->dev);
@@ -1422,6 +1483,12 @@ static int mipi_csis_parse_dt(struct mipi_csis_device *csis)
 
 	of_property_read_u32(node, "clock-frequency", &csis->clk_frequency);
 
+	csis->num_channels = 1;
+	of_property_read_u32(node, "fsl,num-channels", &csis->num_channels);
+	if (csis->num_channels < 1 || csis->num_channels > MIPI_CSIS_MAX_CHANNELS)
+		return dev_err_probe(csis->dev, -EINVAL,
+				     "Invalid fsl,num-channels value\n");
+
 	return 0;
 }
 
@@ -1445,10 +1512,8 @@ static int mipi_csis_probe(struct platform_device *pdev)
 
 	/* Parse DT properties. */
 	ret = mipi_csis_parse_dt(csis);
-	if (ret < 0) {
-		dev_err(dev, "Failed to parse device tree: %d\n", ret);
+	if (ret < 0)
 		return ret;
-	}
 
 	/* Acquire resources. */
 	csis->regs = devm_platform_ioremap_resource(pdev, 0);
-- 
Regards,

Laurent Pinchart

[PATCH v2 12/12] arm64: dts: imx8mp: Specify the number of channels for CSI-2 receivers

[Laurent Pinchart]

The CSI-2 receivers in the i.MX8MP have 3 output channels. Specify this
in the device tree, to enable support for more than one channel in
drivers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index bb24dba7338e..1e52840078df 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -1764,6 +1764,7 @@ mipi_csi_0: csi@32e40000 {
 				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_250M>,
 							 <&clk IMX8MP_CLK_24M>;
 				power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>;
+				fsl,num-channels = <3>;
 				status = "disabled";
 
 				ports {
@@ -1799,6 +1800,7 @@ mipi_csi_1: csi@32e50000 {
 				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_250M>,
 							 <&clk IMX8MP_CLK_24M>;
 				power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_2>;
+				fsl,num-channels = <3>;
 				status = "disabled";
 
 				ports {
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 01/12] media: v4l2-common: Constify media_pad argument to v4l2_get_link_freq()

[Frank Li]

On Thu, Aug 21, 2025 at 03:09:33AM +0300, Laurent Pinchart wrote:
> The v4l2_get_link_freq() macro doesn't modify the pad argument. Make it
> possible to call it with a const media_pad pointer.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/media/v4l2-core/v4l2-common.c | 2 +-
>  include/media/v4l2-common.h           | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index a5334aa35992..4d686338e345 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -551,7 +551,7 @@ s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
>  EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
>
>  #ifdef CONFIG_MEDIA_CONTROLLER
> -s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> +s64 __v4l2_get_link_freq_pad(const struct media_pad *pad, unsigned int mul,
>  			     unsigned int div)
>  {
>  	struct v4l2_mbus_config mbus_config = {};
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 39dd0c78d70f..ede24e60dbec 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -580,10 +580,11 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  #define v4l2_get_link_freq(pad, mul, div)				\
>  	_Generic(pad,							\
> +		 const struct media_pad *: __v4l2_get_link_freq_pad,	\
>  		 struct media_pad *: __v4l2_get_link_freq_pad,		\
>  		 struct v4l2_ctrl_handler *: __v4l2_get_link_freq_ctrl)	\
>  	(pad, mul, div)
> -s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> +s64 __v4l2_get_link_freq_pad(const struct media_pad *pad, unsigned int mul,
>  			     unsigned int div);
>  #else
>  #define v4l2_get_link_freq(handler, mul, div)		\
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 02/12] media: imx-mipi-csis: Simplify access to source pad

[Frank Li]

On Thu, Aug 21, 2025 at 03:09:34AM +0300, Laurent Pinchart wrote:
> The mipi_csis_calculate_params() function needs to access the pad of the
> connected source. The pad is already available in csis->source.pad, but
> the function takes a convoluted path by getting the pad index and
> indexing the source subdev's pads array. Simplify it.

Generally, I like add "No functionality change" in commit message to help
reviewer or developer direct skip this commit when bisect commit to
indentify problem.

Reviewed-by: Frank Li <Frank.Li@nxp.com>

Frank Li

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 2beb5f43c2c0..46f93cd677e3 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -579,13 +579,11 @@ static void __mipi_csis_set_format(struct mipi_csis_device *csis,
>  static int mipi_csis_calculate_params(struct mipi_csis_device *csis,
>  				      const struct csis_pix_format *csis_fmt)
>  {
> -	struct media_pad *src_pad =
> -		&csis->source.sd->entity.pads[csis->source.pad->index];
>  	s64 link_freq;
>  	u32 lane_rate;
>
>  	/* Calculate the line rate from the pixel rate. */
> -	link_freq = v4l2_get_link_freq(src_pad, csis_fmt->width,
> +	link_freq = v4l2_get_link_freq(csis->source.pad, csis_fmt->width,
>  				       csis->bus.num_data_lanes * 2);
>  	if (link_freq < 0) {
>  		dev_err(csis->dev, "Unable to obtain link frequency: %d\n",
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 03/12] media: imx-mipi-csis: Standardize const keyword placement

[Frank Li]

On Thu, Aug 21, 2025 at 03:09:35AM +0300, Laurent Pinchart wrote:
> The const keyword for pointer variables is placed before the type
> everywhere except in one location. Change it to improve consistency.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 46f93cd677e3..0f0863011230 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -1029,7 +1029,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
>  			     struct v4l2_subdev_state *sd_state,
>  			     struct v4l2_subdev_format *sdformat)
>  {
> -	struct csis_pix_format const *csis_fmt;
> +	const struct csis_pix_format *csis_fmt;
>  	struct v4l2_mbus_framefmt *fmt;
>  	unsigned int align;
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 04/12] media: imx-mipi-csis: Shorten name of subdev state variables

[Frank Li]

On Thu, Aug 21, 2025 at 03:09:36AM +0300, Laurent Pinchart wrote:
> Rename subdev state parameters passed to subdev operations from sd_state
> to state. This standardizes the naming of the subdev state variables
> through the driver, and helps shortening lines.

Nit: I like simple sentence.

Rename sd_state to state to standardize the naming of the subdev state
variables and help shortening lines.

Reviewed-by: Frank Li <Frank.Li@nxp.com>

Frank

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 0f0863011230..894d12fef519 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -996,7 +996,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  }
>
>  static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
> -				    struct v4l2_subdev_state *sd_state,
> +				    struct v4l2_subdev_state *state,
>  				    struct v4l2_subdev_mbus_code_enum *code)
>  {
>  	/*
> @@ -1009,7 +1009,7 @@ static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
>  		if (code->index > 0)
>  			return -EINVAL;
>
> -		fmt = v4l2_subdev_state_get_format(sd_state, code->pad);
> +		fmt = v4l2_subdev_state_get_format(state, code->pad);
>  		code->code = fmt->code;
>  		return 0;
>  	}
> @@ -1026,7 +1026,7 @@ static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
>  }
>
>  static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
> -			     struct v4l2_subdev_state *sd_state,
> +			     struct v4l2_subdev_state *state,
>  			     struct v4l2_subdev_format *sdformat)
>  {
>  	const struct csis_pix_format *csis_fmt;
> @@ -1038,7 +1038,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
>  	 * modified.
>  	 */
>  	if (sdformat->pad == CSIS_PAD_SOURCE)
> -		return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
> +		return v4l2_subdev_get_fmt(sd, state, sdformat);
>
>  	if (sdformat->pad != CSIS_PAD_SINK)
>  		return -EINVAL;
> @@ -1076,7 +1076,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
>  			      &sdformat->format.height, 1,
>  			      CSIS_MAX_PIX_HEIGHT, 0, 0);
>
> -	fmt = v4l2_subdev_state_get_format(sd_state, sdformat->pad);
> +	fmt = v4l2_subdev_state_get_format(state, sdformat->pad);
>
>  	fmt->code = csis_fmt->code;
>  	fmt->width = sdformat->format.width;
> @@ -1090,7 +1090,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
>  	sdformat->format = *fmt;
>
>  	/* Propagate the format from sink to source. */
> -	fmt = v4l2_subdev_state_get_format(sd_state, CSIS_PAD_SOURCE);
> +	fmt = v4l2_subdev_state_get_format(state, CSIS_PAD_SOURCE);
>  	*fmt = sdformat->format;
>
>  	/* The format on the source pad might change due to unpacking. */
> @@ -1130,7 +1130,7 @@ static int mipi_csis_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
>  }
>
>  static int mipi_csis_init_state(struct v4l2_subdev *sd,
> -				struct v4l2_subdev_state *sd_state)
> +				struct v4l2_subdev_state *state)
>  {
>  	struct v4l2_subdev_format fmt = {
>  		.pad = CSIS_PAD_SINK,
> @@ -1147,7 +1147,7 @@ static int mipi_csis_init_state(struct v4l2_subdev *sd,
>  		V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt.format.colorspace,
>  					      fmt.format.ycbcr_enc);
>
> -	return mipi_csis_set_fmt(sd, sd_state, &fmt);
> +	return mipi_csis_set_fmt(sd, state, &fmt);
>  }
>
>  static int mipi_csis_log_status(struct v4l2_subdev *sd)
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 05/12] media: imx-mipi-csis: Rename register macros to match reference manual

[Frank Li]

On Thu, Aug 21, 2025 at 03:09:37AM +0300, Laurent Pinchart wrote:
> The CSIS driver uses register macro names that do not match the
> reference manual of the i.MX7[DS] and i.MX8M[MNP] SoCs in which the CSIS
> is integrated. Rename them to match the documentation, making the code
> easier to read alongside the reference manuals.
>
> One of the misnamed register fields is MIPI_CSIS_INT_SRC_ERR_UNKNOWN,
> which led to the corresponding event being logged as "Unknown Error".
> The correct register field name is MIPI_CSIS_INT_SRC_ERR_ID, documented
> as "Unknown ID error". Update the event description accordingly.
>
> While at it, also replace a few *_OFFSET macros with parametric macros
> for consistency, and add the missing MIPI_CSIS_ISP_RESOL_VRESOL and
> MIPI_CSIS_ISP_RESOL_HRESOL register field macros.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 69 +++++++++++-----------
>  1 file changed, 36 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 894d12fef519..1ca327e6be00 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -55,13 +55,13 @@
>  /* CSIS common control */
>  #define MIPI_CSIS_CMN_CTRL			0x04
>  #define MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW	BIT(16)
> -#define MIPI_CSIS_CMN_CTRL_INTER_MODE		BIT(10)
> +#define MIPI_CSIS_CMN_CTRL_INTERLEAVE_MODE_NONE	(0 << 10)
> +#define MIPI_CSIS_CMN_CTRL_INTERLEAVE_MODE_DT	(1 << 10)

BIT(10)?

> +#define MIPI_CSIS_CMN_CTRL_LANE_NUMBER(n)	((n) << 8)
> +#define MIPI_CSIS_CMN_CTRL_LANE_NUMBER_MASK	(3 << 8)

GEN_MASK() is better here, And below other registers.

Frank
>  #define MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW_CTRL	BIT(2)
> -#define MIPI_CSIS_CMN_CTRL_RESET		BIT(1)
> -#define MIPI_CSIS_CMN_CTRL_ENABLE		BIT(0)
> -
> -#define MIPI_CSIS_CMN_CTRL_LANE_NR_OFFSET	8
> -#define MIPI_CSIS_CMN_CTRL_LANE_NR_MASK		(3 << 8)
> +#define MIPI_CSIS_CMN_CTRL_SW_RESET		BIT(1)
> +#define MIPI_CSIS_CMN_CTRL_CSI_EN		BIT(0)
>
>  /* CSIS clock control */
...

> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 06/12] media: imx-mipi-csis: Fix field alignment in register dump

[Frank Li]

On Thu, Aug 21, 2025 at 03:09:38AM +0300, Laurent Pinchart wrote:
> Commit 95a1379004cb ("media: staging: media: imx: imx7-mipi-csis: Dump
> MIPI_CSIS_FRAME_COUNTER_CH0 register") forgot to increase the maximum
> register name length, resulting in misalignment of names printed in the
> kernel log. Fix it.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 1ca327e6be00..35c8d5309424 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -893,7 +893,7 @@ static int mipi_csis_dump_regs(struct mipi_csis_device *csis)
>
>  	for (i = 0; i < ARRAY_SIZE(registers); i++) {
>  		cfg = mipi_csis_read(csis, registers[i].offset);
> -		dev_info(csis->dev, "%14s: 0x%08x\n", registers[i].name, cfg);
> +		dev_info(csis->dev, "%17s: 0x%08x\n", registers[i].name, cfg);
>  	}
>
>  	pm_runtime_put(csis->dev);
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 07/12] media: imx-mipi-csis: Log per-lane start of transmission errors

[Frank Li]

On Thu, Aug 21, 2025 at 03:09:39AM +0300, Laurent Pinchart wrote:
> The CSIS has per-line start of transmission error interrupts. Log them
> all, instead of only the first data lane.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 35c8d5309424..67da8326540b 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -100,7 +100,7 @@
>  #define MIPI_CSIS_INT_SRC_NON_IMAGE_DATA	(0xf << 28)
>  #define MIPI_CSIS_INT_SRC_FRAME_START		BIT(24)
>  #define MIPI_CSIS_INT_SRC_FRAME_END		BIT(20)
> -#define MIPI_CSIS_INT_SRC_ERR_SOT_HS		BIT(16)
> +#define MIPI_CSIS_INT_SRC_ERR_SOT_HS(n)		BIT((n) + 16)
>  #define MIPI_CSIS_INT_SRC_ERR_LOST_FS		BIT(12)
>  #define MIPI_CSIS_INT_SRC_ERR_LOST_FE		BIT(8)
>  #define MIPI_CSIS_INT_SRC_ERR_OVER		BIT(4)
> @@ -241,7 +241,10 @@ struct mipi_csis_event {
>
>  static const struct mipi_csis_event mipi_csis_events[] = {
>  	/* Errors */
> -	{ false, MIPI_CSIS_INT_SRC_ERR_SOT_HS,		"SOT Error" },
> +	{ false, MIPI_CSIS_INT_SRC_ERR_SOT_HS(0),	"SOT 0 Error" },
> +	{ false, MIPI_CSIS_INT_SRC_ERR_SOT_HS(1),	"SOT 1 Error" },
> +	{ false, MIPI_CSIS_INT_SRC_ERR_SOT_HS(2),	"SOT 2 Error" },
> +	{ false, MIPI_CSIS_INT_SRC_ERR_SOT_HS(3),	"SOT 3 Error" },
>  	{ false, MIPI_CSIS_INT_SRC_ERR_LOST_FS,		"Lost Frame Start Error" },
>  	{ false, MIPI_CSIS_INT_SRC_ERR_LOST_FE,		"Lost Frame End Error" },
>  	{ false, MIPI_CSIS_INT_SRC_ERR_OVER,		"FIFO Overflow Error" },
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 08/12] media: imx-mipi-csis: Only set clock rate when specified in DT

[Frank Li]

On Thu, Aug 21, 2025 at 03:09:40AM +0300, Laurent Pinchart wrote:
> The imx-mipi-csis driver sets the rate of the wrap clock to the value
> specified in the device tree's "clock-frequency" property, and defaults
> to 166 MHz otherwise. This is a historical mistake, as clock rate
> selection should have been left to the assigned-clock-rates property.
>
> Honouring the clock-frequency property can't be removed without breaking
> backwards compatibility, and the corresponding code isn't very
> intrusive. The 166 MHz default, on the other hand, prevents
> configuration of the clock rate through assigned-clock-rates, as the
> driver immediately overwrites the rate. This behaviour is confusing and
> has cost debugging time.
>
> There is little value in a 166 MHz default. All mainline device tree
> sources that enable the CSIS specify a clock-frequency explicitly, and
> the default wrap clock configuration on supported platforms is at least
> as high as 166 MHz. Drop the default, and only set the clock rate
> manually when the clock-frequency property is specified.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 23 +++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 67da8326540b..83ba68a20bd1 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -230,8 +230,6 @@
>  #define MIPI_CSIS_PKTDATA_EVEN			0x3000
>  #define MIPI_CSIS_PKTDATA_SIZE			SZ_4K
>
> -#define DEFAULT_SCLK_CSIS_FREQ			166000000UL
> -
>  struct mipi_csis_event {
>  	bool debug;
>  	u32 mask;
> @@ -708,12 +706,17 @@ static int mipi_csis_clk_get(struct mipi_csis_device *csis)
>  	if (ret < 0)
>  		return ret;
>
> -	/* Set clock rate */
> -	ret = clk_set_rate(csis->clks[MIPI_CSIS_CLK_WRAP].clk,
> -			   csis->clk_frequency);
> -	if (ret < 0)
> -		dev_err(csis->dev, "set rate=%d failed: %d\n",
> -			csis->clk_frequency, ret);
> +	if (csis->clk_frequency) {
> +		/*
> +		 * Set the clock rate. This is deprecated, for backward
> +		 * compatibility with old device trees.
> +		 */
> +		ret = clk_set_rate(csis->clks[MIPI_CSIS_CLK_WRAP].clk,
> +				   csis->clk_frequency);
> +		if (ret < 0)
> +			dev_err(csis->dev, "set rate=%d failed: %d\n",
> +				csis->clk_frequency, ret);
> +	}
>
>  	return ret;
>  }
> @@ -1417,9 +1420,7 @@ static int mipi_csis_parse_dt(struct mipi_csis_device *csis)
>  {
>  	struct device_node *node = csis->dev->of_node;
>
> -	if (of_property_read_u32(node, "clock-frequency",
> -				 &csis->clk_frequency))
> -		csis->clk_frequency = DEFAULT_SCLK_CSIS_FREQ;
> +	of_property_read_u32(node, "clock-frequency", &csis->clk_frequency);
>
>  	return 0;
>  }
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 02/12] media: imx-mipi-csis: Simplify access to source pad

[Laurent Pinchart]

On Thu, Aug 21, 2025 at 11:10:38AM -0400, Frank Li wrote:
> On Thu, Aug 21, 2025 at 03:09:34AM +0300, Laurent Pinchart wrote:
> > The mipi_csis_calculate_params() function needs to access the pad of the
> > connected source. The pad is already available in csis->source.pad, but
> > the function takes a convoluted path by getting the pad index and
> > indexing the source subdev's pads array. Simplify it.
> 
> Generally, I like add "No functionality change" in commit message to help
> reviewer or developer direct skip this commit when bisect commit to
> indentify problem.

Good point. I'll add

"No functional change is intended."

> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> 
> Frank Li
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/platform/nxp/imx-mipi-csis.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > index 2beb5f43c2c0..46f93cd677e3 100644
> > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > @@ -579,13 +579,11 @@ static void __mipi_csis_set_format(struct mipi_csis_device *csis,
> >  static int mipi_csis_calculate_params(struct mipi_csis_device *csis,
> >  				      const struct csis_pix_format *csis_fmt)
> >  {
> > -	struct media_pad *src_pad =
> > -		&csis->source.sd->entity.pads[csis->source.pad->index];
> >  	s64 link_freq;
> >  	u32 lane_rate;
> >
> >  	/* Calculate the line rate from the pixel rate. */
> > -	link_freq = v4l2_get_link_freq(src_pad, csis_fmt->width,
> > +	link_freq = v4l2_get_link_freq(csis->source.pad, csis_fmt->width,
> >  				       csis->bus.num_data_lanes * 2);
> >  	if (link_freq < 0) {
> >  		dev_err(csis->dev, "Unable to obtain link frequency: %d\n",

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 11/12] media: imx-mipi-csis: Initial support for multiple output channels

[Frank Li]

On Thu, Aug 21, 2025 at 03:09:43AM +0300, Laurent Pinchart wrote:
> Some CSIS instances feature more than one output channel. Update
> register macros accordingly, parse the number of channels from the
> device tree, and update register dumps and event counters to log
> per-channel data.
>
> Support for routing virtual channels and data types to output channels
> through the subdev internal routing API will come later.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
>
> - Update more per-channel registers
> - Update commit message
> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 239 +++++++++++++--------
>  1 file changed, 152 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 83ba68a20bd1..b1136336a57f 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -54,7 +54,7 @@
>
...
>
> +	{ false, 0, MIPI_CSIS_INT_SRC_FRAME_START(0),		"Frame Start 0" },
> +	{ false, 1, MIPI_CSIS_INT_SRC_FRAME_START(1),		"Frame Start 1" },
> +	{ false, 2, MIPI_CSIS_INT_SRC_FRAME_START(2),		"Frame Start 2" },
> +	{ false, 3, MIPI_CSIS_INT_SRC_FRAME_START(3),		"Frame Start 3" },
> +	{ false, 0, MIPI_CSIS_INT_SRC_FRAME_END(0),		"Frame End 0" },
> +	{ false, 1, MIPI_CSIS_INT_SRC_FRAME_END(1),		"Frame End 1" },
> +	{ false, 2, MIPI_CSIS_INT_SRC_FRAME_END(2),		"Frame End 2" },
> +	{ false, 3, MIPI_CSIS_INT_SRC_FRAME_END(3),		"Frame End 3" },
> +	{ true, 0, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(0),	"VSYNC Falling Edge 0" },
> +	{ true, 1, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(1),	"VSYNC Falling Edge 1" },
> +	{ true, 2, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(2),	"VSYNC Falling Edge 2" },
> +	{ true, 3, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(3),	"VSYNC Falling Edge 3" },
> +	{ true, 0, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(0),	"VSYNC Rising Edge 0" },
> +	{ true, 1, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(1),	"VSYNC Rising Edge 1" },
> +	{ true, 2, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(2),	"VSYNC Rising Edge 2" },
> +	{ true, 3, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(3),	"VSYNC Rising Edge 3" },
>  };
>
> -#define MIPI_CSIS_NUM_EVENTS ARRAY_SIZE(mipi_csis_events)
> +#define MIPI_CSIS_NUM_EVENTS		ARRAY_SIZE(mipi_csis_events)

unneccesary change for this patch

> +#define MIPI_CSIS_NUM_ERROR_EVENTS	(MIPI_CSIS_NUM_EVENTS - 20)
>
>  enum mipi_csis_clk {
>  	MIPI_CSIS_CLK_PCLK,
> @@ -300,7 +334,9 @@ struct mipi_csis_device {
>  	struct clk_bulk_data *clks;
>  	struct reset_control *mrst;
>  	struct regulator *mipi_phy_regulator;
> +
>  	const struct mipi_csis_info *info;
> +	unsigned int num_channels;
>
>  	struct v4l2_subdev sd;
>  	struct media_pad pads[CSIS_PADS_NUM];
> @@ -655,8 +691,8 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis,
>  			MIPI_CSIS_ISP_SYNC_VSYNC_EINTV(0));
>
>  	val = mipi_csis_read(csis, MIPI_CSIS_CLK_CTRL);
> -	val |= MIPI_CSIS_CLK_CTRL_WCLK_SRC;
> -	val |= MIPI_CSIS_CLK_CTRL_CLKGATE_TRAIL_CH0(15);
> +	val |= MIPI_CSIS_CLK_CTRL_WCLK_SRC(0);
> +	val |= MIPI_CSIS_CLK_CTRL_CLKGATE_TRAIL(0, 15);
>  	val &= ~MIPI_CSIS_CLK_CTRL_CLKGATE_EN_MSK;
>  	mipi_csis_write(csis, MIPI_CSIS_CLK_CTRL, val);
>
> @@ -673,7 +709,7 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis,
>  	/* Update the shadow register. */
>  	val = mipi_csis_read(csis, MIPI_CSIS_CMN_CTRL);
>  	mipi_csis_write(csis, MIPI_CSIS_CMN_CTRL,
> -			val | MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW |
> +			val | MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW(0) |
>  			MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW_CTRL);
>  }
>
> @@ -764,16 +800,19 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
>
>  	/* Update the event/error counters */
>  	if ((status & MIPI_CSIS_INT_SRC_ERRORS) || csis->debug.enable) {
> -		for (i = 0; i < MIPI_CSIS_NUM_EVENTS; i++) {
> +		for (i = 0; i < ARRAY_SIZE(csis->events); i++) {

This is nice change, but I think it is not related with this patch. May
need sperate patch.

>  			struct mipi_csis_event *event = &csis->events[i];
>
> +			if (event->channel >= csis->num_channels)
> +				continue;
> +
>  			if ((!event->debug && (status & event->mask)) ||
>  			    (event->debug && (dbg_status & event->mask)))
>  				event->counter++;
>  		}
>  	}
>
> -	if (status & MIPI_CSIS_INT_SRC_FRAME_START)
> +	if (status & MIPI_CSIS_INT_SRC_FRAME_START(0))
>  		mipi_csis_queue_event_sof(csis);
>
>  	spin_unlock_irqrestore(&csis->slock, flags);
> @@ -850,7 +889,7 @@ static void mipi_csis_clear_counters(struct mipi_csis_device *csis)
>  static void mipi_csis_log_counters(struct mipi_csis_device *csis, bool non_errors)
>  {
>  	unsigned int num_events = non_errors ? MIPI_CSIS_NUM_EVENTS
> -				: MIPI_CSIS_NUM_EVENTS - 8;
> +				: MIPI_CSIS_NUM_ERROR_EVENTS;

I think old code logic is strange. err events is not last trail of events
array. when non_errors false, only last 8 events have not logs.

And I found all place call mipi_csis_log_counters(, true) in whole driver.

Frank

>  	unsigned int counters[MIPI_CSIS_NUM_EVENTS];
>  	unsigned long flags;
>  	unsigned int i;
> @@ -861,45 +900,67 @@ static void mipi_csis_log_counters(struct mipi_csis_device *csis, bool non_error
>  	spin_unlock_irqrestore(&csis->slock, flags);
>
>  	for (i = 0; i < num_events; ++i) {
> +		const struct mipi_csis_event *event = &csis->events[i];
> +
> +		if (event->channel >= csis->num_channels)
> +			continue;
> +
>  		if (counters[i] > 0 || csis->debug.enable)
>  			dev_info(csis->dev, "%s events: %d\n",
> -				 csis->events[i].name,
> -				 counters[i]);
> +				 event->name, counters[i]);
>  	}
>  }
>
> +struct mipi_csis_reg_info {
> +	u32 addr;
> +	unsigned int offset;
> +	const char * const name;
> +};
> +
> +static void mipi_csis_dump_channel_reg(struct mipi_csis_device *csis,
> +				       const struct mipi_csis_reg_info *reg,
> +				       unsigned int channel)
> +{
> +	dev_info(csis->dev, "%16s%u: 0x%08x\n", reg->name, channel,
> +		 mipi_csis_read(csis, reg->addr + channel * reg->offset));
> +}
> +
>  static int mipi_csis_dump_regs(struct mipi_csis_device *csis)
>  {
> -	static const struct {
> -		u32 offset;
> -		const char * const name;
> -	} registers[] = {
> -		{ MIPI_CSIS_CMN_CTRL, "CMN_CTRL" },
> -		{ MIPI_CSIS_CLK_CTRL, "CLK_CTRL" },
> -		{ MIPI_CSIS_INT_MSK, "INT_MSK" },
> -		{ MIPI_CSIS_DPHY_STATUS, "DPHY_STATUS" },
> -		{ MIPI_CSIS_DPHY_CMN_CTRL, "DPHY_CMN_CTRL" },
> -		{ MIPI_CSIS_DPHY_SCTRL_L, "DPHY_SCTRL_L" },
> -		{ MIPI_CSIS_DPHY_SCTRL_H, "DPHY_SCTRL_H" },
> -		{ MIPI_CSIS_ISP_CONFIG_CH(0), "ISP_CONFIG_CH0" },
> -		{ MIPI_CSIS_ISP_RESOL_CH(0), "ISP_RESOL_CH0" },
> -		{ MIPI_CSIS_SDW_CONFIG_CH(0), "SDW_CONFIG_CH0" },
> -		{ MIPI_CSIS_SDW_RESOL_CH(0), "SDW_RESOL_CH0" },
> -		{ MIPI_CSIS_DBG_CTRL, "DBG_CTRL" },
> -		{ MIPI_CSIS_FRAME_COUNTER_CH(0), "FRAME_COUNTER_CH0" },
> +	static const struct mipi_csis_reg_info common_registers[] = {
> +		{ MIPI_CSIS_CMN_CTRL, 0, "CMN_CTRL" },
> +		{ MIPI_CSIS_CLK_CTRL, 0, "CLK_CTRL" },
> +		{ MIPI_CSIS_INT_MSK, 0, "INT_MSK" },
> +		{ MIPI_CSIS_DPHY_STATUS, 0, "DPHY_STATUS" },
> +		{ MIPI_CSIS_DPHY_CMN_CTRL, 0, "DPHY_CMN_CTRL" },
> +		{ MIPI_CSIS_DPHY_SCTRL_L, 0, "DPHY_SCTRL_L" },
> +		{ MIPI_CSIS_DPHY_SCTRL_H, 0, "DPHY_SCTRL_H" },
> +		{ MIPI_CSIS_DBG_CTRL, 0, "DBG_CTRL" },
> +	};
> +	static const struct mipi_csis_reg_info channel_registers[] = {
> +		{ MIPI_CSIS_ISP_CONFIG_CH(0), 0x10, "ISP_CONFIG_CH" },
> +		{ MIPI_CSIS_ISP_RESOL_CH(0), 0x10, "ISP_RESOL_CH" },
> +		{ MIPI_CSIS_SDW_CONFIG_CH(0), 0x10, "SDW_CONFIG_CH" },
> +		{ MIPI_CSIS_SDW_RESOL_CH(0), 0x10, "SDW_RESOL_CH" },
> +		{ MIPI_CSIS_FRAME_COUNTER_CH(0), 4, "FRAME_COUNTER_CH" },
>  	};
> -
> -	unsigned int i;
> -	u32 cfg;
>
>  	if (!pm_runtime_get_if_in_use(csis->dev))
>  		return 0;
>
>  	dev_info(csis->dev, "--- REGISTERS ---\n");
>
> -	for (i = 0; i < ARRAY_SIZE(registers); i++) {
> -		cfg = mipi_csis_read(csis, registers[i].offset);
> -		dev_info(csis->dev, "%17s: 0x%08x\n", registers[i].name, cfg);
> +	for (unsigned int i = 0; i < ARRAY_SIZE(common_registers); i++) {
> +		const struct mipi_csis_reg_info *reg = &common_registers[i];
> +
> +		dev_info(csis->dev, "%17s: 0x%08x\n", reg->name,
> +			 mipi_csis_read(csis, reg->addr));
> +	}
> +
> +	for (unsigned int chan = 0; chan < csis->num_channels; chan++) {
> +		for (unsigned int i = 0; i < ARRAY_SIZE(channel_registers); ++i)
> +			mipi_csis_dump_channel_reg(csis, &channel_registers[i],
> +						   chan);
>  	}
>
>  	pm_runtime_put(csis->dev);
> @@ -1422,6 +1483,12 @@ static int mipi_csis_parse_dt(struct mipi_csis_device *csis)
>
>  	of_property_read_u32(node, "clock-frequency", &csis->clk_frequency);
>
> +	csis->num_channels = 1;
> +	of_property_read_u32(node, "fsl,num-channels", &csis->num_channels);
> +	if (csis->num_channels < 1 || csis->num_channels > MIPI_CSIS_MAX_CHANNELS)
> +		return dev_err_probe(csis->dev, -EINVAL,
> +				     "Invalid fsl,num-channels value\n");
> +
>  	return 0;
>  }
>
> @@ -1445,10 +1512,8 @@ static int mipi_csis_probe(struct platform_device *pdev)
>
>  	/* Parse DT properties. */
>  	ret = mipi_csis_parse_dt(csis);
> -	if (ret < 0) {
> -		dev_err(dev, "Failed to parse device tree: %d\n", ret);
> +	if (ret < 0)
>  		return ret;
> -	}
>
>  	/* Acquire resources. */
>  	csis->regs = devm_platform_ioremap_resource(pdev, 0);
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 12/12] arm64: dts: imx8mp: Specify the number of channels for CSI-2 receivers

[Frank Li]

On Thu, Aug 21, 2025 at 03:09:44AM +0300, Laurent Pinchart wrote:
> The CSI-2 receivers in the i.MX8MP have 3 output channels. Specify this
> in the device tree, to enable support for more than one channel in
> drivers.

dt descript hardware, not driver

Specify this to reflect actual hardware feature.

Reviewed-by: Frank Li <Frank.Li@nxp.com>

Frank

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index bb24dba7338e..1e52840078df 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -1764,6 +1764,7 @@ mipi_csi_0: csi@32e40000 {
>  				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_250M>,
>  							 <&clk IMX8MP_CLK_24M>;
>  				power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>;
> +				fsl,num-channels = <3>;
>  				status = "disabled";
>
>  				ports {
> @@ -1799,6 +1800,7 @@ mipi_csi_1: csi@32e50000 {
>  				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_250M>,
>  							 <&clk IMX8MP_CLK_24M>;
>  				power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_2>;
> +				fsl,num-channels = <3>;
>  				status = "disabled";
>
>  				ports {
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 05/12] media: imx-mipi-csis: Rename register macros to match reference manual

[Laurent Pinchart]

Hi Frank,

On Thu, Aug 21, 2025 at 11:25:55AM -0400, Frank Li wrote:
> On Thu, Aug 21, 2025 at 03:09:37AM +0300, Laurent Pinchart wrote:
> > The CSIS driver uses register macro names that do not match the
> > reference manual of the i.MX7[DS] and i.MX8M[MNP] SoCs in which the CSIS
> > is integrated. Rename them to match the documentation, making the code
> > easier to read alongside the reference manuals.
> >
> > One of the misnamed register fields is MIPI_CSIS_INT_SRC_ERR_UNKNOWN,
> > which led to the corresponding event being logged as "Unknown Error".
> > The correct register field name is MIPI_CSIS_INT_SRC_ERR_ID, documented
> > as "Unknown ID error". Update the event description accordingly.
> >
> > While at it, also replace a few *_OFFSET macros with parametric macros
> > for consistency, and add the missing MIPI_CSIS_ISP_RESOL_VRESOL and
> > MIPI_CSIS_ISP_RESOL_HRESOL register field macros.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/platform/nxp/imx-mipi-csis.c | 69 +++++++++++-----------
> >  1 file changed, 36 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > index 894d12fef519..1ca327e6be00 100644
> > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > @@ -55,13 +55,13 @@
> >  /* CSIS common control */
> >  #define MIPI_CSIS_CMN_CTRL			0x04
> >  #define MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW	BIT(16)
> > -#define MIPI_CSIS_CMN_CTRL_INTER_MODE		BIT(10)
> > +#define MIPI_CSIS_CMN_CTRL_INTERLEAVE_MODE_NONE	(0 << 10)
> > +#define MIPI_CSIS_CMN_CTRL_INTERLEAVE_MODE_DT	(1 << 10)
> 
> BIT(10)?

INTERLAVE_MODE is a 2-bit field. I'm working on a series that add
support for the VC interleave mode, which has value 2. I'll however drop
this change and keep BIT(10) for now, as the commit message doesn't
explain why this has been modified.

> > +#define MIPI_CSIS_CMN_CTRL_LANE_NUMBER(n)	((n) << 8)
> > +#define MIPI_CSIS_CMN_CTRL_LANE_NUMBER_MASK	(3 << 8)
> 
> GEN_MASK() is better here, And below other registers.

It is, but I wanted this patch to focus on the renames. I actually have
a patch in my branch to switch to GENMASK for all masks, I will add it
to the next version of this series.

> >  #define MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW_CTRL	BIT(2)
> > -#define MIPI_CSIS_CMN_CTRL_RESET		BIT(1)
> > -#define MIPI_CSIS_CMN_CTRL_ENABLE		BIT(0)
> > -
> > -#define MIPI_CSIS_CMN_CTRL_LANE_NR_OFFSET	8
> > -#define MIPI_CSIS_CMN_CTRL_LANE_NR_MASK		(3 << 8)
> > +#define MIPI_CSIS_CMN_CTRL_SW_RESET		BIT(1)
> > +#define MIPI_CSIS_CMN_CTRL_CSI_EN		BIT(0)
> >
> >  /* CSIS clock control */
> ...

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 11/12] media: imx-mipi-csis: Initial support for multiple output channels

[Laurent Pinchart]

On Thu, Aug 21, 2025 at 11:56:43AM -0400, Frank Li wrote:
> On Thu, Aug 21, 2025 at 03:09:43AM +0300, Laurent Pinchart wrote:
> > Some CSIS instances feature more than one output channel. Update
> > register macros accordingly, parse the number of channels from the
> > device tree, and update register dumps and event counters to log
> > per-channel data.
> >
> > Support for routing virtual channels and data types to output channels
> > through the subdev internal routing API will come later.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Update more per-channel registers
> > - Update commit message
> > ---
> >  drivers/media/platform/nxp/imx-mipi-csis.c | 239 +++++++++++++--------
> >  1 file changed, 152 insertions(+), 87 deletions(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > index 83ba68a20bd1..b1136336a57f 100644
> > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > @@ -54,7 +54,7 @@
> >
> ...
> >
> > +	{ false, 0, MIPI_CSIS_INT_SRC_FRAME_START(0),		"Frame Start 0" },
> > +	{ false, 1, MIPI_CSIS_INT_SRC_FRAME_START(1),		"Frame Start 1" },
> > +	{ false, 2, MIPI_CSIS_INT_SRC_FRAME_START(2),		"Frame Start 2" },
> > +	{ false, 3, MIPI_CSIS_INT_SRC_FRAME_START(3),		"Frame Start 3" },
> > +	{ false, 0, MIPI_CSIS_INT_SRC_FRAME_END(0),		"Frame End 0" },
> > +	{ false, 1, MIPI_CSIS_INT_SRC_FRAME_END(1),		"Frame End 1" },
> > +	{ false, 2, MIPI_CSIS_INT_SRC_FRAME_END(2),		"Frame End 2" },
> > +	{ false, 3, MIPI_CSIS_INT_SRC_FRAME_END(3),		"Frame End 3" },
> > +	{ true, 0, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(0),	"VSYNC Falling Edge 0" },
> > +	{ true, 1, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(1),	"VSYNC Falling Edge 1" },
> > +	{ true, 2, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(2),	"VSYNC Falling Edge 2" },
> > +	{ true, 3, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(3),	"VSYNC Falling Edge 3" },
> > +	{ true, 0, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(0),	"VSYNC Rising Edge 0" },
> > +	{ true, 1, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(1),	"VSYNC Rising Edge 1" },
> > +	{ true, 2, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(2),	"VSYNC Rising Edge 2" },
> > +	{ true, 3, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(3),	"VSYNC Rising Edge 3" },
> >  };
> >
> > -#define MIPI_CSIS_NUM_EVENTS ARRAY_SIZE(mipi_csis_events)
> > +#define MIPI_CSIS_NUM_EVENTS		ARRAY_SIZE(mipi_csis_events)
> 
> unneccesary change for this patch

It's meant to have the same alignment as the next line.

> > +#define MIPI_CSIS_NUM_ERROR_EVENTS	(MIPI_CSIS_NUM_EVENTS - 20)
> >
> >  enum mipi_csis_clk {
> >  	MIPI_CSIS_CLK_PCLK,
> > @@ -300,7 +334,9 @@ struct mipi_csis_device {
> >  	struct clk_bulk_data *clks;
> >  	struct reset_control *mrst;
> >  	struct regulator *mipi_phy_regulator;
> > +
> >  	const struct mipi_csis_info *info;
> > +	unsigned int num_channels;
> >
> >  	struct v4l2_subdev sd;
> >  	struct media_pad pads[CSIS_PADS_NUM];
> > @@ -655,8 +691,8 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis,
> >  			MIPI_CSIS_ISP_SYNC_VSYNC_EINTV(0));
> >
> >  	val = mipi_csis_read(csis, MIPI_CSIS_CLK_CTRL);
> > -	val |= MIPI_CSIS_CLK_CTRL_WCLK_SRC;
> > -	val |= MIPI_CSIS_CLK_CTRL_CLKGATE_TRAIL_CH0(15);
> > +	val |= MIPI_CSIS_CLK_CTRL_WCLK_SRC(0);
> > +	val |= MIPI_CSIS_CLK_CTRL_CLKGATE_TRAIL(0, 15);
> >  	val &= ~MIPI_CSIS_CLK_CTRL_CLKGATE_EN_MSK;
> >  	mipi_csis_write(csis, MIPI_CSIS_CLK_CTRL, val);
> >
> > @@ -673,7 +709,7 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis,
> >  	/* Update the shadow register. */
> >  	val = mipi_csis_read(csis, MIPI_CSIS_CMN_CTRL);
> >  	mipi_csis_write(csis, MIPI_CSIS_CMN_CTRL,
> > -			val | MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW |
> > +			val | MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW(0) |
> >  			MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW_CTRL);
> >  }
> >
> > @@ -764,16 +800,19 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
> >
> >  	/* Update the event/error counters */
> >  	if ((status & MIPI_CSIS_INT_SRC_ERRORS) || csis->debug.enable) {
> > -		for (i = 0; i < MIPI_CSIS_NUM_EVENTS; i++) {
> > +		for (i = 0; i < ARRAY_SIZE(csis->events); i++) {
> 
> This is nice change, but I think it is not related with this patch. May
> need sperate patch.

I think a separate patch just for this one-line change would be
overkill. Given that this patch touches event reporting, I'd rather keep
this here.

> >  			struct mipi_csis_event *event = &csis->events[i];
> >
> > +			if (event->channel >= csis->num_channels)
> > +				continue;
> > +
> >  			if ((!event->debug && (status & event->mask)) ||
> >  			    (event->debug && (dbg_status & event->mask)))
> >  				event->counter++;
> >  		}
> >  	}
> >
> > -	if (status & MIPI_CSIS_INT_SRC_FRAME_START)
> > +	if (status & MIPI_CSIS_INT_SRC_FRAME_START(0))
> >  		mipi_csis_queue_event_sof(csis);
> >
> >  	spin_unlock_irqrestore(&csis->slock, flags);
> > @@ -850,7 +889,7 @@ static void mipi_csis_clear_counters(struct mipi_csis_device *csis)
> >  static void mipi_csis_log_counters(struct mipi_csis_device *csis, bool non_errors)
> >  {
> >  	unsigned int num_events = non_errors ? MIPI_CSIS_NUM_EVENTS
> > -				: MIPI_CSIS_NUM_EVENTS - 8;
> > +				: MIPI_CSIS_NUM_ERROR_EVENTS;
> 
> I think old code logic is strange. err events is not last trail of events
> array. when non_errors false, only last 8 events have not logs.

The error events are at the beginning of the array, and before this
change there was 8 non-error events at the end. The code would log
either all events, or just the error events (all minus the last 8).

> 
> And I found all place call mipi_csis_log_counters(, true) in whole driver.

Indeed. I wonder why. Looking at the code, I think we should only log
non-error counters when csis->debug.enable is set. That's a candidate
for a separate patch.

> >  	unsigned int counters[MIPI_CSIS_NUM_EVENTS];
> >  	unsigned long flags;
> >  	unsigned int i;
> > @@ -861,45 +900,67 @@ static void mipi_csis_log_counters(struct mipi_csis_device *csis, bool non_error
> >  	spin_unlock_irqrestore(&csis->slock, flags);
> >
> >  	for (i = 0; i < num_events; ++i) {
> > +		const struct mipi_csis_event *event = &csis->events[i];
> > +
> > +		if (event->channel >= csis->num_channels)
> > +			continue;
> > +
> >  		if (counters[i] > 0 || csis->debug.enable)
> >  			dev_info(csis->dev, "%s events: %d\n",
> > -				 csis->events[i].name,
> > -				 counters[i]);
> > +				 event->name, counters[i]);
> >  	}
> >  }
> >
> > +struct mipi_csis_reg_info {
> > +	u32 addr;
> > +	unsigned int offset;
> > +	const char * const name;
> > +};
> > +
> > +static void mipi_csis_dump_channel_reg(struct mipi_csis_device *csis,
> > +				       const struct mipi_csis_reg_info *reg,
> > +				       unsigned int channel)
> > +{
> > +	dev_info(csis->dev, "%16s%u: 0x%08x\n", reg->name, channel,
> > +		 mipi_csis_read(csis, reg->addr + channel * reg->offset));
> > +}
> > +
> >  static int mipi_csis_dump_regs(struct mipi_csis_device *csis)
> >  {
> > -	static const struct {
> > -		u32 offset;
> > -		const char * const name;
> > -	} registers[] = {
> > -		{ MIPI_CSIS_CMN_CTRL, "CMN_CTRL" },
> > -		{ MIPI_CSIS_CLK_CTRL, "CLK_CTRL" },
> > -		{ MIPI_CSIS_INT_MSK, "INT_MSK" },
> > -		{ MIPI_CSIS_DPHY_STATUS, "DPHY_STATUS" },
> > -		{ MIPI_CSIS_DPHY_CMN_CTRL, "DPHY_CMN_CTRL" },
> > -		{ MIPI_CSIS_DPHY_SCTRL_L, "DPHY_SCTRL_L" },
> > -		{ MIPI_CSIS_DPHY_SCTRL_H, "DPHY_SCTRL_H" },
> > -		{ MIPI_CSIS_ISP_CONFIG_CH(0), "ISP_CONFIG_CH0" },
> > -		{ MIPI_CSIS_ISP_RESOL_CH(0), "ISP_RESOL_CH0" },
> > -		{ MIPI_CSIS_SDW_CONFIG_CH(0), "SDW_CONFIG_CH0" },
> > -		{ MIPI_CSIS_SDW_RESOL_CH(0), "SDW_RESOL_CH0" },
> > -		{ MIPI_CSIS_DBG_CTRL, "DBG_CTRL" },
> > -		{ MIPI_CSIS_FRAME_COUNTER_CH(0), "FRAME_COUNTER_CH0" },
> > +	static const struct mipi_csis_reg_info common_registers[] = {
> > +		{ MIPI_CSIS_CMN_CTRL, 0, "CMN_CTRL" },
> > +		{ MIPI_CSIS_CLK_CTRL, 0, "CLK_CTRL" },
> > +		{ MIPI_CSIS_INT_MSK, 0, "INT_MSK" },
> > +		{ MIPI_CSIS_DPHY_STATUS, 0, "DPHY_STATUS" },
> > +		{ MIPI_CSIS_DPHY_CMN_CTRL, 0, "DPHY_CMN_CTRL" },
> > +		{ MIPI_CSIS_DPHY_SCTRL_L, 0, "DPHY_SCTRL_L" },
> > +		{ MIPI_CSIS_DPHY_SCTRL_H, 0, "DPHY_SCTRL_H" },
> > +		{ MIPI_CSIS_DBG_CTRL, 0, "DBG_CTRL" },
> > +	};
> > +	static const struct mipi_csis_reg_info channel_registers[] = {
> > +		{ MIPI_CSIS_ISP_CONFIG_CH(0), 0x10, "ISP_CONFIG_CH" },
> > +		{ MIPI_CSIS_ISP_RESOL_CH(0), 0x10, "ISP_RESOL_CH" },
> > +		{ MIPI_CSIS_SDW_CONFIG_CH(0), 0x10, "SDW_CONFIG_CH" },
> > +		{ MIPI_CSIS_SDW_RESOL_CH(0), 0x10, "SDW_RESOL_CH" },
> > +		{ MIPI_CSIS_FRAME_COUNTER_CH(0), 4, "FRAME_COUNTER_CH" },
> >  	};
> > -
> > -	unsigned int i;
> > -	u32 cfg;
> >
> >  	if (!pm_runtime_get_if_in_use(csis->dev))
> >  		return 0;
> >
> >  	dev_info(csis->dev, "--- REGISTERS ---\n");
> >
> > -	for (i = 0; i < ARRAY_SIZE(registers); i++) {
> > -		cfg = mipi_csis_read(csis, registers[i].offset);
> > -		dev_info(csis->dev, "%17s: 0x%08x\n", registers[i].name, cfg);
> > +	for (unsigned int i = 0; i < ARRAY_SIZE(common_registers); i++) {
> > +		const struct mipi_csis_reg_info *reg = &common_registers[i];
> > +
> > +		dev_info(csis->dev, "%17s: 0x%08x\n", reg->name,
> > +			 mipi_csis_read(csis, reg->addr));
> > +	}
> > +
> > +	for (unsigned int chan = 0; chan < csis->num_channels; chan++) {
> > +		for (unsigned int i = 0; i < ARRAY_SIZE(channel_registers); ++i)
> > +			mipi_csis_dump_channel_reg(csis, &channel_registers[i],
> > +						   chan);
> >  	}
> >
> >  	pm_runtime_put(csis->dev);
> > @@ -1422,6 +1483,12 @@ static int mipi_csis_parse_dt(struct mipi_csis_device *csis)
> >
> >  	of_property_read_u32(node, "clock-frequency", &csis->clk_frequency);
> >
> > +	csis->num_channels = 1;
> > +	of_property_read_u32(node, "fsl,num-channels", &csis->num_channels);
> > +	if (csis->num_channels < 1 || csis->num_channels > MIPI_CSIS_MAX_CHANNELS)
> > +		return dev_err_probe(csis->dev, -EINVAL,
> > +				     "Invalid fsl,num-channels value\n");
> > +
> >  	return 0;
> >  }
> >
> > @@ -1445,10 +1512,8 @@ static int mipi_csis_probe(struct platform_device *pdev)
> >
> >  	/* Parse DT properties. */
> >  	ret = mipi_csis_parse_dt(csis);
> > -	if (ret < 0) {
> > -		dev_err(dev, "Failed to parse device tree: %d\n", ret);
> > +	if (ret < 0)
> >  		return ret;
> > -	}
> >
> >  	/* Acquire resources. */
> >  	csis->regs = devm_platform_ioremap_resource(pdev, 0);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 12/12] arm64: dts: imx8mp: Specify the number of channels for CSI-2 receivers

[Laurent Pinchart]

On Thu, Aug 21, 2025 at 12:00:39PM -0400, Frank Li wrote:
> On Thu, Aug 21, 2025 at 03:09:44AM +0300, Laurent Pinchart wrote:
> > The CSI-2 receivers in the i.MX8MP have 3 output channels. Specify this
> > in the device tree, to enable support for more than one channel in
> > drivers.
> 
> dt descript hardware, not driver

Yes, but the whole point is to make use of the information in drivers
:-)

I'll just drop the "in drivers" part.

> Specify this to reflect actual hardware feature.
> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > index bb24dba7338e..1e52840078df 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > @@ -1764,6 +1764,7 @@ mipi_csi_0: csi@32e40000 {
> >  				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_250M>,
> >  							 <&clk IMX8MP_CLK_24M>;
> >  				power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>;
> > +				fsl,num-channels = <3>;
> >  				status = "disabled";
> >
> >  				ports {
> > @@ -1799,6 +1800,7 @@ mipi_csi_1: csi@32e50000 {
> >  				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_250M>,
> >  							 <&clk IMX8MP_CLK_24M>;
> >  				power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_2>;
> > +				fsl,num-channels = <3>;
> >  				status = "disabled";
> >
> >  				ports {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 11/12] media: imx-mipi-csis: Initial support for multiple output channels

[Frank Li]

On Thu, Aug 21, 2025 at 07:30:25PM +0300, Laurent Pinchart wrote:
> On Thu, Aug 21, 2025 at 11:56:43AM -0400, Frank Li wrote:
> > On Thu, Aug 21, 2025 at 03:09:43AM +0300, Laurent Pinchart wrote:
> > > Some CSIS instances feature more than one output channel. Update
> > > register macros accordingly, parse the number of channels from the
> > > device tree, and update register dumps and event counters to log
> > > per-channel data.
> > >
> > > Support for routing virtual channels and data types to output channels
> > > through the subdev internal routing API will come later.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > Changes since v1:
> > >
> > > - Update more per-channel registers
> > > - Update commit message
> > > ---
> > >  drivers/media/platform/nxp/imx-mipi-csis.c | 239 +++++++++++++--------
> > >  1 file changed, 152 insertions(+), 87 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > > index 83ba68a20bd1..b1136336a57f 100644
> > > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > > @@ -54,7 +54,7 @@
> > >
> > ...
> > >
> > > +	{ false, 0, MIPI_CSIS_INT_SRC_FRAME_START(0),		"Frame Start 0" },
> > > +	{ false, 1, MIPI_CSIS_INT_SRC_FRAME_START(1),		"Frame Start 1" },
> > > +	{ false, 2, MIPI_CSIS_INT_SRC_FRAME_START(2),		"Frame Start 2" },
> > > +	{ false, 3, MIPI_CSIS_INT_SRC_FRAME_START(3),		"Frame Start 3" },
> > > +	{ false, 0, MIPI_CSIS_INT_SRC_FRAME_END(0),		"Frame End 0" },
> > > +	{ false, 1, MIPI_CSIS_INT_SRC_FRAME_END(1),		"Frame End 1" },
> > > +	{ false, 2, MIPI_CSIS_INT_SRC_FRAME_END(2),		"Frame End 2" },
> > > +	{ false, 3, MIPI_CSIS_INT_SRC_FRAME_END(3),		"Frame End 3" },
> > > +	{ true, 0, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(0),	"VSYNC Falling Edge 0" },
> > > +	{ true, 1, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(1),	"VSYNC Falling Edge 1" },
> > > +	{ true, 2, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(2),	"VSYNC Falling Edge 2" },
> > > +	{ true, 3, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL(3),	"VSYNC Falling Edge 3" },
> > > +	{ true, 0, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(0),	"VSYNC Rising Edge 0" },
> > > +	{ true, 1, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(1),	"VSYNC Rising Edge 1" },
> > > +	{ true, 2, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(2),	"VSYNC Rising Edge 2" },
> > > +	{ true, 3, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE(3),	"VSYNC Rising Edge 3" },
> > >  };
> > >
> > > -#define MIPI_CSIS_NUM_EVENTS ARRAY_SIZE(mipi_csis_events)
> > > +#define MIPI_CSIS_NUM_EVENTS		ARRAY_SIZE(mipi_csis_events)
> >
> > unneccesary change for this patch
>
> It's meant to have the same alignment as the next line.
>
> > > +#define MIPI_CSIS_NUM_ERROR_EVENTS	(MIPI_CSIS_NUM_EVENTS - 20)
> > >
> > >  enum mipi_csis_clk {
> > >  	MIPI_CSIS_CLK_PCLK,
> > > @@ -300,7 +334,9 @@ struct mipi_csis_device {
> > >  	struct clk_bulk_data *clks;
> > >  	struct reset_control *mrst;
> > >  	struct regulator *mipi_phy_regulator;
> > > +
> > >  	const struct mipi_csis_info *info;
> > > +	unsigned int num_channels;
> > >
> > >  	struct v4l2_subdev sd;
> > >  	struct media_pad pads[CSIS_PADS_NUM];
> > > @@ -655,8 +691,8 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis,
> > >  			MIPI_CSIS_ISP_SYNC_VSYNC_EINTV(0));
> > >
> > >  	val = mipi_csis_read(csis, MIPI_CSIS_CLK_CTRL);
> > > -	val |= MIPI_CSIS_CLK_CTRL_WCLK_SRC;
> > > -	val |= MIPI_CSIS_CLK_CTRL_CLKGATE_TRAIL_CH0(15);
> > > +	val |= MIPI_CSIS_CLK_CTRL_WCLK_SRC(0);
> > > +	val |= MIPI_CSIS_CLK_CTRL_CLKGATE_TRAIL(0, 15);
> > >  	val &= ~MIPI_CSIS_CLK_CTRL_CLKGATE_EN_MSK;
> > >  	mipi_csis_write(csis, MIPI_CSIS_CLK_CTRL, val);
> > >
> > > @@ -673,7 +709,7 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis,
> > >  	/* Update the shadow register. */
> > >  	val = mipi_csis_read(csis, MIPI_CSIS_CMN_CTRL);
> > >  	mipi_csis_write(csis, MIPI_CSIS_CMN_CTRL,
> > > -			val | MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW |
> > > +			val | MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW(0) |
> > >  			MIPI_CSIS_CMN_CTRL_UPDATE_SHADOW_CTRL);
> > >  }
> > >
> > > @@ -764,16 +800,19 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
> > >
> > >  	/* Update the event/error counters */
> > >  	if ((status & MIPI_CSIS_INT_SRC_ERRORS) || csis->debug.enable) {
> > > -		for (i = 0; i < MIPI_CSIS_NUM_EVENTS; i++) {
> > > +		for (i = 0; i < ARRAY_SIZE(csis->events); i++) {
> >
> > This is nice change, but I think it is not related with this patch. May
> > need sperate patch.
>
> I think a separate patch just for this one-line change would be
> overkill. Given that this patch touches event reporting, I'd rather keep
> this here.

It is not big deal. Generally, don't touch it to simple review.

>
> > >  			struct mipi_csis_event *event = &csis->events[i];
> > >
> > > +			if (event->channel >= csis->num_channels)
> > > +				continue;
> > > +
> > >  			if ((!event->debug && (status & event->mask)) ||
> > >  			    (event->debug && (dbg_status & event->mask)))
> > >  				event->counter++;
> > >  		}
> > >  	}
> > >
> > > -	if (status & MIPI_CSIS_INT_SRC_FRAME_START)
> > > +	if (status & MIPI_CSIS_INT_SRC_FRAME_START(0))
> > >  		mipi_csis_queue_event_sof(csis);
> > >
> > >  	spin_unlock_irqrestore(&csis->slock, flags);
> > > @@ -850,7 +889,7 @@ static void mipi_csis_clear_counters(struct mipi_csis_device *csis)
> > >  static void mipi_csis_log_counters(struct mipi_csis_device *csis, bool non_errors)
> > >  {
> > >  	unsigned int num_events = non_errors ? MIPI_CSIS_NUM_EVENTS
> > > -				: MIPI_CSIS_NUM_EVENTS - 8;
> > > +				: MIPI_CSIS_NUM_ERROR_EVENTS;
> >
> > I think old code logic is strange. err events is not last trail of events
> > array. when non_errors false, only last 8 events have not logs.
>
> The error events are at the beginning of the array, and before this
> change there was 8 non-error events at the end. The code would log
> either all events, or just the error events (all minus the last 8).

yes, thanks.

Frank
>
> >
> > And I found all place call mipi_csis_log_counters(, true) in whole driver.
>
> Indeed. I wonder why. Looking at the code, I think we should only log
> non-error counters when csis->debug.enable is set. That's a candidate
> for a separate patch.
>
> > >  	unsigned int counters[MIPI_CSIS_NUM_EVENTS];
> > >  	unsigned long flags;
> > >  	unsigned int i;
> > > @@ -861,45 +900,67 @@ static void mipi_csis_log_counters(struct mipi_csis_device *csis, bool non_error
> > >  	spin_unlock_irqrestore(&csis->slock, flags);
> > >
> > >  	for (i = 0; i < num_events; ++i) {
> > > +		const struct mipi_csis_event *event = &csis->events[i];
> > > +
> > > +		if (event->channel >= csis->num_channels)
> > > +			continue;
> > > +
> > >  		if (counters[i] > 0 || csis->debug.enable)
> > >  			dev_info(csis->dev, "%s events: %d\n",
> > > -				 csis->events[i].name,
> > > -				 counters[i]);
> > > +				 event->name, counters[i]);
> > >  	}
> > >  }
> > >
> > > +struct mipi_csis_reg_info {
> > > +	u32 addr;
> > > +	unsigned int offset;
> > > +	const char * const name;
> > > +};
> > > +
> > > +static void mipi_csis_dump_channel_reg(struct mipi_csis_device *csis,
> > > +				       const struct mipi_csis_reg_info *reg,
> > > +				       unsigned int channel)
> > > +{
> > > +	dev_info(csis->dev, "%16s%u: 0x%08x\n", reg->name, channel,
> > > +		 mipi_csis_read(csis, reg->addr + channel * reg->offset));
> > > +}
> > > +
> > >  static int mipi_csis_dump_regs(struct mipi_csis_device *csis)
> > >  {
> > > -	static const struct {
> > > -		u32 offset;
> > > -		const char * const name;
> > > -	} registers[] = {
> > > -		{ MIPI_CSIS_CMN_CTRL, "CMN_CTRL" },
> > > -		{ MIPI_CSIS_CLK_CTRL, "CLK_CTRL" },
> > > -		{ MIPI_CSIS_INT_MSK, "INT_MSK" },
> > > -		{ MIPI_CSIS_DPHY_STATUS, "DPHY_STATUS" },
> > > -		{ MIPI_CSIS_DPHY_CMN_CTRL, "DPHY_CMN_CTRL" },
> > > -		{ MIPI_CSIS_DPHY_SCTRL_L, "DPHY_SCTRL_L" },
> > > -		{ MIPI_CSIS_DPHY_SCTRL_H, "DPHY_SCTRL_H" },
> > > -		{ MIPI_CSIS_ISP_CONFIG_CH(0), "ISP_CONFIG_CH0" },
> > > -		{ MIPI_CSIS_ISP_RESOL_CH(0), "ISP_RESOL_CH0" },
> > > -		{ MIPI_CSIS_SDW_CONFIG_CH(0), "SDW_CONFIG_CH0" },
> > > -		{ MIPI_CSIS_SDW_RESOL_CH(0), "SDW_RESOL_CH0" },
> > > -		{ MIPI_CSIS_DBG_CTRL, "DBG_CTRL" },
> > > -		{ MIPI_CSIS_FRAME_COUNTER_CH(0), "FRAME_COUNTER_CH0" },
> > > +	static const struct mipi_csis_reg_info common_registers[] = {
> > > +		{ MIPI_CSIS_CMN_CTRL, 0, "CMN_CTRL" },
> > > +		{ MIPI_CSIS_CLK_CTRL, 0, "CLK_CTRL" },
> > > +		{ MIPI_CSIS_INT_MSK, 0, "INT_MSK" },
> > > +		{ MIPI_CSIS_DPHY_STATUS, 0, "DPHY_STATUS" },
> > > +		{ MIPI_CSIS_DPHY_CMN_CTRL, 0, "DPHY_CMN_CTRL" },
> > > +		{ MIPI_CSIS_DPHY_SCTRL_L, 0, "DPHY_SCTRL_L" },
> > > +		{ MIPI_CSIS_DPHY_SCTRL_H, 0, "DPHY_SCTRL_H" },
> > > +		{ MIPI_CSIS_DBG_CTRL, 0, "DBG_CTRL" },
> > > +	};
> > > +	static const struct mipi_csis_reg_info channel_registers[] = {
> > > +		{ MIPI_CSIS_ISP_CONFIG_CH(0), 0x10, "ISP_CONFIG_CH" },
> > > +		{ MIPI_CSIS_ISP_RESOL_CH(0), 0x10, "ISP_RESOL_CH" },
> > > +		{ MIPI_CSIS_SDW_CONFIG_CH(0), 0x10, "SDW_CONFIG_CH" },
> > > +		{ MIPI_CSIS_SDW_RESOL_CH(0), 0x10, "SDW_RESOL_CH" },
> > > +		{ MIPI_CSIS_FRAME_COUNTER_CH(0), 4, "FRAME_COUNTER_CH" },
> > >  	};
> > > -
> > > -	unsigned int i;
> > > -	u32 cfg;
> > >
> > >  	if (!pm_runtime_get_if_in_use(csis->dev))
> > >  		return 0;
> > >
> > >  	dev_info(csis->dev, "--- REGISTERS ---\n");
> > >
> > > -	for (i = 0; i < ARRAY_SIZE(registers); i++) {
> > > -		cfg = mipi_csis_read(csis, registers[i].offset);
> > > -		dev_info(csis->dev, "%17s: 0x%08x\n", registers[i].name, cfg);
> > > +	for (unsigned int i = 0; i < ARRAY_SIZE(common_registers); i++) {
> > > +		const struct mipi_csis_reg_info *reg = &common_registers[i];
> > > +
> > > +		dev_info(csis->dev, "%17s: 0x%08x\n", reg->name,
> > > +			 mipi_csis_read(csis, reg->addr));
> > > +	}
> > > +
> > > +	for (unsigned int chan = 0; chan < csis->num_channels; chan++) {
> > > +		for (unsigned int i = 0; i < ARRAY_SIZE(channel_registers); ++i)
> > > +			mipi_csis_dump_channel_reg(csis, &channel_registers[i],
> > > +						   chan);
> > >  	}
> > >
> > >  	pm_runtime_put(csis->dev);
> > > @@ -1422,6 +1483,12 @@ static int mipi_csis_parse_dt(struct mipi_csis_device *csis)
> > >
> > >  	of_property_read_u32(node, "clock-frequency", &csis->clk_frequency);
> > >
> > > +	csis->num_channels = 1;
> > > +	of_property_read_u32(node, "fsl,num-channels", &csis->num_channels);
> > > +	if (csis->num_channels < 1 || csis->num_channels > MIPI_CSIS_MAX_CHANNELS)
> > > +		return dev_err_probe(csis->dev, -EINVAL,
> > > +				     "Invalid fsl,num-channels value\n");
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -1445,10 +1512,8 @@ static int mipi_csis_probe(struct platform_device *pdev)
> > >
> > >  	/* Parse DT properties. */
> > >  	ret = mipi_csis_parse_dt(csis);
> > > -	if (ret < 0) {
> > > -		dev_err(dev, "Failed to parse device tree: %d\n", ret);
> > > +	if (ret < 0)
> > >  		return ret;
> > > -	}
> > >
> > >  	/* Acquire resources. */
> > >  	csis->regs = devm_platform_ioremap_resource(pdev, 0);
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 04/12] media: imx-mipi-csis: Shorten name of subdev state variables

[Alexander Stein]

Hi Laurent,

thanks for the patch.

Am Donnerstag, 21. August 2025, 02:09:36 CEST schrieb Laurent Pinchart:
> Rename subdev state parameters passed to subdev operations from sd_state
> to state. This standardizes the naming of the subdev state variables
> through the driver, and helps shortening lines.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Maybe add a 'No functionality change', it's just rename. Either way:
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 0f0863011230..894d12fef519 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -996,7 +996,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  }
>  
>  static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
> -				    struct v4l2_subdev_state *sd_state,
> +				    struct v4l2_subdev_state *state,
>  				    struct v4l2_subdev_mbus_code_enum *code)
>  {
>  	/*
> @@ -1009,7 +1009,7 @@ static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
>  		if (code->index > 0)
>  			return -EINVAL;
>  
> -		fmt = v4l2_subdev_state_get_format(sd_state, code->pad);
> +		fmt = v4l2_subdev_state_get_format(state, code->pad);
>  		code->code = fmt->code;
>  		return 0;
>  	}
> @@ -1026,7 +1026,7 @@ static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
>  }
>  
>  static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
> -			     struct v4l2_subdev_state *sd_state,
> +			     struct v4l2_subdev_state *state,
>  			     struct v4l2_subdev_format *sdformat)
>  {
>  	const struct csis_pix_format *csis_fmt;
> @@ -1038,7 +1038,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
>  	 * modified.
>  	 */
>  	if (sdformat->pad == CSIS_PAD_SOURCE)
> -		return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
> +		return v4l2_subdev_get_fmt(sd, state, sdformat);
>  
>  	if (sdformat->pad != CSIS_PAD_SINK)
>  		return -EINVAL;
> @@ -1076,7 +1076,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
>  			      &sdformat->format.height, 1,
>  			      CSIS_MAX_PIX_HEIGHT, 0, 0);
>  
> -	fmt = v4l2_subdev_state_get_format(sd_state, sdformat->pad);
> +	fmt = v4l2_subdev_state_get_format(state, sdformat->pad);
>  
>  	fmt->code = csis_fmt->code;
>  	fmt->width = sdformat->format.width;
> @@ -1090,7 +1090,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
>  	sdformat->format = *fmt;
>  
>  	/* Propagate the format from sink to source. */
> -	fmt = v4l2_subdev_state_get_format(sd_state, CSIS_PAD_SOURCE);
> +	fmt = v4l2_subdev_state_get_format(state, CSIS_PAD_SOURCE);
>  	*fmt = sdformat->format;
>  
>  	/* The format on the source pad might change due to unpacking. */
> @@ -1130,7 +1130,7 @@ static int mipi_csis_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
>  }
>  
>  static int mipi_csis_init_state(struct v4l2_subdev *sd,
> -				struct v4l2_subdev_state *sd_state)
> +				struct v4l2_subdev_state *state)
>  {
>  	struct v4l2_subdev_format fmt = {
>  		.pad = CSIS_PAD_SINK,
> @@ -1147,7 +1147,7 @@ static int mipi_csis_init_state(struct v4l2_subdev *sd,
>  		V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt.format.colorspace,
>  					      fmt.format.ycbcr_enc);
>  
> -	return mipi_csis_set_fmt(sd, sd_state, &fmt);
> +	return mipi_csis_set_fmt(sd, state, &fmt);
>  }
>  
>  static int mipi_csis_log_status(struct v4l2_subdev *sd)
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v2 12/12] arm64: dts: imx8mp: Specify the number of channels for CSI-2 receivers

[Stefan Klug]

Hi Laurent,

Thank you for the patch. 

Quoting Laurent Pinchart (2025-08-21 02:09:44)
> The CSI-2 receivers in the i.MX8MP have 3 output channels. Specify this
> in the device tree, to enable support for more than one channel in
> drivers.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
Tested-by: Stefan Klug <stefan.klug@ideasonboard.com>

Cheers,
Stefan

> ---
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index bb24dba7338e..1e52840078df 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -1764,6 +1764,7 @@ mipi_csi_0: csi@32e40000 {
>                                 assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_250M>,
>                                                          <&clk IMX8MP_CLK_24M>;
>                                 power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>;
> +                               fsl,num-channels = <3>;
>                                 status = "disabled";
>  
>                                 ports {
> @@ -1799,6 +1800,7 @@ mipi_csi_1: csi@32e50000 {
>                                 assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_250M>,
>                                                          <&clk IMX8MP_CLK_24M>;
>                                 power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_2>;
> +                               fsl,num-channels = <3>;
>                                 status = "disabled";
>  
>                                 ports {
> -- 
> Regards,
> 
> Laurent Pinchart
> 
>