[PATCH v3 0/3] add ISI support for iMX93

[PATCH v3 0/3] add ISI support for iMX93

[guoniu.zhou]

From: "Guoniu.zhou" <guoniu.zhou@nxp.com>

NXP i.MX93 family almost reuse ISI IP from i.MX8M family, so add
it support in current ISI driver

Changes in version 3:
- Split [PATCH v2 2/2] into two patches, one create a separate file to
  store gasket operation and the other to add ISI support for i.MX93.
- Drop some debug message in gasket operation.
- Merge .gasket_enable and .gasket_config to .gasket_enable
- Drop some dead code
- Some other small updates

Changes in version 2:
- Remove two patches which used to rename imx8 to imx.
  [PATCH 1/4] media: dt-bindings: media: rename nxp,imx8-isi.yaml to nxp,imx-isi.yaml
  [PATCH 2/4] media: nxp: rename imx8-isi to imx-isi and remove reference to i.MX8
- Modify commit log to more accurately match its goal.
- Remove redundant "media" in patch subject.

Guoniu.zhou (3):
  media: dt-bindings: nxp,imx8-isi: add i.MX93 ISI compatible string
  media: nxp: imx8-isi: move i.MX8 gasket configuration to an ops
    structure
  media: nxp: imx8-isi: add ISI support for i.MX93

 .../bindings/media/nxp,imx8-isi.yaml          |  5 +-
 drivers/media/platform/nxp/imx8-isi/Makefile  |  2 +-
 .../platform/nxp/imx8-isi/imx8-isi-core.c     | 32 +++++++++--
 .../platform/nxp/imx8-isi/imx8-isi-core.h     | 42 ++++++++++++++-
 .../platform/nxp/imx8-isi/imx8-isi-crossbar.c | 42 +++++----------
 .../platform/nxp/imx8-isi/imx8-isi-gasket.c   | 53 +++++++++++++++++++
 6 files changed, 142 insertions(+), 34 deletions(-)
 create mode 100644 drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c

base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
-- 
2.37.1

[PATCH v3 1/3] media: dt-bindings: nxp,imx8-isi: add i.MX93 ISI compatible string

[guoniu.zhou]

From: "Guoniu.zhou" <guoniu.zhou@nxp.com>

Add the compatible string support for i.MX93 ISI.

Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml b/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
index 6038b9b5ab36..e4665469a86c 100644
--- a/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
+++ b/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
@@ -21,6 +21,7 @@ properties:
     enum:
       - fsl,imx8mn-isi
       - fsl,imx8mp-isi
+      - fsl,imx93-isi
 
   reg:
     maxItems: 1
@@ -72,7 +73,9 @@ allOf:
       properties:
         compatible:
           contains:
-            const: fsl,imx8mn-isi
+            enum:
+              - fsl,imx8mn-isi
+              - fsl,imx93-isi
     then:
       properties:
         interrupts:
-- 
2.37.1

[PATCH v3 2/3] media: nxp: imx8-isi: move i.MX8 gasket configuration to an ops structure

[guoniu.zhou]

From: "Guoniu.zhou" <guoniu.zhou@nxp.com>

Move i.MX8MN and i.MX8MP gasket configuration to an ops structure.

Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com>
---
 drivers/media/platform/nxp/imx8-isi/Makefile  |  2 +-
 .../platform/nxp/imx8-isi/imx8-isi-core.c     | 12 ++++--
 .../platform/nxp/imx8-isi/imx8-isi-core.h     | 30 ++++++++++++-
 .../platform/nxp/imx8-isi/imx8-isi-crossbar.c | 42 +++++++------------
 .../platform/nxp/imx8-isi/imx8-isi-gasket.c   | 32 ++++++++++++++
 5 files changed, 85 insertions(+), 33 deletions(-)

diff --git a/drivers/media/platform/nxp/imx8-isi/Makefile b/drivers/media/platform/nxp/imx8-isi/Makefile
index 9bff9297686d..4376e8e0c962 100644
--- a/drivers/media/platform/nxp/imx8-isi/Makefile
+++ b/drivers/media/platform/nxp/imx8-isi/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
 imx8-isi-y := imx8-isi-core.o imx8-isi-crossbar.o imx8-isi-hw.o \
-	imx8-isi-pipe.o imx8-isi-video.o
+	imx8-isi-pipe.o imx8-isi-video.o imx8-isi-gasket.o
 imx8-isi-$(CONFIG_DEBUG_FS) += imx8-isi-debug.o
 imx8-isi-$(CONFIG_VIDEO_IMX8_ISI_M2M) += imx8-isi-m2m.o
 
diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
index 253e77189b69..d645b2f6fa5a 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
@@ -279,6 +279,12 @@ static const struct clk_bulk_data mxc_imx8mn_clks[] = {
 	{ .id = "apb" },
 };
 
+/* Gasket operations for i.MX8MN and i.MX8MP */
+static const struct mxc_gasket_ops mxc_imx8_gasket_ops = {
+	.enable = mxc_imx8_gasket_enable,
+	.disable = mxc_imx8_gasket_disable,
+};
+
 static const struct mxc_isi_plat_data mxc_imx8mn_data = {
 	.model			= MXC_ISI_IMX8MN,
 	.num_ports		= 1,
@@ -289,7 +295,7 @@ static const struct mxc_isi_plat_data mxc_imx8mn_data = {
 	.clks			= mxc_imx8mn_clks,
 	.num_clks		= ARRAY_SIZE(mxc_imx8mn_clks),
 	.buf_active_reverse	= false,
-	.has_gasket		= true,
+	.gasket_ops		= &mxc_imx8_gasket_ops,
 	.has_36bit_dma		= false,
 };
 
@@ -303,7 +309,7 @@ static const struct mxc_isi_plat_data mxc_imx8mp_data = {
 	.clks			= mxc_imx8mn_clks,
 	.num_clks		= ARRAY_SIZE(mxc_imx8mn_clks),
 	.buf_active_reverse	= true,
-	.has_gasket		= true,
+	.gasket_ops		= &mxc_imx8_gasket_ops,
 	.has_36bit_dma		= true,
 };
 
@@ -443,7 +449,7 @@ static int mxc_isi_probe(struct platform_device *pdev)
 		return PTR_ERR(isi->regs);
 	}
 
-	if (isi->pdata->has_gasket) {
+	if (isi->pdata->gasket_ops) {
 		isi->gasket = syscon_regmap_lookup_by_phandle(dev->of_node,
 							      "fsl,blk-ctrl");
 		if (IS_ERR(isi->gasket)) {
diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
index e469788a9e6c..4f920d650153 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
@@ -12,12 +12,14 @@
 
 #include <linux/list.h>
 #include <linux/mutex.h>
+#include <linux/regmap.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
 #include <linux/videodev2.h>
 
 #include <media/media-device.h>
 #include <media/media-entity.h>
+#include <media/mipi-csi2.h>
 #include <media/v4l2-async.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-dev.h>
@@ -59,6 +61,18 @@ struct v4l2_m2m_dev;
 #define MXC_ISI_M2M			"mxc-isi-m2m"
 #define MXC_MAX_PLANES			3
 
+/* GASKET (i.MX8MN and i.MX8MP only) */
+#define GASKET_BASE(n)				(0x0060 + (n) * 0x30)
+
+#define GASKET_CTRL				0x0000
+#define GASKET_CTRL_DATA_TYPE(dt)		((dt) << 8)
+#define GASKET_CTRL_DATA_TYPE_MASK		(0x3f << 8)
+#define GASKET_CTRL_DUAL_COMP_ENABLE		BIT(1)
+#define GASKET_CTRL_ENABLE			BIT(0)
+
+#define GASKET_HSIZE				0x0004
+#define GASKET_VSIZE				0x0008
+
 struct mxc_isi_dev;
 struct mxc_isi_m2m_ctx;
 
@@ -147,6 +161,14 @@ struct mxc_isi_set_thd {
 	struct mxc_isi_panic_thd panic_set_thd_v;
 };
 
+struct mxc_gasket_ops {
+	int (*enable)(struct mxc_isi_dev *isi,
+		      const struct v4l2_mbus_frame_desc *fd,
+		      const struct v4l2_mbus_framefmt *fmt,
+		      const unsigned int port);
+	void (*disable)(struct mxc_isi_dev *isi, const unsigned int port);
+};
+
 enum model {
 	MXC_ISI_IMX8MN,
 	MXC_ISI_IMX8MP,
@@ -159,10 +181,10 @@ struct mxc_isi_plat_data {
 	unsigned int reg_offset;
 	const struct mxc_isi_ier_reg  *ier_reg;
 	const struct mxc_isi_set_thd *set_thd;
+	const struct mxc_gasket_ops *gasket_ops;
 	const struct clk_bulk_data *clks;
 	unsigned int num_clks;
 	bool buf_active_reverse;
-	bool has_gasket;
 	bool has_36bit_dma;
 };
 
@@ -379,6 +401,12 @@ void mxc_isi_channel_set_outbuf(struct mxc_isi_pipe *pipe,
 u32 mxc_isi_channel_irq_status(struct mxc_isi_pipe *pipe, bool clear);
 void mxc_isi_channel_irq_clear(struct mxc_isi_pipe *pipe);
 
+int mxc_imx8_gasket_enable(struct mxc_isi_dev *isi,
+			   const struct v4l2_mbus_frame_desc *fd,
+			   const struct v4l2_mbus_framefmt *fmt,
+			   const unsigned int port);
+void mxc_imx8_gasket_disable(struct mxc_isi_dev *isi, const unsigned int port);
+
 #if IS_ENABLED(CONFIG_DEBUG_FS)
 void mxc_isi_debug_init(struct mxc_isi_dev *isi);
 void mxc_isi_debug_cleanup(struct mxc_isi_dev *isi);
diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
index f7447b2f4d77..d803fda3fdaf 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
@@ -25,32 +25,18 @@ static inline struct mxc_isi_crossbar *to_isi_crossbar(struct v4l2_subdev *sd)
 	return container_of(sd, struct mxc_isi_crossbar, sd);
 }
 
-/* -----------------------------------------------------------------------------
- * Media block control (i.MX8MN and i.MX8MP only)
- */
-#define GASKET_BASE(n)				(0x0060 + (n) * 0x30)
-
-#define GASKET_CTRL				0x0000
-#define GASKET_CTRL_DATA_TYPE(dt)		((dt) << 8)
-#define GASKET_CTRL_DATA_TYPE_MASK		(0x3f << 8)
-#define GASKET_CTRL_DUAL_COMP_ENABLE		BIT(1)
-#define GASKET_CTRL_ENABLE			BIT(0)
-
-#define GASKET_HSIZE				0x0004
-#define GASKET_VSIZE				0x0008
-
 static int mxc_isi_crossbar_gasket_enable(struct mxc_isi_crossbar *xbar,
 					  struct v4l2_subdev_state *state,
 					  struct v4l2_subdev *remote_sd,
 					  u32 remote_pad, unsigned int port)
 {
 	struct mxc_isi_dev *isi = xbar->isi;
+	const struct mxc_gasket_ops *gasket_ops = isi->pdata->gasket_ops;
 	const struct v4l2_mbus_framefmt *fmt;
 	struct v4l2_mbus_frame_desc fd;
-	u32 val;
 	int ret;
 
-	if (!isi->pdata->has_gasket)
+	if (!gasket_ops)
 		return 0;
 
 	/*
@@ -77,16 +63,14 @@ static int mxc_isi_crossbar_gasket_enable(struct mxc_isi_crossbar *xbar,
 	if (!fmt)
 		return -EINVAL;
 
-	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_HSIZE, fmt->width);
-	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_VSIZE, fmt->height);
-
-	val = GASKET_CTRL_DATA_TYPE(fd.entry[0].bus.csi2.dt)
-	    | GASKET_CTRL_ENABLE;
-
-	if (fd.entry[0].bus.csi2.dt == MIPI_CSI2_DT_YUV422_8B)
-		val |= GASKET_CTRL_DUAL_COMP_ENABLE;
-
-	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, val);
+	if (gasket_ops->enable) {
+		ret = gasket_ops->enable(isi, &fd, fmt, port);
+		if (ret) {
+			dev_err(isi->dev,
+				"failed to enable gasket%d\n", port);
+			return ret;
+		}
+	}
 
 	return 0;
 }
@@ -95,11 +79,13 @@ static void mxc_isi_crossbar_gasket_disable(struct mxc_isi_crossbar *xbar,
 					    unsigned int port)
 {
 	struct mxc_isi_dev *isi = xbar->isi;
+	const struct mxc_gasket_ops *gasket_ops = isi->pdata->gasket_ops;
 
-	if (!isi->pdata->has_gasket)
+	if (!gasket_ops)
 		return;
 
-	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, 0);
+	if (gasket_ops->disable)
+		gasket_ops->disable(isi, port);
 }
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
new file mode 100644
index 000000000000..39f8d0e8b15d
--- /dev/null
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019-2023 NXP
+ */
+
+#include "imx8-isi-core.h"
+
+/* Configure and enable gasket for i.MX8MN and i.MX8P */
+int mxc_imx8_gasket_enable(struct mxc_isi_dev *isi,
+			   const struct v4l2_mbus_frame_desc *fd,
+			   const struct v4l2_mbus_framefmt *fmt,
+			   const unsigned int port)
+{
+	u32 val;
+
+	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_HSIZE, fmt->width);
+	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_VSIZE, fmt->height);
+
+	val = GASKET_CTRL_DATA_TYPE(fd->entry[0].bus.csi2.dt);
+	if (fd->entry[0].bus.csi2.dt == MIPI_CSI2_DT_YUV422_8B)
+		val |= GASKET_CTRL_DUAL_COMP_ENABLE;
+
+	val |= GASKET_CTRL_ENABLE;
+	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, val);
+	return 0;
+}
+
+/* Disable gasket for i.MX8MN and i.MX8P */
+void mxc_imx8_gasket_disable(struct mxc_isi_dev *isi, const unsigned int port)
+{
+	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, 0);
+}
-- 
2.37.1

[PATCH v3 3/3] media: nxp: imx8-isi: add ISI support for i.MX93

[guoniu.zhou]

From: "Guoniu.zhou" <guoniu.zhou@nxp.com>

i.MX93 use a different gasket which has different register definition
compared with i.MX8. Hence implement the gasket callbacks in order to
add ISI support for i.MX93.

Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com>
---
 .../platform/nxp/imx8-isi/imx8-isi-core.c     | 20 ++++++++++++++++++
 .../platform/nxp/imx8-isi/imx8-isi-core.h     | 12 +++++++++++
 .../platform/nxp/imx8-isi/imx8-isi-gasket.c   | 21 +++++++++++++++++++
 3 files changed, 53 insertions(+)

diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
index d645b2f6fa5a..24c40e4cfef5 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
@@ -313,6 +313,25 @@ static const struct mxc_isi_plat_data mxc_imx8mp_data = {
 	.has_36bit_dma		= true,
 };
 
+static const struct mxc_gasket_ops mxc_imx93_gasket_ops = {
+	.enable = mxc_imx93_gasket_enable,
+	.disable = mxc_imx93_gasket_disable,
+};
+
+static const struct mxc_isi_plat_data mxc_imx93_data = {
+	.model			= MXC_ISI_IMX93,
+	.num_ports		= 1,
+	.num_channels		= 1,
+	.reg_offset		= 0,
+	.ier_reg		= &mxc_imx8_isi_ier_v2,
+	.set_thd		= &mxc_imx8_isi_thd_v1,
+	.clks			= mxc_imx8mn_clks,
+	.num_clks		= ARRAY_SIZE(mxc_imx8mn_clks),
+	.buf_active_reverse	= true,
+	.gasket_ops		= &mxc_imx93_gasket_ops,
+	.has_36bit_dma		= false,
+};
+
 /* -----------------------------------------------------------------------------
  * Power management
  */
@@ -524,6 +543,7 @@ static int mxc_isi_remove(struct platform_device *pdev)
 static const struct of_device_id mxc_isi_of_match[] = {
 	{ .compatible = "fsl,imx8mn-isi", .data = &mxc_imx8mn_data },
 	{ .compatible = "fsl,imx8mp-isi", .data = &mxc_imx8mp_data },
+	{ .compatible = "fsl,imx93-isi", .data = &mxc_imx93_data },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, mxc_isi_of_match);
diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
index 4f920d650153..f5be5394981e 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
@@ -73,6 +73,11 @@ struct v4l2_m2m_dev;
 #define GASKET_HSIZE				0x0004
 #define GASKET_VSIZE				0x0008
 
+/* dispmix_GPR register (i.MX93 only) */
+#define DISP_MIX_CAMERA_MUX                     0x30
+#define DISP_MIX_CAMERA_MUX_DATA_TYPE(x)        (((x) & 0x3f) << 3)
+#define DISP_MIX_CAMERA_MUX_GASKET_ENABLE       BIT(16)
+
 struct mxc_isi_dev;
 struct mxc_isi_m2m_ctx;
 
@@ -172,6 +177,7 @@ struct mxc_gasket_ops {
 enum model {
 	MXC_ISI_IMX8MN,
 	MXC_ISI_IMX8MP,
+	MXC_ISI_IMX93,
 };
 
 struct mxc_isi_plat_data {
@@ -407,6 +413,12 @@ int mxc_imx8_gasket_enable(struct mxc_isi_dev *isi,
 			   const unsigned int port);
 void mxc_imx8_gasket_disable(struct mxc_isi_dev *isi, const unsigned int port);
 
+int mxc_imx93_gasket_enable(struct mxc_isi_dev *isi,
+			    const struct v4l2_mbus_frame_desc *fd,
+			    const struct v4l2_mbus_framefmt *fmt,
+			    const unsigned int port);
+void mxc_imx93_gasket_disable(struct mxc_isi_dev *isi, const unsigned int port);
+
 #if IS_ENABLED(CONFIG_DEBUG_FS)
 void mxc_isi_debug_init(struct mxc_isi_dev *isi);
 void mxc_isi_debug_cleanup(struct mxc_isi_dev *isi);
diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
index 39f8d0e8b15d..a81c4249a26f 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
@@ -30,3 +30,24 @@ void mxc_imx8_gasket_disable(struct mxc_isi_dev *isi, const unsigned int port)
 {
 	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, 0);
 }
+
+/* Configure and enable gasket for i.MX93 */
+int mxc_imx93_gasket_enable(struct mxc_isi_dev *isi,
+			    const struct v4l2_mbus_frame_desc *fd,
+			    const struct v4l2_mbus_framefmt *fmt,
+			    const unsigned int port)
+{
+	u32 val;
+
+	val = DISP_MIX_CAMERA_MUX_DATA_TYPE(fd->entry[0].bus.csi2.dt);
+	val |= DISP_MIX_CAMERA_MUX_GASKET_ENABLE;
+	regmap_write(isi->gasket, DISP_MIX_CAMERA_MUX, val);
+
+	return 0;
+}
+
+void mxc_imx93_gasket_disable(struct mxc_isi_dev *isi,
+			      unsigned int port)
+{
+	regmap_write(isi->gasket, DISP_MIX_CAMERA_MUX, 0);
+}
-- 
2.37.1

Re: [PATCH v3 2/3] media: nxp: imx8-isi: move i.MX8 gasket configuration to an ops structure

[Laurent Pinchart]

Hi Guoniu,

Thank you for the patch.

On Tue, Jun 27, 2023 at 02:20:16PM +0800, guoniu.zhou@oss.nxp.com wrote:
> From: "Guoniu.zhou" <guoniu.zhou@nxp.com>
> 
> Move i.MX8MN and i.MX8MP gasket configuration to an ops structure.

Commit messages should explain the reason behind the change, not just
what the change does. The *why* is considered even more important than
the *what*, as it's (usually) possible to understand what a patch does
by reading it, while the reason is much more difficult to get from just
looking at the code.

In this case, the commit message could be

"The i.MX93 includes an ISI instance compatible with the imx8-isi
driver, but with a different gasket. To prepare for this, make the
gasket configuration modular by moving the code to an ops structure."

> Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com>
> ---
>  drivers/media/platform/nxp/imx8-isi/Makefile  |  2 +-
>  .../platform/nxp/imx8-isi/imx8-isi-core.c     | 12 ++++--
>  .../platform/nxp/imx8-isi/imx8-isi-core.h     | 30 ++++++++++++-
>  .../platform/nxp/imx8-isi/imx8-isi-crossbar.c | 42 +++++++------------
>  .../platform/nxp/imx8-isi/imx8-isi-gasket.c   | 32 ++++++++++++++
>  5 files changed, 85 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx8-isi/Makefile b/drivers/media/platform/nxp/imx8-isi/Makefile
> index 9bff9297686d..4376e8e0c962 100644
> --- a/drivers/media/platform/nxp/imx8-isi/Makefile
> +++ b/drivers/media/platform/nxp/imx8-isi/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  
>  imx8-isi-y := imx8-isi-core.o imx8-isi-crossbar.o imx8-isi-hw.o \
> -	imx8-isi-pipe.o imx8-isi-video.o
> +	imx8-isi-pipe.o imx8-isi-video.o imx8-isi-gasket.o

Please use alphabetical order.

>  imx8-isi-$(CONFIG_DEBUG_FS) += imx8-isi-debug.o
>  imx8-isi-$(CONFIG_VIDEO_IMX8_ISI_M2M) += imx8-isi-m2m.o
>  
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> index 253e77189b69..d645b2f6fa5a 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> @@ -279,6 +279,12 @@ static const struct clk_bulk_data mxc_imx8mn_clks[] = {
>  	{ .id = "apb" },
>  };
>  
> +/* Gasket operations for i.MX8MN and i.MX8MP */
> +static const struct mxc_gasket_ops mxc_imx8_gasket_ops = {
> +	.enable = mxc_imx8_gasket_enable,
> +	.disable = mxc_imx8_gasket_disable,
> +};
> +

This can be moved to imx8-isi-gasket.c. You will need to add an

extern const struct mxc_gasket_ops mxc_imx8_gasket_ops;

to imx8-isi-core.h, and remove the declarations of the
mxc_imx8_gasket_enable() and mxc_imx8_gasket_disable() disable from the
header (and make those functions static).

>  static const struct mxc_isi_plat_data mxc_imx8mn_data = {
>  	.model			= MXC_ISI_IMX8MN,
>  	.num_ports		= 1,
> @@ -289,7 +295,7 @@ static const struct mxc_isi_plat_data mxc_imx8mn_data = {
>  	.clks			= mxc_imx8mn_clks,
>  	.num_clks		= ARRAY_SIZE(mxc_imx8mn_clks),
>  	.buf_active_reverse	= false,
> -	.has_gasket		= true,
> +	.gasket_ops		= &mxc_imx8_gasket_ops,
>  	.has_36bit_dma		= false,
>  };
>  
> @@ -303,7 +309,7 @@ static const struct mxc_isi_plat_data mxc_imx8mp_data = {
>  	.clks			= mxc_imx8mn_clks,
>  	.num_clks		= ARRAY_SIZE(mxc_imx8mn_clks),
>  	.buf_active_reverse	= true,
> -	.has_gasket		= true,
> +	.gasket_ops		= &mxc_imx8_gasket_ops,
>  	.has_36bit_dma		= true,
>  };
>  
> @@ -443,7 +449,7 @@ static int mxc_isi_probe(struct platform_device *pdev)
>  		return PTR_ERR(isi->regs);
>  	}
>  
> -	if (isi->pdata->has_gasket) {
> +	if (isi->pdata->gasket_ops) {
>  		isi->gasket = syscon_regmap_lookup_by_phandle(dev->of_node,
>  							      "fsl,blk-ctrl");
>  		if (IS_ERR(isi->gasket)) {
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> index e469788a9e6c..4f920d650153 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> @@ -12,12 +12,14 @@
>  
>  #include <linux/list.h>
>  #include <linux/mutex.h>
> +#include <linux/regmap.h>

This isn't needed here.

>  #include <linux/spinlock.h>
>  #include <linux/types.h>
>  #include <linux/videodev2.h>
>  
>  #include <media/media-device.h>
>  #include <media/media-entity.h>
> +#include <media/mipi-csi2.h>

Not needed either.

>  #include <media/v4l2-async.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-dev.h>
> @@ -59,6 +61,18 @@ struct v4l2_m2m_dev;
>  #define MXC_ISI_M2M			"mxc-isi-m2m"
>  #define MXC_MAX_PLANES			3
>  
> +/* GASKET (i.MX8MN and i.MX8MP only) */
> +#define GASKET_BASE(n)				(0x0060 + (n) * 0x30)
> +
> +#define GASKET_CTRL				0x0000
> +#define GASKET_CTRL_DATA_TYPE(dt)		((dt) << 8)
> +#define GASKET_CTRL_DATA_TYPE_MASK		(0x3f << 8)
> +#define GASKET_CTRL_DUAL_COMP_ENABLE		BIT(1)
> +#define GASKET_CTRL_ENABLE			BIT(0)
> +
> +#define GASKET_HSIZE				0x0004
> +#define GASKET_VSIZE				0x0008
> +

Those macros can be moved to imx8-isi-gasket.c.

>  struct mxc_isi_dev;
>  struct mxc_isi_m2m_ctx;
>  
> @@ -147,6 +161,14 @@ struct mxc_isi_set_thd {
>  	struct mxc_isi_panic_thd panic_set_thd_v;
>  };
>  
> +struct mxc_gasket_ops {
> +	int (*enable)(struct mxc_isi_dev *isi,
> +		      const struct v4l2_mbus_frame_desc *fd,
> +		      const struct v4l2_mbus_framefmt *fmt,
> +		      const unsigned int port);
> +	void (*disable)(struct mxc_isi_dev *isi, const unsigned int port);
> +};
> +
>  enum model {
>  	MXC_ISI_IMX8MN,
>  	MXC_ISI_IMX8MP,
> @@ -159,10 +181,10 @@ struct mxc_isi_plat_data {
>  	unsigned int reg_offset;
>  	const struct mxc_isi_ier_reg  *ier_reg;
>  	const struct mxc_isi_set_thd *set_thd;
> +	const struct mxc_gasket_ops *gasket_ops;
>  	const struct clk_bulk_data *clks;
>  	unsigned int num_clks;
>  	bool buf_active_reverse;
> -	bool has_gasket;
>  	bool has_36bit_dma;
>  };
>  
> @@ -379,6 +401,12 @@ void mxc_isi_channel_set_outbuf(struct mxc_isi_pipe *pipe,
>  u32 mxc_isi_channel_irq_status(struct mxc_isi_pipe *pipe, bool clear);
>  void mxc_isi_channel_irq_clear(struct mxc_isi_pipe *pipe);
>  
> +int mxc_imx8_gasket_enable(struct mxc_isi_dev *isi,
> +			   const struct v4l2_mbus_frame_desc *fd,
> +			   const struct v4l2_mbus_framefmt *fmt,
> +			   const unsigned int port);
> +void mxc_imx8_gasket_disable(struct mxc_isi_dev *isi, const unsigned int port);
> +
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>  void mxc_isi_debug_init(struct mxc_isi_dev *isi);
>  void mxc_isi_debug_cleanup(struct mxc_isi_dev *isi);
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> index f7447b2f4d77..d803fda3fdaf 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> @@ -25,32 +25,18 @@ static inline struct mxc_isi_crossbar *to_isi_crossbar(struct v4l2_subdev *sd)
>  	return container_of(sd, struct mxc_isi_crossbar, sd);
>  }
>  
> -/* -----------------------------------------------------------------------------
> - * Media block control (i.MX8MN and i.MX8MP only)
> - */
> -#define GASKET_BASE(n)				(0x0060 + (n) * 0x30)
> -
> -#define GASKET_CTRL				0x0000
> -#define GASKET_CTRL_DATA_TYPE(dt)		((dt) << 8)
> -#define GASKET_CTRL_DATA_TYPE_MASK		(0x3f << 8)
> -#define GASKET_CTRL_DUAL_COMP_ENABLE		BIT(1)
> -#define GASKET_CTRL_ENABLE			BIT(0)
> -
> -#define GASKET_HSIZE				0x0004
> -#define GASKET_VSIZE				0x0008
> -
>  static int mxc_isi_crossbar_gasket_enable(struct mxc_isi_crossbar *xbar,
>  					  struct v4l2_subdev_state *state,
>  					  struct v4l2_subdev *remote_sd,
>  					  u32 remote_pad, unsigned int port)
>  {
>  	struct mxc_isi_dev *isi = xbar->isi;
> +	const struct mxc_gasket_ops *gasket_ops = isi->pdata->gasket_ops;
>  	const struct v4l2_mbus_framefmt *fmt;
>  	struct v4l2_mbus_frame_desc fd;
> -	u32 val;
>  	int ret;
>  
> -	if (!isi->pdata->has_gasket)
> +	if (!gasket_ops)
>  		return 0;
>  
>  	/*
> @@ -77,16 +63,14 @@ static int mxc_isi_crossbar_gasket_enable(struct mxc_isi_crossbar *xbar,
>  	if (!fmt)
>  		return -EINVAL;
>  
> -	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_HSIZE, fmt->width);
> -	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_VSIZE, fmt->height);
> -
> -	val = GASKET_CTRL_DATA_TYPE(fd.entry[0].bus.csi2.dt)
> -	    | GASKET_CTRL_ENABLE;
> -
> -	if (fd.entry[0].bus.csi2.dt == MIPI_CSI2_DT_YUV422_8B)
> -		val |= GASKET_CTRL_DUAL_COMP_ENABLE;
> -
> -	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, val);
> +	if (gasket_ops->enable) {

I would drop this check. Both gasket ops are mandatory, we don't need to
support situations where only enable or disable are provided.

> +		ret = gasket_ops->enable(isi, &fd, fmt, port);
> +		if (ret) {
> +			dev_err(isi->dev,
> +				"failed to enable gasket%d\n", port);
> +			return ret;
> +		}
> +	}
>  
>  	return 0;
>  }
> @@ -95,11 +79,13 @@ static void mxc_isi_crossbar_gasket_disable(struct mxc_isi_crossbar *xbar,
>  					    unsigned int port)
>  {
>  	struct mxc_isi_dev *isi = xbar->isi;
> +	const struct mxc_gasket_ops *gasket_ops = isi->pdata->gasket_ops;
>  
> -	if (!isi->pdata->has_gasket)
> +	if (!gasket_ops)
>  		return;
>  
> -	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, 0);
> +	if (gasket_ops->disable)

Same here, you can drop this check.

> +		gasket_ops->disable(isi, port);
>  }
>  
>  /* -----------------------------------------------------------------------------
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> new file mode 100644
> index 000000000000..39f8d0e8b15d
> --- /dev/null
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019-2023 NXP
> + */
> +

You'll need

#include <linux/regmap.h>
#include <media/mipi-csi2.h>

> +#include "imx8-isi-core.h"
> +
> +/* Configure and enable gasket for i.MX8MN and i.MX8P */
> +int mxc_imx8_gasket_enable(struct mxc_isi_dev *isi,
> +			   const struct v4l2_mbus_frame_desc *fd,
> +			   const struct v4l2_mbus_framefmt *fmt,
> +			   const unsigned int port)
> +{
> +	u32 val;
> +
> +	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_HSIZE, fmt->width);
> +	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_VSIZE, fmt->height);
> +
> +	val = GASKET_CTRL_DATA_TYPE(fd->entry[0].bus.csi2.dt);
> +	if (fd->entry[0].bus.csi2.dt == MIPI_CSI2_DT_YUV422_8B)
> +		val |= GASKET_CTRL_DUAL_COMP_ENABLE;
> +
> +	val |= GASKET_CTRL_ENABLE;
> +	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, val);
> +	return 0;
> +}
> +
> +/* Disable gasket for i.MX8MN and i.MX8P */
> +void mxc_imx8_gasket_disable(struct mxc_isi_dev *isi, const unsigned int port)
> +{
> +	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, 0);
> +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/3] media: nxp: imx8-isi: add ISI support for i.MX93

[Laurent Pinchart]

Hi Guoniu,

Thank you for the patch.

On Tue, Jun 27, 2023 at 02:20:17PM +0800, guoniu.zhou@oss.nxp.com wrote:
> From: "Guoniu.zhou" <guoniu.zhou@nxp.com>
> 
> i.MX93 use a different gasket which has different register definition
> compared with i.MX8. Hence implement the gasket callbacks in order to
> add ISI support for i.MX93.
> 
> Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com>

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

> ---
>  .../platform/nxp/imx8-isi/imx8-isi-core.c     | 20 ++++++++++++++++++
>  .../platform/nxp/imx8-isi/imx8-isi-core.h     | 12 +++++++++++
>  .../platform/nxp/imx8-isi/imx8-isi-gasket.c   | 21 +++++++++++++++++++
>  3 files changed, 53 insertions(+)
> 
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> index d645b2f6fa5a..24c40e4cfef5 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> @@ -313,6 +313,25 @@ static const struct mxc_isi_plat_data mxc_imx8mp_data = {
>  	.has_36bit_dma		= true,
>  };
>  
> +static const struct mxc_gasket_ops mxc_imx93_gasket_ops = {
> +	.enable = mxc_imx93_gasket_enable,
> +	.disable = mxc_imx93_gasket_disable,
> +};
> +
> +static const struct mxc_isi_plat_data mxc_imx93_data = {
> +	.model			= MXC_ISI_IMX93,
> +	.num_ports		= 1,
> +	.num_channels		= 1,
> +	.reg_offset		= 0,
> +	.ier_reg		= &mxc_imx8_isi_ier_v2,
> +	.set_thd		= &mxc_imx8_isi_thd_v1,
> +	.clks			= mxc_imx8mn_clks,
> +	.num_clks		= ARRAY_SIZE(mxc_imx8mn_clks),
> +	.buf_active_reverse	= true,
> +	.gasket_ops		= &mxc_imx93_gasket_ops,
> +	.has_36bit_dma		= false,
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * Power management
>   */
> @@ -524,6 +543,7 @@ static int mxc_isi_remove(struct platform_device *pdev)
>  static const struct of_device_id mxc_isi_of_match[] = {
>  	{ .compatible = "fsl,imx8mn-isi", .data = &mxc_imx8mn_data },
>  	{ .compatible = "fsl,imx8mp-isi", .data = &mxc_imx8mp_data },
> +	{ .compatible = "fsl,imx93-isi", .data = &mxc_imx93_data },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, mxc_isi_of_match);
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> index 4f920d650153..f5be5394981e 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> @@ -73,6 +73,11 @@ struct v4l2_m2m_dev;
>  #define GASKET_HSIZE				0x0004
>  #define GASKET_VSIZE				0x0008
>  
> +/* dispmix_GPR register (i.MX93 only) */
> +#define DISP_MIX_CAMERA_MUX                     0x30
> +#define DISP_MIX_CAMERA_MUX_DATA_TYPE(x)        (((x) & 0x3f) << 3)
> +#define DISP_MIX_CAMERA_MUX_GASKET_ENABLE       BIT(16)
> +
>  struct mxc_isi_dev;
>  struct mxc_isi_m2m_ctx;
>  
> @@ -172,6 +177,7 @@ struct mxc_gasket_ops {
>  enum model {
>  	MXC_ISI_IMX8MN,
>  	MXC_ISI_IMX8MP,
> +	MXC_ISI_IMX93,
>  };
>  
>  struct mxc_isi_plat_data {
> @@ -407,6 +413,12 @@ int mxc_imx8_gasket_enable(struct mxc_isi_dev *isi,
>  			   const unsigned int port);
>  void mxc_imx8_gasket_disable(struct mxc_isi_dev *isi, const unsigned int port);
>  
> +int mxc_imx93_gasket_enable(struct mxc_isi_dev *isi,
> +			    const struct v4l2_mbus_frame_desc *fd,
> +			    const struct v4l2_mbus_framefmt *fmt,
> +			    const unsigned int port);
> +void mxc_imx93_gasket_disable(struct mxc_isi_dev *isi, const unsigned int port);
> +
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>  void mxc_isi_debug_init(struct mxc_isi_dev *isi);
>  void mxc_isi_debug_cleanup(struct mxc_isi_dev *isi);
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> index 39f8d0e8b15d..a81c4249a26f 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> @@ -30,3 +30,24 @@ void mxc_imx8_gasket_disable(struct mxc_isi_dev *isi, const unsigned int port)
>  {
>  	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, 0);
>  }
> +
> +/* Configure and enable gasket for i.MX93 */
> +int mxc_imx93_gasket_enable(struct mxc_isi_dev *isi,
> +			    const struct v4l2_mbus_frame_desc *fd,
> +			    const struct v4l2_mbus_framefmt *fmt,
> +			    const unsigned int port)
> +{
> +	u32 val;
> +
> +	val = DISP_MIX_CAMERA_MUX_DATA_TYPE(fd->entry[0].bus.csi2.dt);
> +	val |= DISP_MIX_CAMERA_MUX_GASKET_ENABLE;
> +	regmap_write(isi->gasket, DISP_MIX_CAMERA_MUX, val);
> +
> +	return 0;
> +}
> +
> +void mxc_imx93_gasket_disable(struct mxc_isi_dev *isi,
> +			      unsigned int port)
> +{
> +	regmap_write(isi->gasket, DISP_MIX_CAMERA_MUX, 0);
> +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/3] media: nxp: imx8-isi: move i.MX8 gasket configuration to an ops structure

[Laurent Pinchart]

On Tue, Jun 27, 2023 at 05:09:13PM +0300, Laurent Pinchart wrote:
> Hi Guoniu,
> 
> Thank you for the patch.
> 
> On Tue, Jun 27, 2023 at 02:20:16PM +0800, guoniu.zhou@oss.nxp.com wrote:
> > From: "Guoniu.zhou" <guoniu.zhou@nxp.com>
> > 
> > Move i.MX8MN and i.MX8MP gasket configuration to an ops structure.
> 
> Commit messages should explain the reason behind the change, not just
> what the change does. The *why* is considered even more important than
> the *what*, as it's (usually) possible to understand what a patch does
> by reading it, while the reason is much more difficult to get from just
> looking at the code.
> 
> In this case, the commit message could be
> 
> "The i.MX93 includes an ISI instance compatible with the imx8-isi
> driver, but with a different gasket. To prepare for this, make the
> gasket configuration modular by moving the code to an ops structure."
> 
> > Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com>
> > ---
> >  drivers/media/platform/nxp/imx8-isi/Makefile  |  2 +-
> >  .../platform/nxp/imx8-isi/imx8-isi-core.c     | 12 ++++--
> >  .../platform/nxp/imx8-isi/imx8-isi-core.h     | 30 ++++++++++++-
> >  .../platform/nxp/imx8-isi/imx8-isi-crossbar.c | 42 +++++++------------
> >  .../platform/nxp/imx8-isi/imx8-isi-gasket.c   | 32 ++++++++++++++
> >  5 files changed, 85 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/imx8-isi/Makefile b/drivers/media/platform/nxp/imx8-isi/Makefile
> > index 9bff9297686d..4376e8e0c962 100644
> > --- a/drivers/media/platform/nxp/imx8-isi/Makefile
> > +++ b/drivers/media/platform/nxp/imx8-isi/Makefile
> > @@ -1,7 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  
> >  imx8-isi-y := imx8-isi-core.o imx8-isi-crossbar.o imx8-isi-hw.o \
> > -	imx8-isi-pipe.o imx8-isi-video.o
> > +	imx8-isi-pipe.o imx8-isi-video.o imx8-isi-gasket.o
> 
> Please use alphabetical order.
> 
> >  imx8-isi-$(CONFIG_DEBUG_FS) += imx8-isi-debug.o
> >  imx8-isi-$(CONFIG_VIDEO_IMX8_ISI_M2M) += imx8-isi-m2m.o
> >  
> > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> > index 253e77189b69..d645b2f6fa5a 100644
> > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> > @@ -279,6 +279,12 @@ static const struct clk_bulk_data mxc_imx8mn_clks[] = {
> >  	{ .id = "apb" },
> >  };
> >  
> > +/* Gasket operations for i.MX8MN and i.MX8MP */
> > +static const struct mxc_gasket_ops mxc_imx8_gasket_ops = {
> > +	.enable = mxc_imx8_gasket_enable,
> > +	.disable = mxc_imx8_gasket_disable,
> > +};
> > +
> 
> This can be moved to imx8-isi-gasket.c. You will need to add an
> 
> extern const struct mxc_gasket_ops mxc_imx8_gasket_ops;
> 
> to imx8-isi-core.h, and remove the declarations of the
> mxc_imx8_gasket_enable() and mxc_imx8_gasket_disable() disable from the
> header (and make those functions static).
> 
> >  static const struct mxc_isi_plat_data mxc_imx8mn_data = {
> >  	.model			= MXC_ISI_IMX8MN,
> >  	.num_ports		= 1,
> > @@ -289,7 +295,7 @@ static const struct mxc_isi_plat_data mxc_imx8mn_data = {
> >  	.clks			= mxc_imx8mn_clks,
> >  	.num_clks		= ARRAY_SIZE(mxc_imx8mn_clks),
> >  	.buf_active_reverse	= false,
> > -	.has_gasket		= true,
> > +	.gasket_ops		= &mxc_imx8_gasket_ops,
> >  	.has_36bit_dma		= false,
> >  };
> >  
> > @@ -303,7 +309,7 @@ static const struct mxc_isi_plat_data mxc_imx8mp_data = {
> >  	.clks			= mxc_imx8mn_clks,
> >  	.num_clks		= ARRAY_SIZE(mxc_imx8mn_clks),
> >  	.buf_active_reverse	= true,
> > -	.has_gasket		= true,
> > +	.gasket_ops		= &mxc_imx8_gasket_ops,
> >  	.has_36bit_dma		= true,
> >  };
> >  
> > @@ -443,7 +449,7 @@ static int mxc_isi_probe(struct platform_device *pdev)
> >  		return PTR_ERR(isi->regs);
> >  	}
> >  
> > -	if (isi->pdata->has_gasket) {
> > +	if (isi->pdata->gasket_ops) {
> >  		isi->gasket = syscon_regmap_lookup_by_phandle(dev->of_node,
> >  							      "fsl,blk-ctrl");
> >  		if (IS_ERR(isi->gasket)) {
> > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > index e469788a9e6c..4f920d650153 100644
> > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > @@ -12,12 +12,14 @@
> >  
> >  #include <linux/list.h>
> >  #include <linux/mutex.h>
> > +#include <linux/regmap.h>
> 
> This isn't needed here.
> 
> >  #include <linux/spinlock.h>
> >  #include <linux/types.h>
> >  #include <linux/videodev2.h>
> >  
> >  #include <media/media-device.h>
> >  #include <media/media-entity.h>
> > +#include <media/mipi-csi2.h>
> 
> Not needed either.
> 
> >  #include <media/v4l2-async.h>
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-dev.h>
> > @@ -59,6 +61,18 @@ struct v4l2_m2m_dev;
> >  #define MXC_ISI_M2M			"mxc-isi-m2m"
> >  #define MXC_MAX_PLANES			3
> >  
> > +/* GASKET (i.MX8MN and i.MX8MP only) */
> > +#define GASKET_BASE(n)				(0x0060 + (n) * 0x30)
> > +
> > +#define GASKET_CTRL				0x0000
> > +#define GASKET_CTRL_DATA_TYPE(dt)		((dt) << 8)
> > +#define GASKET_CTRL_DATA_TYPE_MASK		(0x3f << 8)
> > +#define GASKET_CTRL_DUAL_COMP_ENABLE		BIT(1)
> > +#define GASKET_CTRL_ENABLE			BIT(0)
> > +
> > +#define GASKET_HSIZE				0x0004
> > +#define GASKET_VSIZE				0x0008
> > +
> 
> Those macros can be moved to imx8-isi-gasket.c.
> 
> >  struct mxc_isi_dev;
> >  struct mxc_isi_m2m_ctx;
> >  
> > @@ -147,6 +161,14 @@ struct mxc_isi_set_thd {
> >  	struct mxc_isi_panic_thd panic_set_thd_v;
> >  };
> >  
> > +struct mxc_gasket_ops {
> > +	int (*enable)(struct mxc_isi_dev *isi,
> > +		      const struct v4l2_mbus_frame_desc *fd,
> > +		      const struct v4l2_mbus_framefmt *fmt,
> > +		      const unsigned int port);
> > +	void (*disable)(struct mxc_isi_dev *isi, const unsigned int port);
> > +};
> > +
> >  enum model {
> >  	MXC_ISI_IMX8MN,
> >  	MXC_ISI_IMX8MP,
> > @@ -159,10 +181,10 @@ struct mxc_isi_plat_data {
> >  	unsigned int reg_offset;
> >  	const struct mxc_isi_ier_reg  *ier_reg;
> >  	const struct mxc_isi_set_thd *set_thd;
> > +	const struct mxc_gasket_ops *gasket_ops;
> >  	const struct clk_bulk_data *clks;
> >  	unsigned int num_clks;
> >  	bool buf_active_reverse;
> > -	bool has_gasket;
> >  	bool has_36bit_dma;
> >  };
> >  
> > @@ -379,6 +401,12 @@ void mxc_isi_channel_set_outbuf(struct mxc_isi_pipe *pipe,
> >  u32 mxc_isi_channel_irq_status(struct mxc_isi_pipe *pipe, bool clear);
> >  void mxc_isi_channel_irq_clear(struct mxc_isi_pipe *pipe);
> >  
> > +int mxc_imx8_gasket_enable(struct mxc_isi_dev *isi,
> > +			   const struct v4l2_mbus_frame_desc *fd,
> > +			   const struct v4l2_mbus_framefmt *fmt,
> > +			   const unsigned int port);
> > +void mxc_imx8_gasket_disable(struct mxc_isi_dev *isi, const unsigned int port);
> > +
> >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> >  void mxc_isi_debug_init(struct mxc_isi_dev *isi);
> >  void mxc_isi_debug_cleanup(struct mxc_isi_dev *isi);
> > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > index f7447b2f4d77..d803fda3fdaf 100644
> > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > @@ -25,32 +25,18 @@ static inline struct mxc_isi_crossbar *to_isi_crossbar(struct v4l2_subdev *sd)
> >  	return container_of(sd, struct mxc_isi_crossbar, sd);
> >  }
> >  
> > -/* -----------------------------------------------------------------------------
> > - * Media block control (i.MX8MN and i.MX8MP only)
> > - */
> > -#define GASKET_BASE(n)				(0x0060 + (n) * 0x30)
> > -
> > -#define GASKET_CTRL				0x0000
> > -#define GASKET_CTRL_DATA_TYPE(dt)		((dt) << 8)
> > -#define GASKET_CTRL_DATA_TYPE_MASK		(0x3f << 8)
> > -#define GASKET_CTRL_DUAL_COMP_ENABLE		BIT(1)
> > -#define GASKET_CTRL_ENABLE			BIT(0)
> > -
> > -#define GASKET_HSIZE				0x0004
> > -#define GASKET_VSIZE				0x0008
> > -
> >  static int mxc_isi_crossbar_gasket_enable(struct mxc_isi_crossbar *xbar,
> >  					  struct v4l2_subdev_state *state,
> >  					  struct v4l2_subdev *remote_sd,
> >  					  u32 remote_pad, unsigned int port)
> >  {
> >  	struct mxc_isi_dev *isi = xbar->isi;
> > +	const struct mxc_gasket_ops *gasket_ops = isi->pdata->gasket_ops;
> >  	const struct v4l2_mbus_framefmt *fmt;
> >  	struct v4l2_mbus_frame_desc fd;
> > -	u32 val;
> >  	int ret;
> >  
> > -	if (!isi->pdata->has_gasket)
> > +	if (!gasket_ops)
> >  		return 0;
> >  
> >  	/*
> > @@ -77,16 +63,14 @@ static int mxc_isi_crossbar_gasket_enable(struct mxc_isi_crossbar *xbar,
> >  	if (!fmt)
> >  		return -EINVAL;
> >  
> > -	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_HSIZE, fmt->width);
> > -	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_VSIZE, fmt->height);
> > -
> > -	val = GASKET_CTRL_DATA_TYPE(fd.entry[0].bus.csi2.dt)
> > -	    | GASKET_CTRL_ENABLE;
> > -
> > -	if (fd.entry[0].bus.csi2.dt == MIPI_CSI2_DT_YUV422_8B)
> > -		val |= GASKET_CTRL_DUAL_COMP_ENABLE;
> > -
> > -	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, val);
> > +	if (gasket_ops->enable) {
> 
> I would drop this check. Both gasket ops are mandatory, we don't need to
> support situations where only enable or disable are provided.
> 
> > +		ret = gasket_ops->enable(isi, &fd, fmt, port);
> > +		if (ret) {
> > +			dev_err(isi->dev,
> > +				"failed to enable gasket%d\n", port);
> > +			return ret;
> > +		}

Another comment, the enable operation never fails, so you can make it a
void function and drop the error check.

> > +	}
> >  
> >  	return 0;
> >  }
> > @@ -95,11 +79,13 @@ static void mxc_isi_crossbar_gasket_disable(struct mxc_isi_crossbar *xbar,
> >  					    unsigned int port)
> >  {
> >  	struct mxc_isi_dev *isi = xbar->isi;
> > +	const struct mxc_gasket_ops *gasket_ops = isi->pdata->gasket_ops;
> >  
> > -	if (!isi->pdata->has_gasket)
> > +	if (!gasket_ops)
> >  		return;
> >  
> > -	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, 0);
> > +	if (gasket_ops->disable)
> 
> Same here, you can drop this check.
> 
> > +		gasket_ops->disable(isi, port);
> >  }
> >  
> >  /* -----------------------------------------------------------------------------
> > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> > new file mode 100644
> > index 000000000000..39f8d0e8b15d
> > --- /dev/null
> > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> > @@ -0,0 +1,32 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2019-2023 NXP
> > + */
> > +
> 
> You'll need
> 
> #include <linux/regmap.h>
> #include <media/mipi-csi2.h>
> 
> > +#include "imx8-isi-core.h"
> > +
> > +/* Configure and enable gasket for i.MX8MN and i.MX8P */
> > +int mxc_imx8_gasket_enable(struct mxc_isi_dev *isi,
> > +			   const struct v4l2_mbus_frame_desc *fd,
> > +			   const struct v4l2_mbus_framefmt *fmt,
> > +			   const unsigned int port)
> > +{
> > +	u32 val;
> > +
> > +	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_HSIZE, fmt->width);
> > +	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_VSIZE, fmt->height);
> > +
> > +	val = GASKET_CTRL_DATA_TYPE(fd->entry[0].bus.csi2.dt);
> > +	if (fd->entry[0].bus.csi2.dt == MIPI_CSI2_DT_YUV422_8B)
> > +		val |= GASKET_CTRL_DUAL_COMP_ENABLE;
> > +
> > +	val |= GASKET_CTRL_ENABLE;
> > +	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, val);
> > +	return 0;
> > +}
> > +
> > +/* Disable gasket for i.MX8MN and i.MX8P */
> > +void mxc_imx8_gasket_disable(struct mxc_isi_dev *isi, const unsigned int port)
> > +{
> > +	regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, 0);
> > +}

-- 
Regards,

Laurent Pinchart

RE: [PATCH v3 2/3] media: nxp: imx8-isi: move i.MX8 gasket configuration to an ops structure

[G.N. Zhou (OSS)]

Hi Laurent,

Many thanks for your valuable comments(suggestions), will update in next version.

Best Regards
G.N Zhou


> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 2023年6月27日 22:14
> To: G.N. Zhou (OSS) <guoniu.zhou@oss.nxp.com>
> Cc: linux-media@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> devicetree@vger.kernel.org; mchehab@kernel.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; Xavier Roumegue
> (OSS) <xavier.roumegue@oss.nxp.com>; kernel@pengutronix.de;
> jacopo.mondi@ideasonboard.com; sakari.ailus@linux.intel.com
> Subject: Re: [PATCH v3 2/3] media: nxp: imx8-isi: move i.MX8 gasket
> configuration to an ops structure
> 
> Caution: This is an external email. Please take care when clicking links or opening
> attachments. When in doubt, report the message using the 'Report this email'
> button
> 
> 
> On Tue, Jun 27, 2023 at 05:09:13PM +0300, Laurent Pinchart wrote:
> > Hi Guoniu,
> >
> > Thank you for the patch.
> >
> > On Tue, Jun 27, 2023 at 02:20:16PM +0800, guoniu.zhou@oss.nxp.com
> wrote:
> > > From: "Guoniu.zhou" <guoniu.zhou@nxp.com>
> > >
> > > Move i.MX8MN and i.MX8MP gasket configuration to an ops structure.
> >
> > Commit messages should explain the reason behind the change, not just
> > what the change does. The *why* is considered even more important than
> > the *what*, as it's (usually) possible to understand what a patch does
> > by reading it, while the reason is much more difficult to get from
> > just looking at the code.
> >
> > In this case, the commit message could be
> >
> > "The i.MX93 includes an ISI instance compatible with the imx8-isi
> > driver, but with a different gasket. To prepare for this, make the
> > gasket configuration modular by moving the code to an ops structure."
> >
> > > Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com>
> > > ---
> > >  drivers/media/platform/nxp/imx8-isi/Makefile  |  2 +-
> > >  .../platform/nxp/imx8-isi/imx8-isi-core.c     | 12 ++++--
> > >  .../platform/nxp/imx8-isi/imx8-isi-core.h     | 30 ++++++++++++-
> > >  .../platform/nxp/imx8-isi/imx8-isi-crossbar.c | 42 +++++++------------
> > >  .../platform/nxp/imx8-isi/imx8-isi-gasket.c   | 32 ++++++++++++++
> > >  5 files changed, 85 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/nxp/imx8-isi/Makefile
> > > b/drivers/media/platform/nxp/imx8-isi/Makefile
> > > index 9bff9297686d..4376e8e0c962 100644
> > > --- a/drivers/media/platform/nxp/imx8-isi/Makefile
> > > +++ b/drivers/media/platform/nxp/imx8-isi/Makefile
> > > @@ -1,7 +1,7 @@
> > >  # SPDX-License-Identifier: GPL-2.0-only
> > >
> > >  imx8-isi-y := imx8-isi-core.o imx8-isi-crossbar.o imx8-isi-hw.o \
> > > -   imx8-isi-pipe.o imx8-isi-video.o
> > > +   imx8-isi-pipe.o imx8-isi-video.o imx8-isi-gasket.o
> >
> > Please use alphabetical order.
> >
> > >  imx8-isi-$(CONFIG_DEBUG_FS) += imx8-isi-debug.o
> > >  imx8-isi-$(CONFIG_VIDEO_IMX8_ISI_M2M) += imx8-isi-m2m.o
> > >
> > > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> > > b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> > > index 253e77189b69..d645b2f6fa5a 100644
> > > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> > > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> > > @@ -279,6 +279,12 @@ static const struct clk_bulk_data
> mxc_imx8mn_clks[] = {
> > >     { .id = "apb" },
> > >  };
> > >
> > > +/* Gasket operations for i.MX8MN and i.MX8MP */ static const struct
> > > +mxc_gasket_ops mxc_imx8_gasket_ops = {
> > > +   .enable = mxc_imx8_gasket_enable,
> > > +   .disable = mxc_imx8_gasket_disable, };
> > > +
> >
> > This can be moved to imx8-isi-gasket.c. You will need to add an
> >
> > extern const struct mxc_gasket_ops mxc_imx8_gasket_ops;
> >
> > to imx8-isi-core.h, and remove the declarations of the
> > mxc_imx8_gasket_enable() and mxc_imx8_gasket_disable() disable from
> > the header (and make those functions static).
> >
> > >  static const struct mxc_isi_plat_data mxc_imx8mn_data = {
> > >     .model                  = MXC_ISI_IMX8MN,
> > >     .num_ports              = 1,
> > > @@ -289,7 +295,7 @@ static const struct mxc_isi_plat_data
> mxc_imx8mn_data = {
> > >     .clks                   = mxc_imx8mn_clks,
> > >     .num_clks               = ARRAY_SIZE(mxc_imx8mn_clks),
> > >     .buf_active_reverse     = false,
> > > -   .has_gasket             = true,
> > > +   .gasket_ops             = &mxc_imx8_gasket_ops,
> > >     .has_36bit_dma          = false,
> > >  };
> > >
> > > @@ -303,7 +309,7 @@ static const struct mxc_isi_plat_data
> mxc_imx8mp_data = {
> > >     .clks                   = mxc_imx8mn_clks,
> > >     .num_clks               = ARRAY_SIZE(mxc_imx8mn_clks),
> > >     .buf_active_reverse     = true,
> > > -   .has_gasket             = true,
> > > +   .gasket_ops             = &mxc_imx8_gasket_ops,
> > >     .has_36bit_dma          = true,
> > >  };
> > >
> > > @@ -443,7 +449,7 @@ static int mxc_isi_probe(struct platform_device
> *pdev)
> > >             return PTR_ERR(isi->regs);
> > >     }
> > >
> > > -   if (isi->pdata->has_gasket) {
> > > +   if (isi->pdata->gasket_ops) {
> > >             isi->gasket =
> syscon_regmap_lookup_by_phandle(dev->of_node,
> > >
> "fsl,blk-ctrl");
> > >             if (IS_ERR(isi->gasket)) { diff --git
> > > a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > > b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > > index e469788a9e6c..4f920d650153 100644
> > > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > > @@ -12,12 +12,14 @@
> > >
> > >  #include <linux/list.h>
> > >  #include <linux/mutex.h>
> > > +#include <linux/regmap.h>
> >
> > This isn't needed here.
> >
> > >  #include <linux/spinlock.h>
> > >  #include <linux/types.h>
> > >  #include <linux/videodev2.h>
> > >
> > >  #include <media/media-device.h>
> > >  #include <media/media-entity.h>
> > > +#include <media/mipi-csi2.h>
> >
> > Not needed either.
> >
> > >  #include <media/v4l2-async.h>
> > >  #include <media/v4l2-ctrls.h>
> > >  #include <media/v4l2-dev.h>
> > > @@ -59,6 +61,18 @@ struct v4l2_m2m_dev;
> > >  #define MXC_ISI_M2M                        "mxc-isi-m2m"
> > >  #define MXC_MAX_PLANES                     3
> > >
> > > +/* GASKET (i.MX8MN and i.MX8MP only) */
> > > +#define GASKET_BASE(n)                             (0x0060 + (n) *
> 0x30)
> > > +
> > > +#define GASKET_CTRL                                0x0000
> > > +#define GASKET_CTRL_DATA_TYPE(dt)          ((dt) << 8)
> > > +#define GASKET_CTRL_DATA_TYPE_MASK         (0x3f << 8)
> > > +#define GASKET_CTRL_DUAL_COMP_ENABLE               BIT(1)
> > > +#define GASKET_CTRL_ENABLE                 BIT(0)
> > > +
> > > +#define GASKET_HSIZE                               0x0004
> > > +#define GASKET_VSIZE                               0x0008
> > > +
> >
> > Those macros can be moved to imx8-isi-gasket.c.
> >
> > >  struct mxc_isi_dev;
> > >  struct mxc_isi_m2m_ctx;
> > >
> > > @@ -147,6 +161,14 @@ struct mxc_isi_set_thd {
> > >     struct mxc_isi_panic_thd panic_set_thd_v;  };
> > >
> > > +struct mxc_gasket_ops {
> > > +   int (*enable)(struct mxc_isi_dev *isi,
> > > +                 const struct v4l2_mbus_frame_desc *fd,
> > > +                 const struct v4l2_mbus_framefmt *fmt,
> > > +                 const unsigned int port);
> > > +   void (*disable)(struct mxc_isi_dev *isi, const unsigned int
> > > +port); };
> > > +
> > >  enum model {
> > >     MXC_ISI_IMX8MN,
> > >     MXC_ISI_IMX8MP,
> > > @@ -159,10 +181,10 @@ struct mxc_isi_plat_data {
> > >     unsigned int reg_offset;
> > >     const struct mxc_isi_ier_reg  *ier_reg;
> > >     const struct mxc_isi_set_thd *set_thd;
> > > +   const struct mxc_gasket_ops *gasket_ops;
> > >     const struct clk_bulk_data *clks;
> > >     unsigned int num_clks;
> > >     bool buf_active_reverse;
> > > -   bool has_gasket;
> > >     bool has_36bit_dma;
> > >  };
> > >
> > > @@ -379,6 +401,12 @@ void mxc_isi_channel_set_outbuf(struct
> > > mxc_isi_pipe *pipe,
> > >  u32 mxc_isi_channel_irq_status(struct mxc_isi_pipe *pipe, bool
> > > clear);  void mxc_isi_channel_irq_clear(struct mxc_isi_pipe *pipe);
> > >
> > > +int mxc_imx8_gasket_enable(struct mxc_isi_dev *isi,
> > > +                      const struct v4l2_mbus_frame_desc *fd,
> > > +                      const struct v4l2_mbus_framefmt *fmt,
> > > +                      const unsigned int port); void
> > > +mxc_imx8_gasket_disable(struct mxc_isi_dev *isi, const unsigned int
> > > +port);
> > > +
> > >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> > >  void mxc_isi_debug_init(struct mxc_isi_dev *isi);  void
> > > mxc_isi_debug_cleanup(struct mxc_isi_dev *isi); diff --git
> > > a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > > b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > > index f7447b2f4d77..d803fda3fdaf 100644
> > > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > > @@ -25,32 +25,18 @@ static inline struct mxc_isi_crossbar
> *to_isi_crossbar(struct v4l2_subdev *sd)
> > >     return container_of(sd, struct mxc_isi_crossbar, sd);  }
> > >
> > > -/*
> > > --------------------------------------------------------------------
> > > ---------
> > > - * Media block control (i.MX8MN and i.MX8MP only)
> > > - */
> > > -#define GASKET_BASE(n)                             (0x0060 + (n) *
> 0x30)
> > > -
> > > -#define GASKET_CTRL                                0x0000
> > > -#define GASKET_CTRL_DATA_TYPE(dt)          ((dt) << 8)
> > > -#define GASKET_CTRL_DATA_TYPE_MASK         (0x3f << 8)
> > > -#define GASKET_CTRL_DUAL_COMP_ENABLE               BIT(1)
> > > -#define GASKET_CTRL_ENABLE                 BIT(0)
> > > -
> > > -#define GASKET_HSIZE                               0x0004
> > > -#define GASKET_VSIZE                               0x0008
> > > -
> > >  static int mxc_isi_crossbar_gasket_enable(struct mxc_isi_crossbar *xbar,
> > >                                       struct v4l2_subdev_state
> *state,
> > >                                       struct v4l2_subdev
> *remote_sd,
> > >                                       u32 remote_pad, unsigned int
> > > port)  {
> > >     struct mxc_isi_dev *isi = xbar->isi;
> > > +   const struct mxc_gasket_ops *gasket_ops =
> > > + isi->pdata->gasket_ops;
> > >     const struct v4l2_mbus_framefmt *fmt;
> > >     struct v4l2_mbus_frame_desc fd;
> > > -   u32 val;
> > >     int ret;
> > >
> > > -   if (!isi->pdata->has_gasket)
> > > +   if (!gasket_ops)
> > >             return 0;
> > >
> > >     /*
> > > @@ -77,16 +63,14 @@ static int mxc_isi_crossbar_gasket_enable(struct
> mxc_isi_crossbar *xbar,
> > >     if (!fmt)
> > >             return -EINVAL;
> > >
> > > -   regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_HSIZE,
> fmt->width);
> > > -   regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_VSIZE,
> fmt->height);
> > > -
> > > -   val = GASKET_CTRL_DATA_TYPE(fd.entry[0].bus.csi2.dt)
> > > -       | GASKET_CTRL_ENABLE;
> > > -
> > > -   if (fd.entry[0].bus.csi2.dt == MIPI_CSI2_DT_YUV422_8B)
> > > -           val |= GASKET_CTRL_DUAL_COMP_ENABLE;
> > > -
> > > -   regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, val);
> > > +   if (gasket_ops->enable) {
> >
> > I would drop this check. Both gasket ops are mandatory, we don't need
> > to support situations where only enable or disable are provided.
> >
> > > +           ret = gasket_ops->enable(isi, &fd, fmt, port);
> > > +           if (ret) {
> > > +                   dev_err(isi->dev,
> > > +                           "failed to enable gasket%d\n", port);
> > > +                   return ret;
> > > +           }
> 
> Another comment, the enable operation never fails, so you can make it a void
> function and drop the error check.
> 
> > > +   }
> > >
> > >     return 0;
> > >  }
> > > @@ -95,11 +79,13 @@ static void mxc_isi_crossbar_gasket_disable(struct
> mxc_isi_crossbar *xbar,
> > >                                         unsigned int port)  {
> > >     struct mxc_isi_dev *isi = xbar->isi;
> > > +   const struct mxc_gasket_ops *gasket_ops =
> > > + isi->pdata->gasket_ops;
> > >
> > > -   if (!isi->pdata->has_gasket)
> > > +   if (!gasket_ops)
> > >             return;
> > >
> > > -   regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, 0);
> > > +   if (gasket_ops->disable)
> >
> > Same here, you can drop this check.
> >
> > > +           gasket_ops->disable(isi, port);
> > >  }
> > >
> > >  /* -----------------------------------------------------------------------------
> > > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> > > new file mode 100644
> > > index 000000000000..39f8d0e8b15d
> > > --- /dev/null
> > > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> > > @@ -0,0 +1,32 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright 2019-2023 NXP
> > > + */
> > > +
> >
> > You'll need
> >
> > #include <linux/regmap.h>
> > #include <media/mipi-csi2.h>
> >
> > > +#include "imx8-isi-core.h"
> > > +
> > > +/* Configure and enable gasket for i.MX8MN and i.MX8P */
> > > +int mxc_imx8_gasket_enable(struct mxc_isi_dev *isi,
> > > +                      const struct v4l2_mbus_frame_desc *fd,
> > > +                      const struct v4l2_mbus_framefmt *fmt,
> > > +                      const unsigned int port)
> > > +{
> > > +   u32 val;
> > > +
> > > +   regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_HSIZE,
> fmt->width);
> > > +   regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_VSIZE,
> fmt->height);
> > > +
> > > +   val = GASKET_CTRL_DATA_TYPE(fd->entry[0].bus.csi2.dt);
> > > +   if (fd->entry[0].bus.csi2.dt == MIPI_CSI2_DT_YUV422_8B)
> > > +           val |= GASKET_CTRL_DUAL_COMP_ENABLE;
> > > +
> > > +   val |= GASKET_CTRL_ENABLE;
> > > +   regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, val);
> > > +   return 0;
> > > +}
> > > +
> > > +/* Disable gasket for i.MX8MN and i.MX8P */
> > > +void mxc_imx8_gasket_disable(struct mxc_isi_dev *isi, const unsigned int
> port)
> > > +{
> > > +   regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, 0);
> > > +}
> 
> --
> Regards,
> 
> Laurent Pinchart