[PATCH 0/5] ipu3-cio2 format fixes

[PATCH 0/5] ipu3-cio2 format fixes

[Sakari Ailus]

Hello all,

This set addresses most notable subdev format related issues, namely the
sub-device sink format being unaccessible. The result of accessing it
varied from oopses to crashes.

since v1:

- Validate try format, too

- Set field in subdev format to V4L2_FIELD_NONE

- Add a comment explaining the lock

- Make values that should be unsigned, unsigned

Sakari Ailus (5):
  ipu3-cio2: Return actual subdev format
  ipu3-cio2: Serialise access to pad format
  ipu3-cio2: Use unsigned values where appropriate
  ipu3-cio2: Validate mbus format in setting subdev format
  ipu3-cio2: Make the field on subdev format V4L2_FIELD_NONE

 drivers/media/pci/intel/ipu3/ipu3-cio2.c |  60 +++++----
 drivers/media/pci/intel/ipu3/ipu3-cio2.h | 157 ++++++++++++-----------
 2 files changed, 111 insertions(+), 106 deletions(-)

-- 
2.27.0

[PATCH 1/5] ipu3-cio2: Return actual subdev format

[Sakari Ailus]

Return actual subdev format on ipu3-cio2 subdev pads. The earlier
implementation was based on an infinite recursion that exhausted the
stack.

Reported-by: Tsuchiya Yuto <kitakar@gmail.com>
Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 4e598e937dfe..afa472026ba4 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1232,29 +1232,11 @@ static int cio2_subdev_get_fmt(struct v4l2_subdev *sd,
 			       struct v4l2_subdev_format *fmt)
 {
 	struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev);
-	struct v4l2_subdev_format format;
-	int ret;
 
-	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
 		fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
-		return 0;
-	}
-
-	if (fmt->pad == CIO2_PAD_SINK) {
-		format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-		ret = v4l2_subdev_call(sd, pad, get_fmt, NULL,
-				       &format);
-
-		if (ret)
-			return ret;
-		/* update colorspace etc */
-		q->subdev_fmt.colorspace = format.format.colorspace;
-		q->subdev_fmt.ycbcr_enc = format.format.ycbcr_enc;
-		q->subdev_fmt.quantization = format.format.quantization;
-		q->subdev_fmt.xfer_func = format.format.xfer_func;
-	}
-
-	fmt->format = q->subdev_fmt;
+	else
+		fmt->format = q->subdev_fmt;
 
 	return 0;
 }
-- 
2.27.0

[PATCH 2/5] ipu3-cio2: Serialise access to pad format

[Sakari Ailus]

Pad format can be accessed from user space. Serialise access to it.

Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 11 +++++++++++
 drivers/media/pci/intel/ipu3/ipu3-cio2.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index afa472026ba4..9c7b527a8800 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1233,11 +1233,15 @@ static int cio2_subdev_get_fmt(struct v4l2_subdev *sd,
 {
 	struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev);
 
+	mutex_lock(&q->subdev_lock);
+
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
 		fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
 	else
 		fmt->format = q->subdev_fmt;
 
+	mutex_unlock(&q->subdev_lock);
+
 	return 0;
 }
 
@@ -1261,6 +1265,8 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,
 	if (fmt->pad == CIO2_PAD_SOURCE)
 		return cio2_subdev_get_fmt(sd, cfg, fmt);
 
+	mutex_lock(&q->subdev_lock);
+
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
 		*v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format;
 	} else {
@@ -1271,6 +1277,8 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,
 		fmt->format = q->subdev_fmt;
 	}
 
+	mutex_unlock(&q->subdev_lock);
+
 	return 0;
 }
 
@@ -1529,6 +1537,7 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q)
 
 	/* Initialize miscellaneous variables */
 	mutex_init(&q->lock);
+	mutex_init(&q->subdev_lock);
 
 	/* Initialize formats to default values */
 	fmt = &q->subdev_fmt;
@@ -1646,6 +1655,7 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q)
 	cio2_fbpt_exit(q, &cio2->pci_dev->dev);
 fail_fbpt:
 	mutex_destroy(&q->lock);
+	mutex_destroy(&q->subdev_lock);
 
 	return r;
 }
@@ -1658,6 +1668,7 @@ static void cio2_queue_exit(struct cio2_device *cio2, struct cio2_queue *q)
 	media_entity_cleanup(&q->subdev.entity);
 	cio2_fbpt_exit(q, &cio2->pci_dev->dev);
 	mutex_destroy(&q->lock);
+	mutex_destroy(&q->subdev_lock);
 }
 
 static int cio2_queues_init(struct cio2_device *cio2)
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
index 549b08f88f0c..146492383aa5 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
@@ -335,6 +335,7 @@ struct cio2_queue {
 
 	/* Subdev, /dev/v4l-subdevX */
 	struct v4l2_subdev subdev;
+	struct mutex subdev_lock; /* Serialise acces to subdev_fmt field */
 	struct media_pad subdev_pads[CIO2_PADS];
 	struct v4l2_mbus_framefmt subdev_fmt;
 	atomic_t frame_sequence;
-- 
2.27.0

[PATCH 3/5] ipu3-cio2: Use unsigned values where appropriate

[Sakari Ailus]

Use unsigned values for width, height, bit shifts and registers,
effectively for all definitions that are not signed.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.h | 156 +++++++++++------------
 1 file changed, 78 insertions(+), 78 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
index 146492383aa5..7650d7998a3f 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
@@ -13,20 +13,20 @@
 #define CIO2_PCI_BAR					0
 #define CIO2_DMA_MASK					DMA_BIT_MASK(39)
 
-#define CIO2_IMAGE_MAX_WIDTH				4224
-#define CIO2_IMAGE_MAX_LENGTH				3136
+#define CIO2_IMAGE_MAX_WIDTH				4224U
+#define CIO2_IMAGE_MAX_LENGTH				3136U
 
 /* 32MB = 8xFBPT_entry */
 #define CIO2_MAX_LOPS					8
 #define CIO2_MAX_BUFFERS			(PAGE_SIZE / 16 / CIO2_MAX_LOPS)
 #define CIO2_LOP_ENTRIES			(PAGE_SIZE / sizeof(u32))
 
-#define CIO2_PAD_SINK					0
-#define CIO2_PAD_SOURCE					1
-#define CIO2_PADS					2
+#define CIO2_PAD_SINK					0U
+#define CIO2_PAD_SOURCE					1U
+#define CIO2_PADS					2U
 
-#define CIO2_NUM_DMA_CHAN				20
-#define CIO2_NUM_PORTS					4 /* DPHYs */
+#define CIO2_NUM_DMA_CHAN				20U
+#define CIO2_NUM_PORTS					4U /* DPHYs */
 
 /* 1 for each sensor */
 #define CIO2_QUEUES					CIO2_NUM_PORTS
@@ -66,12 +66,12 @@
 #define CIO2_REG_MIPIBE_FORCE_RAW8	(CIO2_REG_MIPIBE_BASE + 0x20)
 #define CIO2_REG_MIPIBE_FORCE_RAW8_ENABLE		BIT(0)
 #define CIO2_REG_MIPIBE_FORCE_RAW8_USE_TYPEID		BIT(1)
-#define CIO2_REG_MIPIBE_FORCE_RAW8_TYPEID_SHIFT		2
+#define CIO2_REG_MIPIBE_FORCE_RAW8_TYPEID_SHIFT		2U
 
 #define CIO2_REG_MIPIBE_IRQ_STATUS	(CIO2_REG_MIPIBE_BASE + 0x24)
 #define CIO2_REG_MIPIBE_IRQ_CLEAR	(CIO2_REG_MIPIBE_BASE + 0x28)
 #define CIO2_REG_MIPIBE_GLOBAL_LUT_DISREGARD (CIO2_REG_MIPIBE_BASE + 0x68)
-#define CIO2_MIPIBE_GLOBAL_LUT_DISREGARD		1
+#define CIO2_MIPIBE_GLOBAL_LUT_DISREGARD		1U
 #define CIO2_REG_MIPIBE_PKT_STALL_STATUS (CIO2_REG_MIPIBE_BASE + 0x6c)
 #define CIO2_REG_MIPIBE_PARSE_GSP_THROUGH_LP_LUT_REG_IDX \
 					(CIO2_REG_MIPIBE_BASE + 0x70)
@@ -79,10 +79,10 @@
 				       (CIO2_REG_MIPIBE_BASE + 0x74 + 4 * (vc))
 #define CIO2_REG_MIPIBE_LP_LUT_ENTRY(m)	/* m = 0..15 */ \
 					(CIO2_REG_MIPIBE_BASE + 0x84 + 4 * (m))
-#define CIO2_MIPIBE_LP_LUT_ENTRY_DISREGARD		1
-#define CIO2_MIPIBE_LP_LUT_ENTRY_SID_SHIFT		1
-#define CIO2_MIPIBE_LP_LUT_ENTRY_VC_SHIFT		5
-#define CIO2_MIPIBE_LP_LUT_ENTRY_FORMAT_TYPE_SHIFT	7
+#define CIO2_MIPIBE_LP_LUT_ENTRY_DISREGARD		1U
+#define CIO2_MIPIBE_LP_LUT_ENTRY_SID_SHIFT		1U
+#define CIO2_MIPIBE_LP_LUT_ENTRY_VC_SHIFT		5U
+#define CIO2_MIPIBE_LP_LUT_ENTRY_FORMAT_TYPE_SHIFT	7U
 
 /* base register: CIO2_REG_PIPE_BASE(pipe) * CIO2_REG_IRQCTRL_BASE */
 /* IRQ registers are 18-bit wide, see cio2_irq_error for bit definitions */
@@ -113,31 +113,31 @@
 #define CIO2_CGC_ROSC_DCGE				BIT(12)
 #define CIO2_CGC_XOSC_DCGE				BIT(13)
 #define CIO2_CGC_FLIS_DCGE				BIT(14)
-#define CIO2_CGC_CLKGATE_HOLDOFF_SHIFT			20
-#define CIO2_CGC_CSI_CLKGATE_HOLDOFF_SHIFT		24
+#define CIO2_CGC_CLKGATE_HOLDOFF_SHIFT			20U
+#define CIO2_CGC_CSI_CLKGATE_HOLDOFF_SHIFT		24U
 #define CIO2_REG_D0I3C					0x1408
 #define CIO2_D0I3C_I3					BIT(2)	/* Set D0I3 */
 #define CIO2_D0I3C_RR					BIT(3)	/* Restore? */
 #define CIO2_REG_SWRESET				0x140c
-#define CIO2_SWRESET_SWRESET				1
+#define CIO2_SWRESET_SWRESET				1U
 #define CIO2_REG_SENSOR_ACTIVE				0x1410
 #define CIO2_REG_INT_STS				0x1414
 #define CIO2_REG_INT_STS_EXT_OE				0x1418
-#define CIO2_INT_EXT_OE_DMAOE_SHIFT			0
+#define CIO2_INT_EXT_OE_DMAOE_SHIFT			0U
 #define CIO2_INT_EXT_OE_DMAOE_MASK			0x7ffff
-#define CIO2_INT_EXT_OE_OES_SHIFT			24
+#define CIO2_INT_EXT_OE_OES_SHIFT			24U
 #define CIO2_INT_EXT_OE_OES_MASK	(0xf << CIO2_INT_EXT_OE_OES_SHIFT)
 #define CIO2_REG_INT_EN					0x1420
 #define CIO2_REG_INT_EN_IRQ				(1 << 24)
-#define CIO2_REG_INT_EN_IOS(dma)	(1 << (((dma) >> 1) + 12))
+#define CIO2_REG_INT_EN_IOS(dma)	(1U << (((dma) >> 1U) + 12U))
 /*
  * Interrupt on completion bit, Eg. DMA 0-3 maps to bit 0-3,
  * DMA4 & DMA5 map to bit 4 ... DMA18 & DMA19 map to bit 11 Et cetera
  */
-#define CIO2_INT_IOC(dma)	(1 << ((dma) < 4 ? (dma) : ((dma) >> 1) + 2))
+#define CIO2_INT_IOC(dma)	(1U << ((dma) < 4U ? (dma) : ((dma) >> 1U) + 2U))
 #define CIO2_INT_IOC_SHIFT				0
 #define CIO2_INT_IOC_MASK		(0x7ff << CIO2_INT_IOC_SHIFT)
-#define CIO2_INT_IOS_IOLN(dma)		(1 << (((dma) >> 1) + 12))
+#define CIO2_INT_IOS_IOLN(dma)		(1U << (((dma) >> 1U) + 12U))
 #define CIO2_INT_IOS_IOLN_SHIFT				12
 #define CIO2_INT_IOS_IOLN_MASK		(0x3ff << CIO2_INT_IOS_IOLN_SHIFT)
 #define CIO2_INT_IOIE					BIT(22)
@@ -145,32 +145,32 @@
 #define CIO2_INT_IOIRQ					BIT(24)
 #define CIO2_REG_INT_EN_EXT_OE				0x1424
 #define CIO2_REG_DMA_DBG				0x1448
-#define CIO2_REG_DMA_DBG_DMA_INDEX_SHIFT		0
+#define CIO2_REG_DMA_DBG_DMA_INDEX_SHIFT		0U
 #define CIO2_REG_PBM_ARB_CTRL				0x1460
-#define CIO2_PBM_ARB_CTRL_LANES_DIV			0 /* 4-4-2-2 lanes */
-#define CIO2_PBM_ARB_CTRL_LANES_DIV_SHIFT		0
+#define CIO2_PBM_ARB_CTRL_LANES_DIV			0U /* 4-4-2-2 lanes */
+#define CIO2_PBM_ARB_CTRL_LANES_DIV_SHIFT		0U
 #define CIO2_PBM_ARB_CTRL_LE_EN				BIT(7)
-#define CIO2_PBM_ARB_CTRL_PLL_POST_SHTDN		2
-#define CIO2_PBM_ARB_CTRL_PLL_POST_SHTDN_SHIFT		8
-#define CIO2_PBM_ARB_CTRL_PLL_AHD_WK_UP			480
-#define CIO2_PBM_ARB_CTRL_PLL_AHD_WK_UP_SHIFT		16
+#define CIO2_PBM_ARB_CTRL_PLL_POST_SHTDN		2U
+#define CIO2_PBM_ARB_CTRL_PLL_POST_SHTDN_SHIFT		8U
+#define CIO2_PBM_ARB_CTRL_PLL_AHD_WK_UP			480U
+#define CIO2_PBM_ARB_CTRL_PLL_AHD_WK_UP_SHIFT		16U
 #define CIO2_REG_PBM_WMCTRL1				0x1464
-#define CIO2_PBM_WMCTRL1_MIN_2CK_SHIFT			0
-#define CIO2_PBM_WMCTRL1_MID1_2CK_SHIFT			8
-#define CIO2_PBM_WMCTRL1_MID2_2CK_SHIFT			16
+#define CIO2_PBM_WMCTRL1_MIN_2CK_SHIFT			0U
+#define CIO2_PBM_WMCTRL1_MID1_2CK_SHIFT			8U
+#define CIO2_PBM_WMCTRL1_MID2_2CK_SHIFT			16U
 #define CIO2_PBM_WMCTRL1_TS_COUNT_DISABLE		BIT(31)
 #define CIO2_PBM_WMCTRL1_MIN_2CK	(4 << CIO2_PBM_WMCTRL1_MIN_2CK_SHIFT)
 #define CIO2_PBM_WMCTRL1_MID1_2CK	(16 << CIO2_PBM_WMCTRL1_MID1_2CK_SHIFT)
 #define CIO2_PBM_WMCTRL1_MID2_2CK	(21 << CIO2_PBM_WMCTRL1_MID2_2CK_SHIFT)
 #define CIO2_REG_PBM_WMCTRL2				0x1468
-#define CIO2_PBM_WMCTRL2_HWM_2CK			40
-#define CIO2_PBM_WMCTRL2_HWM_2CK_SHIFT			0
-#define CIO2_PBM_WMCTRL2_LWM_2CK			22
-#define CIO2_PBM_WMCTRL2_LWM_2CK_SHIFT			8
-#define CIO2_PBM_WMCTRL2_OBFFWM_2CK			2
-#define CIO2_PBM_WMCTRL2_OBFFWM_2CK_SHIFT		16
-#define CIO2_PBM_WMCTRL2_TRANSDYN			1
-#define CIO2_PBM_WMCTRL2_TRANSDYN_SHIFT			24
+#define CIO2_PBM_WMCTRL2_HWM_2CK			40U
+#define CIO2_PBM_WMCTRL2_HWM_2CK_SHIFT			0U
+#define CIO2_PBM_WMCTRL2_LWM_2CK			22U
+#define CIO2_PBM_WMCTRL2_LWM_2CK_SHIFT			8U
+#define CIO2_PBM_WMCTRL2_OBFFWM_2CK			2U
+#define CIO2_PBM_WMCTRL2_OBFFWM_2CK_SHIFT		16U
+#define CIO2_PBM_WMCTRL2_TRANSDYN			1U
+#define CIO2_PBM_WMCTRL2_TRANSDYN_SHIFT			24U
 #define CIO2_PBM_WMCTRL2_DYNWMEN			BIT(28)
 #define CIO2_PBM_WMCTRL2_OBFF_MEM_EN			BIT(29)
 #define CIO2_PBM_WMCTRL2_OBFF_CPU_EN			BIT(30)
@@ -178,12 +178,12 @@
 #define CIO2_REG_PBM_TS_COUNT				0x146c
 #define CIO2_REG_PBM_FOPN_ABORT				0x1474
 /* below n = 0..3 */
-#define CIO2_PBM_FOPN_ABORT(n)				(0x1 << 8 * (n))
-#define CIO2_PBM_FOPN_FORCE_ABORT(n)			(0x2 << 8 * (n))
-#define CIO2_PBM_FOPN_FRAMEOPEN(n)			(0x8 << 8 * (n))
+#define CIO2_PBM_FOPN_ABORT(n)				(0x1 << 8U * (n))
+#define CIO2_PBM_FOPN_FORCE_ABORT(n)			(0x2 << 8U * (n))
+#define CIO2_PBM_FOPN_FRAMEOPEN(n)			(0x8 << 8U * (n))
 #define CIO2_REG_LTRCTRL				0x1480
 #define CIO2_LTRCTRL_LTRDYNEN				BIT(16)
-#define CIO2_LTRCTRL_LTRSTABLETIME_SHIFT		8
+#define CIO2_LTRCTRL_LTRSTABLETIME_SHIFT		8U
 #define CIO2_LTRCTRL_LTRSTABLETIME_MASK			0xff
 #define CIO2_LTRCTRL_LTRSEL1S3				BIT(7)
 #define CIO2_LTRCTRL_LTRSEL1S2				BIT(6)
@@ -195,28 +195,28 @@
 #define CIO2_LTRCTRL_LTRSEL2S0				BIT(0)
 #define CIO2_REG_LTRVAL23				0x1484
 #define CIO2_REG_LTRVAL01				0x1488
-#define CIO2_LTRVAL02_VAL_SHIFT				0
-#define CIO2_LTRVAL02_SCALE_SHIFT			10
-#define CIO2_LTRVAL13_VAL_SHIFT				16
-#define CIO2_LTRVAL13_SCALE_SHIFT			26
+#define CIO2_LTRVAL02_VAL_SHIFT				0U
+#define CIO2_LTRVAL02_SCALE_SHIFT			10U
+#define CIO2_LTRVAL13_VAL_SHIFT				16U
+#define CIO2_LTRVAL13_SCALE_SHIFT			26U
 
-#define CIO2_LTRVAL0_VAL				175
+#define CIO2_LTRVAL0_VAL				175U
 /* Value times 1024 ns */
-#define CIO2_LTRVAL0_SCALE				2
-#define CIO2_LTRVAL1_VAL				90
-#define CIO2_LTRVAL1_SCALE				2
-#define CIO2_LTRVAL2_VAL				90
-#define CIO2_LTRVAL2_SCALE				2
-#define CIO2_LTRVAL3_VAL				90
-#define CIO2_LTRVAL3_SCALE				2
+#define CIO2_LTRVAL0_SCALE				2U
+#define CIO2_LTRVAL1_VAL				90U
+#define CIO2_LTRVAL1_SCALE				2U
+#define CIO2_LTRVAL2_VAL				90U
+#define CIO2_LTRVAL2_SCALE				2U
+#define CIO2_LTRVAL3_VAL				90U
+#define CIO2_LTRVAL3_SCALE				2U
 
 #define CIO2_REG_CDMABA(n)		(0x1500 + 0x10 * (n))	/* n = 0..19 */
 #define CIO2_REG_CDMARI(n)		(0x1504 + 0x10 * (n))
-#define CIO2_CDMARI_FBPT_RP_SHIFT			0
+#define CIO2_CDMARI_FBPT_RP_SHIFT			0U
 #define CIO2_CDMARI_FBPT_RP_MASK			0xff
 #define CIO2_REG_CDMAC0(n)		(0x1508 + 0x10 * (n))
-#define CIO2_CDMAC0_FBPT_LEN_SHIFT			0
-#define CIO2_CDMAC0_FBPT_WIDTH_SHIFT			8
+#define CIO2_CDMAC0_FBPT_LEN_SHIFT			0U
+#define CIO2_CDMAC0_FBPT_WIDTH_SHIFT			8U
 #define CIO2_CDMAC0_FBPT_NS				BIT(25)
 #define CIO2_CDMAC0_DMA_INTR_ON_FS			BIT(26)
 #define CIO2_CDMAC0_DMA_INTR_ON_FE			BIT(27)
@@ -225,12 +225,12 @@
 #define CIO2_CDMAC0_DMA_EN				BIT(30)
 #define CIO2_CDMAC0_DMA_HALTED				BIT(31)
 #define CIO2_REG_CDMAC1(n)		(0x150c + 0x10 * (n))
-#define CIO2_CDMAC1_LINENUMINT_SHIFT			0
-#define CIO2_CDMAC1_LINENUMUPDATE_SHIFT			16
+#define CIO2_CDMAC1_LINENUMINT_SHIFT			0U
+#define CIO2_CDMAC1_LINENUMUPDATE_SHIFT			16U
 /* n = 0..3 */
 #define CIO2_REG_PXM_PXF_FMT_CFG0(n)	(0x1700 + 0x30 * (n))
-#define CIO2_PXM_PXF_FMT_CFG_SID0_SHIFT			0
-#define CIO2_PXM_PXF_FMT_CFG_SID1_SHIFT			16
+#define CIO2_PXM_PXF_FMT_CFG_SID0_SHIFT			0U
+#define CIO2_PXM_PXF_FMT_CFG_SID1_SHIFT			16U
 #define CIO2_PXM_PXF_FMT_CFG_PCK_64B			(0 << 0)
 #define CIO2_PXM_PXF_FMT_CFG_PCK_32B			(1 << 0)
 #define CIO2_PXM_PXF_FMT_CFG_BPP_08			(0 << 2)
@@ -249,27 +249,27 @@
 #define CIO2_PXM_PXF_FMT_CFG_PSWAP4_2ND_BD		(1 << 10)
 #define CIO2_REG_INT_STS_EXT_IE				0x17e4
 #define CIO2_REG_INT_EN_EXT_IE				0x17e8
-#define CIO2_INT_EXT_IE_ECC_RE(n)			(0x01 << (8 * (n)))
-#define CIO2_INT_EXT_IE_DPHY_NR(n)			(0x02 << (8 * (n)))
-#define CIO2_INT_EXT_IE_ECC_NR(n)			(0x04 << (8 * (n)))
-#define CIO2_INT_EXT_IE_CRCERR(n)			(0x08 << (8 * (n)))
-#define CIO2_INT_EXT_IE_INTERFRAMEDATA(n)		(0x10 << (8 * (n)))
-#define CIO2_INT_EXT_IE_PKT2SHORT(n)			(0x20 << (8 * (n)))
-#define CIO2_INT_EXT_IE_PKT2LONG(n)			(0x40 << (8 * (n)))
-#define CIO2_INT_EXT_IE_IRQ(n)				(0x80 << (8 * (n)))
+#define CIO2_INT_EXT_IE_ECC_RE(n)			(0x01 << (8U * (n)))
+#define CIO2_INT_EXT_IE_DPHY_NR(n)			(0x02 << (8U * (n)))
+#define CIO2_INT_EXT_IE_ECC_NR(n)			(0x04 << (8U * (n)))
+#define CIO2_INT_EXT_IE_CRCERR(n)			(0x08 << (8U * (n)))
+#define CIO2_INT_EXT_IE_INTERFRAMEDATA(n)		(0x10 << (8U * (n)))
+#define CIO2_INT_EXT_IE_PKT2SHORT(n)			(0x20 << (8U * (n)))
+#define CIO2_INT_EXT_IE_PKT2LONG(n)			(0x40 << (8U * (n)))
+#define CIO2_INT_EXT_IE_IRQ(n)				(0x80 << (8U * (n)))
 #define CIO2_REG_PXM_FRF_CFG(n)				(0x1720 + 0x30 * (n))
 #define CIO2_PXM_FRF_CFG_FNSEL				BIT(0)
 #define CIO2_PXM_FRF_CFG_FN_RST				BIT(1)
 #define CIO2_PXM_FRF_CFG_ABORT				BIT(2)
-#define CIO2_PXM_FRF_CFG_CRC_TH_SHIFT			3
+#define CIO2_PXM_FRF_CFG_CRC_TH_SHIFT			3U
 #define CIO2_PXM_FRF_CFG_MSK_ECC_DPHY_NR		BIT(8)
 #define CIO2_PXM_FRF_CFG_MSK_ECC_RE			BIT(9)
 #define CIO2_PXM_FRF_CFG_MSK_ECC_DPHY_NE		BIT(10)
-#define CIO2_PXM_FRF_CFG_EVEN_ODD_MODE_SHIFT		11
+#define CIO2_PXM_FRF_CFG_EVEN_ODD_MODE_SHIFT		11U
 #define CIO2_PXM_FRF_CFG_MASK_CRC_THRES			BIT(13)
 #define CIO2_PXM_FRF_CFG_MASK_CSI_ACCEPT		BIT(14)
 #define CIO2_PXM_FRF_CFG_CIOHC_FS_MODE			BIT(15)
-#define CIO2_PXM_FRF_CFG_CIOHC_FRST_FRM_SHIFT		16
+#define CIO2_PXM_FRF_CFG_CIOHC_FRST_FRM_SHIFT		16U
 #define CIO2_REG_PXM_SID2BID0(n)			(0x1724 + 0x30 * (n))
 #define CIO2_FB_HPLL_FREQ				0x2
 #define CIO2_ISCLK_RATIO				0xc
@@ -278,14 +278,14 @@
 
 #define CIO2_INT_EN_EXT_OE_MASK				0x8f0fffff
 
-#define CIO2_CGC_CLKGATE_HOLDOFF			3
-#define CIO2_CGC_CSI_CLKGATE_HOLDOFF			5
+#define CIO2_CGC_CLKGATE_HOLDOFF			3U
+#define CIO2_CGC_CSI_CLKGATE_HOLDOFF			5U
 
 #define CIO2_PXM_FRF_CFG_CRC_TH				16
 
 #define CIO2_INT_EN_EXT_IE_MASK				0xffffffff
 
-#define CIO2_DMA_CHAN					0
+#define CIO2_DMA_CHAN					0U
 
 #define CIO2_CSIRX_DLY_CNT_CLANE_IDX			-1
 
@@ -302,8 +302,8 @@
 #define CIO2_CSIRX_DLY_CNT_TERMEN_DEFAULT		0x4
 #define CIO2_CSIRX_DLY_CNT_SETTLE_DEFAULT		0x570
 
-#define CIO2_PMCSR_OFFSET				4
-#define CIO2_PMCSR_D0D3_SHIFT				2
+#define CIO2_PMCSR_OFFSET				4U
+#define CIO2_PMCSR_D0D3_SHIFT				2U
 #define CIO2_PMCSR_D3					0x3
 
 struct cio2_csi2_timing {
-- 
2.27.0

[PATCH 4/5] ipu3-cio2: Validate mbus format in setting subdev format

[Sakari Ailus]

Validate media bus code, width and height when setting the subdev format.

This effectively reworks how setting subdev format is implemented in the
driver.

Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 28 ++++++++++++++++--------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 9c7b527a8800..9726091c9985 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1257,6 +1257,9 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,
 			       struct v4l2_subdev_format *fmt)
 {
 	struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev);
+	struct v4l2_mbus_framefmt *mbus;
+	u32 mbus_code = fmt->format.code;
+	unsigned int i;
 
 	/*
 	 * Only allow setting sink pad format;
@@ -1265,18 +1268,25 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,
 	if (fmt->pad == CIO2_PAD_SOURCE)
 		return cio2_subdev_get_fmt(sd, cfg, fmt);
 
-	mutex_lock(&q->subdev_lock);
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
+		mbus = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
+	else
+		mbus = &q->subdev_fmt;
 
-	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
-		*v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format;
-	} else {
-		/* It's the sink, allow changing frame size */
-		q->subdev_fmt.width = fmt->format.width;
-		q->subdev_fmt.height = fmt->format.height;
-		q->subdev_fmt.code = fmt->format.code;
-		fmt->format = q->subdev_fmt;
+	fmt->format.code = formats[0].mbus_code;
+
+	for (i = 0; i < ARRAY_SIZE(formats); i++) {
+		if (formats[i].mbus_code == fmt->format.code) {
+			fmt->format.code = mbus_code;
+			break;
+		}
 	}
 
+	fmt->format.width = min(fmt->format.width, CIO2_IMAGE_MAX_WIDTH);
+	fmt->format.height = min(fmt->format.height, CIO2_IMAGE_MAX_LENGTH);
+
+	mutex_lock(&q->subdev_lock);
+	*mbus = fmt->format;
 	mutex_unlock(&q->subdev_lock);
 
 	return 0;
-- 
2.27.0

[PATCH 5/5] ipu3-cio2: Make the field on subdev format V4L2_FIELD_NONE

[Sakari Ailus]

The ipu3-cio2 doesn't make use of the field and this is reflected in V4L2
buffers as well as the try format. Do this in active format, too.

Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 9726091c9985..a821c40627f9 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1284,6 +1284,7 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,
 
 	fmt->format.width = min(fmt->format.width, CIO2_IMAGE_MAX_WIDTH);
 	fmt->format.height = min(fmt->format.height, CIO2_IMAGE_MAX_LENGTH);
+	fmt->format.field = V4L2_FIELD_NONE;
 
 	mutex_lock(&q->subdev_lock);
 	*mbus = fmt->format;
-- 
2.27.0

Re: [PATCH 0/5] ipu3-cio2 format fixes

[Sakari Ailus]

On Fri, Oct 09, 2020 at 06:07:51PM +0300, Sakari Ailus wrote:
> Hello all,
> 
> This set addresses most notable subdev format related issues, namely the
> sub-device sink format being unaccessible. The result of accessing it
> varied from oopses to crashes.
> 
> since v1:
> 
> - Validate try format, too
> 
> - Set field in subdev format to V4L2_FIELD_NONE
> 
> - Add a comment explaining the lock
> 
> - Make values that should be unsigned, unsigned

This is obviously v2. v1 is here:

<URL:https://lore.kernel.org/linux-media/20201008204747.26320-1-sakari.ailus@linux.intel.com/T/#t>

-- 
Sakari Ailus

Re: [PATCH 2/5] ipu3-cio2: Serialise access to pad format

[Andy Shevchenko]

On Fri, Oct 9, 2020 at 6:10 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Pad format can be accessed from user space. Serialise access to it.

Same nit-picks as per v1, feel free to ignore it.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] ipu3-cio2: Validate mbus format in setting subdev format

[Andy Shevchenko]

On Fri, Oct 9, 2020 at 6:10 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Validate media bus code, width and height when setting the subdev format.
>
> This effectively reworks how setting subdev format is implemented in the
> driver.

Does it make it dependent on patch 3/5?
Or maybe you can use min_t() and update to min() in a separate patch?

> Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 28 ++++++++++++++++--------
>  1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 9c7b527a8800..9726091c9985 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1257,6 +1257,9 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,
>                                struct v4l2_subdev_format *fmt)
>  {
>         struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev);
> +       struct v4l2_mbus_framefmt *mbus;
> +       u32 mbus_code = fmt->format.code;
> +       unsigned int i;
>
>         /*
>          * Only allow setting sink pad format;
> @@ -1265,18 +1268,25 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,
>         if (fmt->pad == CIO2_PAD_SOURCE)
>                 return cio2_subdev_get_fmt(sd, cfg, fmt);
>
> -       mutex_lock(&q->subdev_lock);
> +       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> +               mbus = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> +       else
> +               mbus = &q->subdev_fmt;
>
> -       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> -               *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format;
> -       } else {
> -               /* It's the sink, allow changing frame size */
> -               q->subdev_fmt.width = fmt->format.width;
> -               q->subdev_fmt.height = fmt->format.height;
> -               q->subdev_fmt.code = fmt->format.code;
> -               fmt->format = q->subdev_fmt;
> +       fmt->format.code = formats[0].mbus_code;
> +
> +       for (i = 0; i < ARRAY_SIZE(formats); i++) {
> +               if (formats[i].mbus_code == fmt->format.code) {
> +                       fmt->format.code = mbus_code;
> +                       break;
> +               }
>         }
>
> +       fmt->format.width = min(fmt->format.width, CIO2_IMAGE_MAX_WIDTH);
> +       fmt->format.height = min(fmt->format.height, CIO2_IMAGE_MAX_LENGTH);
> +
> +       mutex_lock(&q->subdev_lock);
> +       *mbus = fmt->format;
>         mutex_unlock(&q->subdev_lock);
>
>         return 0;
> --
> 2.27.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] ipu3-cio2 format fixes

[Andy Shevchenko]

On Fri, Oct 9, 2020 at 6:11 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> On Fri, Oct 09, 2020 at 06:07:51PM +0300, Sakari Ailus wrote:
> > Hello all,
> >
> > This set addresses most notable subdev format related issues, namely the
> > sub-device sink format being unaccessible. The result of accessing it
> > varied from oopses to crashes.
> >
> > since v1:
> >
> > - Validate try format, too
> >
> > - Set field in subdev format to V4L2_FIELD_NONE
> >
> > - Add a comment explaining the lock
> >
> > - Make values that should be unsigned, unsigned
>
> This is obviously v2. v1 is here:

v2 is good enough, but few nit-picks here and there. In any case
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
> <URL:https://lore.kernel.org/linux-media/20201008204747.26320-1-sakari.ailus@linux.intel.com/T/#t>
>
> --
> Sakari Ailus



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/5] ipu3-cio2: Make the field on subdev format V4L2_FIELD_NONE

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Fri, Oct 09, 2020 at 06:07:56PM +0300, Sakari Ailus wrote:
> The ipu3-cio2 doesn't make use of the field and this is reflected in V4L2
> buffers as well as the try format. Do this in active format, too.
> 
> Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

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

> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 9726091c9985..a821c40627f9 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1284,6 +1284,7 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,
>  
>  	fmt->format.width = min(fmt->format.width, CIO2_IMAGE_MAX_WIDTH);
>  	fmt->format.height = min(fmt->format.height, CIO2_IMAGE_MAX_LENGTH);
> +	fmt->format.field = V4L2_FIELD_NONE;
>  
>  	mutex_lock(&q->subdev_lock);
>  	*mbus = fmt->format;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/5] ipu3-cio2: Validate mbus format in setting subdev format

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Fri, Oct 09, 2020 at 06:07:55PM +0300, Sakari Ailus wrote:
> Validate media bus code, width and height when setting the subdev format.
> 
> This effectively reworks how setting subdev format is implemented in the
> driver.
> 
> Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 28 ++++++++++++++++--------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 9c7b527a8800..9726091c9985 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1257,6 +1257,9 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,
>  			       struct v4l2_subdev_format *fmt)
>  {
>  	struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev);
> +	struct v4l2_mbus_framefmt *mbus;
> +	u32 mbus_code = fmt->format.code;
> +	unsigned int i;
>  
>  	/*
>  	 * Only allow setting sink pad format;
> @@ -1265,18 +1268,25 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,
>  	if (fmt->pad == CIO2_PAD_SOURCE)
>  		return cio2_subdev_get_fmt(sd, cfg, fmt);
>  
> -	mutex_lock(&q->subdev_lock);
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> +		mbus = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> +	else
> +		mbus = &q->subdev_fmt;
>  
> -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		*v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format;
> -	} else {
> -		/* It's the sink, allow changing frame size */
> -		q->subdev_fmt.width = fmt->format.width;
> -		q->subdev_fmt.height = fmt->format.height;
> -		q->subdev_fmt.code = fmt->format.code;
> -		fmt->format = q->subdev_fmt;
> +	fmt->format.code = formats[0].mbus_code;
> +
> +	for (i = 0; i < ARRAY_SIZE(formats); i++) {
> +		if (formats[i].mbus_code == fmt->format.code) {
> +			fmt->format.code = mbus_code;
> +			break;
> +		}
>  	}

I really, really wish C had a for...else construct as in Python :-(

>  
> +	fmt->format.width = min(fmt->format.width, CIO2_IMAGE_MAX_WIDTH);
> +	fmt->format.height = min(fmt->format.height, CIO2_IMAGE_MAX_LENGTH);

This looks good, but it would be worth renaming CIO2_IMAGE_MAX_LENGTH to
CIO2_IMAGE_MAX_HEIGHT in a separate patch.

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

> +
> +	mutex_lock(&q->subdev_lock);
> +	*mbus = fmt->format;
>  	mutex_unlock(&q->subdev_lock);
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] ipu3-cio2: Use unsigned values where appropriate

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Fri, Oct 09, 2020 at 06:07:54PM +0300, Sakari Ailus wrote:
> Use unsigned values for width, height, bit shifts and registers,
> effectively for all definitions that are not signed.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.h | 156 +++++++++++------------
>  1 file changed, 78 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> index 146492383aa5..7650d7998a3f 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> @@ -13,20 +13,20 @@
>  #define CIO2_PCI_BAR					0
>  #define CIO2_DMA_MASK					DMA_BIT_MASK(39)
>  
> -#define CIO2_IMAGE_MAX_WIDTH				4224
> -#define CIO2_IMAGE_MAX_LENGTH				3136
> +#define CIO2_IMAGE_MAX_WIDTH				4224U
> +#define CIO2_IMAGE_MAX_LENGTH				3136U
>  
>  /* 32MB = 8xFBPT_entry */
>  #define CIO2_MAX_LOPS					8
>  #define CIO2_MAX_BUFFERS			(PAGE_SIZE / 16 / CIO2_MAX_LOPS)
>  #define CIO2_LOP_ENTRIES			(PAGE_SIZE / sizeof(u32))
>  
> -#define CIO2_PAD_SINK					0
> -#define CIO2_PAD_SOURCE					1
> -#define CIO2_PADS					2
> +#define CIO2_PAD_SINK					0U
> +#define CIO2_PAD_SOURCE					1U
> +#define CIO2_PADS					2U

I would have done this only for values that are meant to be used in
arithmetic expressions, such as CIO2_IMAGE_MAX_WIDTH and
CIO2_IMAGE_MAX_LENGTH. Up to you.

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

>  
> -#define CIO2_NUM_DMA_CHAN				20
> -#define CIO2_NUM_PORTS					4 /* DPHYs */
> +#define CIO2_NUM_DMA_CHAN				20U
> +#define CIO2_NUM_PORTS					4U /* DPHYs */
>  
>  /* 1 for each sensor */
>  #define CIO2_QUEUES					CIO2_NUM_PORTS
> @@ -66,12 +66,12 @@
>  #define CIO2_REG_MIPIBE_FORCE_RAW8	(CIO2_REG_MIPIBE_BASE + 0x20)
>  #define CIO2_REG_MIPIBE_FORCE_RAW8_ENABLE		BIT(0)
>  #define CIO2_REG_MIPIBE_FORCE_RAW8_USE_TYPEID		BIT(1)
> -#define CIO2_REG_MIPIBE_FORCE_RAW8_TYPEID_SHIFT		2
> +#define CIO2_REG_MIPIBE_FORCE_RAW8_TYPEID_SHIFT		2U
>  
>  #define CIO2_REG_MIPIBE_IRQ_STATUS	(CIO2_REG_MIPIBE_BASE + 0x24)
>  #define CIO2_REG_MIPIBE_IRQ_CLEAR	(CIO2_REG_MIPIBE_BASE + 0x28)
>  #define CIO2_REG_MIPIBE_GLOBAL_LUT_DISREGARD (CIO2_REG_MIPIBE_BASE + 0x68)
> -#define CIO2_MIPIBE_GLOBAL_LUT_DISREGARD		1
> +#define CIO2_MIPIBE_GLOBAL_LUT_DISREGARD		1U
>  #define CIO2_REG_MIPIBE_PKT_STALL_STATUS (CIO2_REG_MIPIBE_BASE + 0x6c)
>  #define CIO2_REG_MIPIBE_PARSE_GSP_THROUGH_LP_LUT_REG_IDX \
>  					(CIO2_REG_MIPIBE_BASE + 0x70)
> @@ -79,10 +79,10 @@
>  				       (CIO2_REG_MIPIBE_BASE + 0x74 + 4 * (vc))
>  #define CIO2_REG_MIPIBE_LP_LUT_ENTRY(m)	/* m = 0..15 */ \
>  					(CIO2_REG_MIPIBE_BASE + 0x84 + 4 * (m))
> -#define CIO2_MIPIBE_LP_LUT_ENTRY_DISREGARD		1
> -#define CIO2_MIPIBE_LP_LUT_ENTRY_SID_SHIFT		1
> -#define CIO2_MIPIBE_LP_LUT_ENTRY_VC_SHIFT		5
> -#define CIO2_MIPIBE_LP_LUT_ENTRY_FORMAT_TYPE_SHIFT	7
> +#define CIO2_MIPIBE_LP_LUT_ENTRY_DISREGARD		1U
> +#define CIO2_MIPIBE_LP_LUT_ENTRY_SID_SHIFT		1U
> +#define CIO2_MIPIBE_LP_LUT_ENTRY_VC_SHIFT		5U
> +#define CIO2_MIPIBE_LP_LUT_ENTRY_FORMAT_TYPE_SHIFT	7U
>  
>  /* base register: CIO2_REG_PIPE_BASE(pipe) * CIO2_REG_IRQCTRL_BASE */
>  /* IRQ registers are 18-bit wide, see cio2_irq_error for bit definitions */
> @@ -113,31 +113,31 @@
>  #define CIO2_CGC_ROSC_DCGE				BIT(12)
>  #define CIO2_CGC_XOSC_DCGE				BIT(13)
>  #define CIO2_CGC_FLIS_DCGE				BIT(14)
> -#define CIO2_CGC_CLKGATE_HOLDOFF_SHIFT			20
> -#define CIO2_CGC_CSI_CLKGATE_HOLDOFF_SHIFT		24
> +#define CIO2_CGC_CLKGATE_HOLDOFF_SHIFT			20U
> +#define CIO2_CGC_CSI_CLKGATE_HOLDOFF_SHIFT		24U
>  #define CIO2_REG_D0I3C					0x1408
>  #define CIO2_D0I3C_I3					BIT(2)	/* Set D0I3 */
>  #define CIO2_D0I3C_RR					BIT(3)	/* Restore? */
>  #define CIO2_REG_SWRESET				0x140c
> -#define CIO2_SWRESET_SWRESET				1
> +#define CIO2_SWRESET_SWRESET				1U
>  #define CIO2_REG_SENSOR_ACTIVE				0x1410
>  #define CIO2_REG_INT_STS				0x1414
>  #define CIO2_REG_INT_STS_EXT_OE				0x1418
> -#define CIO2_INT_EXT_OE_DMAOE_SHIFT			0
> +#define CIO2_INT_EXT_OE_DMAOE_SHIFT			0U
>  #define CIO2_INT_EXT_OE_DMAOE_MASK			0x7ffff
> -#define CIO2_INT_EXT_OE_OES_SHIFT			24
> +#define CIO2_INT_EXT_OE_OES_SHIFT			24U
>  #define CIO2_INT_EXT_OE_OES_MASK	(0xf << CIO2_INT_EXT_OE_OES_SHIFT)
>  #define CIO2_REG_INT_EN					0x1420
>  #define CIO2_REG_INT_EN_IRQ				(1 << 24)
> -#define CIO2_REG_INT_EN_IOS(dma)	(1 << (((dma) >> 1) + 12))
> +#define CIO2_REG_INT_EN_IOS(dma)	(1U << (((dma) >> 1U) + 12U))
>  /*
>   * Interrupt on completion bit, Eg. DMA 0-3 maps to bit 0-3,
>   * DMA4 & DMA5 map to bit 4 ... DMA18 & DMA19 map to bit 11 Et cetera
>   */
> -#define CIO2_INT_IOC(dma)	(1 << ((dma) < 4 ? (dma) : ((dma) >> 1) + 2))
> +#define CIO2_INT_IOC(dma)	(1U << ((dma) < 4U ? (dma) : ((dma) >> 1U) + 2U))
>  #define CIO2_INT_IOC_SHIFT				0
>  #define CIO2_INT_IOC_MASK		(0x7ff << CIO2_INT_IOC_SHIFT)
> -#define CIO2_INT_IOS_IOLN(dma)		(1 << (((dma) >> 1) + 12))
> +#define CIO2_INT_IOS_IOLN(dma)		(1U << (((dma) >> 1U) + 12U))
>  #define CIO2_INT_IOS_IOLN_SHIFT				12
>  #define CIO2_INT_IOS_IOLN_MASK		(0x3ff << CIO2_INT_IOS_IOLN_SHIFT)
>  #define CIO2_INT_IOIE					BIT(22)
> @@ -145,32 +145,32 @@
>  #define CIO2_INT_IOIRQ					BIT(24)
>  #define CIO2_REG_INT_EN_EXT_OE				0x1424
>  #define CIO2_REG_DMA_DBG				0x1448
> -#define CIO2_REG_DMA_DBG_DMA_INDEX_SHIFT		0
> +#define CIO2_REG_DMA_DBG_DMA_INDEX_SHIFT		0U
>  #define CIO2_REG_PBM_ARB_CTRL				0x1460
> -#define CIO2_PBM_ARB_CTRL_LANES_DIV			0 /* 4-4-2-2 lanes */
> -#define CIO2_PBM_ARB_CTRL_LANES_DIV_SHIFT		0
> +#define CIO2_PBM_ARB_CTRL_LANES_DIV			0U /* 4-4-2-2 lanes */
> +#define CIO2_PBM_ARB_CTRL_LANES_DIV_SHIFT		0U
>  #define CIO2_PBM_ARB_CTRL_LE_EN				BIT(7)
> -#define CIO2_PBM_ARB_CTRL_PLL_POST_SHTDN		2
> -#define CIO2_PBM_ARB_CTRL_PLL_POST_SHTDN_SHIFT		8
> -#define CIO2_PBM_ARB_CTRL_PLL_AHD_WK_UP			480
> -#define CIO2_PBM_ARB_CTRL_PLL_AHD_WK_UP_SHIFT		16
> +#define CIO2_PBM_ARB_CTRL_PLL_POST_SHTDN		2U
> +#define CIO2_PBM_ARB_CTRL_PLL_POST_SHTDN_SHIFT		8U
> +#define CIO2_PBM_ARB_CTRL_PLL_AHD_WK_UP			480U
> +#define CIO2_PBM_ARB_CTRL_PLL_AHD_WK_UP_SHIFT		16U
>  #define CIO2_REG_PBM_WMCTRL1				0x1464
> -#define CIO2_PBM_WMCTRL1_MIN_2CK_SHIFT			0
> -#define CIO2_PBM_WMCTRL1_MID1_2CK_SHIFT			8
> -#define CIO2_PBM_WMCTRL1_MID2_2CK_SHIFT			16
> +#define CIO2_PBM_WMCTRL1_MIN_2CK_SHIFT			0U
> +#define CIO2_PBM_WMCTRL1_MID1_2CK_SHIFT			8U
> +#define CIO2_PBM_WMCTRL1_MID2_2CK_SHIFT			16U
>  #define CIO2_PBM_WMCTRL1_TS_COUNT_DISABLE		BIT(31)
>  #define CIO2_PBM_WMCTRL1_MIN_2CK	(4 << CIO2_PBM_WMCTRL1_MIN_2CK_SHIFT)
>  #define CIO2_PBM_WMCTRL1_MID1_2CK	(16 << CIO2_PBM_WMCTRL1_MID1_2CK_SHIFT)
>  #define CIO2_PBM_WMCTRL1_MID2_2CK	(21 << CIO2_PBM_WMCTRL1_MID2_2CK_SHIFT)
>  #define CIO2_REG_PBM_WMCTRL2				0x1468
> -#define CIO2_PBM_WMCTRL2_HWM_2CK			40
> -#define CIO2_PBM_WMCTRL2_HWM_2CK_SHIFT			0
> -#define CIO2_PBM_WMCTRL2_LWM_2CK			22
> -#define CIO2_PBM_WMCTRL2_LWM_2CK_SHIFT			8
> -#define CIO2_PBM_WMCTRL2_OBFFWM_2CK			2
> -#define CIO2_PBM_WMCTRL2_OBFFWM_2CK_SHIFT		16
> -#define CIO2_PBM_WMCTRL2_TRANSDYN			1
> -#define CIO2_PBM_WMCTRL2_TRANSDYN_SHIFT			24
> +#define CIO2_PBM_WMCTRL2_HWM_2CK			40U
> +#define CIO2_PBM_WMCTRL2_HWM_2CK_SHIFT			0U
> +#define CIO2_PBM_WMCTRL2_LWM_2CK			22U
> +#define CIO2_PBM_WMCTRL2_LWM_2CK_SHIFT			8U
> +#define CIO2_PBM_WMCTRL2_OBFFWM_2CK			2U
> +#define CIO2_PBM_WMCTRL2_OBFFWM_2CK_SHIFT		16U
> +#define CIO2_PBM_WMCTRL2_TRANSDYN			1U
> +#define CIO2_PBM_WMCTRL2_TRANSDYN_SHIFT			24U
>  #define CIO2_PBM_WMCTRL2_DYNWMEN			BIT(28)
>  #define CIO2_PBM_WMCTRL2_OBFF_MEM_EN			BIT(29)
>  #define CIO2_PBM_WMCTRL2_OBFF_CPU_EN			BIT(30)
> @@ -178,12 +178,12 @@
>  #define CIO2_REG_PBM_TS_COUNT				0x146c
>  #define CIO2_REG_PBM_FOPN_ABORT				0x1474
>  /* below n = 0..3 */
> -#define CIO2_PBM_FOPN_ABORT(n)				(0x1 << 8 * (n))
> -#define CIO2_PBM_FOPN_FORCE_ABORT(n)			(0x2 << 8 * (n))
> -#define CIO2_PBM_FOPN_FRAMEOPEN(n)			(0x8 << 8 * (n))
> +#define CIO2_PBM_FOPN_ABORT(n)				(0x1 << 8U * (n))
> +#define CIO2_PBM_FOPN_FORCE_ABORT(n)			(0x2 << 8U * (n))
> +#define CIO2_PBM_FOPN_FRAMEOPEN(n)			(0x8 << 8U * (n))
>  #define CIO2_REG_LTRCTRL				0x1480
>  #define CIO2_LTRCTRL_LTRDYNEN				BIT(16)
> -#define CIO2_LTRCTRL_LTRSTABLETIME_SHIFT		8
> +#define CIO2_LTRCTRL_LTRSTABLETIME_SHIFT		8U
>  #define CIO2_LTRCTRL_LTRSTABLETIME_MASK			0xff
>  #define CIO2_LTRCTRL_LTRSEL1S3				BIT(7)
>  #define CIO2_LTRCTRL_LTRSEL1S2				BIT(6)
> @@ -195,28 +195,28 @@
>  #define CIO2_LTRCTRL_LTRSEL2S0				BIT(0)
>  #define CIO2_REG_LTRVAL23				0x1484
>  #define CIO2_REG_LTRVAL01				0x1488
> -#define CIO2_LTRVAL02_VAL_SHIFT				0
> -#define CIO2_LTRVAL02_SCALE_SHIFT			10
> -#define CIO2_LTRVAL13_VAL_SHIFT				16
> -#define CIO2_LTRVAL13_SCALE_SHIFT			26
> +#define CIO2_LTRVAL02_VAL_SHIFT				0U
> +#define CIO2_LTRVAL02_SCALE_SHIFT			10U
> +#define CIO2_LTRVAL13_VAL_SHIFT				16U
> +#define CIO2_LTRVAL13_SCALE_SHIFT			26U
>  
> -#define CIO2_LTRVAL0_VAL				175
> +#define CIO2_LTRVAL0_VAL				175U
>  /* Value times 1024 ns */
> -#define CIO2_LTRVAL0_SCALE				2
> -#define CIO2_LTRVAL1_VAL				90
> -#define CIO2_LTRVAL1_SCALE				2
> -#define CIO2_LTRVAL2_VAL				90
> -#define CIO2_LTRVAL2_SCALE				2
> -#define CIO2_LTRVAL3_VAL				90
> -#define CIO2_LTRVAL3_SCALE				2
> +#define CIO2_LTRVAL0_SCALE				2U
> +#define CIO2_LTRVAL1_VAL				90U
> +#define CIO2_LTRVAL1_SCALE				2U
> +#define CIO2_LTRVAL2_VAL				90U
> +#define CIO2_LTRVAL2_SCALE				2U
> +#define CIO2_LTRVAL3_VAL				90U
> +#define CIO2_LTRVAL3_SCALE				2U
>  
>  #define CIO2_REG_CDMABA(n)		(0x1500 + 0x10 * (n))	/* n = 0..19 */
>  #define CIO2_REG_CDMARI(n)		(0x1504 + 0x10 * (n))
> -#define CIO2_CDMARI_FBPT_RP_SHIFT			0
> +#define CIO2_CDMARI_FBPT_RP_SHIFT			0U
>  #define CIO2_CDMARI_FBPT_RP_MASK			0xff
>  #define CIO2_REG_CDMAC0(n)		(0x1508 + 0x10 * (n))
> -#define CIO2_CDMAC0_FBPT_LEN_SHIFT			0
> -#define CIO2_CDMAC0_FBPT_WIDTH_SHIFT			8
> +#define CIO2_CDMAC0_FBPT_LEN_SHIFT			0U
> +#define CIO2_CDMAC0_FBPT_WIDTH_SHIFT			8U
>  #define CIO2_CDMAC0_FBPT_NS				BIT(25)
>  #define CIO2_CDMAC0_DMA_INTR_ON_FS			BIT(26)
>  #define CIO2_CDMAC0_DMA_INTR_ON_FE			BIT(27)
> @@ -225,12 +225,12 @@
>  #define CIO2_CDMAC0_DMA_EN				BIT(30)
>  #define CIO2_CDMAC0_DMA_HALTED				BIT(31)
>  #define CIO2_REG_CDMAC1(n)		(0x150c + 0x10 * (n))
> -#define CIO2_CDMAC1_LINENUMINT_SHIFT			0
> -#define CIO2_CDMAC1_LINENUMUPDATE_SHIFT			16
> +#define CIO2_CDMAC1_LINENUMINT_SHIFT			0U
> +#define CIO2_CDMAC1_LINENUMUPDATE_SHIFT			16U
>  /* n = 0..3 */
>  #define CIO2_REG_PXM_PXF_FMT_CFG0(n)	(0x1700 + 0x30 * (n))
> -#define CIO2_PXM_PXF_FMT_CFG_SID0_SHIFT			0
> -#define CIO2_PXM_PXF_FMT_CFG_SID1_SHIFT			16
> +#define CIO2_PXM_PXF_FMT_CFG_SID0_SHIFT			0U
> +#define CIO2_PXM_PXF_FMT_CFG_SID1_SHIFT			16U
>  #define CIO2_PXM_PXF_FMT_CFG_PCK_64B			(0 << 0)
>  #define CIO2_PXM_PXF_FMT_CFG_PCK_32B			(1 << 0)
>  #define CIO2_PXM_PXF_FMT_CFG_BPP_08			(0 << 2)
> @@ -249,27 +249,27 @@
>  #define CIO2_PXM_PXF_FMT_CFG_PSWAP4_2ND_BD		(1 << 10)
>  #define CIO2_REG_INT_STS_EXT_IE				0x17e4
>  #define CIO2_REG_INT_EN_EXT_IE				0x17e8
> -#define CIO2_INT_EXT_IE_ECC_RE(n)			(0x01 << (8 * (n)))
> -#define CIO2_INT_EXT_IE_DPHY_NR(n)			(0x02 << (8 * (n)))
> -#define CIO2_INT_EXT_IE_ECC_NR(n)			(0x04 << (8 * (n)))
> -#define CIO2_INT_EXT_IE_CRCERR(n)			(0x08 << (8 * (n)))
> -#define CIO2_INT_EXT_IE_INTERFRAMEDATA(n)		(0x10 << (8 * (n)))
> -#define CIO2_INT_EXT_IE_PKT2SHORT(n)			(0x20 << (8 * (n)))
> -#define CIO2_INT_EXT_IE_PKT2LONG(n)			(0x40 << (8 * (n)))
> -#define CIO2_INT_EXT_IE_IRQ(n)				(0x80 << (8 * (n)))
> +#define CIO2_INT_EXT_IE_ECC_RE(n)			(0x01 << (8U * (n)))
> +#define CIO2_INT_EXT_IE_DPHY_NR(n)			(0x02 << (8U * (n)))
> +#define CIO2_INT_EXT_IE_ECC_NR(n)			(0x04 << (8U * (n)))
> +#define CIO2_INT_EXT_IE_CRCERR(n)			(0x08 << (8U * (n)))
> +#define CIO2_INT_EXT_IE_INTERFRAMEDATA(n)		(0x10 << (8U * (n)))
> +#define CIO2_INT_EXT_IE_PKT2SHORT(n)			(0x20 << (8U * (n)))
> +#define CIO2_INT_EXT_IE_PKT2LONG(n)			(0x40 << (8U * (n)))
> +#define CIO2_INT_EXT_IE_IRQ(n)				(0x80 << (8U * (n)))
>  #define CIO2_REG_PXM_FRF_CFG(n)				(0x1720 + 0x30 * (n))
>  #define CIO2_PXM_FRF_CFG_FNSEL				BIT(0)
>  #define CIO2_PXM_FRF_CFG_FN_RST				BIT(1)
>  #define CIO2_PXM_FRF_CFG_ABORT				BIT(2)
> -#define CIO2_PXM_FRF_CFG_CRC_TH_SHIFT			3
> +#define CIO2_PXM_FRF_CFG_CRC_TH_SHIFT			3U
>  #define CIO2_PXM_FRF_CFG_MSK_ECC_DPHY_NR		BIT(8)
>  #define CIO2_PXM_FRF_CFG_MSK_ECC_RE			BIT(9)
>  #define CIO2_PXM_FRF_CFG_MSK_ECC_DPHY_NE		BIT(10)
> -#define CIO2_PXM_FRF_CFG_EVEN_ODD_MODE_SHIFT		11
> +#define CIO2_PXM_FRF_CFG_EVEN_ODD_MODE_SHIFT		11U
>  #define CIO2_PXM_FRF_CFG_MASK_CRC_THRES			BIT(13)
>  #define CIO2_PXM_FRF_CFG_MASK_CSI_ACCEPT		BIT(14)
>  #define CIO2_PXM_FRF_CFG_CIOHC_FS_MODE			BIT(15)
> -#define CIO2_PXM_FRF_CFG_CIOHC_FRST_FRM_SHIFT		16
> +#define CIO2_PXM_FRF_CFG_CIOHC_FRST_FRM_SHIFT		16U
>  #define CIO2_REG_PXM_SID2BID0(n)			(0x1724 + 0x30 * (n))
>  #define CIO2_FB_HPLL_FREQ				0x2
>  #define CIO2_ISCLK_RATIO				0xc
> @@ -278,14 +278,14 @@
>  
>  #define CIO2_INT_EN_EXT_OE_MASK				0x8f0fffff
>  
> -#define CIO2_CGC_CLKGATE_HOLDOFF			3
> -#define CIO2_CGC_CSI_CLKGATE_HOLDOFF			5
> +#define CIO2_CGC_CLKGATE_HOLDOFF			3U
> +#define CIO2_CGC_CSI_CLKGATE_HOLDOFF			5U
>  
>  #define CIO2_PXM_FRF_CFG_CRC_TH				16
>  
>  #define CIO2_INT_EN_EXT_IE_MASK				0xffffffff
>  
> -#define CIO2_DMA_CHAN					0
> +#define CIO2_DMA_CHAN					0U
>  
>  #define CIO2_CSIRX_DLY_CNT_CLANE_IDX			-1
>  
> @@ -302,8 +302,8 @@
>  #define CIO2_CSIRX_DLY_CNT_TERMEN_DEFAULT		0x4
>  #define CIO2_CSIRX_DLY_CNT_SETTLE_DEFAULT		0x570
>  
> -#define CIO2_PMCSR_OFFSET				4
> -#define CIO2_PMCSR_D0D3_SHIFT				2
> +#define CIO2_PMCSR_OFFSET				4U
> +#define CIO2_PMCSR_D0D3_SHIFT				2U
>  #define CIO2_PMCSR_D3					0x3
>  
>  struct cio2_csi2_timing {

-- 
Regards,

Laurent Pinchart

RE: [PATCH 2/5] ipu3-cio2: Serialise access to pad format

[Cao, Bingbu]

Sakari, thank you for the patch.

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

________________________
BRs,  
Bingbu Cao                          


> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> Sent: Friday, October 9, 2020 11:08 PM
> To: linux-media@vger.kernel.org
> Cc: Tsuchiya Yuto <kitakar@gmail.com>; Cao, Bingbu <bingbu.cao@intel.com>;
> Zhi, Yong <yong.zhi@intel.com>; Qiu, Tian Shu <tian.shu.qiu@intel.com>;
> laurent.pinchart@ideasonboard.com
> Subject: [PATCH 2/5] ipu3-cio2: Serialise access to pad format
> 
> Pad format can be accessed from user space. Serialise access to it.
> 
> Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 11 +++++++++++
> drivers/media/pci/intel/ipu3/ipu3-cio2.h |  1 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index afa472026ba4..9c7b527a8800 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1233,11 +1233,15 @@ static int cio2_subdev_get_fmt(struct v4l2_subdev
> *sd,  {
>  	struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev);
> 
> +	mutex_lock(&q->subdev_lock);
> +
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
>  		fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
>  	else
>  		fmt->format = q->subdev_fmt;
> 
> +	mutex_unlock(&q->subdev_lock);
> +
>  	return 0;
>  }
> 
> @@ -1261,6 +1265,8 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,
>  	if (fmt->pad == CIO2_PAD_SOURCE)
>  		return cio2_subdev_get_fmt(sd, cfg, fmt);
> 
> +	mutex_lock(&q->subdev_lock);
> +
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
>  		*v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format;
>  	} else {
> @@ -1271,6 +1277,8 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,
>  		fmt->format = q->subdev_fmt;
>  	}
> 
> +	mutex_unlock(&q->subdev_lock);
> +
>  	return 0;
>  }
> 
> @@ -1529,6 +1537,7 @@ static int cio2_queue_init(struct cio2_device *cio2,
> struct cio2_queue *q)
> 
>  	/* Initialize miscellaneous variables */
>  	mutex_init(&q->lock);
> +	mutex_init(&q->subdev_lock);
> 
>  	/* Initialize formats to default values */
>  	fmt = &q->subdev_fmt;
> @@ -1646,6 +1655,7 @@ static int cio2_queue_init(struct cio2_device *cio2,
> struct cio2_queue *q)
>  	cio2_fbpt_exit(q, &cio2->pci_dev->dev);
>  fail_fbpt:
>  	mutex_destroy(&q->lock);
> +	mutex_destroy(&q->subdev_lock);
> 
>  	return r;
>  }
> @@ -1658,6 +1668,7 @@ static void cio2_queue_exit(struct cio2_device *cio2,
> struct cio2_queue *q)
>  	media_entity_cleanup(&q->subdev.entity);
>  	cio2_fbpt_exit(q, &cio2->pci_dev->dev);
>  	mutex_destroy(&q->lock);
> +	mutex_destroy(&q->subdev_lock);
>  }
> 
>  static int cio2_queues_init(struct cio2_device *cio2) diff --git
> a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> index 549b08f88f0c..146492383aa5 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> @@ -335,6 +335,7 @@ struct cio2_queue {
> 
>  	/* Subdev, /dev/v4l-subdevX */
>  	struct v4l2_subdev subdev;
> +	struct mutex subdev_lock; /* Serialise acces to subdev_fmt field */
>  	struct media_pad subdev_pads[CIO2_PADS];
>  	struct v4l2_mbus_framefmt subdev_fmt;
>  	atomic_t frame_sequence;
> --
> 2.27.0

RE: [PATCH 5/5] ipu3-cio2: Make the field on subdev format V4L2_FIELD_NONE

[Cao, Bingbu]

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

________________________
BRs,  
Bingbu Cao                          


> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> Sent: Friday, October 9, 2020 11:08 PM
> To: linux-media@vger.kernel.org
> Cc: Tsuchiya Yuto <kitakar@gmail.com>; Cao, Bingbu <bingbu.cao@intel.com>;
> Zhi, Yong <yong.zhi@intel.com>; Qiu, Tian Shu <tian.shu.qiu@intel.com>;
> laurent.pinchart@ideasonboard.com
> Subject: [PATCH 5/5] ipu3-cio2: Make the field on subdev format
> V4L2_FIELD_NONE
> 
> The ipu3-cio2 doesn't make use of the field and this is reflected in V4L2
> buffers as well as the try format. Do this in active format, too.
> 
> Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 9726091c9985..a821c40627f9 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1284,6 +1284,7 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,
> 
>  	fmt->format.width = min(fmt->format.width, CIO2_IMAGE_MAX_WIDTH);
>  	fmt->format.height = min(fmt->format.height, CIO2_IMAGE_MAX_LENGTH);
> +	fmt->format.field = V4L2_FIELD_NONE;
> 
>  	mutex_lock(&q->subdev_lock);
>  	*mbus = fmt->format;
> --
> 2.27.0

RE: [PATCH 1/5] ipu3-cio2: Return actual subdev format

[Cao, Bingbu]

Sakari, thanks for your patch.

Reviewed-by: Bingbu Cao <Bingbu.cao@intel.com>

________________________
BRs,  
Bingbu Cao                          


> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> Sent: Friday, October 9, 2020 11:08 PM
> To: linux-media@vger.kernel.org
> Cc: Tsuchiya Yuto <kitakar@gmail.com>; Cao, Bingbu <bingbu.cao@intel.com>;
> Zhi, Yong <yong.zhi@intel.com>; Qiu, Tian Shu <tian.shu.qiu@intel.com>;
> laurent.pinchart@ideasonboard.com
> Subject: [PATCH 1/5] ipu3-cio2: Return actual subdev format
> 
> Return actual subdev format on ipu3-cio2 subdev pads. The earlier
> implementation was based on an infinite recursion that exhausted the stack.
> 
> Reported-by: Tsuchiya Yuto <kitakar@gmail.com>
> Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 24 +++---------------------
>  1 file changed, 3 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 4e598e937dfe..afa472026ba4 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1232,29 +1232,11 @@ static int cio2_subdev_get_fmt(struct v4l2_subdev
> *sd,
>  			       struct v4l2_subdev_format *fmt)  {
>  	struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev);
> -	struct v4l2_subdev_format format;
> -	int ret;
> 
> -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
>  		fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> -		return 0;
> -	}
> -
> -	if (fmt->pad == CIO2_PAD_SINK) {
> -		format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> -		ret = v4l2_subdev_call(sd, pad, get_fmt, NULL,
> -				       &format);
> -
> -		if (ret)
> -			return ret;
> -		/* update colorspace etc */
> -		q->subdev_fmt.colorspace = format.format.colorspace;
> -		q->subdev_fmt.ycbcr_enc = format.format.ycbcr_enc;
> -		q->subdev_fmt.quantization = format.format.quantization;
> -		q->subdev_fmt.xfer_func = format.format.xfer_func;
> -	}
> -
> -	fmt->format = q->subdev_fmt;
> +	else
> +		fmt->format = q->subdev_fmt;
> 
>  	return 0;
>  }
> --
> 2.27.0

Re: [PATCH 0/5] ipu3-cio2 format fixes

[Sakari Ailus]

Hi Andy,

On Fri, Oct 09, 2020 at 07:23:57PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 9, 2020 at 6:11 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > On Fri, Oct 09, 2020 at 06:07:51PM +0300, Sakari Ailus wrote:
> > > Hello all,
> > >
> > > This set addresses most notable subdev format related issues, namely the
> > > sub-device sink format being unaccessible. The result of accessing it
> > > varied from oopses to crashes.
> > >
> > > since v1:
> > >
> > > - Validate try format, too
> > >
> > > - Set field in subdev format to V4L2_FIELD_NONE
> > >
> > > - Add a comment explaining the lock
> > >
> > > - Make values that should be unsigned, unsigned
> >
> > This is obviously v2. v1 is here:
> 
> v2 is good enough, but few nit-picks here and there. In any case
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thanks!

-- 
Sakari Ailus

Re: [PATCH 3/5] ipu3-cio2: Use unsigned values where appropriate

[Sakari Ailus]

On Fri, Oct 09, 2020 at 08:16:38PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Fri, Oct 09, 2020 at 06:07:54PM +0300, Sakari Ailus wrote:
> > Use unsigned values for width, height, bit shifts and registers,
> > effectively for all definitions that are not signed.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/pci/intel/ipu3/ipu3-cio2.h | 156 +++++++++++------------
> >  1 file changed, 78 insertions(+), 78 deletions(-)
> > 
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> > index 146492383aa5..7650d7998a3f 100644
> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> > @@ -13,20 +13,20 @@
> >  #define CIO2_PCI_BAR					0
> >  #define CIO2_DMA_MASK					DMA_BIT_MASK(39)
> >  
> > -#define CIO2_IMAGE_MAX_WIDTH				4224
> > -#define CIO2_IMAGE_MAX_LENGTH				3136
> > +#define CIO2_IMAGE_MAX_WIDTH				4224U
> > +#define CIO2_IMAGE_MAX_LENGTH				3136U
> >  
> >  /* 32MB = 8xFBPT_entry */
> >  #define CIO2_MAX_LOPS					8
> >  #define CIO2_MAX_BUFFERS			(PAGE_SIZE / 16 / CIO2_MAX_LOPS)
> >  #define CIO2_LOP_ENTRIES			(PAGE_SIZE / sizeof(u32))
> >  
> > -#define CIO2_PAD_SINK					0
> > -#define CIO2_PAD_SOURCE					1
> > -#define CIO2_PADS					2
> > +#define CIO2_PAD_SINK					0U
> > +#define CIO2_PAD_SOURCE					1U
> > +#define CIO2_PADS					2U
> 
> I would have done this only for values that are meant to be used in
> arithmetic expressions, such as CIO2_IMAGE_MAX_WIDTH and
> CIO2_IMAGE_MAX_LENGTH. Up to you.

I'd say the register values are inherently unsigned albeit it doesn't
really matter if the sign bit is there in most cases.

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

Thanks!

> 
> >  
> > -#define CIO2_NUM_DMA_CHAN				20
> > -#define CIO2_NUM_PORTS					4 /* DPHYs */
> > +#define CIO2_NUM_DMA_CHAN				20U
> > +#define CIO2_NUM_PORTS					4U /* DPHYs */
> >  
> >  /* 1 for each sensor */
> >  #define CIO2_QUEUES					CIO2_NUM_PORTS
> > @@ -66,12 +66,12 @@
> >  #define CIO2_REG_MIPIBE_FORCE_RAW8	(CIO2_REG_MIPIBE_BASE + 0x20)
> >  #define CIO2_REG_MIPIBE_FORCE_RAW8_ENABLE		BIT(0)
> >  #define CIO2_REG_MIPIBE_FORCE_RAW8_USE_TYPEID		BIT(1)
> > -#define CIO2_REG_MIPIBE_FORCE_RAW8_TYPEID_SHIFT		2
> > +#define CIO2_REG_MIPIBE_FORCE_RAW8_TYPEID_SHIFT		2U
> >  
> >  #define CIO2_REG_MIPIBE_IRQ_STATUS	(CIO2_REG_MIPIBE_BASE + 0x24)
> >  #define CIO2_REG_MIPIBE_IRQ_CLEAR	(CIO2_REG_MIPIBE_BASE + 0x28)
> >  #define CIO2_REG_MIPIBE_GLOBAL_LUT_DISREGARD (CIO2_REG_MIPIBE_BASE + 0x68)
> > -#define CIO2_MIPIBE_GLOBAL_LUT_DISREGARD		1
> > +#define CIO2_MIPIBE_GLOBAL_LUT_DISREGARD		1U
> >  #define CIO2_REG_MIPIBE_PKT_STALL_STATUS (CIO2_REG_MIPIBE_BASE + 0x6c)
> >  #define CIO2_REG_MIPIBE_PARSE_GSP_THROUGH_LP_LUT_REG_IDX \
> >  					(CIO2_REG_MIPIBE_BASE + 0x70)
> > @@ -79,10 +79,10 @@
> >  				       (CIO2_REG_MIPIBE_BASE + 0x74 + 4 * (vc))
> >  #define CIO2_REG_MIPIBE_LP_LUT_ENTRY(m)	/* m = 0..15 */ \
> >  					(CIO2_REG_MIPIBE_BASE + 0x84 + 4 * (m))
> > -#define CIO2_MIPIBE_LP_LUT_ENTRY_DISREGARD		1
> > -#define CIO2_MIPIBE_LP_LUT_ENTRY_SID_SHIFT		1
> > -#define CIO2_MIPIBE_LP_LUT_ENTRY_VC_SHIFT		5
> > -#define CIO2_MIPIBE_LP_LUT_ENTRY_FORMAT_TYPE_SHIFT	7
> > +#define CIO2_MIPIBE_LP_LUT_ENTRY_DISREGARD		1U
> > +#define CIO2_MIPIBE_LP_LUT_ENTRY_SID_SHIFT		1U
> > +#define CIO2_MIPIBE_LP_LUT_ENTRY_VC_SHIFT		5U
> > +#define CIO2_MIPIBE_LP_LUT_ENTRY_FORMAT_TYPE_SHIFT	7U
> >  
> >  /* base register: CIO2_REG_PIPE_BASE(pipe) * CIO2_REG_IRQCTRL_BASE */
> >  /* IRQ registers are 18-bit wide, see cio2_irq_error for bit definitions */
> > @@ -113,31 +113,31 @@
> >  #define CIO2_CGC_ROSC_DCGE				BIT(12)
> >  #define CIO2_CGC_XOSC_DCGE				BIT(13)
> >  #define CIO2_CGC_FLIS_DCGE				BIT(14)
> > -#define CIO2_CGC_CLKGATE_HOLDOFF_SHIFT			20
> > -#define CIO2_CGC_CSI_CLKGATE_HOLDOFF_SHIFT		24
> > +#define CIO2_CGC_CLKGATE_HOLDOFF_SHIFT			20U
> > +#define CIO2_CGC_CSI_CLKGATE_HOLDOFF_SHIFT		24U
> >  #define CIO2_REG_D0I3C					0x1408
> >  #define CIO2_D0I3C_I3					BIT(2)	/* Set D0I3 */
> >  #define CIO2_D0I3C_RR					BIT(3)	/* Restore? */
> >  #define CIO2_REG_SWRESET				0x140c
> > -#define CIO2_SWRESET_SWRESET				1
> > +#define CIO2_SWRESET_SWRESET				1U
> >  #define CIO2_REG_SENSOR_ACTIVE				0x1410
> >  #define CIO2_REG_INT_STS				0x1414
> >  #define CIO2_REG_INT_STS_EXT_OE				0x1418
> > -#define CIO2_INT_EXT_OE_DMAOE_SHIFT			0
> > +#define CIO2_INT_EXT_OE_DMAOE_SHIFT			0U
> >  #define CIO2_INT_EXT_OE_DMAOE_MASK			0x7ffff
> > -#define CIO2_INT_EXT_OE_OES_SHIFT			24
> > +#define CIO2_INT_EXT_OE_OES_SHIFT			24U
> >  #define CIO2_INT_EXT_OE_OES_MASK	(0xf << CIO2_INT_EXT_OE_OES_SHIFT)
> >  #define CIO2_REG_INT_EN					0x1420
> >  #define CIO2_REG_INT_EN_IRQ				(1 << 24)
> > -#define CIO2_REG_INT_EN_IOS(dma)	(1 << (((dma) >> 1) + 12))
> > +#define CIO2_REG_INT_EN_IOS(dma)	(1U << (((dma) >> 1U) + 12U))
> >  /*
> >   * Interrupt on completion bit, Eg. DMA 0-3 maps to bit 0-3,
> >   * DMA4 & DMA5 map to bit 4 ... DMA18 & DMA19 map to bit 11 Et cetera
> >   */
> > -#define CIO2_INT_IOC(dma)	(1 << ((dma) < 4 ? (dma) : ((dma) >> 1) + 2))
> > +#define CIO2_INT_IOC(dma)	(1U << ((dma) < 4U ? (dma) : ((dma) >> 1U) + 2U))
> >  #define CIO2_INT_IOC_SHIFT				0
> >  #define CIO2_INT_IOC_MASK		(0x7ff << CIO2_INT_IOC_SHIFT)
> > -#define CIO2_INT_IOS_IOLN(dma)		(1 << (((dma) >> 1) + 12))
> > +#define CIO2_INT_IOS_IOLN(dma)		(1U << (((dma) >> 1U) + 12U))
> >  #define CIO2_INT_IOS_IOLN_SHIFT				12
> >  #define CIO2_INT_IOS_IOLN_MASK		(0x3ff << CIO2_INT_IOS_IOLN_SHIFT)
> >  #define CIO2_INT_IOIE					BIT(22)
> > @@ -145,32 +145,32 @@
> >  #define CIO2_INT_IOIRQ					BIT(24)
> >  #define CIO2_REG_INT_EN_EXT_OE				0x1424
> >  #define CIO2_REG_DMA_DBG				0x1448
> > -#define CIO2_REG_DMA_DBG_DMA_INDEX_SHIFT		0
> > +#define CIO2_REG_DMA_DBG_DMA_INDEX_SHIFT		0U
> >  #define CIO2_REG_PBM_ARB_CTRL				0x1460
> > -#define CIO2_PBM_ARB_CTRL_LANES_DIV			0 /* 4-4-2-2 lanes */
> > -#define CIO2_PBM_ARB_CTRL_LANES_DIV_SHIFT		0
> > +#define CIO2_PBM_ARB_CTRL_LANES_DIV			0U /* 4-4-2-2 lanes */
> > +#define CIO2_PBM_ARB_CTRL_LANES_DIV_SHIFT		0U
> >  #define CIO2_PBM_ARB_CTRL_LE_EN				BIT(7)
> > -#define CIO2_PBM_ARB_CTRL_PLL_POST_SHTDN		2
> > -#define CIO2_PBM_ARB_CTRL_PLL_POST_SHTDN_SHIFT		8
> > -#define CIO2_PBM_ARB_CTRL_PLL_AHD_WK_UP			480
> > -#define CIO2_PBM_ARB_CTRL_PLL_AHD_WK_UP_SHIFT		16
> > +#define CIO2_PBM_ARB_CTRL_PLL_POST_SHTDN		2U
> > +#define CIO2_PBM_ARB_CTRL_PLL_POST_SHTDN_SHIFT		8U
> > +#define CIO2_PBM_ARB_CTRL_PLL_AHD_WK_UP			480U
> > +#define CIO2_PBM_ARB_CTRL_PLL_AHD_WK_UP_SHIFT		16U
> >  #define CIO2_REG_PBM_WMCTRL1				0x1464
> > -#define CIO2_PBM_WMCTRL1_MIN_2CK_SHIFT			0
> > -#define CIO2_PBM_WMCTRL1_MID1_2CK_SHIFT			8
> > -#define CIO2_PBM_WMCTRL1_MID2_2CK_SHIFT			16
> > +#define CIO2_PBM_WMCTRL1_MIN_2CK_SHIFT			0U
> > +#define CIO2_PBM_WMCTRL1_MID1_2CK_SHIFT			8U
> > +#define CIO2_PBM_WMCTRL1_MID2_2CK_SHIFT			16U
> >  #define CIO2_PBM_WMCTRL1_TS_COUNT_DISABLE		BIT(31)
> >  #define CIO2_PBM_WMCTRL1_MIN_2CK	(4 << CIO2_PBM_WMCTRL1_MIN_2CK_SHIFT)
> >  #define CIO2_PBM_WMCTRL1_MID1_2CK	(16 << CIO2_PBM_WMCTRL1_MID1_2CK_SHIFT)
> >  #define CIO2_PBM_WMCTRL1_MID2_2CK	(21 << CIO2_PBM_WMCTRL1_MID2_2CK_SHIFT)
> >  #define CIO2_REG_PBM_WMCTRL2				0x1468
> > -#define CIO2_PBM_WMCTRL2_HWM_2CK			40
> > -#define CIO2_PBM_WMCTRL2_HWM_2CK_SHIFT			0
> > -#define CIO2_PBM_WMCTRL2_LWM_2CK			22
> > -#define CIO2_PBM_WMCTRL2_LWM_2CK_SHIFT			8
> > -#define CIO2_PBM_WMCTRL2_OBFFWM_2CK			2
> > -#define CIO2_PBM_WMCTRL2_OBFFWM_2CK_SHIFT		16
> > -#define CIO2_PBM_WMCTRL2_TRANSDYN			1
> > -#define CIO2_PBM_WMCTRL2_TRANSDYN_SHIFT			24
> > +#define CIO2_PBM_WMCTRL2_HWM_2CK			40U
> > +#define CIO2_PBM_WMCTRL2_HWM_2CK_SHIFT			0U
> > +#define CIO2_PBM_WMCTRL2_LWM_2CK			22U
> > +#define CIO2_PBM_WMCTRL2_LWM_2CK_SHIFT			8U
> > +#define CIO2_PBM_WMCTRL2_OBFFWM_2CK			2U
> > +#define CIO2_PBM_WMCTRL2_OBFFWM_2CK_SHIFT		16U
> > +#define CIO2_PBM_WMCTRL2_TRANSDYN			1U
> > +#define CIO2_PBM_WMCTRL2_TRANSDYN_SHIFT			24U
> >  #define CIO2_PBM_WMCTRL2_DYNWMEN			BIT(28)
> >  #define CIO2_PBM_WMCTRL2_OBFF_MEM_EN			BIT(29)
> >  #define CIO2_PBM_WMCTRL2_OBFF_CPU_EN			BIT(30)
> > @@ -178,12 +178,12 @@
> >  #define CIO2_REG_PBM_TS_COUNT				0x146c
> >  #define CIO2_REG_PBM_FOPN_ABORT				0x1474
> >  /* below n = 0..3 */
> > -#define CIO2_PBM_FOPN_ABORT(n)				(0x1 << 8 * (n))
> > -#define CIO2_PBM_FOPN_FORCE_ABORT(n)			(0x2 << 8 * (n))
> > -#define CIO2_PBM_FOPN_FRAMEOPEN(n)			(0x8 << 8 * (n))
> > +#define CIO2_PBM_FOPN_ABORT(n)				(0x1 << 8U * (n))
> > +#define CIO2_PBM_FOPN_FORCE_ABORT(n)			(0x2 << 8U * (n))
> > +#define CIO2_PBM_FOPN_FRAMEOPEN(n)			(0x8 << 8U * (n))
> >  #define CIO2_REG_LTRCTRL				0x1480
> >  #define CIO2_LTRCTRL_LTRDYNEN				BIT(16)
> > -#define CIO2_LTRCTRL_LTRSTABLETIME_SHIFT		8
> > +#define CIO2_LTRCTRL_LTRSTABLETIME_SHIFT		8U
> >  #define CIO2_LTRCTRL_LTRSTABLETIME_MASK			0xff
> >  #define CIO2_LTRCTRL_LTRSEL1S3				BIT(7)
> >  #define CIO2_LTRCTRL_LTRSEL1S2				BIT(6)
> > @@ -195,28 +195,28 @@
> >  #define CIO2_LTRCTRL_LTRSEL2S0				BIT(0)
> >  #define CIO2_REG_LTRVAL23				0x1484
> >  #define CIO2_REG_LTRVAL01				0x1488
> > -#define CIO2_LTRVAL02_VAL_SHIFT				0
> > -#define CIO2_LTRVAL02_SCALE_SHIFT			10
> > -#define CIO2_LTRVAL13_VAL_SHIFT				16
> > -#define CIO2_LTRVAL13_SCALE_SHIFT			26
> > +#define CIO2_LTRVAL02_VAL_SHIFT				0U
> > +#define CIO2_LTRVAL02_SCALE_SHIFT			10U
> > +#define CIO2_LTRVAL13_VAL_SHIFT				16U
> > +#define CIO2_LTRVAL13_SCALE_SHIFT			26U
> >  
> > -#define CIO2_LTRVAL0_VAL				175
> > +#define CIO2_LTRVAL0_VAL				175U
> >  /* Value times 1024 ns */
> > -#define CIO2_LTRVAL0_SCALE				2
> > -#define CIO2_LTRVAL1_VAL				90
> > -#define CIO2_LTRVAL1_SCALE				2
> > -#define CIO2_LTRVAL2_VAL				90
> > -#define CIO2_LTRVAL2_SCALE				2
> > -#define CIO2_LTRVAL3_VAL				90
> > -#define CIO2_LTRVAL3_SCALE				2
> > +#define CIO2_LTRVAL0_SCALE				2U
> > +#define CIO2_LTRVAL1_VAL				90U
> > +#define CIO2_LTRVAL1_SCALE				2U
> > +#define CIO2_LTRVAL2_VAL				90U
> > +#define CIO2_LTRVAL2_SCALE				2U
> > +#define CIO2_LTRVAL3_VAL				90U
> > +#define CIO2_LTRVAL3_SCALE				2U
> >  
> >  #define CIO2_REG_CDMABA(n)		(0x1500 + 0x10 * (n))	/* n = 0..19 */
> >  #define CIO2_REG_CDMARI(n)		(0x1504 + 0x10 * (n))
> > -#define CIO2_CDMARI_FBPT_RP_SHIFT			0
> > +#define CIO2_CDMARI_FBPT_RP_SHIFT			0U
> >  #define CIO2_CDMARI_FBPT_RP_MASK			0xff
> >  #define CIO2_REG_CDMAC0(n)		(0x1508 + 0x10 * (n))
> > -#define CIO2_CDMAC0_FBPT_LEN_SHIFT			0
> > -#define CIO2_CDMAC0_FBPT_WIDTH_SHIFT			8
> > +#define CIO2_CDMAC0_FBPT_LEN_SHIFT			0U
> > +#define CIO2_CDMAC0_FBPT_WIDTH_SHIFT			8U
> >  #define CIO2_CDMAC0_FBPT_NS				BIT(25)
> >  #define CIO2_CDMAC0_DMA_INTR_ON_FS			BIT(26)
> >  #define CIO2_CDMAC0_DMA_INTR_ON_FE			BIT(27)
> > @@ -225,12 +225,12 @@
> >  #define CIO2_CDMAC0_DMA_EN				BIT(30)
> >  #define CIO2_CDMAC0_DMA_HALTED				BIT(31)
> >  #define CIO2_REG_CDMAC1(n)		(0x150c + 0x10 * (n))
> > -#define CIO2_CDMAC1_LINENUMINT_SHIFT			0
> > -#define CIO2_CDMAC1_LINENUMUPDATE_SHIFT			16
> > +#define CIO2_CDMAC1_LINENUMINT_SHIFT			0U
> > +#define CIO2_CDMAC1_LINENUMUPDATE_SHIFT			16U
> >  /* n = 0..3 */
> >  #define CIO2_REG_PXM_PXF_FMT_CFG0(n)	(0x1700 + 0x30 * (n))
> > -#define CIO2_PXM_PXF_FMT_CFG_SID0_SHIFT			0
> > -#define CIO2_PXM_PXF_FMT_CFG_SID1_SHIFT			16
> > +#define CIO2_PXM_PXF_FMT_CFG_SID0_SHIFT			0U
> > +#define CIO2_PXM_PXF_FMT_CFG_SID1_SHIFT			16U
> >  #define CIO2_PXM_PXF_FMT_CFG_PCK_64B			(0 << 0)
> >  #define CIO2_PXM_PXF_FMT_CFG_PCK_32B			(1 << 0)
> >  #define CIO2_PXM_PXF_FMT_CFG_BPP_08			(0 << 2)
> > @@ -249,27 +249,27 @@
> >  #define CIO2_PXM_PXF_FMT_CFG_PSWAP4_2ND_BD		(1 << 10)
> >  #define CIO2_REG_INT_STS_EXT_IE				0x17e4
> >  #define CIO2_REG_INT_EN_EXT_IE				0x17e8
> > -#define CIO2_INT_EXT_IE_ECC_RE(n)			(0x01 << (8 * (n)))
> > -#define CIO2_INT_EXT_IE_DPHY_NR(n)			(0x02 << (8 * (n)))
> > -#define CIO2_INT_EXT_IE_ECC_NR(n)			(0x04 << (8 * (n)))
> > -#define CIO2_INT_EXT_IE_CRCERR(n)			(0x08 << (8 * (n)))
> > -#define CIO2_INT_EXT_IE_INTERFRAMEDATA(n)		(0x10 << (8 * (n)))
> > -#define CIO2_INT_EXT_IE_PKT2SHORT(n)			(0x20 << (8 * (n)))
> > -#define CIO2_INT_EXT_IE_PKT2LONG(n)			(0x40 << (8 * (n)))
> > -#define CIO2_INT_EXT_IE_IRQ(n)				(0x80 << (8 * (n)))
> > +#define CIO2_INT_EXT_IE_ECC_RE(n)			(0x01 << (8U * (n)))
> > +#define CIO2_INT_EXT_IE_DPHY_NR(n)			(0x02 << (8U * (n)))
> > +#define CIO2_INT_EXT_IE_ECC_NR(n)			(0x04 << (8U * (n)))
> > +#define CIO2_INT_EXT_IE_CRCERR(n)			(0x08 << (8U * (n)))
> > +#define CIO2_INT_EXT_IE_INTERFRAMEDATA(n)		(0x10 << (8U * (n)))
> > +#define CIO2_INT_EXT_IE_PKT2SHORT(n)			(0x20 << (8U * (n)))
> > +#define CIO2_INT_EXT_IE_PKT2LONG(n)			(0x40 << (8U * (n)))
> > +#define CIO2_INT_EXT_IE_IRQ(n)				(0x80 << (8U * (n)))
> >  #define CIO2_REG_PXM_FRF_CFG(n)				(0x1720 + 0x30 * (n))
> >  #define CIO2_PXM_FRF_CFG_FNSEL				BIT(0)
> >  #define CIO2_PXM_FRF_CFG_FN_RST				BIT(1)
> >  #define CIO2_PXM_FRF_CFG_ABORT				BIT(2)
> > -#define CIO2_PXM_FRF_CFG_CRC_TH_SHIFT			3
> > +#define CIO2_PXM_FRF_CFG_CRC_TH_SHIFT			3U
> >  #define CIO2_PXM_FRF_CFG_MSK_ECC_DPHY_NR		BIT(8)
> >  #define CIO2_PXM_FRF_CFG_MSK_ECC_RE			BIT(9)
> >  #define CIO2_PXM_FRF_CFG_MSK_ECC_DPHY_NE		BIT(10)
> > -#define CIO2_PXM_FRF_CFG_EVEN_ODD_MODE_SHIFT		11
> > +#define CIO2_PXM_FRF_CFG_EVEN_ODD_MODE_SHIFT		11U
> >  #define CIO2_PXM_FRF_CFG_MASK_CRC_THRES			BIT(13)
> >  #define CIO2_PXM_FRF_CFG_MASK_CSI_ACCEPT		BIT(14)
> >  #define CIO2_PXM_FRF_CFG_CIOHC_FS_MODE			BIT(15)
> > -#define CIO2_PXM_FRF_CFG_CIOHC_FRST_FRM_SHIFT		16
> > +#define CIO2_PXM_FRF_CFG_CIOHC_FRST_FRM_SHIFT		16U
> >  #define CIO2_REG_PXM_SID2BID0(n)			(0x1724 + 0x30 * (n))
> >  #define CIO2_FB_HPLL_FREQ				0x2
> >  #define CIO2_ISCLK_RATIO				0xc
> > @@ -278,14 +278,14 @@
> >  
> >  #define CIO2_INT_EN_EXT_OE_MASK				0x8f0fffff
> >  
> > -#define CIO2_CGC_CLKGATE_HOLDOFF			3
> > -#define CIO2_CGC_CSI_CLKGATE_HOLDOFF			5
> > +#define CIO2_CGC_CLKGATE_HOLDOFF			3U
> > +#define CIO2_CGC_CSI_CLKGATE_HOLDOFF			5U
> >  
> >  #define CIO2_PXM_FRF_CFG_CRC_TH				16
> >  
> >  #define CIO2_INT_EN_EXT_IE_MASK				0xffffffff
> >  
> > -#define CIO2_DMA_CHAN					0
> > +#define CIO2_DMA_CHAN					0U
> >  
> >  #define CIO2_CSIRX_DLY_CNT_CLANE_IDX			-1
> >  
> > @@ -302,8 +302,8 @@
> >  #define CIO2_CSIRX_DLY_CNT_TERMEN_DEFAULT		0x4
> >  #define CIO2_CSIRX_DLY_CNT_SETTLE_DEFAULT		0x570
> >  
> > -#define CIO2_PMCSR_OFFSET				4
> > -#define CIO2_PMCSR_D0D3_SHIFT				2
> > +#define CIO2_PMCSR_OFFSET				4U
> > +#define CIO2_PMCSR_D0D3_SHIFT				2U
> >  #define CIO2_PMCSR_D3					0x3
> >  
> >  struct cio2_csi2_timing {
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Sakari Ailus

Re: [PATCH 4/5] ipu3-cio2: Validate mbus format in setting subdev format

[Sakari Ailus]

On Fri, Oct 09, 2020 at 07:22:38PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 9, 2020 at 6:10 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Validate media bus code, width and height when setting the subdev format.
> >
> > This effectively reworks how setting subdev format is implemented in the
> > driver.
> 
> Does it make it dependent on patch 3/5?

Good question. They're in the same set but these probably should be
backported. If fact, I think I should add Cc: stable to most of these
patches.

> Or maybe you can use min_t() and update to min() in a separate patch?

I would have said that's overkill but considering the stable tree it's not.
I'll do that for v3.

Thanks for the review!

-- 
Sakari Ailus