[PATCH 00/14] media: imx: Miscellaneous fixes for i.MX7 CSI-2 receiver

[PATCH 00/14] media: imx: Miscellaneous fixes for i.MX7 CSI-2 receiver

[Laurent Pinchart]

Hello,

This patch series completes (for now :-)) my rework of the CSI-2
receiver found in the i.MX7 SoCs.

Patches 01/14 and 02/14 have already been tested and acked. The next
four patches (03/14 to 06/14) fix format handling by adding missing
formats and removing unsupported formats, and patches 07/14 to 09/14
then clean up and fix image width alignment handling.

The next three patches (10/14 to 12/14) are again miscellaneous
cleanups. Patch 13/14 removes usage of the only imx-media-utils helper
used in this driver as the helper isn't compatible with the i.MX7 CSI-2
receiver formats. Patch 14/14 finally implements the subdev
.enum_mbus_code() pad operation to allow enumeration of media bus codes
from userspace.

The patches have been tested on an i.MX7Q with a 10-bit greyscale MIPI
CSI-2 sensor.

Laurent Pinchart (14):
  media: imx: imx7-mipi-csis: Cleanup and fix subdev pad format handling
  media: imx: imx7-mipi-csis: Centralize initialization of pad formats
  media: imx: imx7-mipi-csis: Add missing RAW formats
  media: imx: imx7-mipi-csis: Expose correct YUV formats
  media: imx: imx7-mipi-csis: Fix MEDIA_BUS_FMT_UYVY8_2X8 data alignment
  media: imx: imx7-mipi-csis: Add MEDIA_BUS_FMT_UYVY10_2X10 support
  media: imx: imx7-mipi-csis: Rename data_alignment field to width
  media: imx: imx7-mipi-csis: Align image width based on format
  media: imx: imx7-mipi-csis: Never set MIPI_CSIS_ISPCFG_ALIGN_32BIT
  media: imx: imx7-mipi-csis: Align macro definitions
  media: imx: imx7-mipi-csis: Remove link setup on source pad
  media: imx: imx7-mipi-csis: Cleanup includes
  media: imx: imx7-mipi-csis: Don't use imx-media-utils helpers
  media: imx: imx7-mipi-csis: Implement the .enum_mbus_code() operation

 drivers/staging/media/imx/imx7-mipi-csis.c | 446 +++++++++++++--------
 1 file changed, 274 insertions(+), 172 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 01/14] media: imx: imx7-mipi-csis: Cleanup and fix subdev pad format handling

[Laurent Pinchart]

The subdev set pad format operation currently misbehaves in multiple ways:

- mipi_csis_try_format() unconditionally stores the format in the device
  state, even for V4L2_SUBDEV_FORMAT_TRY.

- The format is never stored in the pad cfg, but the pad cfg format
  always overwrites the format requested by the user.

- The sink format is not propagated to the source.

Fix all this by reworking the set format operation as follows:

1. For the source pad, turn set() into get() as the source format is not
   modifiable.
2. Validate the requested format and updated the stored format
   accordingly.
3. Return the format actually set.
4. Propagate the format from sink to source.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 82 ++++++++++------------
 1 file changed, 37 insertions(+), 45 deletions(-)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index de17a1d22873..25017ab78095 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -669,28 +669,6 @@ static int mipi_csis_init_cfg(struct v4l2_subdev *mipi_sd,
 	return 0;
 }
 
-static struct csis_pix_format const *
-mipi_csis_try_format(struct v4l2_subdev *mipi_sd, struct v4l2_mbus_framefmt *mf)
-{
-	struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
-	struct csis_pix_format const *csis_fmt;
-
-	csis_fmt = find_csis_format(mf->code);
-	if (!csis_fmt)
-		csis_fmt = &mipi_csis_formats[0];
-
-	v4l_bound_align_image(&mf->width, 1, CSIS_MAX_PIX_WIDTH,
-			      csis_fmt->pix_width_alignment,
-			      &mf->height, 1, CSIS_MAX_PIX_HEIGHT, 1,
-			      0);
-
-	state->format_mbus.code = csis_fmt->code;
-	state->format_mbus.width = mf->width;
-	state->format_mbus.height = mf->height;
-
-	return csis_fmt;
-}
-
 static struct v4l2_mbus_framefmt *
 mipi_csis_get_format(struct csi_state *state,
 		     struct v4l2_subdev_pad_config *cfg,
@@ -703,53 +681,67 @@ mipi_csis_get_format(struct csi_state *state,
 	return &state->format_mbus;
 }
 
-static int mipi_csis_set_fmt(struct v4l2_subdev *mipi_sd,
+static int mipi_csis_get_fmt(struct v4l2_subdev *mipi_sd,
 			     struct v4l2_subdev_pad_config *cfg,
 			     struct v4l2_subdev_format *sdformat)
 {
 	struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
-	struct csis_pix_format const *csis_fmt;
 	struct v4l2_mbus_framefmt *fmt;
 
-	if (sdformat->pad >= CSIS_PADS_NUM)
-		return -EINVAL;
-
-	fmt = mipi_csis_get_format(state, cfg, sdformat->which, sdformat->pad);
-
 	mutex_lock(&state->lock);
-	if (sdformat->pad == CSIS_PAD_SOURCE) {
-		sdformat->format = *fmt;
-		goto unlock;
-	}
-
-	csis_fmt = mipi_csis_try_format(mipi_sd, &sdformat->format);
-
+	fmt = mipi_csis_get_format(state, cfg, sdformat->which, sdformat->pad);
 	sdformat->format = *fmt;
-
-	if (csis_fmt && sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		state->csis_fmt = csis_fmt;
-	else
-		cfg->try_fmt = sdformat->format;
-
-unlock:
 	mutex_unlock(&state->lock);
 
 	return 0;
 }
 
-static int mipi_csis_get_fmt(struct v4l2_subdev *mipi_sd,
+static int mipi_csis_set_fmt(struct v4l2_subdev *mipi_sd,
 			     struct v4l2_subdev_pad_config *cfg,
 			     struct v4l2_subdev_format *sdformat)
 {
 	struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
+	struct csis_pix_format const *csis_fmt;
 	struct v4l2_mbus_framefmt *fmt;
 
-	mutex_lock(&state->lock);
+	/*
+	 * The CSIS can't transcode in any way, the source format can't be
+	 * modified.
+	 */
+	if (sdformat->pad == CSIS_PAD_SOURCE)
+		return mipi_csis_get_fmt(mipi_sd, cfg, sdformat);
+
+	if (sdformat->pad != CSIS_PAD_SINK)
+		return -EINVAL;
 
 	fmt = mipi_csis_get_format(state, cfg, sdformat->which, sdformat->pad);
 
+	mutex_lock(&state->lock);
+
+	/* Validate the media bus code and clamp the size. */
+	csis_fmt = find_csis_format(sdformat->format.code);
+	if (!csis_fmt)
+		csis_fmt = &mipi_csis_formats[0];
+
+	fmt->code = csis_fmt->code;
+	fmt->width = sdformat->format.width;
+	fmt->height = sdformat->format.height;
+
+	v4l_bound_align_image(&fmt->width, 1, CSIS_MAX_PIX_WIDTH,
+			      csis_fmt->pix_width_alignment,
+			      &fmt->height, 1, CSIS_MAX_PIX_HEIGHT, 1, 0);
+
 	sdformat->format = *fmt;
 
+	/* Propagate the format from sink to source. */
+	fmt = mipi_csis_get_format(state, cfg, sdformat->which,
+				   CSIS_PAD_SOURCE);
+	*fmt = sdformat->format;
+
+	/* Store the CSIS format descriptor for active formats. */
+	if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		state->csis_fmt = csis_fmt;
+
 	mutex_unlock(&state->lock);
 
 	return 0;
-- 
Regards,

Laurent Pinchart

[PATCH 02/14] media: imx: imx7-mipi-csis: Centralize initialization of pad formats

[Laurent Pinchart]

Pad formats for the active configuration are manually initialized in
mipi_csis_subdev_init(), while pad formats for the TRY configurations
are initialized by the subdev .init_cfg() operation. This creates a risk
of the two configurations not being synchronized. Fix it by initializing
formats in the .init_cfg() operation only, and calling it from
mipi_csis_subdev_init().

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 56 ++++++++++++----------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 25017ab78095..52d59047ee36 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -649,26 +649,6 @@ static int mipi_csis_link_setup(struct media_entity *entity,
 	return ret;
 }
 
-static int mipi_csis_init_cfg(struct v4l2_subdev *mipi_sd,
-			      struct v4l2_subdev_pad_config *cfg)
-{
-	struct v4l2_mbus_framefmt *mf;
-	unsigned int i;
-	int ret;
-
-	for (i = 0; i < CSIS_PADS_NUM; i++) {
-		mf = v4l2_subdev_get_try_format(mipi_sd, cfg, i);
-
-		ret = imx_media_init_mbus_fmt(mf, MIPI_CSIS_DEF_PIX_HEIGHT,
-					      MIPI_CSIS_DEF_PIX_WIDTH, 0,
-					      V4L2_FIELD_NONE, NULL);
-		if (ret < 0)
-			return ret;
-	}
-
-	return 0;
-}
-
 static struct v4l2_mbus_framefmt *
 mipi_csis_get_format(struct csi_state *state,
 		     struct v4l2_subdev_pad_config *cfg,
@@ -681,6 +661,37 @@ mipi_csis_get_format(struct csi_state *state,
 	return &state->format_mbus;
 }
 
+static int mipi_csis_init_cfg(struct v4l2_subdev *mipi_sd,
+			      struct v4l2_subdev_pad_config *cfg)
+{
+	struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
+	struct v4l2_mbus_framefmt *fmt_sink;
+	struct v4l2_mbus_framefmt *fmt_source;
+	enum v4l2_subdev_format_whence which;
+	int ret;
+
+	which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
+	fmt_sink = mipi_csis_get_format(state, cfg, which, CSIS_PAD_SINK);
+	ret = imx_media_init_mbus_fmt(fmt_sink, MIPI_CSIS_DEF_PIX_WIDTH,
+				      MIPI_CSIS_DEF_PIX_HEIGHT, 0,
+				      V4L2_FIELD_NONE, NULL);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * When called from mipi_csis_subdev_init() to initialize the active
+	 * configuration, cfg is NULL, which indicates there's no source pad
+	 * configuration to set.
+	 */
+	if (!cfg)
+		return 0;
+
+	fmt_source = mipi_csis_get_format(state, cfg, which, CSIS_PAD_SOURCE);
+	*fmt_source = *fmt_sink;
+
+	return 0;
+}
+
 static int mipi_csis_get_fmt(struct v4l2_subdev *mipi_sd,
 			     struct v4l2_subdev_pad_config *cfg,
 			     struct v4l2_subdev_format *sdformat)
@@ -875,10 +886,7 @@ static int mipi_csis_subdev_init(struct v4l2_subdev *mipi_sd,
 	mipi_sd->dev = &pdev->dev;
 
 	state->csis_fmt = &mipi_csis_formats[0];
-	state->format_mbus.code = mipi_csis_formats[0].code;
-	state->format_mbus.width = MIPI_CSIS_DEF_PIX_WIDTH;
-	state->format_mbus.height = MIPI_CSIS_DEF_PIX_HEIGHT;
-	state->format_mbus.field = V4L2_FIELD_NONE;
+	mipi_csis_init_cfg(mipi_sd, NULL);
 
 	v4l2_set_subdevdata(mipi_sd, &pdev->dev);
 
-- 
Regards,

Laurent Pinchart

[PATCH 03/14] media: imx: imx7-mipi-csis: Add missing RAW formats

[Laurent Pinchart]

Add support for all the missing 8-, 10-, 12- and 14-bit RAW formats.
This include all Bayer combinations, as well as greyscale. No media bus
code exist for Y14 so this is currently left out.

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

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 52d59047ee36..dec144d327b5 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -143,6 +143,7 @@
 #define MIPI_CSIS_ISPCFG_FMT_RAW8		(0x2a << 2)
 #define MIPI_CSIS_ISPCFG_FMT_RAW10		(0x2b << 2)
 #define MIPI_CSIS_ISPCFG_FMT_RAW12		(0x2c << 2)
+#define MIPI_CSIS_ISPCFG_FMT_RAW14		(0x2d << 2)
 
 /* User defined formats, x = 1...4 */
 #define MIPI_CSIS_ISPCFG_FMT_USER(x)	((0x30 + (x) - 1) << 2)
@@ -264,34 +265,93 @@ struct csis_pix_format {
 };
 
 static const struct csis_pix_format mipi_csis_formats[] = {
+	/* YUV formats. */
 	{
-		.code = MEDIA_BUS_FMT_SBGGR10_1X10,
-		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW10,
-		.data_alignment = 16,
-	}, {
 		.code = MEDIA_BUS_FMT_VYUY8_2X8,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_YCBCR422_8BIT,
 		.data_alignment = 16,
 	}, {
+		.code = MEDIA_BUS_FMT_YUYV8_2X8,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_YCBCR422_8BIT,
+		.data_alignment = 16,
+	},
+	/* RAW (Bayer and greyscale) formats. */
+	{
 		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW8,
 		.data_alignment = 8,
 	}, {
-		.code = MEDIA_BUS_FMT_YUYV8_2X8,
-		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_YCBCR422_8BIT,
-		.data_alignment = 16,
+		.code = MEDIA_BUS_FMT_SGBRG8_1X8,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW8,
+		.data_alignment = 8,
+	}, {
+		.code = MEDIA_BUS_FMT_SGRBG8_1X8,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW8,
+		.data_alignment = 8,
+	}, {
+		.code = MEDIA_BUS_FMT_SRGGB8_1X8,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW8,
+		.data_alignment = 8,
 	}, {
 		.code = MEDIA_BUS_FMT_Y8_1X8,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW8,
 		.data_alignment = 8,
+	}, {
+		.code = MEDIA_BUS_FMT_SBGGR10_1X10,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW10,
+		.data_alignment = 10,
+	}, {
+		.code = MEDIA_BUS_FMT_SGBRG10_1X10,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW10,
+		.data_alignment = 10,
+	}, {
+		.code = MEDIA_BUS_FMT_SGRBG10_1X10,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW10,
+		.data_alignment = 10,
+	}, {
+		.code = MEDIA_BUS_FMT_SRGGB10_1X10,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW10,
+		.data_alignment = 10,
 	}, {
 		.code = MEDIA_BUS_FMT_Y10_1X10,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW10,
-		.data_alignment = 16,
+		.data_alignment = 10,
+	}, {
+		.code = MEDIA_BUS_FMT_SBGGR12_1X12,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW12,
+		.data_alignment = 12,
+	}, {
+		.code = MEDIA_BUS_FMT_SGBRG12_1X12,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW12,
+		.data_alignment = 12,
+	}, {
+		.code = MEDIA_BUS_FMT_SGRBG12_1X12,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW12,
+		.data_alignment = 12,
+	}, {
+		.code = MEDIA_BUS_FMT_SRGGB12_1X12,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW12,
+		.data_alignment = 12,
 	}, {
 		.code = MEDIA_BUS_FMT_Y12_1X12,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW12,
-		.data_alignment = 16,
+		.data_alignment = 12,
+	}, {
+		.code = MEDIA_BUS_FMT_SBGGR14_1X14,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW14,
+		.data_alignment = 14,
+	}, {
+		.code = MEDIA_BUS_FMT_SGBRG14_1X14,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW14,
+		.data_alignment = 14,
+	}, {
+		.code = MEDIA_BUS_FMT_SGRBG14_1X14,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW14,
+		.data_alignment = 14,
+	}, {
+		.code = MEDIA_BUS_FMT_SRGGB14_1X14,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW14,
+		.data_alignment = 14,
 	}
 };
 
-- 
Regards,

Laurent Pinchart

[PATCH 04/14] media: imx: imx7-mipi-csis: Expose correct YUV formats

[Laurent Pinchart]

The imx7-mipi-csis driver claims to support MEDIA_BUS_FMT_VYUY8_2X8 and
MEDIA_BUS_FMT_YUYV8_2X8, but this is not correct. When receiving
YUV 4:2:2 data on the CSI-2 bus, the output format is
MEDIA_BUS_FMT_UYVY8_2X8. Fix this.

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

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index dec144d327b5..7515f1022271 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -267,11 +267,7 @@ struct csis_pix_format {
 static const struct csis_pix_format mipi_csis_formats[] = {
 	/* YUV formats. */
 	{
-		.code = MEDIA_BUS_FMT_VYUY8_2X8,
-		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_YCBCR422_8BIT,
-		.data_alignment = 16,
-	}, {
-		.code = MEDIA_BUS_FMT_YUYV8_2X8,
+		.code = MEDIA_BUS_FMT_UYVY8_2X8,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_YCBCR422_8BIT,
 		.data_alignment = 16,
 	},
-- 
Regards,

Laurent Pinchart

[PATCH 05/14] media: imx: imx7-mipi-csis: Fix MEDIA_BUS_FMT_UYVY8_2X8 data alignment

[Laurent Pinchart]

The MEDIA_BUS_FMT_UYVY8_2X8 format reports a data alignment of 16 bits,
which isn't correct as it is output on an 8-bit bus. Fix it.

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

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 7515f1022271..ffb1c87532f1 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -269,7 +269,7 @@ static const struct csis_pix_format mipi_csis_formats[] = {
 	{
 		.code = MEDIA_BUS_FMT_UYVY8_2X8,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_YCBCR422_8BIT,
-		.data_alignment = 16,
+		.data_alignment = 8,
 	},
 	/* RAW (Bayer and greyscale) formats. */
 	{
-- 
Regards,

Laurent Pinchart

[PATCH 06/14] media: imx: imx7-mipi-csis: Add MEDIA_BUS_FMT_UYVY10_2X10 support

[Laurent Pinchart]

Add support for 10-bit YUV 4:2:2.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index ffb1c87532f1..d8521e2b8a4f 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -270,6 +270,10 @@ static const struct csis_pix_format mipi_csis_formats[] = {
 		.code = MEDIA_BUS_FMT_UYVY8_2X8,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_YCBCR422_8BIT,
 		.data_alignment = 8,
+	}, {
+		.code = MEDIA_BUS_FMT_UYVY10_2X10,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_YCBCR422_8BIT,
+		.data_alignment = 10,
 	},
 	/* RAW (Bayer and greyscale) formats. */
 	{
-- 
Regards,

Laurent Pinchart

[PATCH 07/14] media: imx: imx7-mipi-csis: Rename data_alignment field to width

[Laurent Pinchart]

The csis_pix_format data_alignment field stores the bus width. Rename it
accordingly.

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

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index d8521e2b8a4f..c664895fb780 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -261,7 +261,7 @@ struct csis_pix_format {
 	unsigned int pix_width_alignment;
 	u32 code;
 	u32 fmt_reg;
-	u8 data_alignment;
+	u8 width;
 };
 
 static const struct csis_pix_format mipi_csis_formats[] = {
@@ -269,89 +269,89 @@ static const struct csis_pix_format mipi_csis_formats[] = {
 	{
 		.code = MEDIA_BUS_FMT_UYVY8_2X8,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_YCBCR422_8BIT,
-		.data_alignment = 8,
+		.width = 8,
 	}, {
 		.code = MEDIA_BUS_FMT_UYVY10_2X10,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_YCBCR422_8BIT,
-		.data_alignment = 10,
+		.width = 10,
 	},
 	/* RAW (Bayer and greyscale) formats. */
 	{
 		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW8,
-		.data_alignment = 8,
+		.width = 8,
 	}, {
 		.code = MEDIA_BUS_FMT_SGBRG8_1X8,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW8,
-		.data_alignment = 8,
+		.width = 8,
 	}, {
 		.code = MEDIA_BUS_FMT_SGRBG8_1X8,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW8,
-		.data_alignment = 8,
+		.width = 8,
 	}, {
 		.code = MEDIA_BUS_FMT_SRGGB8_1X8,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW8,
-		.data_alignment = 8,
+		.width = 8,
 	}, {
 		.code = MEDIA_BUS_FMT_Y8_1X8,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW8,
-		.data_alignment = 8,
+		.width = 8,
 	}, {
 		.code = MEDIA_BUS_FMT_SBGGR10_1X10,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW10,
-		.data_alignment = 10,
+		.width = 10,
 	}, {
 		.code = MEDIA_BUS_FMT_SGBRG10_1X10,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW10,
-		.data_alignment = 10,
+		.width = 10,
 	}, {
 		.code = MEDIA_BUS_FMT_SGRBG10_1X10,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW10,
-		.data_alignment = 10,
+		.width = 10,
 	}, {
 		.code = MEDIA_BUS_FMT_SRGGB10_1X10,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW10,
-		.data_alignment = 10,
+		.width = 10,
 	}, {
 		.code = MEDIA_BUS_FMT_Y10_1X10,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW10,
-		.data_alignment = 10,
+		.width = 10,
 	}, {
 		.code = MEDIA_BUS_FMT_SBGGR12_1X12,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW12,
-		.data_alignment = 12,
+		.width = 12,
 	}, {
 		.code = MEDIA_BUS_FMT_SGBRG12_1X12,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW12,
-		.data_alignment = 12,
+		.width = 12,
 	}, {
 		.code = MEDIA_BUS_FMT_SGRBG12_1X12,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW12,
-		.data_alignment = 12,
+		.width = 12,
 	}, {
 		.code = MEDIA_BUS_FMT_SRGGB12_1X12,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW12,
-		.data_alignment = 12,
+		.width = 12,
 	}, {
 		.code = MEDIA_BUS_FMT_Y12_1X12,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW12,
-		.data_alignment = 12,
+		.width = 12,
 	}, {
 		.code = MEDIA_BUS_FMT_SBGGR14_1X14,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW14,
-		.data_alignment = 14,
+		.width = 14,
 	}, {
 		.code = MEDIA_BUS_FMT_SGBRG14_1X14,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW14,
-		.data_alignment = 14,
+		.width = 14,
 	}, {
 		.code = MEDIA_BUS_FMT_SGRBG14_1X14,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW14,
-		.data_alignment = 14,
+		.width = 14,
 	}, {
 		.code = MEDIA_BUS_FMT_SRGGB14_1X14,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW14,
-		.data_alignment = 14,
+		.width = 14,
 	}
 };
 
@@ -498,7 +498,7 @@ static void mipi_csis_set_params(struct csi_state *state)
 	mipi_csis_set_hsync_settle(state, state->hs_settle);
 
 	val = mipi_csis_read(state, MIPI_CSIS_ISPCONFIG_CH0);
-	if (state->csis_fmt->data_alignment == 32)
+	if (state->csis_fmt->width == 32)
 		val |= MIPI_CSIS_ISPCFG_ALIGN_32BIT;
 	else
 		val &= ~MIPI_CSIS_ISPCFG_ALIGN_32BIT;
-- 
Regards,

Laurent Pinchart

[PATCH 08/14] media: imx: imx7-mipi-csis: Align image width based on format

[Laurent Pinchart]

The total number of bits per line needs to be a multiple of 8, which
requires aligning the image width based on the format width. The
csis_pix_format structure contains a pix_width_alignment field that
serves this purpose, but the field is never set. Instead of fixing that,
calculate the alignment constraints based on the bus width for the
format, and drop the unneeded pix_width_alignment field.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 29 +++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index c664895fb780..ef5fabfcdada 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -258,7 +258,6 @@ struct csi_state {
 };
 
 struct csis_pix_format {
-	unsigned int pix_width_alignment;
 	u32 code;
 	u32 fmt_reg;
 	u8 width;
@@ -774,6 +773,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *mipi_sd,
 	struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
 	struct csis_pix_format const *csis_fmt;
 	struct v4l2_mbus_framefmt *fmt;
+	unsigned int align;
 
 	/*
 	 * The CSIS can't transcode in any way, the source format can't be
@@ -798,8 +798,31 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *mipi_sd,
 	fmt->width = sdformat->format.width;
 	fmt->height = sdformat->format.height;
 
-	v4l_bound_align_image(&fmt->width, 1, CSIS_MAX_PIX_WIDTH,
-			      csis_fmt->pix_width_alignment,
+	/*
+	 * The total number of bits per line must be a multiple of 8. We thus
+	 * need to align the width for formats that are not multiples of 8
+	 * bits.
+	 */
+	switch (csis_fmt->width % 8) {
+	case 0:
+		align = 1;
+		break;
+	case 4:
+		align = 2;
+		break;
+	case 2:
+	case 6:
+		align = 4;
+		break;
+	case 1:
+	case 3:
+	case 5:
+	case 7:
+		align = 8;
+		break;
+	}
+
+	v4l_bound_align_image(&fmt->width, 1, CSIS_MAX_PIX_WIDTH, align,
 			      &fmt->height, 1, CSIS_MAX_PIX_HEIGHT, 1, 0);
 
 	sdformat->format = *fmt;
-- 
Regards,

Laurent Pinchart

[PATCH 09/14] media: imx: imx7-mipi-csis: Never set MIPI_CSIS_ISPCFG_ALIGN_32BIT

[Laurent Pinchart]

The MIPI_CSIS_ISPCFG_ALIGN_32BIT bit enables output of 32-bit data. The
driver sets it based on the select format, but no format uses a 32-bit
bus width, so the bit is never set in practice. This isn't likely to
change any time soon, as the CSI IP core connected at the output of the
CSIS doesn't support 32-bit data width. Hardcode the bit to 0.

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

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index ef5fabfcdada..922657b856b6 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -464,7 +464,8 @@ static void __mipi_csis_set_format(struct csi_state *state)
 
 	/* Color format */
 	val = mipi_csis_read(state, MIPI_CSIS_ISPCONFIG_CH0);
-	val = (val & ~MIPI_CSIS_ISPCFG_FMT_MASK) | state->csis_fmt->fmt_reg;
+	val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
+	val |= state->csis_fmt->fmt_reg;
 	mipi_csis_write(state, MIPI_CSIS_ISPCONFIG_CH0, val);
 
 	/* Pixel resolution */
@@ -496,13 +497,6 @@ static void mipi_csis_set_params(struct csi_state *state)
 
 	mipi_csis_set_hsync_settle(state, state->hs_settle);
 
-	val = mipi_csis_read(state, MIPI_CSIS_ISPCONFIG_CH0);
-	if (state->csis_fmt->width == 32)
-		val |= MIPI_CSIS_ISPCFG_ALIGN_32BIT;
-	else
-		val &= ~MIPI_CSIS_ISPCFG_ALIGN_32BIT;
-	mipi_csis_write(state, MIPI_CSIS_ISPCONFIG_CH0, val);
-
 	val = (0 << MIPI_CSIS_ISPSYNC_HSYNC_LINTV_OFFSET) |
 		(0 << MIPI_CSIS_ISPSYNC_VSYNC_SINTV_OFFSET) |
 		(0 << MIPI_CSIS_ISPSYNC_VSYNC_EINTV_OFFSET);
-- 
Regards,

Laurent Pinchart

[PATCH 10/14] media: imx: imx7-mipi-csis: Align macro definitions

[Laurent Pinchart]

The register macros at the top of the file have their value not aligned
on the same column, hindering readability. Fix it.

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

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 922657b856b6..40004a98f801 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -31,15 +31,15 @@
 
 #include "imx-media.h"
 
-#define CSIS_DRIVER_NAME	"imx7-mipi-csis"
-#define CSIS_SUBDEV_NAME	CSIS_DRIVER_NAME
+#define CSIS_DRIVER_NAME			"imx7-mipi-csis"
+#define CSIS_SUBDEV_NAME			CSIS_DRIVER_NAME
 
-#define CSIS_PAD_SINK		0
-#define CSIS_PAD_SOURCE		1
-#define CSIS_PADS_NUM		2
+#define CSIS_PAD_SINK				0
+#define CSIS_PAD_SOURCE				1
+#define CSIS_PADS_NUM				2
 
-#define MIPI_CSIS_DEF_PIX_WIDTH		640
-#define MIPI_CSIS_DEF_PIX_HEIGHT	480
+#define MIPI_CSIS_DEF_PIX_WIDTH			640
+#define MIPI_CSIS_DEF_PIX_HEIGHT		480
 
 /* Register map definition */
 
@@ -64,42 +64,42 @@
 #define MIPI_CSIS_CLK_CTRL_WCLK_SRC		BIT(0)
 
 /* CSIS Interrupt mask */
-#define MIPI_CSIS_INTMSK		0x10
-#define MIPI_CSIS_INTMSK_EVEN_BEFORE	BIT(31)
-#define MIPI_CSIS_INTMSK_EVEN_AFTER	BIT(30)
-#define MIPI_CSIS_INTMSK_ODD_BEFORE	BIT(29)
-#define MIPI_CSIS_INTMSK_ODD_AFTER	BIT(28)
-#define MIPI_CSIS_INTMSK_FRAME_START	BIT(24)
-#define MIPI_CSIS_INTMSK_FRAME_END	BIT(20)
-#define MIPI_CSIS_INTMSK_ERR_SOT_HS	BIT(16)
-#define MIPI_CSIS_INTMSK_ERR_LOST_FS	BIT(12)
-#define MIPI_CSIS_INTMSK_ERR_LOST_FE	BIT(8)
-#define MIPI_CSIS_INTMSK_ERR_OVER	BIT(4)
-#define MIPI_CSIS_INTMSK_ERR_WRONG_CFG	BIT(3)
-#define MIPI_CSIS_INTMSK_ERR_ECC	BIT(2)
-#define MIPI_CSIS_INTMSK_ERR_CRC	BIT(1)
-#define MIPI_CSIS_INTMSK_ERR_UNKNOWN	BIT(0)
+#define MIPI_CSIS_INTMSK			0x10
+#define MIPI_CSIS_INTMSK_EVEN_BEFORE		BIT(31)
+#define MIPI_CSIS_INTMSK_EVEN_AFTER		BIT(30)
+#define MIPI_CSIS_INTMSK_ODD_BEFORE		BIT(29)
+#define MIPI_CSIS_INTMSK_ODD_AFTER		BIT(28)
+#define MIPI_CSIS_INTMSK_FRAME_START		BIT(24)
+#define MIPI_CSIS_INTMSK_FRAME_END		BIT(20)
+#define MIPI_CSIS_INTMSK_ERR_SOT_HS		BIT(16)
+#define MIPI_CSIS_INTMSK_ERR_LOST_FS		BIT(12)
+#define MIPI_CSIS_INTMSK_ERR_LOST_FE		BIT(8)
+#define MIPI_CSIS_INTMSK_ERR_OVER		BIT(4)
+#define MIPI_CSIS_INTMSK_ERR_WRONG_CFG		BIT(3)
+#define MIPI_CSIS_INTMSK_ERR_ECC		BIT(2)
+#define MIPI_CSIS_INTMSK_ERR_CRC		BIT(1)
+#define MIPI_CSIS_INTMSK_ERR_UNKNOWN		BIT(0)
 
 /* CSIS Interrupt source */
-#define MIPI_CSIS_INTSRC		0x14
-#define MIPI_CSIS_INTSRC_EVEN_BEFORE	BIT(31)
-#define MIPI_CSIS_INTSRC_EVEN_AFTER	BIT(30)
-#define MIPI_CSIS_INTSRC_EVEN		BIT(30)
-#define MIPI_CSIS_INTSRC_ODD_BEFORE	BIT(29)
-#define MIPI_CSIS_INTSRC_ODD_AFTER	BIT(28)
-#define MIPI_CSIS_INTSRC_ODD		(0x3 << 28)
-#define MIPI_CSIS_INTSRC_NON_IMAGE_DATA	(0xf << 28)
-#define MIPI_CSIS_INTSRC_FRAME_START	BIT(24)
-#define MIPI_CSIS_INTSRC_FRAME_END	BIT(20)
-#define MIPI_CSIS_INTSRC_ERR_SOT_HS	BIT(16)
-#define MIPI_CSIS_INTSRC_ERR_LOST_FS	BIT(12)
-#define MIPI_CSIS_INTSRC_ERR_LOST_FE	BIT(8)
-#define MIPI_CSIS_INTSRC_ERR_OVER	BIT(4)
-#define MIPI_CSIS_INTSRC_ERR_WRONG_CFG	BIT(3)
-#define MIPI_CSIS_INTSRC_ERR_ECC	BIT(2)
-#define MIPI_CSIS_INTSRC_ERR_CRC	BIT(1)
-#define MIPI_CSIS_INTSRC_ERR_UNKNOWN	BIT(0)
-#define MIPI_CSIS_INTSRC_ERRORS		0xfffff
+#define MIPI_CSIS_INTSRC			0x14
+#define MIPI_CSIS_INTSRC_EVEN_BEFORE		BIT(31)
+#define MIPI_CSIS_INTSRC_EVEN_AFTER		BIT(30)
+#define MIPI_CSIS_INTSRC_EVEN			BIT(30)
+#define MIPI_CSIS_INTSRC_ODD_BEFORE		BIT(29)
+#define MIPI_CSIS_INTSRC_ODD_AFTER		BIT(28)
+#define MIPI_CSIS_INTSRC_ODD			(0x3 << 28)
+#define MIPI_CSIS_INTSRC_NON_IMAGE_DATA		(0xf << 28)
+#define MIPI_CSIS_INTSRC_FRAME_START		BIT(24)
+#define MIPI_CSIS_INTSRC_FRAME_END		BIT(20)
+#define MIPI_CSIS_INTSRC_ERR_SOT_HS		BIT(16)
+#define MIPI_CSIS_INTSRC_ERR_LOST_FS		BIT(12)
+#define MIPI_CSIS_INTSRC_ERR_LOST_FE		BIT(8)
+#define MIPI_CSIS_INTSRC_ERR_OVER		BIT(4)
+#define MIPI_CSIS_INTSRC_ERR_WRONG_CFG		BIT(3)
+#define MIPI_CSIS_INTSRC_ERR_ECC		BIT(2)
+#define MIPI_CSIS_INTSRC_ERR_CRC		BIT(1)
+#define MIPI_CSIS_INTSRC_ERR_UNKNOWN		BIT(0)
+#define MIPI_CSIS_INTSRC_ERRORS			0xfffff
 
 /* D-PHY status control */
 #define MIPI_CSIS_DPHYSTATUS			0x20
@@ -121,19 +121,19 @@
 #define MIPI_CSIS_DPHYCTRL_ENABLE		(0x1f << 0)
 
 /* D-PHY Master and Slave Control register Low */
-#define MIPI_CSIS_DPHYBCTRL_L		0x30
+#define MIPI_CSIS_DPHYBCTRL_L			0x30
 /* D-PHY Master and Slave Control register High */
-#define MIPI_CSIS_DPHYBCTRL_H		0x34
+#define MIPI_CSIS_DPHYBCTRL_H			0x34
 /* D-PHY Slave Control register Low */
-#define MIPI_CSIS_DPHYSCTRL_L		0x38
+#define MIPI_CSIS_DPHYSCTRL_L			0x38
 /* D-PHY Slave Control register High */
-#define MIPI_CSIS_DPHYSCTRL_H		0x3c
+#define MIPI_CSIS_DPHYSCTRL_H			0x3c
 
 /* ISP Configuration register */
-#define MIPI_CSIS_ISPCONFIG_CH0		0x40
-#define MIPI_CSIS_ISPCONFIG_CH1		0x50
-#define MIPI_CSIS_ISPCONFIG_CH2		0x60
-#define MIPI_CSIS_ISPCONFIG_CH3		0x70
+#define MIPI_CSIS_ISPCONFIG_CH0			0x40
+#define MIPI_CSIS_ISPCONFIG_CH1			0x50
+#define MIPI_CSIS_ISPCONFIG_CH2			0x60
+#define MIPI_CSIS_ISPCONFIG_CH3			0x70
 
 #define MIPI_CSIS_ISPCFG_MEM_FULL_GAP_MSK	(0xff << 24)
 #define MIPI_CSIS_ISPCFG_MEM_FULL_GAP(x)	((x) << 24)
@@ -146,33 +146,33 @@
 #define MIPI_CSIS_ISPCFG_FMT_RAW14		(0x2d << 2)
 
 /* User defined formats, x = 1...4 */
-#define MIPI_CSIS_ISPCFG_FMT_USER(x)	((0x30 + (x) - 1) << 2)
-#define MIPI_CSIS_ISPCFG_FMT_MASK	(0x3f << 2)
+#define MIPI_CSIS_ISPCFG_FMT_USER(x)		((0x30 + (x) - 1) << 2)
+#define MIPI_CSIS_ISPCFG_FMT_MASK		(0x3f << 2)
 
 /* ISP Image Resolution register */
-#define MIPI_CSIS_ISPRESOL_CH0		0x44
-#define MIPI_CSIS_ISPRESOL_CH1		0x54
-#define MIPI_CSIS_ISPRESOL_CH2		0x64
-#define MIPI_CSIS_ISPRESOL_CH3		0x74
-#define CSIS_MAX_PIX_WIDTH		0xffff
-#define CSIS_MAX_PIX_HEIGHT		0xffff
+#define MIPI_CSIS_ISPRESOL_CH0			0x44
+#define MIPI_CSIS_ISPRESOL_CH1			0x54
+#define MIPI_CSIS_ISPRESOL_CH2			0x64
+#define MIPI_CSIS_ISPRESOL_CH3			0x74
+#define CSIS_MAX_PIX_WIDTH			0xffff
+#define CSIS_MAX_PIX_HEIGHT			0xffff
 
 /* ISP SYNC register */
-#define MIPI_CSIS_ISPSYNC_CH0		0x48
-#define MIPI_CSIS_ISPSYNC_CH1		0x58
-#define MIPI_CSIS_ISPSYNC_CH2		0x68
-#define MIPI_CSIS_ISPSYNC_CH3		0x78
+#define MIPI_CSIS_ISPSYNC_CH0			0x48
+#define MIPI_CSIS_ISPSYNC_CH1			0x58
+#define MIPI_CSIS_ISPSYNC_CH2			0x68
+#define MIPI_CSIS_ISPSYNC_CH3			0x78
 
 #define MIPI_CSIS_ISPSYNC_HSYNC_LINTV_OFFSET	18
 #define MIPI_CSIS_ISPSYNC_VSYNC_SINTV_OFFSET	12
 #define MIPI_CSIS_ISPSYNC_VSYNC_EINTV_OFFSET	0
 
 /* Non-image packet data buffers */
-#define MIPI_CSIS_PKTDATA_ODD		0x2000
-#define MIPI_CSIS_PKTDATA_EVEN		0x3000
-#define MIPI_CSIS_PKTDATA_SIZE		SZ_4K
+#define MIPI_CSIS_PKTDATA_ODD			0x2000
+#define MIPI_CSIS_PKTDATA_EVEN			0x3000
+#define MIPI_CSIS_PKTDATA_SIZE			SZ_4K
 
-#define DEFAULT_SCLK_CSIS_FREQ		166000000UL
+#define DEFAULT_SCLK_CSIS_FREQ			166000000UL
 
 enum {
 	ST_POWERED	= 1,
-- 
Regards,

Laurent Pinchart

[PATCH 11/14] media: imx: imx7-mipi-csis: Remove link setup on source pad

[Laurent Pinchart]

The driver rejects enablement of multiple links on its source pad. This
isn't needed, as the CSIS doesn't care. Drop it.

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

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 40004a98f801..7112969d1cf4 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -254,7 +254,6 @@ struct csi_state {
 
 	struct csis_hw_reset hw_reset;
 	struct regulator *mipi_phy_regulator;
-	bool sink_linked;
 };
 
 struct csis_pix_format {
@@ -675,17 +674,7 @@ static int mipi_csis_link_setup(struct media_entity *entity,
 
 	mutex_lock(&state->lock);
 
-	if (local_pad->flags & MEDIA_PAD_FL_SOURCE) {
-		if (flags & MEDIA_LNK_FL_ENABLED) {
-			if (state->sink_linked) {
-				ret = -EBUSY;
-				goto out;
-			}
-			state->sink_linked = true;
-		} else {
-			state->sink_linked = false;
-		}
-	} else {
+	if (local_pad->flags & MEDIA_PAD_FL_SINK) {
 		if (flags & MEDIA_LNK_FL_ENABLED) {
 			if (state->src_sd) {
 				ret = -EBUSY;
-- 
Regards,

Laurent Pinchart

[PATCH 12/14] media: imx: imx7-mipi-csis: Cleanup includes

[Laurent Pinchart]

Remove unneeded includes, add needed ones, and sort them alphabetically.

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

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 7112969d1cf4..0829980d7af5 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -14,15 +14,14 @@
 #include <linux/errno.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
-#include <linux/irq.h>
 #include <linux/kernel.h>
-#include <linux/mfd/syscon.h>
 #include <linux/module.h>
-#include <linux/of_graph.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
-#include <linux/reset.h>
 #include <linux/regulator/consumer.h>
+#include <linux/reset.h>
 #include <linux/spinlock.h>
 
 #include <media/v4l2-device.h>
-- 
Regards,

Laurent Pinchart

[PATCH 13/14] media: imx: imx7-mipi-csis: Don't use imx-media-utils helpers

[Laurent Pinchart]

The imx7-mipi-csis only uses the imx_media_init_mbus_fmt() function from
the imx-media-utils helpers. The helpers don't support all the media bus
formats used by this driver, and are thus a bad fit. As the MIPI CSIS is
a standalone IP core that could be integrated in other SoCs, let's not
use the helper.

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

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 0829980d7af5..66ff73919371 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -28,8 +28,6 @@
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
-#include "imx-media.h"
-
 #define CSIS_DRIVER_NAME			"imx7-mipi-csis"
 #define CSIS_SUBDEV_NAME			CSIS_DRIVER_NAME
 
@@ -709,15 +707,21 @@ static int mipi_csis_init_cfg(struct v4l2_subdev *mipi_sd,
 	struct v4l2_mbus_framefmt *fmt_sink;
 	struct v4l2_mbus_framefmt *fmt_source;
 	enum v4l2_subdev_format_whence which;
-	int ret;
 
 	which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
 	fmt_sink = mipi_csis_get_format(state, cfg, which, CSIS_PAD_SINK);
-	ret = imx_media_init_mbus_fmt(fmt_sink, MIPI_CSIS_DEF_PIX_WIDTH,
-				      MIPI_CSIS_DEF_PIX_HEIGHT, 0,
-				      V4L2_FIELD_NONE, NULL);
-	if (ret < 0)
-		return ret;
+
+	fmt_sink->code = MEDIA_BUS_FMT_UYVY8_2X8;
+	fmt_sink->width = MIPI_CSIS_DEF_PIX_WIDTH;
+	fmt_sink->height = MIPI_CSIS_DEF_PIX_HEIGHT;
+	fmt_sink->field = V4L2_FIELD_NONE;
+
+	fmt_sink->colorspace = V4L2_COLORSPACE_SMPTE170M;
+	fmt_sink->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt_sink->colorspace);
+	fmt_sink->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt_sink->colorspace);
+	fmt_sink->quantization =
+		V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt_sink->colorspace,
+					      fmt_sink->ycbcr_enc);
 
 	/*
 	 * When called from mipi_csis_subdev_init() to initialize the active
-- 
Regards,

Laurent Pinchart

[PATCH 14/14] media: imx: imx7-mipi-csis: Implement the .enum_mbus_code() operation

[Laurent Pinchart]

Implement the subdev pad .enum_mbus_code() operation to enumerate media
bus codes on the sink and source pads.

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

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 66ff73919371..38f465256879 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -752,6 +752,38 @@ static int mipi_csis_get_fmt(struct v4l2_subdev *mipi_sd,
 	return 0;
 }
 
+static int mipi_csis_enum_mbus_code(struct v4l2_subdev *mipi_sd,
+				    struct v4l2_subdev_pad_config *cfg,
+				    struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
+
+	/*
+	 * The CSIS can't transcode in any way, the source format is identical
+	 * to the sink format.
+	 */
+	if (code->pad == CSIS_PAD_SOURCE) {
+		struct v4l2_mbus_framefmt *fmt;
+
+		if (code->index > 0)
+			return -EINVAL;
+
+		fmt = mipi_csis_get_format(state, cfg, code->which, code->pad);
+		code->code = fmt->code;
+		return 0;
+	}
+
+	if (code->pad != CSIS_PAD_SINK)
+		return -EINVAL;
+
+	if (code->index >= ARRAY_SIZE(mipi_csis_formats))
+		return -EINVAL;
+
+	code->code = mipi_csis_formats[code->index].code;
+
+	return 0;
+}
+
 static int mipi_csis_set_fmt(struct v4l2_subdev *mipi_sd,
 			     struct v4l2_subdev_pad_config *cfg,
 			     struct v4l2_subdev_format *sdformat)
@@ -881,6 +913,7 @@ static const struct v4l2_subdev_video_ops mipi_csis_video_ops = {
 
 static const struct v4l2_subdev_pad_ops mipi_csis_pad_ops = {
 	.init_cfg		= mipi_csis_init_cfg,
+	.enum_mbus_code		= mipi_csis_enum_mbus_code,
 	.get_fmt		= mipi_csis_get_fmt,
 	.set_fmt		= mipi_csis_set_fmt,
 };
-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/14] media: imx: Miscellaneous fixes for i.MX7 CSI-2 receiver

[Rui Miguel Silva]

Hi Laurent,
Thanks for this series.

On Fri, Mar 13, 2020 at 01:47:08AM +0200, Laurent Pinchart wrote:
> Hello,
> 
> This patch series completes (for now :-)) my rework of the CSI-2
> receiver found in the i.MX7 SoCs.
> 
> Patches 01/14 and 02/14 have already been tested and acked. The next
> four patches (03/14 to 06/14) fix format handling by adding missing
> formats and removing unsupported formats, and patches 07/14 to 09/14
> then clean up and fix image width alignment handling.
> 
> The next three patches (10/14 to 12/14) are again miscellaneous
> cleanups. Patch 13/14 removes usage of the only imx-media-utils helper
> used in this driver as the helper isn't compatible with the i.MX7 CSI-2
> receiver formats. Patch 14/14 finally implements the subdev
> .enum_mbus_code() pad operation to allow enumeration of media bus codes
> from userspace.
> 
> The patches have been tested on an i.MX7Q with a 10-bit greyscale MIPI
> CSI-2 sensor.

For the all series:
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>

------
Cheers,
     Rui

> 
> Laurent Pinchart (14):
>   media: imx: imx7-mipi-csis: Cleanup and fix subdev pad format handling
>   media: imx: imx7-mipi-csis: Centralize initialization of pad formats
>   media: imx: imx7-mipi-csis: Add missing RAW formats
>   media: imx: imx7-mipi-csis: Expose correct YUV formats
>   media: imx: imx7-mipi-csis: Fix MEDIA_BUS_FMT_UYVY8_2X8 data alignment
>   media: imx: imx7-mipi-csis: Add MEDIA_BUS_FMT_UYVY10_2X10 support
>   media: imx: imx7-mipi-csis: Rename data_alignment field to width
>   media: imx: imx7-mipi-csis: Align image width based on format
>   media: imx: imx7-mipi-csis: Never set MIPI_CSIS_ISPCFG_ALIGN_32BIT
>   media: imx: imx7-mipi-csis: Align macro definitions
>   media: imx: imx7-mipi-csis: Remove link setup on source pad
>   media: imx: imx7-mipi-csis: Cleanup includes
>   media: imx: imx7-mipi-csis: Don't use imx-media-utils helpers
>   media: imx: imx7-mipi-csis: Implement the .enum_mbus_code() operation
> 
>  drivers/staging/media/imx/imx7-mipi-csis.c | 446 +++++++++++++--------
>  1 file changed, 274 insertions(+), 172 deletions(-)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH 13/14] media: imx: imx7-mipi-csis: Don't use imx-media-utils helpers

[Hans Verkuil]

On 3/13/20 12:47 AM, Laurent Pinchart wrote:
> The imx7-mipi-csis only uses the imx_media_init_mbus_fmt() function from
> the imx-media-utils helpers. The helpers don't support all the media bus
> formats used by this driver, and are thus a bad fit. As the MIPI CSIS is
> a standalone IP core that could be integrated in other SoCs, let's not
> use the helper.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index 0829980d7af5..66ff73919371 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -28,8 +28,6 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> -#include "imx-media.h"
> -
>  #define CSIS_DRIVER_NAME			"imx7-mipi-csis"
>  #define CSIS_SUBDEV_NAME			CSIS_DRIVER_NAME
>  
> @@ -709,15 +707,21 @@ static int mipi_csis_init_cfg(struct v4l2_subdev *mipi_sd,
>  	struct v4l2_mbus_framefmt *fmt_sink;
>  	struct v4l2_mbus_framefmt *fmt_source;
>  	enum v4l2_subdev_format_whence which;
> -	int ret;
>  
>  	which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
>  	fmt_sink = mipi_csis_get_format(state, cfg, which, CSIS_PAD_SINK);
> -	ret = imx_media_init_mbus_fmt(fmt_sink, MIPI_CSIS_DEF_PIX_WIDTH,
> -				      MIPI_CSIS_DEF_PIX_HEIGHT, 0,
> -				      V4L2_FIELD_NONE, NULL);
> -	if (ret < 0)
> -		return ret;
> +
> +	fmt_sink->code = MEDIA_BUS_FMT_UYVY8_2X8;
> +	fmt_sink->width = MIPI_CSIS_DEF_PIX_WIDTH;
> +	fmt_sink->height = MIPI_CSIS_DEF_PIX_HEIGHT;
> +	fmt_sink->field = V4L2_FIELD_NONE;
> +
> +	fmt_sink->colorspace = V4L2_COLORSPACE_SMPTE170M;

Why V4L2_COLORSPACE_SMPTE170M?

After grepping a bit more in the imx code I see that the colorspace
handling is wrong in any case, so I will just accept this patch, but
this really should be fixed driver-wide.

It looks like the driver makes the typical mistake of thinking that
YUV formats use SMPTE170M colorspace and RGB formats use SRGB, but that's
not true. For drivers like this that typically are used with sensors
initializing the colorspace to SRGB is a good default. But the actual
colorspace comes from the sensor or the video receiver, the imx driver can't
know. And YUV vs RGB memory formats is just a different pixel encoding and
is unrelated to the colorspace.

Regards,

	Hans

> +	fmt_sink->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt_sink->colorspace);
> +	fmt_sink->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt_sink->colorspace);
> +	fmt_sink->quantization =
> +		V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt_sink->colorspace,
> +					      fmt_sink->ycbcr_enc);
>  
>  	/*
>  	 * When called from mipi_csis_subdev_init() to initialize the active
> 

Re: [PATCH 13/14] media: imx: imx7-mipi-csis: Don't use imx-media-utils helpers

[Laurent Pinchart]

Hi Hans,

On Wed, Mar 25, 2020 at 10:13:05AM +0100, Hans Verkuil wrote:
> On 3/13/20 12:47 AM, Laurent Pinchart wrote:
> > The imx7-mipi-csis only uses the imx_media_init_mbus_fmt() function from
> > the imx-media-utils helpers. The helpers don't support all the media bus
> > formats used by this driver, and are thus a bad fit. As the MIPI CSIS is
> > a standalone IP core that could be integrated in other SoCs, let's not
> > use the helper.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/staging/media/imx/imx7-mipi-csis.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> > index 0829980d7af5..66ff73919371 100644
> > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > @@ -28,8 +28,6 @@
> >  #include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-subdev.h>
> >  
> > -#include "imx-media.h"
> > -
> >  #define CSIS_DRIVER_NAME			"imx7-mipi-csis"
> >  #define CSIS_SUBDEV_NAME			CSIS_DRIVER_NAME
> >  
> > @@ -709,15 +707,21 @@ static int mipi_csis_init_cfg(struct v4l2_subdev *mipi_sd,
> >  	struct v4l2_mbus_framefmt *fmt_sink;
> >  	struct v4l2_mbus_framefmt *fmt_source;
> >  	enum v4l2_subdev_format_whence which;
> > -	int ret;
> >  
> >  	which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> >  	fmt_sink = mipi_csis_get_format(state, cfg, which, CSIS_PAD_SINK);
> > -	ret = imx_media_init_mbus_fmt(fmt_sink, MIPI_CSIS_DEF_PIX_WIDTH,
> > -				      MIPI_CSIS_DEF_PIX_HEIGHT, 0,
> > -				      V4L2_FIELD_NONE, NULL);
> > -	if (ret < 0)
> > -		return ret;
> > +
> > +	fmt_sink->code = MEDIA_BUS_FMT_UYVY8_2X8;
> > +	fmt_sink->width = MIPI_CSIS_DEF_PIX_WIDTH;
> > +	fmt_sink->height = MIPI_CSIS_DEF_PIX_HEIGHT;
> > +	fmt_sink->field = V4L2_FIELD_NONE;
> > +
> > +	fmt_sink->colorspace = V4L2_COLORSPACE_SMPTE170M;
> 
> Why V4L2_COLORSPACE_SMPTE170M?
> 
> After grepping a bit more in the imx code I see that the colorspace
> handling is wrong in any case, so I will just accept this patch, but
> this really should be fixed driver-wide.

That's exactly why V4L2_COLORSPACE_SMPTE170M :-) It's what the driver
uses today, I didn't want to change it in this patch. I agree it should
be fixed on top.

> It looks like the driver makes the typical mistake of thinking that
> YUV formats use SMPTE170M colorspace and RGB formats use SRGB, but that's
> not true. For drivers like this that typically are used with sensors
> initializing the colorspace to SRGB is a good default. But the actual
> colorspace comes from the sensor or the video receiver, the imx driver can't
> know. And YUV vs RGB memory formats is just a different pixel encoding and
> is unrelated to the colorspace.
> 
> > +	fmt_sink->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt_sink->colorspace);
> > +	fmt_sink->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt_sink->colorspace);
> > +	fmt_sink->quantization =
> > +		V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt_sink->colorspace,
> > +					      fmt_sink->ycbcr_enc);
> >  
> >  	/*
> >  	 * When called from mipi_csis_subdev_init() to initialize the active

-- 
Regards,

Laurent Pinchart

Re: [PATCH 13/14] media: imx: imx7-mipi-csis: Don't use imx-media-utils helpers

[Steve Longerbeam]

Hi Hans,

On 3/25/20 2:13 AM, Hans Verkuil wrote:
> On 3/13/20 12:47 AM, Laurent Pinchart wrote:
>> The imx7-mipi-csis only uses the imx_media_init_mbus_fmt() function from
>> the imx-media-utils helpers. The helpers don't support all the media bus
>> formats used by this driver, and are thus a bad fit. As the MIPI CSIS is
>> a standalone IP core that could be integrated in other SoCs, let's not
>> use the helper.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>   drivers/staging/media/imx/imx7-mipi-csis.c | 20 ++++++++++++--------
>>   1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
>> index 0829980d7af5..66ff73919371 100644
>> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
>> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
>> @@ -28,8 +28,6 @@
>>   #include <media/v4l2-fwnode.h>
>>   #include <media/v4l2-subdev.h>
>>   
>> -#include "imx-media.h"
>> -
>>   #define CSIS_DRIVER_NAME			"imx7-mipi-csis"
>>   #define CSIS_SUBDEV_NAME			CSIS_DRIVER_NAME
>>   
>> @@ -709,15 +707,21 @@ static int mipi_csis_init_cfg(struct v4l2_subdev *mipi_sd,
>>   	struct v4l2_mbus_framefmt *fmt_sink;
>>   	struct v4l2_mbus_framefmt *fmt_source;
>>   	enum v4l2_subdev_format_whence which;
>> -	int ret;
>>   
>>   	which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
>>   	fmt_sink = mipi_csis_get_format(state, cfg, which, CSIS_PAD_SINK);
>> -	ret = imx_media_init_mbus_fmt(fmt_sink, MIPI_CSIS_DEF_PIX_WIDTH,
>> -				      MIPI_CSIS_DEF_PIX_HEIGHT, 0,
>> -				      V4L2_FIELD_NONE, NULL);
>> -	if (ret < 0)
>> -		return ret;
>> +
>> +	fmt_sink->code = MEDIA_BUS_FMT_UYVY8_2X8;
>> +	fmt_sink->width = MIPI_CSIS_DEF_PIX_WIDTH;
>> +	fmt_sink->height = MIPI_CSIS_DEF_PIX_HEIGHT;
>> +	fmt_sink->field = V4L2_FIELD_NONE;
>> +
>> +	fmt_sink->colorspace = V4L2_COLORSPACE_SMPTE170M;
> Why V4L2_COLORSPACE_SMPTE170M?
>
> After grepping a bit more in the imx code I see that the colorspace
> handling is wrong in any case, so I will just accept this patch, but
> this really should be fixed driver-wide.
>
> It looks like the driver makes the typical mistake of thinking that
> YUV formats use SMPTE170M colorspace and RGB formats use SRGB, but that's
> not true.

The imx6 driver mostly just propagates the colorspace from sink to 
source pads that comes originally from the sensor, and computes the 
other colorimetry parameters based on the propagated colorspace, see 
imx_media_try_colorimetry(). But I found one location where the 
colorspace is set to V4L2_COLORSPACE_SRGB if the pixel format is RGB, 
and V4L2_COLORSPACE_SMPTE170M if the pixel format is YUV, in the 
function init_mbus_colorimetry(). That's not necessarily a good choice 
for a default colorspace, since SMPTE170M could have a RGB encoding, or 
SRGB could have a YUV encoding. I'll send a patch that just defaults to 
IPUV3_COLORSPACE_RGB in init_mbus_colorimetry().

Steve

>   For drivers like this that typically are used with sensors
> initializing the colorspace to SRGB is a good default. But the actual
> colorspace comes from the sensor or the video receiver, the imx driver can't
> know. And YUV vs RGB memory formats is just a different pixel encoding and
> is unrelated to the colorspace.
>
> Regards,
>
> 	Hans
>
>> +	fmt_sink->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt_sink->colorspace);
>> +	fmt_sink->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt_sink->colorspace);
>> +	fmt_sink->quantization =
>> +		V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt_sink->colorspace,
>> +					      fmt_sink->ycbcr_enc);
>>   
>>   	/*
>>   	 * When called from mipi_csis_subdev_init() to initialize the active
>>