* [PATCH 1/5] ipu3-cio2: Return actual subdev format
2020-10-09 15:07 [PATCH 0/5] ipu3-cio2 format fixes Sakari Ailus
@ 2020-10-09 15:07 ` Sakari Ailus
2020-10-12 1:50 ` Cao, Bingbu
2020-10-09 15:07 ` [PATCH 2/5] ipu3-cio2: Serialise access to pad format Sakari Ailus
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Sakari Ailus @ 2020-10-09 15:07 UTC (permalink / raw)
To: linux-media
Cc: Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu, laurent.pinchart
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* RE: [PATCH 1/5] ipu3-cio2: Return actual subdev format
2020-10-09 15:07 ` [PATCH 1/5] ipu3-cio2: Return actual subdev format Sakari Ailus
@ 2020-10-12 1:50 ` Cao, Bingbu
0 siblings, 0 replies; 19+ messages in thread
From: Cao, Bingbu @ 2020-10-12 1:50 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: Tsuchiya Yuto, Zhi, Yong, Qiu, Tian Shu, laurent.pinchart
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/5] ipu3-cio2: Serialise access to pad format
2020-10-09 15:07 [PATCH 0/5] ipu3-cio2 format fixes Sakari Ailus
2020-10-09 15:07 ` [PATCH 1/5] ipu3-cio2: Return actual subdev format Sakari Ailus
@ 2020-10-09 15:07 ` Sakari Ailus
2020-10-09 16:19 ` Andy Shevchenko
2020-10-12 1:34 ` Cao, Bingbu
2020-10-09 15:07 ` [PATCH 3/5] ipu3-cio2: Use unsigned values where appropriate Sakari Ailus
` (3 subsequent siblings)
5 siblings, 2 replies; 19+ messages in thread
From: Sakari Ailus @ 2020-10-09 15:07 UTC (permalink / raw)
To: linux-media
Cc: Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu, laurent.pinchart
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/5] ipu3-cio2: Serialise access to pad format
2020-10-09 15:07 ` [PATCH 2/5] ipu3-cio2: Serialise access to pad format Sakari Ailus
@ 2020-10-09 16:19 ` Andy Shevchenko
2020-10-12 1:34 ` Cao, Bingbu
1 sibling, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2020-10-09 16:19 UTC (permalink / raw)
To: Sakari Ailus
Cc: Linux Media Mailing List, Tsuchiya Yuto, Bingbu Cao, Yong Zhi,
Tianshu Qiu, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH 2/5] ipu3-cio2: Serialise access to pad format
2020-10-09 15:07 ` [PATCH 2/5] ipu3-cio2: Serialise access to pad format Sakari Ailus
2020-10-09 16:19 ` Andy Shevchenko
@ 2020-10-12 1:34 ` Cao, Bingbu
1 sibling, 0 replies; 19+ messages in thread
From: Cao, Bingbu @ 2020-10-12 1:34 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: Tsuchiya Yuto, Zhi, Yong, Qiu, Tian Shu, laurent.pinchart
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/5] ipu3-cio2: Use unsigned values where appropriate
2020-10-09 15:07 [PATCH 0/5] ipu3-cio2 format fixes Sakari Ailus
2020-10-09 15:07 ` [PATCH 1/5] ipu3-cio2: Return actual subdev format Sakari Ailus
2020-10-09 15:07 ` [PATCH 2/5] ipu3-cio2: Serialise access to pad format Sakari Ailus
@ 2020-10-09 15:07 ` Sakari Ailus
2020-10-09 17:16 ` Laurent Pinchart
2020-10-09 15:07 ` [PATCH 4/5] ipu3-cio2: Validate mbus format in setting subdev format Sakari Ailus
` (2 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Sakari Ailus @ 2020-10-09 15:07 UTC (permalink / raw)
To: linux-media
Cc: Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu, laurent.pinchart
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/5] ipu3-cio2: Use unsigned values where appropriate
2020-10-09 15:07 ` [PATCH 3/5] ipu3-cio2: Use unsigned values where appropriate Sakari Ailus
@ 2020-10-09 17:16 ` Laurent Pinchart
2020-10-12 9:02 ` Sakari Ailus
0 siblings, 1 reply; 19+ messages in thread
From: Laurent Pinchart @ 2020-10-09 17:16 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/5] ipu3-cio2: Use unsigned values where appropriate
2020-10-09 17:16 ` Laurent Pinchart
@ 2020-10-12 9:02 ` Sakari Ailus
0 siblings, 0 replies; 19+ messages in thread
From: Sakari Ailus @ 2020-10-12 9:02 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/5] ipu3-cio2: Validate mbus format in setting subdev format
2020-10-09 15:07 [PATCH 0/5] ipu3-cio2 format fixes Sakari Ailus
` (2 preceding siblings ...)
2020-10-09 15:07 ` [PATCH 3/5] ipu3-cio2: Use unsigned values where appropriate Sakari Ailus
@ 2020-10-09 15:07 ` Sakari Ailus
2020-10-09 16:22 ` Andy Shevchenko
2020-10-09 17:14 ` Laurent Pinchart
2020-10-09 15:07 ` [PATCH 5/5] ipu3-cio2: Make the field on subdev format V4L2_FIELD_NONE Sakari Ailus
2020-10-09 15:08 ` [PATCH 0/5] ipu3-cio2 format fixes Sakari Ailus
5 siblings, 2 replies; 19+ messages in thread
From: Sakari Ailus @ 2020-10-09 15:07 UTC (permalink / raw)
To: linux-media
Cc: Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu, laurent.pinchart
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/5] ipu3-cio2: Validate mbus format in setting subdev format
2020-10-09 15:07 ` [PATCH 4/5] ipu3-cio2: Validate mbus format in setting subdev format Sakari Ailus
@ 2020-10-09 16:22 ` Andy Shevchenko
2020-10-12 9:26 ` Sakari Ailus
2020-10-09 17:14 ` Laurent Pinchart
1 sibling, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2020-10-09 16:22 UTC (permalink / raw)
To: Sakari Ailus
Cc: Linux Media Mailing List, Tsuchiya Yuto, Bingbu Cao, Yong Zhi,
Tianshu Qiu, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/5] ipu3-cio2: Validate mbus format in setting subdev format
2020-10-09 16:22 ` Andy Shevchenko
@ 2020-10-12 9:26 ` Sakari Ailus
0 siblings, 0 replies; 19+ messages in thread
From: Sakari Ailus @ 2020-10-12 9:26 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linux Media Mailing List, Tsuchiya Yuto, Bingbu Cao, Yong Zhi,
Tianshu Qiu, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/5] ipu3-cio2: Validate mbus format in setting subdev format
2020-10-09 15:07 ` [PATCH 4/5] ipu3-cio2: Validate mbus format in setting subdev format Sakari Ailus
2020-10-09 16:22 ` Andy Shevchenko
@ 2020-10-09 17:14 ` Laurent Pinchart
1 sibling, 0 replies; 19+ messages in thread
From: Laurent Pinchart @ 2020-10-09 17:14 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/5] ipu3-cio2: Make the field on subdev format V4L2_FIELD_NONE
2020-10-09 15:07 [PATCH 0/5] ipu3-cio2 format fixes Sakari Ailus
` (3 preceding siblings ...)
2020-10-09 15:07 ` [PATCH 4/5] ipu3-cio2: Validate mbus format in setting subdev format Sakari Ailus
@ 2020-10-09 15:07 ` Sakari Ailus
2020-10-09 17:10 ` Laurent Pinchart
2020-10-12 1:44 ` Cao, Bingbu
2020-10-09 15:08 ` [PATCH 0/5] ipu3-cio2 format fixes Sakari Ailus
5 siblings, 2 replies; 19+ messages in thread
From: Sakari Ailus @ 2020-10-09 15:07 UTC (permalink / raw)
To: linux-media
Cc: Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu, laurent.pinchart
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 5/5] ipu3-cio2: Make the field on subdev format V4L2_FIELD_NONE
2020-10-09 15:07 ` [PATCH 5/5] ipu3-cio2: Make the field on subdev format V4L2_FIELD_NONE Sakari Ailus
@ 2020-10-09 17:10 ` Laurent Pinchart
2020-10-12 1:44 ` Cao, Bingbu
1 sibling, 0 replies; 19+ messages in thread
From: Laurent Pinchart @ 2020-10-09 17:10 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu
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
^ permalink raw reply [flat|nested] 19+ messages in thread* RE: [PATCH 5/5] ipu3-cio2: Make the field on subdev format V4L2_FIELD_NONE
2020-10-09 15:07 ` [PATCH 5/5] ipu3-cio2: Make the field on subdev format V4L2_FIELD_NONE Sakari Ailus
2020-10-09 17:10 ` Laurent Pinchart
@ 2020-10-12 1:44 ` Cao, Bingbu
1 sibling, 0 replies; 19+ messages in thread
From: Cao, Bingbu @ 2020-10-12 1:44 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: Tsuchiya Yuto, Zhi, Yong, Qiu, Tian Shu, laurent.pinchart
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/5] ipu3-cio2 format fixes
2020-10-09 15:07 [PATCH 0/5] ipu3-cio2 format fixes Sakari Ailus
` (4 preceding siblings ...)
2020-10-09 15:07 ` [PATCH 5/5] ipu3-cio2: Make the field on subdev format V4L2_FIELD_NONE Sakari Ailus
@ 2020-10-09 15:08 ` Sakari Ailus
2020-10-09 16:23 ` Andy Shevchenko
5 siblings, 1 reply; 19+ messages in thread
From: Sakari Ailus @ 2020-10-09 15:08 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu,
laurent.pinchart
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 0/5] ipu3-cio2 format fixes
2020-10-09 15:08 ` [PATCH 0/5] ipu3-cio2 format fixes Sakari Ailus
@ 2020-10-09 16:23 ` Andy Shevchenko
2020-10-12 8:13 ` Sakari Ailus
0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2020-10-09 16:23 UTC (permalink / raw)
To: Sakari Ailus
Cc: Sakari Ailus, Linux Media Mailing List, Tsuchiya Yuto,
Bingbu Cao, Yong Zhi, Tianshu Qiu, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/5] ipu3-cio2 format fixes
2020-10-09 16:23 ` Andy Shevchenko
@ 2020-10-12 8:13 ` Sakari Ailus
0 siblings, 0 replies; 19+ messages in thread
From: Sakari Ailus @ 2020-10-12 8:13 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Sakari Ailus, Linux Media Mailing List, Tsuchiya Yuto,
Bingbu Cao, Yong Zhi, Tianshu Qiu, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 19+ messages in thread