[PATCH 0/8] media: imx: Destage imx7-mipi-csis with fixes on top

[PATCH 0/8] media: imx: Destage imx7-mipi-csis with fixes on top

[Jacopo Mondi]

Hello,
  this series includes patches from two series previously sent
https://lore.kernel.org/linux-media/20220119112024.11339-1-jacopo@jmondi.org/
https://lore.kernel.org/linux-media/20220211180216.290133-1-jacopo@jmondi.org/

Which can now be marked as superseded.

The first 2 patches performs the de-staging of the imx7-mipi-csis driver and
takes into account comments recevied there.

The rest of the series builds on top of the comment received on:
https://lore.kernel.org/linux-media/20220119112024.11339-3-jacopo@jmondi.org/

If DUAL pixel mode is used in the CSIS driver, then the CSI block of the IMX8MM
SoC needs to be operated in dual mode as well. To do so, create per-SoC
configurations in imx7-media-csi.c and only set dual mode for the MM model
leaving the other ones untouched as they connect to a different CSI-2 receiver
which instead operates in single mode.

I've only tested on i.MX8MP which is not affected by these changes, so I
hope I've not broke anything. Laurent could you test on MM to see if it works
now ?

On top two small patches I was carrying in my tree to add more formats to the
CSIS driver.

Series based on top of the most recent media master branch.

Thanks
  j

Jacopo Mondi (9):
  media: imx: De-stage imx7-mipi-csis
  media: imx: Rename imx7-mipi-csis.c to imx-mipi-csis.c
  staging: media: imx: Add more compatible strings
  staging: media: imx: Define per-SoC info
  staging: media: imx: Use DUAL pixel mode if available
  media: imx: imx-mipi-csis: Set PIXEL_MODE for YUV422
  media: imx: imx-mipi-csis: Add RGB565_1X16
  media: imx: imx-mipi-csis: Add RGB/BGR888

 Documentation/admin-guide/media/imx7.rst      |  2 +-
 ...-mipi-csi2.yaml => nxp,imx-mipi-csi2.yaml} |  2 +-
 .../bindings/media/nxp,imx7-csi.yaml          |  1 +
 MAINTAINERS                                   |  4 +-
 drivers/media/platform/Kconfig                |  1 +
 drivers/media/platform/Makefile               |  1 +
 drivers/media/platform/imx/Kconfig            | 24 +++++++++
 drivers/media/platform/imx/Makefile           |  1 +
 .../platform/imx/imx-mipi-csis.c}             | 44 ++++++++++++++--
 drivers/staging/media/imx/Makefile            |  1 -
 drivers/staging/media/imx/imx-media.h         | 44 ++++++++++++++++
 drivers/staging/media/imx/imx7-media-csi.c    | 52 ++++++++++++++-----
 12 files changed, 153 insertions(+), 24 deletions(-)
 rename Documentation/devicetree/bindings/media/{nxp,imx7-mipi-csi2.yaml => nxp,imx-mipi-csi2.yaml} (98%)
 create mode 100644 drivers/media/platform/imx/Kconfig
 create mode 100644 drivers/media/platform/imx/Makefile
 rename drivers/{staging/media/imx/imx7-mipi-csis.c => media/platform/imx/imx-mipi-csis.c} (97%)

--
2.35.0

[PATCH 1/8] media: imx: De-stage imx7-mipi-csis

[Jacopo Mondi]

The imx7-mipi-csis driver is in a good state and can be destaged.

Move the imx7-mipi-csis.c driver to the newly created
drivers/media/platform/imx directory and plumb the related
options in Kconfig and in Makefile.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 MAINTAINERS                                   |  2 +-
 drivers/media/platform/Kconfig                |  1 +
 drivers/media/platform/Makefile               |  1 +
 drivers/media/platform/imx/Kconfig            | 24 +++++++++++++++++++
 drivers/media/platform/imx/Makefile           |  1 +
 .../platform}/imx/imx7-mipi-csis.c            |  0
 drivers/staging/media/imx/Makefile            |  1 -
 7 files changed, 28 insertions(+), 2 deletions(-)
 create mode 100644 drivers/media/platform/imx/Kconfig
 create mode 100644 drivers/media/platform/imx/Makefile
 rename drivers/{staging/media => media/platform}/imx/imx7-mipi-csis.c (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 83d27b57016f..5bdb8c881b0b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11891,8 +11891,8 @@ T:	git git://linuxtv.org/media_tree.git
 F:	Documentation/admin-guide/media/imx7.rst
 F:	Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
 F:	Documentation/devicetree/bindings/media/nxp,imx7-mipi-csi2.yaml
+F:	drivers/media/platform/imx/imx7-mipi-csis.c
 F:	drivers/staging/media/imx/imx7-media-csi.c
-F:	drivers/staging/media/imx/imx7-mipi-csis.c
 
 MEDIA DRIVERS FOR HELENE
 M:	Abylay Ospan <aospan@netup.ru>
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 9fbdba0fd1e7..d9eeccffea69 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -171,6 +171,7 @@ source "drivers/media/platform/xilinx/Kconfig"
 source "drivers/media/platform/rcar-vin/Kconfig"
 source "drivers/media/platform/atmel/Kconfig"
 source "drivers/media/platform/sunxi/Kconfig"
+source "drivers/media/platform/imx/Kconfig"
 
 config VIDEO_TI_CAL
 	tristate "TI CAL (Camera Adaptation Layer) driver"
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 28eb4aadbf45..a9466c854610 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -20,6 +20,7 @@ obj-y	+= ti-vpe/
 obj-$(CONFIG_VIDEO_MX2_EMMAPRP)		+= mx2_emmaprp.o
 obj-$(CONFIG_VIDEO_CODA)		+= coda/
 
+obj-$(CONFIG_VIDEO_IMX)			+= imx/
 obj-$(CONFIG_VIDEO_IMX_PXP)		+= imx-pxp.o
 obj-$(CONFIG_VIDEO_IMX8_JPEG)		+= imx-jpeg/
 
diff --git a/drivers/media/platform/imx/Kconfig b/drivers/media/platform/imx/Kconfig
new file mode 100644
index 000000000000..683863572c20
--- /dev/null
+++ b/drivers/media/platform/imx/Kconfig
@@ -0,0 +1,24 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+menuconfig VIDEO_IMX
+	bool "V4L2 capture drivers for NXP i.MX devices"
+	depends on ARCH_MXC || COMPILE_TEST
+	depends on VIDEO_DEV && VIDEO_V4L2
+	help
+	  Say yes here to enable support for capture drivers on i.MX SoCs.
+	  Support for the single SoC features are selectable in the sub-menu
+	  options.
+
+if VIDEO_IMX
+
+config VIDEO_IMX_MIPI_CSIS
+	tristate "MIPI CSI-2 CSIS receiver found on i.MX7 and i.MX8 models"
+	select MEDIA_CONTROLLER
+	select V4L2_FWNODE
+	select VIDEO_V4L2_SUBDEV_API
+	default n
+	help
+	  Video4Linux2 sub-device driver for the MIPI CSI-2 CSIS receiver
+	  v3.3/v3.6.3 found on some i.MX7 and i.MX8 SoCs.
+
+endif # VIDEO_IMX
diff --git a/drivers/media/platform/imx/Makefile b/drivers/media/platform/imx/Makefile
new file mode 100644
index 000000000000..ee272234c8d7
--- /dev/null
+++ b/drivers/media/platform/imx/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VIDEO_IMX_MIPI_CSIS) += imx7-mipi-csis.o
diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/media/platform/imx/imx7-mipi-csis.c
similarity index 100%
rename from drivers/staging/media/imx/imx7-mipi-csis.c
rename to drivers/media/platform/imx/imx7-mipi-csis.c
diff --git a/drivers/staging/media/imx/Makefile b/drivers/staging/media/imx/Makefile
index 19c2fc54d424..d82be898145b 100644
--- a/drivers/staging/media/imx/Makefile
+++ b/drivers/staging/media/imx/Makefile
@@ -15,5 +15,4 @@ obj-$(CONFIG_VIDEO_IMX_CSI) += imx6-media-csi.o
 obj-$(CONFIG_VIDEO_IMX_CSI) += imx6-mipi-csi2.o
 
 obj-$(CONFIG_VIDEO_IMX7_CSI) += imx7-media-csi.o
-obj-$(CONFIG_VIDEO_IMX7_CSI) += imx7-mipi-csis.o
 obj-$(CONFIG_VIDEO_IMX7_CSI) += imx8mq-mipi-csi2.o
-- 
2.35.0

[PATCH 2/8] media: imx: Rename imx7-mipi-csis.c to imx-mipi-csis.c

[Jacopo Mondi]

Rename the imx7-mipi-csis.c driver to remove the reference to i.MX7.

The driver is for an IP core found on i.MX7 and i.MX8 SoC, so do not
specify a SoC version number in the driver name.

Remove the references to the i.MX7 SoC in the driver symbols and expand
the driver's header with more information about the IP core the driver
controls.

Also rename the associated bindings documentation.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 Documentation/admin-guide/media/imx7.rst               |  2 +-
 ...{nxp,imx7-mipi-csi2.yaml => nxp,imx-mipi-csi2.yaml} |  2 +-
 MAINTAINERS                                            |  4 ++--
 drivers/media/platform/imx/Makefile                    |  2 +-
 .../platform/imx/{imx7-mipi-csis.c => imx-mipi-csis.c} | 10 +++++++---
 5 files changed, 12 insertions(+), 8 deletions(-)
 rename Documentation/devicetree/bindings/media/{nxp,imx7-mipi-csi2.yaml => nxp,imx-mipi-csi2.yaml} (98%)
 rename drivers/media/platform/imx/{imx7-mipi-csis.c => imx-mipi-csis.c} (99%)

diff --git a/Documentation/admin-guide/media/imx7.rst b/Documentation/admin-guide/media/imx7.rst
index 4785ae8ac978..2fa27718f52a 100644
--- a/Documentation/admin-guide/media/imx7.rst
+++ b/Documentation/admin-guide/media/imx7.rst
@@ -33,7 +33,7 @@ reference manual [#f1]_.
 Entities
 --------
 
-imx7-mipi-csi2
+imx-mipi-csi2
 --------------
 
 This is the MIPI CSI-2 receiver entity. It has one sink pad to receive the pixel
diff --git a/Documentation/devicetree/bindings/media/nxp,imx7-mipi-csi2.yaml b/Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
similarity index 98%
rename from Documentation/devicetree/bindings/media/nxp,imx7-mipi-csi2.yaml
rename to Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
index e2e6e9aa0fe6..36b135bf9f2a 100644
--- a/Documentation/devicetree/bindings/media/nxp,imx7-mipi-csi2.yaml
+++ b/Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/media/nxp,imx7-mipi-csi2.yaml#
+$id: http://devicetree.org/schemas/media/nxp,imx-mipi-csi2.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: NXP i.MX7 and i.MX8 MIPI CSI-2 receiver
diff --git a/MAINTAINERS b/MAINTAINERS
index 5bdb8c881b0b..d919ea3ed250 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11889,9 +11889,9 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
 F:	Documentation/admin-guide/media/imx7.rst
+F:	Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
 F:	Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
-F:	Documentation/devicetree/bindings/media/nxp,imx7-mipi-csi2.yaml
-F:	drivers/media/platform/imx/imx7-mipi-csis.c
+F:	drivers/media/platform/imx/imx-mipi-csis.c
 F:	drivers/staging/media/imx/imx7-media-csi.c
 
 MEDIA DRIVERS FOR HELENE
diff --git a/drivers/media/platform/imx/Makefile b/drivers/media/platform/imx/Makefile
index ee272234c8d7..f72bdbe8e6ef 100644
--- a/drivers/media/platform/imx/Makefile
+++ b/drivers/media/platform/imx/Makefile
@@ -1 +1 @@
-obj-$(CONFIG_VIDEO_IMX_MIPI_CSIS) += imx7-mipi-csis.o
+obj-$(CONFIG_VIDEO_IMX_MIPI_CSIS) += imx-mipi-csis.o
diff --git a/drivers/media/platform/imx/imx7-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
similarity index 99%
rename from drivers/media/platform/imx/imx7-mipi-csis.c
rename to drivers/media/platform/imx/imx-mipi-csis.c
index a22d0e6b3d44..f433758c8935 100644
--- a/drivers/media/platform/imx/imx7-mipi-csis.c
+++ b/drivers/media/platform/imx/imx-mipi-csis.c
@@ -1,6 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Freescale i.MX7 SoC series MIPI-CSI V3.3 receiver driver
+ * Samsung CSIS MIPI CSI-2 receiver driver.
+ *
+ * The Samsung CSIS IP is a MIPI CSI-2 receiver found in various NXP i.MX7 and
+ * i.MX8 SoCs. The i.MX7 features version 3.3 of the IP, while i.MX8 features
+ * version 3.6.3
  *
  * Copyright (C) 2019 Linaro Ltd
  * Copyright (C) 2015-2016 Freescale Semiconductor, Inc. All Rights Reserved.
@@ -31,7 +35,7 @@
 #include <media/v4l2-mc.h>
 #include <media/v4l2-subdev.h>
 
-#define CSIS_DRIVER_NAME			"imx7-mipi-csis"
+#define CSIS_DRIVER_NAME			"imx-mipi-csis"
 
 #define CSIS_PAD_SINK				0
 #define CSIS_PAD_SOURCE				1
@@ -1515,4 +1519,4 @@ module_platform_driver(mipi_csis_driver);
 
 MODULE_DESCRIPTION("i.MX7 & i.MX8 MIPI CSI-2 receiver driver");
 MODULE_LICENSE("GPL v2");
-MODULE_ALIAS("platform:imx7-mipi-csi2");
+MODULE_ALIAS("platform:imx-mipi-csi2");
-- 
2.35.0

Re: [PATCH 2/8] media: imx: Rename imx7-mipi-csis.c to imx-mipi-csis.c

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Feb 14, 2022 at 07:43:12PM +0100, Jacopo Mondi wrote:
> Rename the imx7-mipi-csis.c driver to remove the reference to i.MX7.
> 
> The driver is for an IP core found on i.MX7 and i.MX8 SoC, so do not
> specify a SoC version number in the driver name.
> 
> Remove the references to the i.MX7 SoC in the driver symbols and expand
> the driver's header with more information about the IP core the driver
> controls.
> 
> Also rename the associated bindings documentation.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  Documentation/admin-guide/media/imx7.rst               |  2 +-
>  ...{nxp,imx7-mipi-csi2.yaml => nxp,imx-mipi-csi2.yaml} |  2 +-
>  MAINTAINERS                                            |  4 ++--
>  drivers/media/platform/imx/Makefile                    |  2 +-
>  .../platform/imx/{imx7-mipi-csis.c => imx-mipi-csis.c} | 10 +++++++---
>  5 files changed, 12 insertions(+), 8 deletions(-)
>  rename Documentation/devicetree/bindings/media/{nxp,imx7-mipi-csi2.yaml => nxp,imx-mipi-csi2.yaml} (98%)
>  rename drivers/media/platform/imx/{imx7-mipi-csis.c => imx-mipi-csis.c} (99%)
> 
> diff --git a/Documentation/admin-guide/media/imx7.rst b/Documentation/admin-guide/media/imx7.rst
> index 4785ae8ac978..2fa27718f52a 100644
> --- a/Documentation/admin-guide/media/imx7.rst
> +++ b/Documentation/admin-guide/media/imx7.rst
> @@ -33,7 +33,7 @@ reference manual [#f1]_.
>  Entities
>  --------
>  
> -imx7-mipi-csi2
> +imx-mipi-csi2
>  --------------
>  
>  This is the MIPI CSI-2 receiver entity. It has one sink pad to receive the pixel
> diff --git a/Documentation/devicetree/bindings/media/nxp,imx7-mipi-csi2.yaml b/Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
> similarity index 98%
> rename from Documentation/devicetree/bindings/media/nxp,imx7-mipi-csi2.yaml
> rename to Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
> index e2e6e9aa0fe6..36b135bf9f2a 100644
> --- a/Documentation/devicetree/bindings/media/nxp,imx7-mipi-csi2.yaml
> +++ b/Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>  %YAML 1.2
>  ---
> -$id: http://devicetree.org/schemas/media/nxp,imx7-mipi-csi2.yaml#
> +$id: http://devicetree.org/schemas/media/nxp,imx-mipi-csi2.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
>  title: NXP i.MX7 and i.MX8 MIPI CSI-2 receiver
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5bdb8c881b0b..d919ea3ed250 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11889,9 +11889,9 @@ L:	linux-media@vger.kernel.org
>  S:	Maintained
>  T:	git git://linuxtv.org/media_tree.git
>  F:	Documentation/admin-guide/media/imx7.rst
> +F:	Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
>  F:	Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> -F:	Documentation/devicetree/bindings/media/nxp,imx7-mipi-csi2.yaml
> -F:	drivers/media/platform/imx/imx7-mipi-csis.c
> +F:	drivers/media/platform/imx/imx-mipi-csis.c
>  F:	drivers/staging/media/imx/imx7-media-csi.c
>  
>  MEDIA DRIVERS FOR HELENE
> diff --git a/drivers/media/platform/imx/Makefile b/drivers/media/platform/imx/Makefile
> index ee272234c8d7..f72bdbe8e6ef 100644
> --- a/drivers/media/platform/imx/Makefile
> +++ b/drivers/media/platform/imx/Makefile
> @@ -1 +1 @@
> -obj-$(CONFIG_VIDEO_IMX_MIPI_CSIS) += imx7-mipi-csis.o
> +obj-$(CONFIG_VIDEO_IMX_MIPI_CSIS) += imx-mipi-csis.o
> diff --git a/drivers/media/platform/imx/imx7-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> similarity index 99%
> rename from drivers/media/platform/imx/imx7-mipi-csis.c
> rename to drivers/media/platform/imx/imx-mipi-csis.c
> index a22d0e6b3d44..f433758c8935 100644
> --- a/drivers/media/platform/imx/imx7-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -1,6 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * Freescale i.MX7 SoC series MIPI-CSI V3.3 receiver driver
> + * Samsung CSIS MIPI CSI-2 receiver driver.
> + *
> + * The Samsung CSIS IP is a MIPI CSI-2 receiver found in various NXP i.MX7 and
> + * i.MX8 SoCs. The i.MX7 features version 3.3 of the IP, while i.MX8 features
> + * version 3.6.3

s/$/./

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

>   *
>   * Copyright (C) 2019 Linaro Ltd
>   * Copyright (C) 2015-2016 Freescale Semiconductor, Inc. All Rights Reserved.
> @@ -31,7 +35,7 @@
>  #include <media/v4l2-mc.h>
>  #include <media/v4l2-subdev.h>
>  
> -#define CSIS_DRIVER_NAME			"imx7-mipi-csis"
> +#define CSIS_DRIVER_NAME			"imx-mipi-csis"
>  
>  #define CSIS_PAD_SINK				0
>  #define CSIS_PAD_SOURCE				1
> @@ -1515,4 +1519,4 @@ module_platform_driver(mipi_csis_driver);
>  
>  MODULE_DESCRIPTION("i.MX7 & i.MX8 MIPI CSI-2 receiver driver");
>  MODULE_LICENSE("GPL v2");
> -MODULE_ALIAS("platform:imx7-mipi-csi2");
> +MODULE_ALIAS("platform:imx-mipi-csi2");

-- 
Regards,

Laurent Pinchart

[PATCH 3/8] staging: media: imx: Add more compatible strings

[Jacopo Mondi]

The imx7-media-csi driver controls the CSI (CMOS Sensor Interface)
peripheral available on several SoC of different generations.

The current situation when it comes to compatible strings is rather
confused:
- Bindings document imx6ul, imx7 and imx8mm
- Driver supports imx6ul, imx7 and imx8mq
- The IMX8MM and IMX8MQ DTS use the correct compatible strings with a
  fallback to the generic "imx7-csi" identifier:
  arch/arm64/boot/dts/freescale/imx8mq.dtsi: compatible = "fsl,imx8mq-csi",
  arch/arm64/boot/dts/freescale/imx8mm.dtsi: compatible = "fsl,imx8mm-csi",

Tidy-up the situation by adding the IMX8MQ compatible string to the
bindings documentation andathe IMX8MM identifier to the driver, to allow
to distinguish the SoC the CSI peripheral is integrated on in the
following patches.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml | 1 +
 drivers/staging/media/imx/imx7-media-csi.c                | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
index 4f7b78265336..0f1505d85111 100644
--- a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
+++ b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
@@ -21,6 +21,7 @@ properties:
           - fsl,imx7-csi
           - fsl,imx6ul-csi
       - items:
+          - const: fsl,imx8mq-csi
           - const: fsl,imx8mm-csi
           - const: fsl,imx7-csi
 
diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index 32311fc0e2a4..59100e409709 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -162,6 +162,7 @@
 enum imx_csi_model {
 	IMX7_CSI_IMX7 = 0,
 	IMX7_CSI_IMX8MQ,
+	IMX7_CSI_IMX8MM,
 };
 
 struct imx7_csi {
@@ -1277,6 +1278,7 @@ static int imx7_csi_remove(struct platform_device *pdev)
 
 static const struct of_device_id imx7_csi_of_match[] = {
 	{ .compatible = "fsl,imx8mq-csi", .data = (void *)IMX7_CSI_IMX8MQ },
+	{ .compatible = "fsl,imx8mm-csi", .data = (void *)IMX7_CSI_IMX8MM },
 	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 },
 	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 },
 	{ },
-- 
2.35.0

Re: [PATCH 3/8] staging: media: imx: Add more compatible strings

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Feb 14, 2022 at 07:43:13PM +0100, Jacopo Mondi wrote:
> The imx7-media-csi driver controls the CSI (CMOS Sensor Interface)
> peripheral available on several SoC of different generations.
> 
> The current situation when it comes to compatible strings is rather
> confused:
> - Bindings document imx6ul, imx7 and imx8mm
> - Driver supports imx6ul, imx7 and imx8mq
> - The IMX8MM and IMX8MQ DTS use the correct compatible strings with a
>   fallback to the generic "imx7-csi" identifier:
>   arch/arm64/boot/dts/freescale/imx8mq.dtsi: compatible = "fsl,imx8mq-csi",
>   arch/arm64/boot/dts/freescale/imx8mm.dtsi: compatible = "fsl,imx8mm-csi",
> 
> Tidy-up the situation by adding the IMX8MQ compatible string to the
> bindings documentation andathe IMX8MM identifier to the driver, to allow
> to distinguish the SoC the CSI peripheral is integrated on in the
> following patches.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml | 1 +
>  drivers/staging/media/imx/imx7-media-csi.c                | 2 ++

I think Rob would prefer this being split in two patches, and I think it
would make sense, as you're fixing two separate issues.

>  2 files changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> index 4f7b78265336..0f1505d85111 100644
> --- a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> +++ b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> @@ -21,6 +21,7 @@ properties:
>            - fsl,imx7-csi
>            - fsl,imx6ul-csi
>        - items:
> +          - const: fsl,imx8mq-csi
>            - const: fsl,imx8mm-csi
>            - const: fsl,imx7-csi

I don't think you intended to require the following:

	compatible = "fsl,imx8mq-csi", "fsl,imx8mm-csi", "fsl,imx7-csi";

You probably want

 properties:
   compatible:
     oneOf:
       - enum:
+          - fsl,imx8mq-csi
           - fsl,imx7-csi
           - fsl,imx6ul-csi
       - items:
           - const: fsl,imx8mm-csi
           - const: fsl,imx7-csi

instead.

> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index 32311fc0e2a4..59100e409709 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -162,6 +162,7 @@
>  enum imx_csi_model {
>  	IMX7_CSI_IMX7 = 0,
>  	IMX7_CSI_IMX8MQ,
> +	IMX7_CSI_IMX8MM,
>  };
>  
>  struct imx7_csi {
> @@ -1277,6 +1278,7 @@ static int imx7_csi_remove(struct platform_device *pdev)
>  
>  static const struct of_device_id imx7_csi_of_match[] = {
>  	{ .compatible = "fsl,imx8mq-csi", .data = (void *)IMX7_CSI_IMX8MQ },
> +	{ .compatible = "fsl,imx8mm-csi", .data = (void *)IMX7_CSI_IMX8MM },

This isn't needed, as the i.MX8MM CSI bridgge is considered fully
backward-compatible with the i.MX7 version. I'd introduce this change in
the patch where you start using IMX7_CSI_IMX8MM, and I would then add
the following to the DT binding:

 properties:
   compatible:
     oneOf:
       - enum:
           - fsl,imx8mq-csi
+          - fsl,imx8mm-csi
           - fsl,imx7-csi
           - fsl,imx6ul-csi
       - items:
           - const: fsl,imx8mm-csi
           - const: fsl,imx7-csi

to allow setting

	compatible = "fsl,imx8mm-csi";

without the imx7 fallback if we consider the i.MX8MM version different.
If the driver can operate correctly on the i.MX8MM when using the i.MX7
fallback code paths (possibly minor issues that are not considered
fatal, such as missing features) then you could skip this binding
change.

>  	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 },
>  	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 },
>  	{ },

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/8] staging: media: imx: Add more compatible strings

[Jacopo Mondi]

Hi Laurent

On Mon, Feb 14, 2022 at 09:15:07PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Feb 14, 2022 at 07:43:13PM +0100, Jacopo Mondi wrote:
> > The imx7-media-csi driver controls the CSI (CMOS Sensor Interface)
> > peripheral available on several SoC of different generations.
> >
> > The current situation when it comes to compatible strings is rather
> > confused:
> > - Bindings document imx6ul, imx7 and imx8mm
> > - Driver supports imx6ul, imx7 and imx8mq
> > - The IMX8MM and IMX8MQ DTS use the correct compatible strings with a
> >   fallback to the generic "imx7-csi" identifier:
> >   arch/arm64/boot/dts/freescale/imx8mq.dtsi: compatible = "fsl,imx8mq-csi",
> >   arch/arm64/boot/dts/freescale/imx8mm.dtsi: compatible = "fsl,imx8mm-csi",
> >
> > Tidy-up the situation by adding the IMX8MQ compatible string to the
> > bindings documentation andathe IMX8MM identifier to the driver, to allow
> > to distinguish the SoC the CSI peripheral is integrated on in the
> > following patches.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml | 1 +
> >  drivers/staging/media/imx/imx7-media-csi.c                | 2 ++
>
> I think Rob would prefer this being split in two patches, and I think it
> would make sense, as you're fixing two separate issues.
>
> >  2 files changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> > index 4f7b78265336..0f1505d85111 100644
> > --- a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> > +++ b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> > @@ -21,6 +21,7 @@ properties:
> >            - fsl,imx7-csi
> >            - fsl,imx6ul-csi
> >        - items:
> > +          - const: fsl,imx8mq-csi
> >            - const: fsl,imx8mm-csi
> >            - const: fsl,imx7-csi
>
> I don't think you intended to require the following:
>
> 	compatible = "fsl,imx8mq-csi", "fsl,imx8mm-csi", "fsl,imx7-csi";

No, I kind of superficially added the mq version where the mm was
already and went on :)

Care to explain why currently we have two const for the "8mm" and the
"imx7" versions ?

>
> You probably want
>
>  properties:
>    compatible:
>      oneOf:
>        - enum:
> +          - fsl,imx8mq-csi
>            - fsl,imx7-csi
>            - fsl,imx6ul-csi
>        - items:
>            - const: fsl,imx8mm-csi
>            - const: fsl,imx7-csi
>
> instead.

I'm not aware of how how many revisions of the imx7 and imx6 versions
exists, nor how they differ, but the existing distinction feels a bit
weird.

The const items should be the compatible fallbacks, should them be
generic, why is 8mm among them ? Shouldn't we specify the precise SoC
version in the list of possible enum items only ?

Something like

      oneOf:
        - enum:
          - fsl,imx8mq-csi
          - fsl,imx8mm-csi
          - fsl,imx6ul-csi
        - const:
          - fsl,imx7-csi

In example I see:

arch/arm64/boot/dts/freescale/imx8mq.dtsi: compatible = "fsl,imx8mq-csi", "fsl,imx7-csi";

Where this should either be
                                           compatible = "fsl,imx8mq-csi"
or
                                           compatible = "fsl,imx8mm-csi", "fsl,imx7-csi";

?


>
> > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> > index 32311fc0e2a4..59100e409709 100644
> > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > @@ -162,6 +162,7 @@
> >  enum imx_csi_model {
> >  	IMX7_CSI_IMX7 = 0,
> >  	IMX7_CSI_IMX8MQ,
> > +	IMX7_CSI_IMX8MM,
> >  };
> >
> >  struct imx7_csi {
> > @@ -1277,6 +1278,7 @@ static int imx7_csi_remove(struct platform_device *pdev)
> >
> >  static const struct of_device_id imx7_csi_of_match[] = {
> >  	{ .compatible = "fsl,imx8mq-csi", .data = (void *)IMX7_CSI_IMX8MQ },
> > +	{ .compatible = "fsl,imx8mm-csi", .data = (void *)IMX7_CSI_IMX8MM },
>
> This isn't needed, as the i.MX8MM CSI bridgge is considered fully
> backward-compatible with the i.MX7 version. I'd introduce this change in
> the patch where you start using IMX7_CSI_IMX8MM, and I would then add
> the following to the DT binding:
>
>  properties:
>    compatible:
>      oneOf:
>        - enum:
>            - fsl,imx8mq-csi
> +          - fsl,imx8mm-csi
>            - fsl,imx7-csi
>            - fsl,imx6ul-csi
>        - items:
>            - const: fsl,imx8mm-csi
>            - const: fsl,imx7-csi
>
> to allow setting
>
> 	compatible = "fsl,imx8mm-csi";
>
> without the imx7 fallback if we consider the i.MX8MM version different.
> If the driver can operate correctly on the i.MX8MM when using the i.MX7
> fallback code paths (possibly minor issues that are not considered
> fatal, such as missing features) then you could skip this binding
> change.

Sorry, but shouldn't:

        compatible = "fsl,imx8mm-csi", fsl,imx7-csi"

allow me to match on imx8mm already, without the above change.

I think what I don't get is why imx8mm is a 'generic fallback' in
first place.

>
> >  	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 },
> >  	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 },
> >  	{ },
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 3/8] staging: media: imx: Add more compatible strings

[Laurent Pinchart]

Hi Jacopo,

On Tue, Feb 15, 2022 at 09:36:31AM +0100, Jacopo Mondi wrote:
> On Mon, Feb 14, 2022 at 09:15:07PM +0200, Laurent Pinchart wrote:
> > On Mon, Feb 14, 2022 at 07:43:13PM +0100, Jacopo Mondi wrote:
> > > The imx7-media-csi driver controls the CSI (CMOS Sensor Interface)
> > > peripheral available on several SoC of different generations.
> > >
> > > The current situation when it comes to compatible strings is rather
> > > confused:
> > > - Bindings document imx6ul, imx7 and imx8mm
> > > - Driver supports imx6ul, imx7 and imx8mq
> > > - The IMX8MM and IMX8MQ DTS use the correct compatible strings with a
> > >   fallback to the generic "imx7-csi" identifier:
> > >   arch/arm64/boot/dts/freescale/imx8mq.dtsi: compatible = "fsl,imx8mq-csi",
> > >   arch/arm64/boot/dts/freescale/imx8mm.dtsi: compatible = "fsl,imx8mm-csi",
> > >
> > > Tidy-up the situation by adding the IMX8MQ compatible string to the
> > > bindings documentation andathe IMX8MM identifier to the driver, to allow
> > > to distinguish the SoC the CSI peripheral is integrated on in the
> > > following patches.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml | 1 +
> > >  drivers/staging/media/imx/imx7-media-csi.c                | 2 ++
> >
> > I think Rob would prefer this being split in two patches, and I think it
> > would make sense, as you're fixing two separate issues.
> >
> > >  2 files changed, 3 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> > > index 4f7b78265336..0f1505d85111 100644
> > > --- a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> > > +++ b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> > > @@ -21,6 +21,7 @@ properties:
> > >            - fsl,imx7-csi
> > >            - fsl,imx6ul-csi
> > >        - items:
> > > +          - const: fsl,imx8mq-csi
> > >            - const: fsl,imx8mm-csi
> > >            - const: fsl,imx7-csi
> >
> > I don't think you intended to require the following:
> >
> > 	compatible = "fsl,imx8mq-csi", "fsl,imx8mm-csi", "fsl,imx7-csi";
> 
> No, I kind of superficially added the mq version where the mm was
> already and went on :)
> 
> Care to explain why currently we have two const for the "8mm" and the
> "imx7" versions ?

Because the imx8mm version was considered compatible with the imx7, so

       - items:
           - const: fsl,imx8mm-csi
           - const: fsl,imx7-csi

will validate

	compatible = "fsl,imx8mm-csi", "fsl,imx7-csi";

The first imx8mm compatible string is ignored by the driver, but
documented to support future drivers changes that would require
differentiating between the two versions.

> > You probably want
> >
> >  properties:
> >    compatible:
> >      oneOf:
> >        - enum:
> > +          - fsl,imx8mq-csi
> >            - fsl,imx7-csi
> >            - fsl,imx6ul-csi
> >        - items:
> >            - const: fsl,imx8mm-csi
> >            - const: fsl,imx7-csi
> >
> > instead.
> 
> I'm not aware of how how many revisions of the imx7 and imx6 versions
> exists, nor how they differ, but the existing distinction feels a bit
> weird.
> 
> The const items should be the compatible fallbacks, should them be
> generic, why is 8mm among them ? Shouldn't we specify the precise SoC
> version in the list of possible enum items only ?

No, the const items are not the compatible fallbacks. imx8mm isn't a
generic fallback. "items" requires *all* items to be present in order to
validate.

> Something like
> 
>       oneOf:
>         - enum:
>           - fsl,imx8mq-csi
>           - fsl,imx8mm-csi
>           - fsl,imx6ul-csi
>         - const:
>           - fsl,imx7-csi

That's not a valid schema.

> 
> In example I see:
> 
> arch/arm64/boot/dts/freescale/imx8mq.dtsi: compatible = "fsl,imx8mq-csi", "fsl,imx7-csi";
> 
> Where this should either be
>                                            compatible = "fsl,imx8mq-csi"
> or
>                                            compatible = "fsl,imx8mm-csi", "fsl,imx7-csi";
> 
> ?
> 
> > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> > > index 32311fc0e2a4..59100e409709 100644
> > > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > > @@ -162,6 +162,7 @@
> > >  enum imx_csi_model {
> > >  	IMX7_CSI_IMX7 = 0,
> > >  	IMX7_CSI_IMX8MQ,
> > > +	IMX7_CSI_IMX8MM,
> > >  };
> > >
> > >  struct imx7_csi {
> > > @@ -1277,6 +1278,7 @@ static int imx7_csi_remove(struct platform_device *pdev)
> > >
> > >  static const struct of_device_id imx7_csi_of_match[] = {
> > >  	{ .compatible = "fsl,imx8mq-csi", .data = (void *)IMX7_CSI_IMX8MQ },
> > > +	{ .compatible = "fsl,imx8mm-csi", .data = (void *)IMX7_CSI_IMX8MM },
> >
> > This isn't needed, as the i.MX8MM CSI bridgge is considered fully
> > backward-compatible with the i.MX7 version. I'd introduce this change in
> > the patch where you start using IMX7_CSI_IMX8MM, and I would then add
> > the following to the DT binding:
> >
> >  properties:
> >    compatible:
> >      oneOf:
> >        - enum:
> >            - fsl,imx8mq-csi
> > +          - fsl,imx8mm-csi
> >            - fsl,imx7-csi
> >            - fsl,imx6ul-csi
> >        - items:
> >            - const: fsl,imx8mm-csi
> >            - const: fsl,imx7-csi
> >
> > to allow setting
> >
> > 	compatible = "fsl,imx8mm-csi";
> >
> > without the imx7 fallback if we consider the i.MX8MM version different.
> > If the driver can operate correctly on the i.MX8MM when using the i.MX7
> > fallback code paths (possibly minor issues that are not considered
> > fatal, such as missing features) then you could skip this binding
> > change.
> 
> Sorry, but shouldn't:
> 
>         compatible = "fsl,imx8mm-csi", fsl,imx7-csi"
> 
> allow me to match on imx8mm already, without the above change.
> 
> I think what I don't get is why imx8mm is a 'generic fallback' in
> first place.

See above.

> > >  	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 },
> > >  	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 },
> > >  	{ },

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/8] staging: media: imx: Add more compatible strings

[Jacopo Mondi]

On Tue, Feb 15, 2022 at 10:45:41AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tue, Feb 15, 2022 at 09:36:31AM +0100, Jacopo Mondi wrote:
> > On Mon, Feb 14, 2022 at 09:15:07PM +0200, Laurent Pinchart wrote:
> > > On Mon, Feb 14, 2022 at 07:43:13PM +0100, Jacopo Mondi wrote:
> > > > The imx7-media-csi driver controls the CSI (CMOS Sensor Interface)
> > > > peripheral available on several SoC of different generations.
> > > >
> > > > The current situation when it comes to compatible strings is rather
> > > > confused:
> > > > - Bindings document imx6ul, imx7 and imx8mm
> > > > - Driver supports imx6ul, imx7 and imx8mq
> > > > - The IMX8MM and IMX8MQ DTS use the correct compatible strings with a
> > > >   fallback to the generic "imx7-csi" identifier:
> > > >   arch/arm64/boot/dts/freescale/imx8mq.dtsi: compatible = "fsl,imx8mq-csi",
> > > >   arch/arm64/boot/dts/freescale/imx8mm.dtsi: compatible = "fsl,imx8mm-csi",
> > > >
> > > > Tidy-up the situation by adding the IMX8MQ compatible string to the
> > > > bindings documentation andathe IMX8MM identifier to the driver, to allow
> > > > to distinguish the SoC the CSI peripheral is integrated on in the
> > > > following patches.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml | 1 +
> > > >  drivers/staging/media/imx/imx7-media-csi.c                | 2 ++
> > >
> > > I think Rob would prefer this being split in two patches, and I think it
> > > would make sense, as you're fixing two separate issues.
> > >
> > > >  2 files changed, 3 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> > > > index 4f7b78265336..0f1505d85111 100644
> > > > --- a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> > > > @@ -21,6 +21,7 @@ properties:
> > > >            - fsl,imx7-csi
> > > >            - fsl,imx6ul-csi
> > > >        - items:
> > > > +          - const: fsl,imx8mq-csi
> > > >            - const: fsl,imx8mm-csi
> > > >            - const: fsl,imx7-csi
> > >
> > > I don't think you intended to require the following:
> > >
> > > 	compatible = "fsl,imx8mq-csi", "fsl,imx8mm-csi", "fsl,imx7-csi";
> >
> > No, I kind of superficially added the mq version where the mm was
> > already and went on :)
> >
> > Care to explain why currently we have two const for the "8mm" and the
> > "imx7" versions ?
>
> Because the imx8mm version was considered compatible with the imx7, so
>
>        - items:
>            - const: fsl,imx8mm-csi
>            - const: fsl,imx7-csi
>
> will validate
>
> 	compatible = "fsl,imx8mm-csi", "fsl,imx7-csi";
>
> The first imx8mm compatible string is ignored by the driver, but
> documented to support future drivers changes that would require
> differentiating between the two versions.
>

ah that's why the driver doesn't match on mm, yet

> > > You probably want
> > >
> > >  properties:
> > >    compatible:
> > >      oneOf:
> > >        - enum:
> > > +          - fsl,imx8mq-csi
> > >            - fsl,imx7-csi
> > >            - fsl,imx6ul-csi
> > >        - items:
> > >            - const: fsl,imx8mm-csi
> > >            - const: fsl,imx7-csi
> > >
> > > instead.
> >
> > I'm not aware of how how many revisions of the imx7 and imx6 versions
> > exists, nor how they differ, but the existing distinction feels a bit
> > weird.
> >
> > The const items should be the compatible fallbacks, should them be
> > generic, why is 8mm among them ? Shouldn't we specify the precise SoC
> > version in the list of possible enum items only ?
>
> No, the const items are not the compatible fallbacks. imx8mm isn't a
> generic fallback. "items" requires *all* items to be present in order to
> validate.
>
> > Something like
> >
> >       oneOf:
> >         - enum:
> >           - fsl,imx8mq-csi
> >           - fsl,imx8mm-csi
> >           - fsl,imx6ul-csi
> >         - const:
> >           - fsl,imx7-csi
>
> That's not a valid schema.
>

I meant

       oneOf:
         - enum:
           - fsl,imx8mq-csi
           - fsl,imx8mm-csi
           - fsl,imx6ul-csi
         - items:
           - const: fsl,imx7-csi

But I now understand why 8mm and imx7 were there.

And the oneOf indeed doesn't allow to use imx7 as a fallback but
rather as an alternative to the precise SoC identifiers.

> >
> > In example I see:
> >
> > arch/arm64/boot/dts/freescale/imx8mq.dtsi: compatible = "fsl,imx8mq-csi", "fsl,imx7-csi";
> >
> > Where this should either be
> >                                            compatible = "fsl,imx8mq-csi"
> > or
> >                                            compatible = "fsl,imx8mm-csi", "fsl,imx7-csi";
> >
> > ?
> >
> > > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> > > > index 32311fc0e2a4..59100e409709 100644
> > > > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > > > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > > > @@ -162,6 +162,7 @@
> > > >  enum imx_csi_model {
> > > >  	IMX7_CSI_IMX7 = 0,
> > > >  	IMX7_CSI_IMX8MQ,
> > > > +	IMX7_CSI_IMX8MM,
> > > >  };
> > > >
> > > >  struct imx7_csi {
> > > > @@ -1277,6 +1278,7 @@ static int imx7_csi_remove(struct platform_device *pdev)
> > > >
> > > >  static const struct of_device_id imx7_csi_of_match[] = {
> > > >  	{ .compatible = "fsl,imx8mq-csi", .data = (void *)IMX7_CSI_IMX8MQ },
> > > > +	{ .compatible = "fsl,imx8mm-csi", .data = (void *)IMX7_CSI_IMX8MM },
> > >
> > > This isn't needed, as the i.MX8MM CSI bridgge is considered fully
> > > backward-compatible with the i.MX7 version. I'd introduce this change in
> > > the patch where you start using IMX7_CSI_IMX8MM, and I would then add
> > > the following to the DT binding:
> > >
> > >  properties:
> > >    compatible:
> > >      oneOf:
> > >        - enum:
> > >            - fsl,imx8mq-csi
> > > +          - fsl,imx8mm-csi
> > >            - fsl,imx7-csi
> > >            - fsl,imx6ul-csi
> > >        - items:
> > >            - const: fsl,imx8mm-csi
> > >            - const: fsl,imx7-csi
> > >
> > > to allow setting
> > >
> > > 	compatible = "fsl,imx8mm-csi";
> > >
> > > without the imx7 fallback if we consider the i.MX8MM version different.
> > > If the driver can operate correctly on the i.MX8MM when using the i.MX7
> > > fallback code paths (possibly minor issues that are not considered
> > > fatal, such as missing features) then you could skip this binding
> > > change.
> >
> > Sorry, but shouldn't:
> >
> >         compatible = "fsl,imx8mm-csi", fsl,imx7-csi"
> >
> > allow me to match on imx8mm already, without the above change.
> >
> > I think what I don't get is why imx8mm is a 'generic fallback' in
> > first place.
>
> See above.
>
> > > >  	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 },
> > > >  	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 },
> > > >  	{ },
>
> --
> Regards,
>
> Laurent Pinchart

[PATCH 4/8] staging: media: imx: Define per-SoC info

[Jacopo Mondi]

Define the imx-media-info structure which contains CSI configuration
parameter that depend on the SoC version the peripheral is integrated
in.

Replace the existing 'model' field with the newly defined structure.

Only define the SoC id and the supported pixel sampling modes for the
moment.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/staging/media/imx/imx-media.h      | 44 ++++++++++++++++++++++
 drivers/staging/media/imx/imx7-media-csi.c | 44 ++++++++++++++--------
 2 files changed, 73 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index f263fc3adbb9..1b0b660413cb 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -18,6 +18,16 @@
 #define IMX_MEDIA_DEF_PIX_WIDTH		640
 #define IMX_MEDIA_DEF_PIX_HEIGHT	480
 
+/*
+ * Enumeration of the SoC models the peripheral is integrated in.
+ */
+enum soc_id {
+	IMX6UL,
+	IMX7,
+	IMX8MM,
+	IMX8MQ,
+};
+
 /*
  * Enumeration of the IPU internal sub-devices
  */
@@ -141,10 +151,44 @@ struct imx_media_pad_vdev {
 	struct list_head list;
 };
 
+/*
+ * enum sample_mode_id - Define the CSI Rx queue sample size
+ *
+ * The pixel sampling mode defines the possible sampling methods from the
+ * CSI Rx queue to the next processing block of the capture pipeline.
+ *
+ * The supported methods depends on the SoC model and on synthesis time
+ * configurations.
+ *
+ * @MODE_SINGLE: Single pixel mode sampling
+ * @MODE_DUAL: Double pixel mode sampling
+ * @MODE_QUAD: Quad pixel mode sampling
+ */
+enum sample_mode_id {
+	MODE_SINGLE = BIT(0),
+	MODE_DUAL = BIT(1),
+	MODE_QUAD = BIT(2),
+};
+
+/*
+ * Information and configurations dependent on the SoC the peripheral is
+ * integrated in.
+ *
+ * @soc_id: The SoC identifier. See &enum soc_id.
+ * @sample_modes: Mask of supported pixel modes. See &enum sample_mode_id.
+ */
+struct imx_media_info {
+	enum soc_id soc_id;
+	u8 sample_modes;
+};
+
 struct imx_media_dev {
 	struct media_device md;
 	struct v4l2_device  v4l2_dev;
 
+	/* Per-model information. */
+	const struct imx_media_info *info;
+
 	/* the pipeline object */
 	struct media_pipeline pipe;
 
diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index 59100e409709..112096774961 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -159,12 +159,6 @@
 #define CSI_CSICR18			0x48
 #define CSI_CSICR19			0x4c
 
-enum imx_csi_model {
-	IMX7_CSI_IMX7 = 0,
-	IMX7_CSI_IMX8MQ,
-	IMX7_CSI_IMX8MM,
-};
-
 struct imx7_csi {
 	struct device *dev;
 	struct v4l2_subdev sd;
@@ -200,8 +194,6 @@ struct imx7_csi {
 	bool is_csi2;
 
 	struct completion last_eof_completion;
-
-	enum imx_csi_model model;
 };
 
 static struct imx7_csi *
@@ -562,6 +554,8 @@ static void imx7_csi_baseaddr_switch_on_second_frame(struct imx7_csi *csi)
 
 static void imx7_csi_enable(struct imx7_csi *csi)
 {
+	struct imx_media_dev *imxmd = csi->imxmd;
+
 	/* Clear the Rx FIFO and reflash the DMA controller. */
 	imx7_csi_rx_fifo_clear(csi);
 	imx7_csi_dma_reflash(csi);
@@ -576,7 +570,7 @@ static void imx7_csi_enable(struct imx7_csi *csi)
 	imx7_csi_dmareq_rff_enable(csi);
 	imx7_csi_hw_enable(csi);
 
-	if (csi->model == IMX7_CSI_IMX8MQ)
+	if (imxmd->info->soc_id == IMX8MQ)
 		imx7_csi_baseaddr_switch_on_second_frame(csi);
 }
 
@@ -1181,8 +1175,6 @@ static int imx7_csi_probe(struct platform_device *pdev)
 	if (IS_ERR(csi->regbase))
 		return PTR_ERR(csi->regbase);
 
-	csi->model = (enum imx_csi_model)(uintptr_t)of_device_get_match_data(&pdev->dev);
-
 	spin_lock_init(&csi->irqlock);
 	mutex_init(&csi->lock);
 
@@ -1202,6 +1194,8 @@ static int imx7_csi_probe(struct platform_device *pdev)
 	}
 	platform_set_drvdata(pdev, &csi->sd);
 
+	imxmd->info = of_device_get_match_data(dev);
+
 	ret = imx_media_of_add_csi(imxmd, node);
 	if (ret < 0 && ret != -ENODEV && ret != -EEXIST)
 		goto cleanup;
@@ -1276,11 +1270,31 @@ static int imx7_csi_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct imx_media_info imx8mq_info = {
+	.soc_id = IMX8MQ,
+	.sample_modes = MODE_SINGLE,
+};
+
+static const struct imx_media_info imx8mm_info = {
+	.soc_id = IMX8MM,
+	.sample_modes = MODE_SINGLE | MODE_DUAL,
+};
+
+static const struct imx_media_info imx7_info = {
+	.soc_id = IMX7,
+	.sample_modes = MODE_SINGLE,
+};
+
+static const struct imx_media_info imx6ul_info = {
+	.soc_id = IMX6UL,
+	.sample_modes = MODE_SINGLE,
+};
+
 static const struct of_device_id imx7_csi_of_match[] = {
-	{ .compatible = "fsl,imx8mq-csi", .data = (void *)IMX7_CSI_IMX8MQ },
-	{ .compatible = "fsl,imx8mm-csi", .data = (void *)IMX7_CSI_IMX8MM },
-	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 },
-	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 },
+	{ .compatible = "fsl,imx8mq-csi", .data = &imx8mq_info },
+	{ .compatible = "fsl,imx8mm-csi", .data = &imx8mm_info },
+	{ .compatible = "fsl,imx7-csi", .data = &imx7_info },
+	{ .compatible = "fsl,imx6ul-csi", .data = &imx6ul_info },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
-- 
2.35.0

Re: [PATCH 4/8] staging: media: imx: Define per-SoC info

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Feb 14, 2022 at 07:43:14PM +0100, Jacopo Mondi wrote:
> Define the imx-media-info structure which contains CSI configuration
> parameter that depend on the SoC version the peripheral is integrated
> in.
> 
> Replace the existing 'model' field with the newly defined structure.
> 
> Only define the SoC id and the supported pixel sampling modes for the
> moment.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/staging/media/imx/imx-media.h      | 44 ++++++++++++++++++++++
>  drivers/staging/media/imx/imx7-media-csi.c | 44 ++++++++++++++--------
>  2 files changed, 73 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index f263fc3adbb9..1b0b660413cb 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -18,6 +18,16 @@
>  #define IMX_MEDIA_DEF_PIX_WIDTH		640
>  #define IMX_MEDIA_DEF_PIX_HEIGHT	480
>  
> +/*
> + * Enumeration of the SoC models the peripheral is integrated in.
> + */
> +enum soc_id {
> +	IMX6UL,
> +	IMX7,
> +	IMX8MM,
> +	IMX8MQ,

Those names are too generic.

> +};
> +
>  /*
>   * Enumeration of the IPU internal sub-devices
>   */
> @@ -141,10 +151,44 @@ struct imx_media_pad_vdev {
>  	struct list_head list;
>  };
>  
> +/*
> + * enum sample_mode_id - Define the CSI Rx queue sample size
> + *
> + * The pixel sampling mode defines the possible sampling methods from the
> + * CSI Rx queue to the next processing block of the capture pipeline.
> + *
> + * The supported methods depends on the SoC model and on synthesis time
> + * configurations.
> + *
> + * @MODE_SINGLE: Single pixel mode sampling
> + * @MODE_DUAL: Double pixel mode sampling
> + * @MODE_QUAD: Quad pixel mode sampling
> + */
> +enum sample_mode_id {
> +	MODE_SINGLE = BIT(0),
> +	MODE_DUAL = BIT(1),
> +	MODE_QUAD = BIT(2),

Here too.

> +};

Let's limit this to the imx7-media-csi driver, it's unrelated to the
i.MX6 IPUv3 and should not be part of common helpers. It doesn't seem
like any subsequent patch in this series use the sample mode or the
soc_id in common helpers, so it should hopefully not be a bit issue.

I would also like to see a comment somewhere (in this patch or one of
the subsequent ones) that explains in more details how the CSIS and CSI
bridge are connected, and how various bits affect data signal routing
between the two. I can help if necessary.

> +/*
> + * Information and configurations dependent on the SoC the peripheral is
> + * integrated in.
> + *
> + * @soc_id: The SoC identifier. See &enum soc_id.
> + * @sample_modes: Mask of supported pixel modes. See &enum sample_mode_id.
> + */
> +struct imx_media_info {
> +	enum soc_id soc_id;
> +	u8 sample_modes;
> +};
> +
>  struct imx_media_dev {
>  	struct media_device md;
>  	struct v4l2_device  v4l2_dev;
>  
> +	/* Per-model information. */
> +	const struct imx_media_info *info;
> +
>  	/* the pipeline object */
>  	struct media_pipeline pipe;
>  
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index 59100e409709..112096774961 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -159,12 +159,6 @@
>  #define CSI_CSICR18			0x48
>  #define CSI_CSICR19			0x4c
>  
> -enum imx_csi_model {
> -	IMX7_CSI_IMX7 = 0,
> -	IMX7_CSI_IMX8MQ,
> -	IMX7_CSI_IMX8MM,
> -};

I think you can keep this instead of soc_id.

> -
>  struct imx7_csi {
>  	struct device *dev;
>  	struct v4l2_subdev sd;
> @@ -200,8 +194,6 @@ struct imx7_csi {
>  	bool is_csi2;
>  
>  	struct completion last_eof_completion;
> -
> -	enum imx_csi_model model;
>  };
>  
>  static struct imx7_csi *
> @@ -562,6 +554,8 @@ static void imx7_csi_baseaddr_switch_on_second_frame(struct imx7_csi *csi)
>  
>  static void imx7_csi_enable(struct imx7_csi *csi)
>  {
> +	struct imx_media_dev *imxmd = csi->imxmd;
> +
>  	/* Clear the Rx FIFO and reflash the DMA controller. */
>  	imx7_csi_rx_fifo_clear(csi);
>  	imx7_csi_dma_reflash(csi);
> @@ -576,7 +570,7 @@ static void imx7_csi_enable(struct imx7_csi *csi)
>  	imx7_csi_dmareq_rff_enable(csi);
>  	imx7_csi_hw_enable(csi);
>  
> -	if (csi->model == IMX7_CSI_IMX8MQ)
> +	if (imxmd->info->soc_id == IMX8MQ)
>  		imx7_csi_baseaddr_switch_on_second_frame(csi);
>  }
>  
> @@ -1181,8 +1175,6 @@ static int imx7_csi_probe(struct platform_device *pdev)
>  	if (IS_ERR(csi->regbase))
>  		return PTR_ERR(csi->regbase);
>  
> -	csi->model = (enum imx_csi_model)(uintptr_t)of_device_get_match_data(&pdev->dev);
> -
>  	spin_lock_init(&csi->irqlock);
>  	mutex_init(&csi->lock);
>  
> @@ -1202,6 +1194,8 @@ static int imx7_csi_probe(struct platform_device *pdev)
>  	}
>  	platform_set_drvdata(pdev, &csi->sd);
>  
> +	imxmd->info = of_device_get_match_data(dev);
> +
>  	ret = imx_media_of_add_csi(imxmd, node);
>  	if (ret < 0 && ret != -ENODEV && ret != -EEXIST)
>  		goto cleanup;
> @@ -1276,11 +1270,31 @@ static int imx7_csi_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct imx_media_info imx8mq_info = {
> +	.soc_id = IMX8MQ,
> +	.sample_modes = MODE_SINGLE,
> +};
> +
> +static const struct imx_media_info imx8mm_info = {
> +	.soc_id = IMX8MM,
> +	.sample_modes = MODE_SINGLE | MODE_DUAL,
> +};
> +
> +static const struct imx_media_info imx7_info = {
> +	.soc_id = IMX7,
> +	.sample_modes = MODE_SINGLE,
> +};
> +
> +static const struct imx_media_info imx6ul_info = {
> +	.soc_id = IMX6UL,
> +	.sample_modes = MODE_SINGLE,
> +};
> +
>  static const struct of_device_id imx7_csi_of_match[] = {
> -	{ .compatible = "fsl,imx8mq-csi", .data = (void *)IMX7_CSI_IMX8MQ },
> -	{ .compatible = "fsl,imx8mm-csi", .data = (void *)IMX7_CSI_IMX8MM },
> -	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 },
> -	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 },
> +	{ .compatible = "fsl,imx8mq-csi", .data = &imx8mq_info },
> +	{ .compatible = "fsl,imx8mm-csi", .data = &imx8mm_info },
> +	{ .compatible = "fsl,imx7-csi", .data = &imx7_info },
> +	{ .compatible = "fsl,imx6ul-csi", .data = &imx6ul_info },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, imx7_csi_of_match);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/8] staging: media: imx: Define per-SoC info

[Jacopo Mondi]

Hi Laurent

On Mon, Feb 14, 2022 at 09:20:41PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Feb 14, 2022 at 07:43:14PM +0100, Jacopo Mondi wrote:
> > Define the imx-media-info structure which contains CSI configuration
> > parameter that depend on the SoC version the peripheral is integrated
> > in.
> >
> > Replace the existing 'model' field with the newly defined structure.
> >
> > Only define the SoC id and the supported pixel sampling modes for the
> > moment.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/staging/media/imx/imx-media.h      | 44 ++++++++++++++++++++++
> >  drivers/staging/media/imx/imx7-media-csi.c | 44 ++++++++++++++--------
> >  2 files changed, 73 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> > index f263fc3adbb9..1b0b660413cb 100644
> > --- a/drivers/staging/media/imx/imx-media.h
> > +++ b/drivers/staging/media/imx/imx-media.h
> > @@ -18,6 +18,16 @@
> >  #define IMX_MEDIA_DEF_PIX_WIDTH		640
> >  #define IMX_MEDIA_DEF_PIX_HEIGHT	480
> >
> > +/*
> > + * Enumeration of the SoC models the peripheral is integrated in.
> > + */
> > +enum soc_id {
> > +	IMX6UL,
> > +	IMX7,
> > +	IMX8MM,
> > +	IMX8MQ,
>
> Those names are too generic.
>

C++ scoped enums got me used to shorten the single enum items :)
Of course this should be made specific.

> > +};
> > +
> >  /*
> >   * Enumeration of the IPU internal sub-devices
> >   */
> > @@ -141,10 +151,44 @@ struct imx_media_pad_vdev {
> >  	struct list_head list;
> >  };
> >
> > +/*
> > + * enum sample_mode_id - Define the CSI Rx queue sample size
> > + *
> > + * The pixel sampling mode defines the possible sampling methods from the
> > + * CSI Rx queue to the next processing block of the capture pipeline.
> > + *
> > + * The supported methods depends on the SoC model and on synthesis time
> > + * configurations.
> > + *
> > + * @MODE_SINGLE: Single pixel mode sampling
> > + * @MODE_DUAL: Double pixel mode sampling
> > + * @MODE_QUAD: Quad pixel mode sampling
> > + */
> > +enum sample_mode_id {
> > +	MODE_SINGLE = BIT(0),
> > +	MODE_DUAL = BIT(1),
> > +	MODE_QUAD = BIT(2),
>
> Here too.
>
> > +};
>
> Let's limit this to the imx7-media-csi driver, it's unrelated to the
> i.MX6 IPUv3 and should not be part of common helpers. It doesn't seem

Ok, I had no idea this header was used by i.MX6 too. What a mess.

> like any subsequent patch in this series use the sample mode or the
> soc_id in common helpers, so it should hopefully not be a bit issue.
>

No it would not.

> I would also like to see a comment somewhere (in this patch or one of
> the subsequent ones) that explains in more details how the CSIS and CSI
> bridge are connected, and how various bits affect data signal routing
> between the two. I can help if necessary.
>

I can try but I'm not sure I have the full picture in mind.

> > +/*
> > + * Information and configurations dependent on the SoC the peripheral is
> > + * integrated in.
> > + *
> > + * @soc_id: The SoC identifier. See &enum soc_id.
> > + * @sample_modes: Mask of supported pixel modes. See &enum sample_mode_id.
> > + */
> > +struct imx_media_info {
> > +	enum soc_id soc_id;
> > +	u8 sample_modes;
> > +};
> > +
> >  struct imx_media_dev {
> >  	struct media_device md;
> >  	struct v4l2_device  v4l2_dev;
> >
> > +	/* Per-model information. */
> > +	const struct imx_media_info *info;
> > +
> >  	/* the pipeline object */
> >  	struct media_pipeline pipe;
> >
> > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> > index 59100e409709..112096774961 100644
> > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > @@ -159,12 +159,6 @@
> >  #define CSI_CSICR18			0x48
> >  #define CSI_CSICR19			0x4c
> >
> > -enum imx_csi_model {
> > -	IMX7_CSI_IMX7 = 0,
> > -	IMX7_CSI_IMX8MQ,
> > -	IMX7_CSI_IMX8MM,
> > -};
>
> I think you can keep this instead of soc_id.
>

Ahem. I develed this patches on a downstream where I could test and
where these where not there yet. When I rebased on media-master I
found out about these ones, and I replaced them without too much
thinking.

> > -
> >  struct imx7_csi {
> >  	struct device *dev;
> >  	struct v4l2_subdev sd;
> > @@ -200,8 +194,6 @@ struct imx7_csi {
> >  	bool is_csi2;
> >
> >  	struct completion last_eof_completion;
> > -
> > -	enum imx_csi_model model;
> >  };
> >
> >  static struct imx7_csi *
> > @@ -562,6 +554,8 @@ static void imx7_csi_baseaddr_switch_on_second_frame(struct imx7_csi *csi)
> >
> >  static void imx7_csi_enable(struct imx7_csi *csi)
> >  {
> > +	struct imx_media_dev *imxmd = csi->imxmd;
> > +
> >  	/* Clear the Rx FIFO and reflash the DMA controller. */
> >  	imx7_csi_rx_fifo_clear(csi);
> >  	imx7_csi_dma_reflash(csi);
> > @@ -576,7 +570,7 @@ static void imx7_csi_enable(struct imx7_csi *csi)
> >  	imx7_csi_dmareq_rff_enable(csi);
> >  	imx7_csi_hw_enable(csi);
> >
> > -	if (csi->model == IMX7_CSI_IMX8MQ)
> > +	if (imxmd->info->soc_id == IMX8MQ)
> >  		imx7_csi_baseaddr_switch_on_second_frame(csi);
> >  }
> >
> > @@ -1181,8 +1175,6 @@ static int imx7_csi_probe(struct platform_device *pdev)
> >  	if (IS_ERR(csi->regbase))
> >  		return PTR_ERR(csi->regbase);
> >
> > -	csi->model = (enum imx_csi_model)(uintptr_t)of_device_get_match_data(&pdev->dev);
> > -
> >  	spin_lock_init(&csi->irqlock);
> >  	mutex_init(&csi->lock);
> >
> > @@ -1202,6 +1194,8 @@ static int imx7_csi_probe(struct platform_device *pdev)
> >  	}
> >  	platform_set_drvdata(pdev, &csi->sd);
> >
> > +	imxmd->info = of_device_get_match_data(dev);
> > +
> >  	ret = imx_media_of_add_csi(imxmd, node);
> >  	if (ret < 0 && ret != -ENODEV && ret != -EEXIST)
> >  		goto cleanup;
> > @@ -1276,11 +1270,31 @@ static int imx7_csi_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >
> > +static const struct imx_media_info imx8mq_info = {
> > +	.soc_id = IMX8MQ,
> > +	.sample_modes = MODE_SINGLE,
> > +};
> > +
> > +static const struct imx_media_info imx8mm_info = {
> > +	.soc_id = IMX8MM,
> > +	.sample_modes = MODE_SINGLE | MODE_DUAL,
> > +};
> > +
> > +static const struct imx_media_info imx7_info = {
> > +	.soc_id = IMX7,
> > +	.sample_modes = MODE_SINGLE,
> > +};
> > +
> > +static const struct imx_media_info imx6ul_info = {
> > +	.soc_id = IMX6UL,
> > +	.sample_modes = MODE_SINGLE,
> > +};
> > +
> >  static const struct of_device_id imx7_csi_of_match[] = {
> > -	{ .compatible = "fsl,imx8mq-csi", .data = (void *)IMX7_CSI_IMX8MQ },
> > -	{ .compatible = "fsl,imx8mm-csi", .data = (void *)IMX7_CSI_IMX8MM },
> > -	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 },
> > -	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 },
> > +	{ .compatible = "fsl,imx8mq-csi", .data = &imx8mq_info },
> > +	{ .compatible = "fsl,imx8mm-csi", .data = &imx8mm_info },
> > +	{ .compatible = "fsl,imx7-csi", .data = &imx7_info },
> > +	{ .compatible = "fsl,imx6ul-csi", .data = &imx6ul_info },
> >  	{ },
> >  };
> >  MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
>
> --
> Regards,
>
> Laurent Pinchart

[PATCH 5/8] staging: media: imx: Use DUAL pixel mode if available

[Jacopo Mondi]

The pixel sampling mode controls the size of data sampled from the CSI
Rx queue. The supported sample size depends on the configuration of the
preceding block in the capture pipeline and is then dependent on the SoC
version the CSI peripheral is integrated on.

When capturing YUV422 data if dual sample mode is available use it.

This change is particularly relevant for the IMX8MM SoC which uses the
CSIS CSI-2 receiver which operates in dual pixel mode.

Other SoCs should be unaffected by this change and should continue to
operate as before.

Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/staging/media/imx/imx7-media-csi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index 112096774961..a8bdfb0bb0ee 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -426,6 +426,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
 {
 	struct imx_media_video_dev *vdev = csi->vdev;
 	struct v4l2_pix_format *out_pix = &vdev->fmt;
+	struct imx_media_dev *imxmd = csi->imxmd;
 	int width = out_pix->width;
 	u32 stride = 0;
 	u32 cr3 = BIT_FRMCNT_RST;
@@ -436,7 +437,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
 	cr18 &= ~(BIT_CSI_HW_ENABLE | BIT_MIPI_DATA_FORMAT_MASK |
 		  BIT_DATA_FROM_MIPI | BIT_BASEADDR_CHG_ERR_EN |
 		  BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL |
-		  BIT_DEINTERLACE_EN);
+		  BIT_DEINTERLACE_EN | BIT_MIPI_DOUBLE_CMPNT);
 
 	if (out_pix->field == V4L2_FIELD_INTERLACED) {
 		cr18 |= BIT_DEINTERLACE_EN;
@@ -500,6 +501,13 @@ static void imx7_csi_configure(struct imx7_csi *csi)
 		case MEDIA_BUS_FMT_YUYV8_2X8:
 		case MEDIA_BUS_FMT_YUYV8_1X16:
 			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
+
+			/* If dual mode is supported use it. */
+			if (imxmd->info->sample_modes & MODE_DUAL) {
+				cr18 |= BIT_MIPI_DOUBLE_CMPNT;
+				cr3 |= BIT_TWO_8BIT_SENSOR;
+			}
+
 			break;
 		}
 	}
-- 
2.35.0

Re: [PATCH 5/8] staging: media: imx: Use DUAL pixel mode if available

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Feb 14, 2022 at 07:43:15PM +0100, Jacopo Mondi wrote:
> The pixel sampling mode controls the size of data sampled from the CSI
> Rx queue. The supported sample size depends on the configuration of the
> preceding block in the capture pipeline and is then dependent on the SoC
> version the CSI peripheral is integrated on.
> 
> When capturing YUV422 data if dual sample mode is available use it.
> 
> This change is particularly relevant for the IMX8MM SoC which uses the
> CSIS CSI-2 receiver which operates in dual pixel mode.
> 
> Other SoCs should be unaffected by this change and should continue to
> operate as before.
> 
> Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/staging/media/imx/imx7-media-csi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index 112096774961..a8bdfb0bb0ee 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -426,6 +426,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  {
>  	struct imx_media_video_dev *vdev = csi->vdev;
>  	struct v4l2_pix_format *out_pix = &vdev->fmt;
> +	struct imx_media_dev *imxmd = csi->imxmd;
>  	int width = out_pix->width;
>  	u32 stride = 0;
>  	u32 cr3 = BIT_FRMCNT_RST;
> @@ -436,7 +437,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  	cr18 &= ~(BIT_CSI_HW_ENABLE | BIT_MIPI_DATA_FORMAT_MASK |
>  		  BIT_DATA_FROM_MIPI | BIT_BASEADDR_CHG_ERR_EN |
>  		  BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL |
> -		  BIT_DEINTERLACE_EN);
> +		  BIT_DEINTERLACE_EN | BIT_MIPI_DOUBLE_CMPNT);
>  
>  	if (out_pix->field == V4L2_FIELD_INTERLACED) {
>  		cr18 |= BIT_DEINTERLACE_EN;
> @@ -500,6 +501,13 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  		case MEDIA_BUS_FMT_YUYV8_2X8:
>  		case MEDIA_BUS_FMT_YUYV8_1X16:
>  			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
> +
> +			/* If dual mode is supported use it. */
> +			if (imxmd->info->sample_modes & MODE_DUAL) {
> +				cr18 |= BIT_MIPI_DOUBLE_CMPNT;
> +				cr3 |= BIT_TWO_8BIT_SENSOR;
> +			}

I would implement this differently:

		case MEDIA_BUS_FMT_UYVY8_2X8:
		case MEDIA_BUS_FMT_YUYV8_2X8:
			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
			break;

		case MEDIA_BUS_FMT_UYVY8_1X16:
		case MEDIA_BUS_FMT_YUYV8_1X16:
			cr3 |= BIT_TWO_8BIT_SENSOR;
			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B
			     |  BIT_MIPI_DOUBLE_CMPNT;
			break;

This would support either option here. What you will then need to change
is imx7_csi_enum_mbus_code() and imx7_csi_try_fmt(), to allow/disallow
the 2X8 and 1X16 variants based on the SoC. This is important for the
i.MX7, which has both a CSI-2 input and a parallel input. When using the
CSIS it can (and should) use double component mode, while when using the
parallel input it can work in 8-bit or 16-bit mode depending on how the
sensor is wired.

> +
>  			break;
>  		}
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/8] staging: media: imx: Use DUAL pixel mode if available

[Jacopo Mondi]

Hi Laurent,

On Tue, Feb 15, 2022 at 09:12:44AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Feb 14, 2022 at 07:43:15PM +0100, Jacopo Mondi wrote:
> > The pixel sampling mode controls the size of data sampled from the CSI
> > Rx queue. The supported sample size depends on the configuration of the
> > preceding block in the capture pipeline and is then dependent on the SoC
> > version the CSI peripheral is integrated on.
> >
> > When capturing YUV422 data if dual sample mode is available use it.
> >
> > This change is particularly relevant for the IMX8MM SoC which uses the
> > CSIS CSI-2 receiver which operates in dual pixel mode.
> >
> > Other SoCs should be unaffected by this change and should continue to
> > operate as before.
> >
> > Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/staging/media/imx/imx7-media-csi.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> > index 112096774961..a8bdfb0bb0ee 100644
> > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > @@ -426,6 +426,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >  {
> >  	struct imx_media_video_dev *vdev = csi->vdev;
> >  	struct v4l2_pix_format *out_pix = &vdev->fmt;
> > +	struct imx_media_dev *imxmd = csi->imxmd;
> >  	int width = out_pix->width;
> >  	u32 stride = 0;
> >  	u32 cr3 = BIT_FRMCNT_RST;
> > @@ -436,7 +437,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >  	cr18 &= ~(BIT_CSI_HW_ENABLE | BIT_MIPI_DATA_FORMAT_MASK |
> >  		  BIT_DATA_FROM_MIPI | BIT_BASEADDR_CHG_ERR_EN |
> >  		  BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL |
> > -		  BIT_DEINTERLACE_EN);
> > +		  BIT_DEINTERLACE_EN | BIT_MIPI_DOUBLE_CMPNT);
> >
> >  	if (out_pix->field == V4L2_FIELD_INTERLACED) {
> >  		cr18 |= BIT_DEINTERLACE_EN;
> > @@ -500,6 +501,13 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >  		case MEDIA_BUS_FMT_YUYV8_2X8:
> >  		case MEDIA_BUS_FMT_YUYV8_1X16:
> >  			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
> > +
> > +			/* If dual mode is supported use it. */
> > +			if (imxmd->info->sample_modes & MODE_DUAL) {
> > +				cr18 |= BIT_MIPI_DOUBLE_CMPNT;
> > +				cr3 |= BIT_TWO_8BIT_SENSOR;
> > +			}
>
> I would implement this differently:
>
> 		case MEDIA_BUS_FMT_UYVY8_2X8:
> 		case MEDIA_BUS_FMT_YUYV8_2X8:
> 			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
> 			break;
>
> 		case MEDIA_BUS_FMT_UYVY8_1X16:
> 		case MEDIA_BUS_FMT_YUYV8_1X16:
> 			cr3 |= BIT_TWO_8BIT_SENSOR;
> 			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B
> 			     |  BIT_MIPI_DOUBLE_CMPNT;
> 			break;
>
> This would support either option here. What you will then need to change
> is imx7_csi_enum_mbus_code() and imx7_csi_try_fmt(), to allow/disallow
> the 2X8 and 1X16 variants based on the SoC. This is important for the

Exactly. And being the format list in the shared helper I decided it
was really not worth adding any SoC specific code to those helpers
which should ideally be nuked.

Should we decouple the helpers to get to a point where we can have
SoC-specific formats list ? Then I would be more than happy to use the
above construct.

There is one quirk I'm not sure about: How does the CSI connect to the
transmitter ? It is my undertanding that in i.MX8 connects to the the
CSI-2 receiver (either CSIS or the Northwest tech one). Does in older
SoC revisions connects directly to a parallel sensor ?

I'm asking also because there is a comment that reports

		/*
		 * CSI-2 sources are supposed to use the 1X16 formats, but not
		 * all of them comply. Support both variants.
		 */

But if the preceding block is the CSI-2 receiver, we control what
formats it exposes and we can use both 2X8 or 1X16 depending on the
SoC specificities and we don't care about the CSI-2 Tx supported
formats. Does that comment make sense in your opinion ?

Also be aware that, in example, if we expose from the CSIS source pad
both 2X8 and 1X16 we create a condition where userspace could
configure the pipeline uncorrectly.

Let's draw a table (for i.MX8 only as I don't know about the 7)


        8MM/CSI   8MQ/CSI  8MP/ISI
CSIS    Dual              Dual
NW              Single


Each block would then support

CSIS    Dual
NW      Single
CSI     Dual/Single
ISI     Dual

So we create a potential for a misconfiguration in 8MM with

        CSIS = Dual
        CSI = Single

or for the 8MQ

        NW = Single
        CSI = Double

If we don't create a list of SoC specific formats. Nothing bad, -EPIPE
will be returned, but maybe we should avoid that ?


> i.MX7, which has both a CSI-2 input and a parallel input. When using the
> CSIS it can (and should) use double component mode, while when using the
> parallel input it can work in 8-bit or 16-bit mode depending on how the
> sensor is wired.

I didn't know i.MX7 used the CSIS :/
Ah wait, the driver was called imx7-mipi-csi2 :/

>
> > +
> >  			break;
> >  		}
> >  	}
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 5/8] staging: media: imx: Use DUAL pixel mode if available

[Laurent Pinchart]

Hi Jacopo,

On Tue, Feb 15, 2022 at 09:59:18AM +0100, Jacopo Mondi wrote:
> On Tue, Feb 15, 2022 at 09:12:44AM +0200, Laurent Pinchart wrote:
> > On Mon, Feb 14, 2022 at 07:43:15PM +0100, Jacopo Mondi wrote:
> > > The pixel sampling mode controls the size of data sampled from the CSI
> > > Rx queue. The supported sample size depends on the configuration of the
> > > preceding block in the capture pipeline and is then dependent on the SoC
> > > version the CSI peripheral is integrated on.
> > >
> > > When capturing YUV422 data if dual sample mode is available use it.
> > >
> > > This change is particularly relevant for the IMX8MM SoC which uses the
> > > CSIS CSI-2 receiver which operates in dual pixel mode.
> > >
> > > Other SoCs should be unaffected by this change and should continue to
> > > operate as before.
> > >
> > > Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  drivers/staging/media/imx/imx7-media-csi.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> > > index 112096774961..a8bdfb0bb0ee 100644
> > > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > > @@ -426,6 +426,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> > >  {
> > >  	struct imx_media_video_dev *vdev = csi->vdev;
> > >  	struct v4l2_pix_format *out_pix = &vdev->fmt;
> > > +	struct imx_media_dev *imxmd = csi->imxmd;
> > >  	int width = out_pix->width;
> > >  	u32 stride = 0;
> > >  	u32 cr3 = BIT_FRMCNT_RST;
> > > @@ -436,7 +437,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> > >  	cr18 &= ~(BIT_CSI_HW_ENABLE | BIT_MIPI_DATA_FORMAT_MASK |
> > >  		  BIT_DATA_FROM_MIPI | BIT_BASEADDR_CHG_ERR_EN |
> > >  		  BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL |
> > > -		  BIT_DEINTERLACE_EN);
> > > +		  BIT_DEINTERLACE_EN | BIT_MIPI_DOUBLE_CMPNT);
> > >
> > >  	if (out_pix->field == V4L2_FIELD_INTERLACED) {
> > >  		cr18 |= BIT_DEINTERLACE_EN;
> > > @@ -500,6 +501,13 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> > >  		case MEDIA_BUS_FMT_YUYV8_2X8:
> > >  		case MEDIA_BUS_FMT_YUYV8_1X16:
> > >  			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
> > > +
> > > +			/* If dual mode is supported use it. */
> > > +			if (imxmd->info->sample_modes & MODE_DUAL) {
> > > +				cr18 |= BIT_MIPI_DOUBLE_CMPNT;
> > > +				cr3 |= BIT_TWO_8BIT_SENSOR;
> > > +			}
> >
> > I would implement this differently:
> >
> > 		case MEDIA_BUS_FMT_UYVY8_2X8:
> > 		case MEDIA_BUS_FMT_YUYV8_2X8:
> > 			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
> > 			break;
> >
> > 		case MEDIA_BUS_FMT_UYVY8_1X16:
> > 		case MEDIA_BUS_FMT_YUYV8_1X16:
> > 			cr3 |= BIT_TWO_8BIT_SENSOR;
> > 			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B
> > 			     |  BIT_MIPI_DOUBLE_CMPNT;
> > 			break;
> >
> > This would support either option here. What you will then need to change
> > is imx7_csi_enum_mbus_code() and imx7_csi_try_fmt(), to allow/disallow
> > the 2X8 and 1X16 variants based on the SoC. This is important for the
> 
> Exactly. And being the format list in the shared helper I decided it
> was really not worth adding any SoC specific code to those helpers
> which should ideally be nuked.
> 
> Should we decouple the helpers to get to a point where we can have
> SoC-specific formats list ? Then I would be more than happy to use the
> above construct.

Yes, we should. I have started working on this, but haven't had time to
complete the work yet.

> There is one quirk I'm not sure about: How does the CSI connect to the
> transmitter ? It is my undertanding that in i.MX8 connects to the the
> CSI-2 receiver (either CSIS or the Northwest tech one). Does in older
> SoC revisions connects directly to a parallel sensor ?

On i.MX7 there's a mux that allows selecting the CSI-2 receiver or the
parallel input. It's implemented using the video-mux driver, see
imx7s.dtsi for details.

> I'm asking also because there is a comment that reports
> 
> 		/*
> 		 * CSI-2 sources are supposed to use the 1X16 formats, but not
> 		 * all of them comply. Support both variants.
> 		 */
> 
> But if the preceding block is the CSI-2 receiver, we control what
> formats it exposes and we can use both 2X8 or 1X16 depending on the
> SoC specificities and we don't care about the CSI-2 Tx supported
> formats. Does that comment make sense in your opinion ?

I think the comment should be dropped. There are CSI-2 sensor drivers
that incorrectly use the 2X8 formats instead of 1X16, but I don't think
we should work around that here, the sensor drivers should be fixed
instead.

> Also be aware that, in example, if we expose from the CSIS source pad
> both 2X8 and 1X16 we create a condition where userspace could
> configure the pipeline uncorrectly.
> 
> Let's draw a table (for i.MX8 only as I don't know about the 7)
> 
> 
>         8MM/CSI   8MQ/CSI  8MP/ISI
> CSIS    Dual              Dual
> NW              Single
> 
> 
> Each block would then support
> 
> CSIS    Dual
> NW      Single
> CSI     Dual/Single
> ISI     Dual
> 
> So we create a potential for a misconfiguration in 8MM with
> 
>         CSIS = Dual
>         CSI = Single
> 
> or for the 8MQ
> 
>         NW = Single
>         CSI = Double
> 
> If we don't create a list of SoC specific formats. Nothing bad, -EPIPE
> will be returned, but maybe we should avoid that ?

Format propagation is typically done from source to sink in userspace,
so the risk of picking an incorrect format is very low (and it would be
a userspace bug in any case). Nonetheless, it would be nice to avoid
exposing formats that can't be used, but that can be done later.

> > i.MX7, which has both a CSI-2 input and a parallel input. When using the
> > CSIS it can (and should) use double component mode, while when using the
> > parallel input it can work in 8-bit or 16-bit mode depending on how the
> > sensor is wired.
> 
> I didn't know i.MX7 used the CSIS :/
> Ah wait, the driver was called imx7-mipi-csi2 :/

:-)

> > > +
> > >  			break;
> > >  		}
> > >  	}

-- 
Regards,

Laurent Pinchart

[PATCH 6/8] media: imx: imx-mipi-csis: Set PIXEL_MODE for YUV422

[Jacopo Mondi]

Bits 13 and 12 of the ISP_CONFIGn register configure the PIXEL_MODE
which specifies the sampling size, in pixel component units, on the
CSI-2 output data interface when data are transferred to memory.

The register description in the chip manual specifies that DUAL mode
should be used for YUV422 data but does not clarify the reason.

Verify if other YUV formats require the same setting and what is the
appropriate setting for RAW and sRGB formats.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
---
 drivers/media/platform/imx/imx-mipi-csis.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
index f433758c8935..98a7538a6ce3 100644
--- a/drivers/media/platform/imx/imx-mipi-csis.c
+++ b/drivers/media/platform/imx/imx-mipi-csis.c
@@ -173,6 +173,7 @@
 #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)
@@ -506,7 +507,12 @@ static void __mipi_csis_set_format(struct csi_state *state)

 	/* Color format */
 	val = mipi_csis_read(state, MIPI_CSIS_ISP_CONFIG_CH(0));
-	val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
+	val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK
+		| MIPI_CSIS_ISPCFG_PIXEL_MASK);
+
+	if (state->csis_fmt->data_type == MIPI_CSI2_DATA_TYPE_YUV422_8)
+		val |= MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
+
 	val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type);
 	mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val);

--
2.35.0

Re: [PATCH 6/8] media: imx: imx-mipi-csis: Set PIXEL_MODE for YUV422

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Feb 14, 2022 at 07:43:16PM +0100, Jacopo Mondi wrote:
> Bits 13 and 12 of the ISP_CONFIGn register configure the PIXEL_MODE
> which specifies the sampling size, in pixel component units, on the
> CSI-2 output data interface when data are transferred to memory.
> 
> The register description in the chip manual specifies that DUAL mode
> should be used for YUV422 data but does not clarify the reason.
> 
> Verify if other YUV formats require the same setting and what is the
> appropriate setting for RAW and sRGB formats.

If it's an action item, shouldn't it be in a TODO comment in the code
instead ?

While it shouldn't be difficult to test this in RAW8 mode, I'd leave it
for later, as I don't want to get into the rabbit hole of adding
S[RGB]{4}8_0_5X16 or S[RGB]{4}10_0_5X20 formats now :-)

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index f433758c8935..98a7538a6ce3 100644
> --- a/drivers/media/platform/imx/imx-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -173,6 +173,7 @@
>  #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)
> @@ -506,7 +507,12 @@ static void __mipi_csis_set_format(struct csi_state *state)
> 
>  	/* Color format */
>  	val = mipi_csis_read(state, MIPI_CSIS_ISP_CONFIG_CH(0));
> -	val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
> +	val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK
> +		| MIPI_CSIS_ISPCFG_PIXEL_MASK);
> +
> +	if (state->csis_fmt->data_type == MIPI_CSI2_DATA_TYPE_YUV422_8)
> +		val |= MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
> +
>  	val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type);
>  	mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val);
> 

-- 
Regards,

Laurent Pinchart

[PATCH 7/8] media: imx: imx-mipi-csis: Add RGB565_1X16

[Jacopo Mondi]

Add RGB565_1X16 to the enumeration of supported image formats.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/platform/imx/imx-mipi-csis.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
index 98a7538a6ce3..9e0a478dba75 100644
--- a/drivers/media/platform/imx/imx-mipi-csis.c
+++ b/drivers/media/platform/imx/imx-mipi-csis.c
@@ -360,6 +360,12 @@ static const struct csis_pix_format mipi_csis_formats[] = {
 		.data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
 		.width = 16,
 	},
+	/* RGB formats. */
+	{
+		.code = MEDIA_BUS_FMT_RGB565_1X16,
+		.data_type = MIPI_CSI2_DATA_TYPE_RGB565,
+		.width = 16,
+	},
 	/* RAW (Bayer and greyscale) formats. */
 	{
 		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
-- 
2.35.0

Re: [PATCH 7/8] media: imx: imx-mipi-csis: Add RGB565_1X16

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Feb 14, 2022 at 07:43:17PM +0100, Jacopo Mondi wrote:
> Add RGB565_1X16 to the enumeration of supported image formats.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

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

> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index 98a7538a6ce3..9e0a478dba75 100644
> --- a/drivers/media/platform/imx/imx-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -360,6 +360,12 @@ static const struct csis_pix_format mipi_csis_formats[] = {
>  		.data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
>  		.width = 16,
>  	},
> +	/* RGB formats. */
> +	{
> +		.code = MEDIA_BUS_FMT_RGB565_1X16,
> +		.data_type = MIPI_CSI2_DATA_TYPE_RGB565,
> +		.width = 16,
> +	},
>  	/* RAW (Bayer and greyscale) formats. */
>  	{
>  		.code = MEDIA_BUS_FMT_SBGGR8_1X8,

-- 
Regards,

Laurent Pinchart

[PATCH 8/8] media: imx: imx-mipi-csis: Add RGB/BGR888

[Jacopo Mondi]

Add support for the RGB888_1X24 and BGR888_1X24 image formats.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/platform/imx/imx-mipi-csis.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
index 9e0a478dba75..0d5870b3010a 100644
--- a/drivers/media/platform/imx/imx-mipi-csis.c
+++ b/drivers/media/platform/imx/imx-mipi-csis.c
@@ -366,6 +366,16 @@ static const struct csis_pix_format mipi_csis_formats[] = {
 		.data_type = MIPI_CSI2_DATA_TYPE_RGB565,
 		.width = 16,
 	},
+	{
+		.code = MEDIA_BUS_FMT_RGB888_1X24,
+		.data_type = MIPI_CSI2_DATA_TYPE_RGB888,
+		.width = 24,
+	},
+	{
+		.code = MEDIA_BUS_FMT_BGR888_1X24,
+		.data_type = MIPI_CSI2_DATA_TYPE_RGB888,
+		.width = 24,
+	},
 	/* RAW (Bayer and greyscale) formats. */
 	{
 		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
-- 
2.35.0

Re: [PATCH 8/8] media: imx: imx-mipi-csis: Add RGB/BGR888

[Laurent Pinchart]

Hi Jacopo,

(Adding Sakari to the recipients)

Thank you for the patch.

On Mon, Feb 14, 2022 at 07:43:18PM +0100, Jacopo Mondi wrote:
> Add support for the RGB888_1X24 and BGR888_1X24 image formats.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index 9e0a478dba75..0d5870b3010a 100644
> --- a/drivers/media/platform/imx/imx-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -366,6 +366,16 @@ static const struct csis_pix_format mipi_csis_formats[] = {
>  		.data_type = MIPI_CSI2_DATA_TYPE_RGB565,
>  		.width = 16,
>  	},
> +	{

	}, {

to match the rest of the array. Same below.

> +		.code = MEDIA_BUS_FMT_RGB888_1X24,
> +		.data_type = MIPI_CSI2_DATA_TYPE_RGB888,
> +		.width = 24,
> +	},
> +	{
> +		.code = MEDIA_BUS_FMT_BGR888_1X24,
> +		.data_type = MIPI_CSI2_DATA_TYPE_RGB888,
> +		.width = 24,
> +	},

CSI-2 specifies the order of bits on the bus for RGB888, with data being
transmitted in the B, G, R order. The recommended format when stored in
memory is V4L2_PIX_FMT_BGR24 (B in byte 0, G in byte 1, R in byte 2).
There is no recommended deserialized bus format though.

On the output side of the CSIS, we have information. Given figure 13-23
("Pixel alignment") in the i.MX8MP reference manual, and the definition
of RGB_SWAP in MIPI_CSI2_ISP_CONFIG_CH0 that reads

    Swapping RGB sequence

    0  MSB is R and LSB is B.
    1  MSB is B and LSB is R (swapped).

I think MEDIA_BUS_FMT_RGB888_1X24 is appropriate.

On the input side of the CSIS, however, it's a different story, similar
to YUV formats. For YUV 4:2:2 we have picked MEDIA_BUS_FMT_UYVY8_1X16
arbitrarily to represent the CSI-2 bus format. It doesn't correspond to
the physical reality, but it doesn't matter much. We thus need to do the
same for RGB888. If we follow the same convention as for YUV 4:2:2,
which transmits data in the U, Y, V, Y order, we should then pick
BGR888_1X24. However, the CSIS driver would then need to translate from
the input format BGR888_1X24 to the output format RGB888_1X24, which
adds a bit of complexity.

Picking RGB888_1X24 for the CSI-2 bus is thus tempting, but it's a
choice that will affect all drivers, so I wouldn't make this based
solely on ease of implementation in a particular driver. I'm thus
tempted to go for BGR888_1X24 to be consistent with YUV 4:2:2. Another
option would be to add a new RGB888_CSI2 media bus format. In retrospect
we should have done the same for YUV 4:2:2. Mistakes happen.

Sakari, what do you think ?

If we go for BGR888_1X24, the translation between BGR888_1X24 and
RGB888_1X24 may not be that hard to implement. You could add an output
field to the csis_pix_format structure to store the output media bus
code for a given input code, and I think the implementation would then
remain relatively simple.

Finally, we can also support the swapped RGB format (which is
non-standard in CSI-2 but is supported by some sensors, the same way as
YUV permutations are often supported too), but it will require setting
RGB_SWAP. You can add a rgb_swap field to csis_pix_format for this.

I'd split this patch in two, adding MEDIA_BUS_FMT_RGB888_1X24 first, and
then adding MEDIA_BUS_FMT_BGR888_1X24 with the new rgb_swap field. The
first patch should capture the above explanation.

>  	/* RAW (Bayer and greyscale) formats. */
>  	{
>  		.code = MEDIA_BUS_FMT_SBGGR8_1X8,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 8/8] media: imx: imx-mipi-csis: Add RGB/BGR888

[Sakari Ailus]

Hi Laurent, Jacopo,

Thanks for cc'ing me.

On Tue, Feb 15, 2022 at 09:46:26AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> (Adding Sakari to the recipients)
> 
> Thank you for the patch.
> 
> On Mon, Feb 14, 2022 at 07:43:18PM +0100, Jacopo Mondi wrote:
> > Add support for the RGB888_1X24 and BGR888_1X24 image formats.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/platform/imx/imx-mipi-csis.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> > index 9e0a478dba75..0d5870b3010a 100644
> > --- a/drivers/media/platform/imx/imx-mipi-csis.c
> > +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> > @@ -366,6 +366,16 @@ static const struct csis_pix_format mipi_csis_formats[] = {
> >  		.data_type = MIPI_CSI2_DATA_TYPE_RGB565,
> >  		.width = 16,
> >  	},
> > +	{
> 
> 	}, {
> 
> to match the rest of the array. Same below.
> 
> > +		.code = MEDIA_BUS_FMT_RGB888_1X24,
> > +		.data_type = MIPI_CSI2_DATA_TYPE_RGB888,
> > +		.width = 24,
> > +	},
> > +	{
> > +		.code = MEDIA_BUS_FMT_BGR888_1X24,
> > +		.data_type = MIPI_CSI2_DATA_TYPE_RGB888,
> > +		.width = 24,
> > +	},
> 
> CSI-2 specifies the order of bits on the bus for RGB888, with data being
> transmitted in the B, G, R order. The recommended format when stored in
> memory is V4L2_PIX_FMT_BGR24 (B in byte 0, G in byte 1, R in byte 2).
> There is no recommended deserialized bus format though.
> 
> On the output side of the CSIS, we have information. Given figure 13-23
> ("Pixel alignment") in the i.MX8MP reference manual, and the definition
> of RGB_SWAP in MIPI_CSI2_ISP_CONFIG_CH0 that reads
> 
>     Swapping RGB sequence
> 
>     0  MSB is R and LSB is B.
>     1  MSB is B and LSB is R (swapped).
> 
> I think MEDIA_BUS_FMT_RGB888_1X24 is appropriate.
> 
> On the input side of the CSIS, however, it's a different story, similar
> to YUV formats. For YUV 4:2:2 we have picked MEDIA_BUS_FMT_UYVY8_1X16
> arbitrarily to represent the CSI-2 bus format. It doesn't correspond to
> the physical reality, but it doesn't matter much. We thus need to do the
> same for RGB888. If we follow the same convention as for YUV 4:2:2,
> which transmits data in the U, Y, V, Y order, we should then pick
> BGR888_1X24. However, the CSIS driver would then need to translate from
> the input format BGR888_1X24 to the output format RGB888_1X24, which
> adds a bit of complexity.
> 
> Picking RGB888_1X24 for the CSI-2 bus is thus tempting, but it's a
> choice that will affect all drivers, so I wouldn't make this based
> solely on ease of implementation in a particular driver. I'm thus
> tempted to go for BGR888_1X24 to be consistent with YUV 4:2:2. Another
> option would be to add a new RGB888_CSI2 media bus format. In retrospect
> we should have done the same for YUV 4:2:2. Mistakes happen.
> 
> Sakari, what do you think ?

We first started adding support for raw Bayer formats for CSI-2 so at the
time there was little room for confusing the format with another one. Also
few of these formats were eventually used on the parallel bus, making some
of the formats look a little bit artificial.

We've discussed separating the serial bus formats a few times since but
always stuck using single-sample parallel bus format for the serial buses.
As the existing formats will remain as-is, we'd have a mix of differently
named formats returned from an enumeration from increasing number of
drivers. That isn't pretty, but then 24 bit deep Bayer formats won't fit
the mbus format table anyway.

I don't have a strong opinion on this, apart from maintaining the pixel
order. Maybe I'd still create a parallel single-sample format for this one.

If we'd differentiate the formats on CSI-2 bus, I'd just call them
"serial", not "CSI-2".

> 
> If we go for BGR888_1X24, the translation between BGR888_1X24 and
> RGB888_1X24 may not be that hard to implement. You could add an output
> field to the csis_pix_format structure to store the output media bus
> code for a given input code, and I think the implementation would then
> remain relatively simple.
> 
> Finally, we can also support the swapped RGB format (which is
> non-standard in CSI-2 but is supported by some sensors, the same way as
> YUV permutations are often supported too), but it will require setting
> RGB_SWAP. You can add a rgb_swap field to csis_pix_format for this.
> 
> I'd split this patch in two, adding MEDIA_BUS_FMT_RGB888_1X24 first, and
> then adding MEDIA_BUS_FMT_BGR888_1X24 with the new rgb_swap field. The
> first patch should capture the above explanation.
> 
> >  	/* RAW (Bayer and greyscale) formats. */
> >  	{
> >  		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 8/8] media: imx: imx-mipi-csis: Add RGB/BGR888

[Laurent Pinchart]

Hi Sakari,

On Tue, Feb 15, 2022 at 11:07:39PM +0200, Sakari Ailus wrote:
> On Tue, Feb 15, 2022 at 09:46:26AM +0200, Laurent Pinchart wrote:
> > On Mon, Feb 14, 2022 at 07:43:18PM +0100, Jacopo Mondi wrote:
> > > Add support for the RGB888_1X24 and BGR888_1X24 image formats.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  drivers/media/platform/imx/imx-mipi-csis.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> > > index 9e0a478dba75..0d5870b3010a 100644
> > > --- a/drivers/media/platform/imx/imx-mipi-csis.c
> > > +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> > > @@ -366,6 +366,16 @@ static const struct csis_pix_format mipi_csis_formats[] = {
> > >  		.data_type = MIPI_CSI2_DATA_TYPE_RGB565,
> > >  		.width = 16,
> > >  	},
> > > +	{
> > 
> > 	}, {
> > 
> > to match the rest of the array. Same below.
> > 
> > > +		.code = MEDIA_BUS_FMT_RGB888_1X24,
> > > +		.data_type = MIPI_CSI2_DATA_TYPE_RGB888,
> > > +		.width = 24,
> > > +	},
> > > +	{
> > > +		.code = MEDIA_BUS_FMT_BGR888_1X24,
> > > +		.data_type = MIPI_CSI2_DATA_TYPE_RGB888,
> > > +		.width = 24,
> > > +	},
> > 
> > CSI-2 specifies the order of bits on the bus for RGB888, with data being
> > transmitted in the B, G, R order. The recommended format when stored in
> > memory is V4L2_PIX_FMT_BGR24 (B in byte 0, G in byte 1, R in byte 2).
> > There is no recommended deserialized bus format though.
> > 
> > On the output side of the CSIS, we have information. Given figure 13-23
> > ("Pixel alignment") in the i.MX8MP reference manual, and the definition
> > of RGB_SWAP in MIPI_CSI2_ISP_CONFIG_CH0 that reads
> > 
> >     Swapping RGB sequence
> > 
> >     0  MSB is R and LSB is B.
> >     1  MSB is B and LSB is R (swapped).
> > 
> > I think MEDIA_BUS_FMT_RGB888_1X24 is appropriate.
> > 
> > On the input side of the CSIS, however, it's a different story, similar
> > to YUV formats. For YUV 4:2:2 we have picked MEDIA_BUS_FMT_UYVY8_1X16
> > arbitrarily to represent the CSI-2 bus format. It doesn't correspond to
> > the physical reality, but it doesn't matter much. We thus need to do the
> > same for RGB888. If we follow the same convention as for YUV 4:2:2,
> > which transmits data in the U, Y, V, Y order, we should then pick
> > BGR888_1X24. However, the CSIS driver would then need to translate from
> > the input format BGR888_1X24 to the output format RGB888_1X24, which
> > adds a bit of complexity.
> > 
> > Picking RGB888_1X24 for the CSI-2 bus is thus tempting, but it's a
> > choice that will affect all drivers, so I wouldn't make this based
> > solely on ease of implementation in a particular driver. I'm thus
> > tempted to go for BGR888_1X24 to be consistent with YUV 4:2:2. Another
> > option would be to add a new RGB888_CSI2 media bus format. In retrospect
> > we should have done the same for YUV 4:2:2. Mistakes happen.
> > 
> > Sakari, what do you think ?
> 
> We first started adding support for raw Bayer formats for CSI-2 so at the
> time there was little room for confusing the format with another one. Also
> few of these formats were eventually used on the parallel bus, making some
> of the formats look a little bit artificial.
> 
> We've discussed separating the serial bus formats a few times since but
> always stuck using single-sample parallel bus format for the serial buses.
> As the existing formats will remain as-is, we'd have a mix of differently
> named formats returned from an enumeration from increasing number of
> drivers. That isn't pretty, but then 24 bit deep Bayer formats won't fit
> the mbus format table anyway.
> 
> I don't have a strong opinion on this, apart from maintaining the pixel
> order. Maybe I'd still create a parallel single-sample format for this one.

We have MEDIA_BUS_FMT_RGB888_1X24 and MEDIA_BUS_FMT_BGR888_1X24 already.
None of them are intrinsicly better, but I think
MEDIA_BUS_FMT_BGR888_1X24 would match the convention used by
MEDIA_BUS_FMT_UYVY8_1X16, so I would pick that one. Ack ?

> If we'd differentiate the formats on CSI-2 bus, I'd just call them
> "serial", not "CSI-2".

Good point. We would have to figure out how to differentiate the two
ways to transfer YUV 4:2:0 (legacy and non-legacy) over CSI-2 though,
maybe the legacy way could be named using CSI-2, while the non-legacy
way would be named serial and shared with other buses. We don't have to
solve it now.

> > If we go for BGR888_1X24, the translation between BGR888_1X24 and
> > RGB888_1X24 may not be that hard to implement. You could add an output
> > field to the csis_pix_format structure to store the output media bus
> > code for a given input code, and I think the implementation would then
> > remain relatively simple.
> > 
> > Finally, we can also support the swapped RGB format (which is
> > non-standard in CSI-2 but is supported by some sensors, the same way as
> > YUV permutations are often supported too), but it will require setting
> > RGB_SWAP. You can add a rgb_swap field to csis_pix_format for this.
> > 
> > I'd split this patch in two, adding MEDIA_BUS_FMT_RGB888_1X24 first, and
> > then adding MEDIA_BUS_FMT_BGR888_1X24 with the new rgb_swap field. The
> > first patch should capture the above explanation.
> > 
> > >  	/* RAW (Bayer and greyscale) formats. */
> > >  	{
> > >  		.code = MEDIA_BUS_FMT_SBGGR8_1X8,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 8/8] media: imx: imx-mipi-csis: Add RGB/BGR888

[Sakari Ailus]

Hi Laurent,

On Tue, Feb 15, 2022 at 11:52:44PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tue, Feb 15, 2022 at 11:07:39PM +0200, Sakari Ailus wrote:
> > On Tue, Feb 15, 2022 at 09:46:26AM +0200, Laurent Pinchart wrote:
> > > On Mon, Feb 14, 2022 at 07:43:18PM +0100, Jacopo Mondi wrote:
> > > > Add support for the RGB888_1X24 and BGR888_1X24 image formats.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  drivers/media/platform/imx/imx-mipi-csis.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> > > > index 9e0a478dba75..0d5870b3010a 100644
> > > > --- a/drivers/media/platform/imx/imx-mipi-csis.c
> > > > +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> > > > @@ -366,6 +366,16 @@ static const struct csis_pix_format mipi_csis_formats[] = {
> > > >  		.data_type = MIPI_CSI2_DATA_TYPE_RGB565,
> > > >  		.width = 16,
> > > >  	},
> > > > +	{
> > > 
> > > 	}, {
> > > 
> > > to match the rest of the array. Same below.
> > > 
> > > > +		.code = MEDIA_BUS_FMT_RGB888_1X24,
> > > > +		.data_type = MIPI_CSI2_DATA_TYPE_RGB888,
> > > > +		.width = 24,
> > > > +	},
> > > > +	{
> > > > +		.code = MEDIA_BUS_FMT_BGR888_1X24,
> > > > +		.data_type = MIPI_CSI2_DATA_TYPE_RGB888,
> > > > +		.width = 24,
> > > > +	},
> > > 
> > > CSI-2 specifies the order of bits on the bus for RGB888, with data being
> > > transmitted in the B, G, R order. The recommended format when stored in
> > > memory is V4L2_PIX_FMT_BGR24 (B in byte 0, G in byte 1, R in byte 2).
> > > There is no recommended deserialized bus format though.
> > > 
> > > On the output side of the CSIS, we have information. Given figure 13-23
> > > ("Pixel alignment") in the i.MX8MP reference manual, and the definition
> > > of RGB_SWAP in MIPI_CSI2_ISP_CONFIG_CH0 that reads
> > > 
> > >     Swapping RGB sequence
> > > 
> > >     0  MSB is R and LSB is B.
> > >     1  MSB is B and LSB is R (swapped).
> > > 
> > > I think MEDIA_BUS_FMT_RGB888_1X24 is appropriate.
> > > 
> > > On the input side of the CSIS, however, it's a different story, similar
> > > to YUV formats. For YUV 4:2:2 we have picked MEDIA_BUS_FMT_UYVY8_1X16
> > > arbitrarily to represent the CSI-2 bus format. It doesn't correspond to
> > > the physical reality, but it doesn't matter much. We thus need to do the
> > > same for RGB888. If we follow the same convention as for YUV 4:2:2,
> > > which transmits data in the U, Y, V, Y order, we should then pick
> > > BGR888_1X24. However, the CSIS driver would then need to translate from
> > > the input format BGR888_1X24 to the output format RGB888_1X24, which
> > > adds a bit of complexity.
> > > 
> > > Picking RGB888_1X24 for the CSI-2 bus is thus tempting, but it's a
> > > choice that will affect all drivers, so I wouldn't make this based
> > > solely on ease of implementation in a particular driver. I'm thus
> > > tempted to go for BGR888_1X24 to be consistent with YUV 4:2:2. Another
> > > option would be to add a new RGB888_CSI2 media bus format. In retrospect
> > > we should have done the same for YUV 4:2:2. Mistakes happen.
> > > 
> > > Sakari, what do you think ?
> > 
> > We first started adding support for raw Bayer formats for CSI-2 so at the
> > time there was little room for confusing the format with another one. Also
> > few of these formats were eventually used on the parallel bus, making some
> > of the formats look a little bit artificial.
> > 
> > We've discussed separating the serial bus formats a few times since but
> > always stuck using single-sample parallel bus format for the serial buses.
> > As the existing formats will remain as-is, we'd have a mix of differently
> > named formats returned from an enumeration from increasing number of
> > drivers. That isn't pretty, but then 24 bit deep Bayer formats won't fit
> > the mbus format table anyway.
> > 
> > I don't have a strong opinion on this, apart from maintaining the pixel
> > order. Maybe I'd still create a parallel single-sample format for this one.
> 
> We have MEDIA_BUS_FMT_RGB888_1X24 and MEDIA_BUS_FMT_BGR888_1X24 already.
> None of them are intrinsicly better, but I think
> MEDIA_BUS_FMT_BGR888_1X24 would match the convention used by
> MEDIA_BUS_FMT_UYVY8_1X16, so I would pick that one. Ack ?

Agreed.

> > If we'd differentiate the formats on CSI-2 bus, I'd just call them
> > "serial", not "CSI-2".
> 
> Good point. We would have to figure out how to differentiate the two
> ways to transfer YUV 4:2:0 (legacy and non-legacy) over CSI-2 though,
> maybe the legacy way could be named using CSI-2, while the non-legacy
> way would be named serial and shared with other buses. We don't have to
> solve it now.

Sounds good to me.

> > > If we go for BGR888_1X24, the translation between BGR888_1X24 and
> > > RGB888_1X24 may not be that hard to implement. You could add an output
> > > field to the csis_pix_format structure to store the output media bus
> > > code for a given input code, and I think the implementation would then
> > > remain relatively simple.
> > > 
> > > Finally, we can also support the swapped RGB format (which is
> > > non-standard in CSI-2 but is supported by some sensors, the same way as
> > > YUV permutations are often supported too), but it will require setting
> > > RGB_SWAP. You can add a rgb_swap field to csis_pix_format for this.
> > > 
> > > I'd split this patch in two, adding MEDIA_BUS_FMT_RGB888_1X24 first, and
> > > then adding MEDIA_BUS_FMT_BGR888_1X24 with the new rgb_swap field. The
> > > first patch should capture the above explanation.
> > > 
> > > >  	/* RAW (Bayer and greyscale) formats. */
> > > >  	{
> > > >  		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Sakari Ailus