1. Patchew
  2. linux
  3. [PATCH v2 0/6] media: ti, cdns: Multiple pixel support and misc fixes
  4. View patch

[PATCH v2 5/6] media: cadence: cdns-csi2rx: Support multiple pixels per clock cycle

Jai Luthra posted 6 patches 8 months, 1 week ago
There is a newer version of this series
[PATCH v2 5/6] media: cadence: cdns-csi2rx: Support multiple pixels per clock cycle
Posted by Jai Luthra 8 months, 1 week ago 
The output pixel interface is a parallel bus (32 bits), which
supports sending multiple pixels (1, 2 or 4) per clock cycle for
smaller pixel widths like RAW8-RAW16.

Dual-pixel and Quad-pixel modes can be a requirement if the export rate
of the Cadence IP in Single-pixel mode maxes out before the maximum
supported DPHY-RX frequency, which is the case with TI's integration of
this IP [1].

So, we export a function that lets the downstream hardware block request
a higher pixel-per-clock on a particular output pad.

We check if we can support the requested pixels per clock given the
known maximum for the currently configured format. If not, we set it
to the highest feasible value and return this value to the caller.

[1] Section 12.6.1.4.8.14 CSI_RX_IF Programming Restrictions of AM62 TRM

Link: https://www.ti.com/lit/pdf/spruj16
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/platform/cadence/cdns-csi2rx.c | 75 +++++++++++++++++++++-------
 drivers/media/platform/cadence/cdns-csi2rx.h | 19 +++++++
 2 files changed, 76 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
index 608298c72462031515d9ad01c6b267bf7375a5bf..154eaacc39ad294db0524e88be888bd0929af071 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2017 Cadence Design Systems Inc.
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/io.h>
@@ -22,6 +23,8 @@
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
+#include "cdns-csi2rx.h"
+
 #define CSI2RX_DEVICE_CFG_REG			0x000
 
 #define CSI2RX_SOFT_RESET_REG			0x004
@@ -53,6 +56,8 @@
 
 #define CSI2RX_STREAM_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x00c)
 #define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF		(1 << 8)
+#define CSI2RX_STREAM_CFG_NUM_PIXELS_MASK		GENMASK(5, 4)
+#define CSI2RX_STREAM_CFG_NUM_PIXELS(n)			((n) >> 1)
 
 #define CSI2RX_LANES_MAX	4
 #define CSI2RX_STREAMS_MAX	4
@@ -68,7 +73,10 @@ enum csi2rx_pads {
 
 struct csi2rx_fmt {
 	u32				code;
+	/* width of a single pixel on CSI-2 bus */
 	u8				bpp;
+	/* max pixels per clock supported on output bus */
+	u8				max_pixels;
 };
 
 struct csi2rx_priv {
@@ -90,6 +98,7 @@ struct csi2rx_priv {
 	struct reset_control		*pixel_rst[CSI2RX_STREAMS_MAX];
 	struct phy			*dphy;
 
+	u8				num_pixels[CSI2RX_STREAMS_MAX];
 	u8				lanes[CSI2RX_LANES_MAX];
 	u8				num_lanes;
 	u8				max_lanes;
@@ -106,22 +115,22 @@ struct csi2rx_priv {
 };
 
 static const struct csi2rx_fmt formats[] = {
-	{ .code	= MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, },
-	{ .code	= MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, },
-	{ .code	= MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, },
-	{ .code	= MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, },
-	{ .code	= MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, },
-	{ .code	= MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, },
-	{ .code	= MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, },
-	{ .code	= MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, },
-	{ .code	= MEDIA_BUS_FMT_Y8_1X8,     .bpp = 8, },
-	{ .code	= MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, },
-	{ .code	= MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, },
-	{ .code	= MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, },
-	{ .code	= MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, },
-	{ .code	= MEDIA_BUS_FMT_RGB565_1X16,  .bpp = 16, },
-	{ .code	= MEDIA_BUS_FMT_RGB888_1X24,  .bpp = 24, },
-	{ .code	= MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, },
+	{ .code	= MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, .max_pixels = 4, },
+	{ .code	= MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, .max_pixels = 4, },
+	{ .code	= MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, .max_pixels = 4, },
+	{ .code	= MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, .max_pixels = 4, },
+	{ .code	= MEDIA_BUS_FMT_Y8_1X8,     .bpp = 8, .max_pixels = 4, },
+	{ .code	= MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_RGB565_1X16,  .bpp = 16, .max_pixels = 1, },
+	{ .code	= MEDIA_BUS_FMT_RGB888_1X24,  .bpp = 24, .max_pixels = 1, },
+	{ .code	= MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, .max_pixels = 1, },
 };
 
 static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
@@ -276,8 +285,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
 
 		reset_control_deassert(csi2rx->pixel_rst[i]);
 
-		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
-		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
+		reg = CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF;
+		reg |= FIELD_PREP(CSI2RX_STREAM_CFG_NUM_PIXELS_MASK,
+				  csi2rx->num_pixels[i]);
+		writel(reg, csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
 
 		/*
 		 * Enable one virtual channel. When multiple virtual channels
@@ -458,6 +469,34 @@ static int csi2rx_init_state(struct v4l2_subdev *subdev,
 	return csi2rx_set_fmt(subdev, state, &format);
 }
 
+int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
+			      u8 *ppc)
+{
+	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+	const struct csi2rx_fmt *csi_fmt;
+	struct v4l2_subdev_state *state;
+	struct v4l2_mbus_framefmt *fmt;
+	int ret = 0;
+
+	if (!ppc || pad < CSI2RX_PAD_SOURCE_STREAM0 || pad >= CSI2RX_PAD_MAX)
+		return -EINVAL;
+
+	state = v4l2_subdev_lock_and_get_active_state(subdev);
+	fmt = v4l2_subdev_state_get_format(state, pad);
+	csi_fmt = csi2rx_get_fmt_by_code(fmt->code);
+
+	/* Reduce requested PPC if it is too high */
+	*ppc = min(*ppc, csi_fmt->max_pixels);
+
+	v4l2_subdev_unlock_state(state);
+
+	csi2rx->num_pixels[pad - CSI2RX_PAD_SOURCE_STREAM0] =
+		CSI2RX_STREAM_CFG_NUM_PIXELS(*ppc);
+
+	return ret;
+}
+EXPORT_SYMBOL(cdns_csi2rx_negotiate_ppc);
+
 static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
 	.enum_mbus_code	= csi2rx_enum_mbus_code,
 	.get_fmt	= v4l2_subdev_get_fmt,
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.h b/drivers/media/platform/cadence/cdns-csi2rx.h
new file mode 100644
index 0000000000000000000000000000000000000000..128d47e8513c99c083f49e249e876be6d19389f6
--- /dev/null
+++ b/drivers/media/platform/cadence/cdns-csi2rx.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+#ifndef CDNS_CSI2RX_H
+#define CDNS_CSI2RX_H
+
+#include <media/v4l2-subdev.h>
+
+/**
+ * cdns_csi2rx_negotiate_ppc - Negotiate pixel-per-clock on output interface
+ *
+ * @subdev: point to &struct v4l2_subdev
+ * @pad: pad number of the source pad
+ * @ppc: pointer to requested pixel-per-clock value
+ *
+ * Returns 0 on success, negative error code otherwise.
+ */
+int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
+			      u8 *ppc);
+
+#endif

-- 
2.49.0
Re: [PATCH v2 5/6] media: cadence: cdns-csi2rx: Support multiple pixels per clock cycle
Posted by Sakari Ailus 5 months, 3 weeks ago 
Hi Jai,

Thanks for the patchset.

On Thu, Apr 10, 2025 at 12:19:03PM +0530, Jai Luthra wrote:
> The output pixel interface is a parallel bus (32 bits), which
> supports sending multiple pixels (1, 2 or 4) per clock cycle for
> smaller pixel widths like RAW8-RAW16.
> 
> Dual-pixel and Quad-pixel modes can be a requirement if the export rate
> of the Cadence IP in Single-pixel mode maxes out before the maximum
> supported DPHY-RX frequency, which is the case with TI's integration of
> this IP [1].
> 
> So, we export a function that lets the downstream hardware block request
> a higher pixel-per-clock on a particular output pad.
> 
> We check if we can support the requested pixels per clock given the
> known maximum for the currently configured format. If not, we set it
> to the highest feasible value and return this value to the caller.
> 
> [1] Section 12.6.1.4.8.14 CSI_RX_IF Programming Restrictions of AM62 TRM
> 
> Link: https://www.ti.com/lit/pdf/spruj16
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/platform/cadence/cdns-csi2rx.c | 75 +++++++++++++++++++++-------
>  drivers/media/platform/cadence/cdns-csi2rx.h | 19 +++++++
>  2 files changed, 76 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 608298c72462031515d9ad01c6b267bf7375a5bf..154eaacc39ad294db0524e88be888bd0929af071 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2017 Cadence Design Systems Inc.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/io.h>
> @@ -22,6 +23,8 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> +#include "cdns-csi2rx.h"
> +
>  #define CSI2RX_DEVICE_CFG_REG			0x000
>  
>  #define CSI2RX_SOFT_RESET_REG			0x004
> @@ -53,6 +56,8 @@
>  
>  #define CSI2RX_STREAM_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x00c)
>  #define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF		(1 << 8)

Not a fault of this patch but this should use BIT(). Or at the very least
(1U << 8). I.e. this isn't a bug but the pattern is bad. It'd be nice to
fix this in a separate patch.

> +#define CSI2RX_STREAM_CFG_NUM_PIXELS_MASK		GENMASK(5, 4)
> +#define CSI2RX_STREAM_CFG_NUM_PIXELS(n)			((n) >> 1)
>  
>  #define CSI2RX_LANES_MAX	4
>  #define CSI2RX_STREAMS_MAX	4
> @@ -68,7 +73,10 @@ enum csi2rx_pads {
>  
>  struct csi2rx_fmt {
>  	u32				code;
> +	/* width of a single pixel on CSI-2 bus */
>  	u8				bpp;
> +	/* max pixels per clock supported on output bus */
> +	u8				max_pixels;
>  };
>  
>  struct csi2rx_priv {
> @@ -90,6 +98,7 @@ struct csi2rx_priv {
>  	struct reset_control		*pixel_rst[CSI2RX_STREAMS_MAX];
>  	struct phy			*dphy;
>  
> +	u8				num_pixels[CSI2RX_STREAMS_MAX];
>  	u8				lanes[CSI2RX_LANES_MAX];
>  	u8				num_lanes;
>  	u8				max_lanes;
> @@ -106,22 +115,22 @@ struct csi2rx_priv {
>  };
>  
>  static const struct csi2rx_fmt formats[] = {
> -	{ .code	= MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, },
> -	{ .code	= MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, },
> -	{ .code	= MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, },
> -	{ .code	= MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, },
> -	{ .code	= MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, },
> -	{ .code	= MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, },
> -	{ .code	= MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, },
> -	{ .code	= MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, },
> -	{ .code	= MEDIA_BUS_FMT_Y8_1X8,     .bpp = 8, },
> -	{ .code	= MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, },
> -	{ .code	= MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, },
> -	{ .code	= MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, },
> -	{ .code	= MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, },
> -	{ .code	= MEDIA_BUS_FMT_RGB565_1X16,  .bpp = 16, },
> -	{ .code	= MEDIA_BUS_FMT_RGB888_1X24,  .bpp = 24, },
> -	{ .code	= MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, },
> +	{ .code	= MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, .max_pixels = 4, },
> +	{ .code	= MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, .max_pixels = 4, },
> +	{ .code	= MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, .max_pixels = 4, },
> +	{ .code	= MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, .max_pixels = 4, },
> +	{ .code	= MEDIA_BUS_FMT_Y8_1X8,     .bpp = 8, .max_pixels = 4, },
> +	{ .code	= MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_RGB565_1X16,  .bpp = 16, .max_pixels = 1, },
> +	{ .code	= MEDIA_BUS_FMT_RGB888_1X24,  .bpp = 24, .max_pixels = 1, },
> +	{ .code	= MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, .max_pixels = 1, },
>  };
>  
>  static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
> @@ -276,8 +285,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  
>  		reset_control_deassert(csi2rx->pixel_rst[i]);
>  
> -		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
> -		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> +		reg = CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF;
> +		reg |= FIELD_PREP(CSI2RX_STREAM_CFG_NUM_PIXELS_MASK,
> +				  csi2rx->num_pixels[i]);
> +		writel(reg, csi2rx->base + CSI2RX_STREAM_CFG_REG(i));

I'd write this as:

		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF |
		       FIELD_PREP(CSI2RX_STREAM_CFG_NUM_PIXELS_MASK,
				  csi2rx->num_pixels[i]),
		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));

But up to you.

>  
>  		/*
>  		 * Enable one virtual channel. When multiple virtual channels
> @@ -458,6 +469,34 @@ static int csi2rx_init_state(struct v4l2_subdev *subdev,
>  	return csi2rx_set_fmt(subdev, state, &format);
>  }
>  
> +int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
> +			      u8 *ppc)
> +{
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +	const struct csi2rx_fmt *csi_fmt;
> +	struct v4l2_subdev_state *state;
> +	struct v4l2_mbus_framefmt *fmt;
> +	int ret = 0;

ret is redundant.

> +
> +	if (!ppc || pad < CSI2RX_PAD_SOURCE_STREAM0 || pad >= CSI2RX_PAD_MAX)
> +		return -EINVAL;
> +
> +	state = v4l2_subdev_lock_and_get_active_state(subdev);
> +	fmt = v4l2_subdev_state_get_format(state, pad);
> +	csi_fmt = csi2rx_get_fmt_by_code(fmt->code);
> +
> +	/* Reduce requested PPC if it is too high */
> +	*ppc = min(*ppc, csi_fmt->max_pixels);
> +
> +	v4l2_subdev_unlock_state(state);
> +
> +	csi2rx->num_pixels[pad - CSI2RX_PAD_SOURCE_STREAM0] =
> +		CSI2RX_STREAM_CFG_NUM_PIXELS(*ppc);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(cdns_csi2rx_negotiate_ppc);

EXPORT_SYMBOL_GPL(). Or maybe use a namespace?

> +
>  static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
>  	.enum_mbus_code	= csi2rx_enum_mbus_code,
>  	.get_fmt	= v4l2_subdev_get_fmt,
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.h b/drivers/media/platform/cadence/cdns-csi2rx.h

I wonder if it'd be better to put this under include/media.

> new file mode 100644
> index 0000000000000000000000000000000000000000..128d47e8513c99c083f49e249e876be6d19389f6
> --- /dev/null
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +#ifndef CDNS_CSI2RX_H
> +#define CDNS_CSI2RX_H
> +
> +#include <media/v4l2-subdev.h>
> +
> +/**
> + * cdns_csi2rx_negotiate_ppc - Negotiate pixel-per-clock on output interface
> + *
> + * @subdev: point to &struct v4l2_subdev
> + * @pad: pad number of the source pad
> + * @ppc: pointer to requested pixel-per-clock value
> + *
> + * Returns 0 on success, negative error code otherwise.
> + */
> +int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
> +			      u8 *ppc);
> +
> +#endif
> 

-- 
Regards,

Sakari Ailus
Re: [PATCH v2 5/6] media: cadence: cdns-csi2rx: Support multiple pixels per clock cycle
Posted by Jai Luthra 5 months, 3 weeks ago 
Hi Sakari,

Thanks for the review.

Quoting Sakari Ailus (2025-06-25 23:00:22)
> Hi Jai,
> 
> Thanks for the patchset.
> 
> On Thu, Apr 10, 2025 at 12:19:03PM +0530, Jai Luthra wrote:
> > The output pixel interface is a parallel bus (32 bits), which
> > supports sending multiple pixels (1, 2 or 4) per clock cycle for
> > smaller pixel widths like RAW8-RAW16.
> > 
> > Dual-pixel and Quad-pixel modes can be a requirement if the export rate
> > of the Cadence IP in Single-pixel mode maxes out before the maximum
> > supported DPHY-RX frequency, which is the case with TI's integration of
> > this IP [1].
> > 
> > So, we export a function that lets the downstream hardware block request
> > a higher pixel-per-clock on a particular output pad.
> > 
> > We check if we can support the requested pixels per clock given the
> > known maximum for the currently configured format. If not, we set it
> > to the highest feasible value and return this value to the caller.
> > 
> > [1] Section 12.6.1.4.8.14 CSI_RX_IF Programming Restrictions of AM62 TRM
> > 
> > Link: https://www.ti.com/lit/pdf/spruj16
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  drivers/media/platform/cadence/cdns-csi2rx.c | 75 +++++++++++++++++++++-------
> >  drivers/media/platform/cadence/cdns-csi2rx.h | 19 +++++++
> >  2 files changed, 76 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index 608298c72462031515d9ad01c6b267bf7375a5bf..154eaacc39ad294db0524e88be888bd0929af071 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -5,6 +5,7 @@
> >   * Copyright (C) 2017 Cadence Design Systems Inc.
> >   */
> >  
> > +#include <linux/bitfield.h>
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> >  #include <linux/io.h>
> > @@ -22,6 +23,8 @@
> >  #include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-subdev.h>
> >  
> > +#include "cdns-csi2rx.h"
> > +
> >  #define CSI2RX_DEVICE_CFG_REG                        0x000
> >  
> >  #define CSI2RX_SOFT_RESET_REG                        0x004
> > @@ -53,6 +56,8 @@
> >  
> >  #define CSI2RX_STREAM_CFG_REG(n)             (CSI2RX_STREAM_BASE(n) + 0x00c)
> >  #define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF                (1 << 8)
> 
> Not a fault of this patch but this should use BIT(). Or at the very least
> (1U << 8). I.e. this isn't a bug but the pattern is bad. It'd be nice to
> fix this in a separate patch.
> 

Ah, that's my bad, rest of the driver does already use BIT().. will fix in
v3.

> > +#define CSI2RX_STREAM_CFG_NUM_PIXELS_MASK            GENMASK(5, 4)
> > +#define CSI2RX_STREAM_CFG_NUM_PIXELS(n)                      ((n) >> 1)
> >  
> >  #define CSI2RX_LANES_MAX     4
> >  #define CSI2RX_STREAMS_MAX   4
> > @@ -68,7 +73,10 @@ enum csi2rx_pads {
> >  
> >  struct csi2rx_fmt {
> >       u32                             code;
> > +     /* width of a single pixel on CSI-2 bus */
> >       u8                              bpp;
> > +     /* max pixels per clock supported on output bus */
> > +     u8                              max_pixels;
> >  };
> >  
> >  struct csi2rx_priv {
> > @@ -90,6 +98,7 @@ struct csi2rx_priv {
> >       struct reset_control            *pixel_rst[CSI2RX_STREAMS_MAX];
> >       struct phy                      *dphy;
> >  
> > +     u8                              num_pixels[CSI2RX_STREAMS_MAX];
> >       u8                              lanes[CSI2RX_LANES_MAX];
> >       u8                              num_lanes;
> >       u8                              max_lanes;
> > @@ -106,22 +115,22 @@ struct csi2rx_priv {
> >  };
> >  
> >  static const struct csi2rx_fmt formats[] = {
> > -     { .code = MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, },
> > -     { .code = MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, },
> > -     { .code = MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, },
> > -     { .code = MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, },
> > -     { .code = MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, },
> > -     { .code = MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, },
> > -     { .code = MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, },
> > -     { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, },
> > -     { .code = MEDIA_BUS_FMT_Y8_1X8,     .bpp = 8, },
> > -     { .code = MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, },
> > -     { .code = MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, },
> > -     { .code = MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, },
> > -     { .code = MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, },
> > -     { .code = MEDIA_BUS_FMT_RGB565_1X16,  .bpp = 16, },
> > -     { .code = MEDIA_BUS_FMT_RGB888_1X24,  .bpp = 24, },
> > -     { .code = MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, },
> > +     { .code = MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, .max_pixels = 4, },
> > +     { .code = MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, .max_pixels = 4, },
> > +     { .code = MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, .max_pixels = 4, },
> > +     { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, .max_pixels = 4, },
> > +     { .code = MEDIA_BUS_FMT_Y8_1X8,     .bpp = 8, .max_pixels = 4, },
> > +     { .code = MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_RGB565_1X16,  .bpp = 16, .max_pixels = 1, },
> > +     { .code = MEDIA_BUS_FMT_RGB888_1X24,  .bpp = 24, .max_pixels = 1, },
> > +     { .code = MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, .max_pixels = 1, },
> >  };
> >  
> >  static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
> > @@ -276,8 +285,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
> >  
> >               reset_control_deassert(csi2rx->pixel_rst[i]);
> >  
> > -             writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
> > -                    csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> > +             reg = CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF;
> > +             reg |= FIELD_PREP(CSI2RX_STREAM_CFG_NUM_PIXELS_MASK,
> > +                               csi2rx->num_pixels[i]);
> > +             writel(reg, csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> 
> I'd write this as:
> 
>                 writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF |
>                        FIELD_PREP(CSI2RX_STREAM_CFG_NUM_PIXELS_MASK,
>                                   csi2rx->num_pixels[i]),
>                        csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> 
> But up to you.
> 

Will do in v3.

> >  
> >               /*
> >                * Enable one virtual channel. When multiple virtual channels
> > @@ -458,6 +469,34 @@ static int csi2rx_init_state(struct v4l2_subdev *subdev,
> >       return csi2rx_set_fmt(subdev, state, &format);
> >  }
> >  
> > +int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
> > +                           u8 *ppc)
> > +{
> > +     struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > +     const struct csi2rx_fmt *csi_fmt;
> > +     struct v4l2_subdev_state *state;
> > +     struct v4l2_mbus_framefmt *fmt;
> > +     int ret = 0;
> 
> ret is redundant.
> 

Good catch, will fix.

> > +
> > +     if (!ppc || pad < CSI2RX_PAD_SOURCE_STREAM0 || pad >= CSI2RX_PAD_MAX)
> > +             return -EINVAL;
> > +
> > +     state = v4l2_subdev_lock_and_get_active_state(subdev);
> > +     fmt = v4l2_subdev_state_get_format(state, pad);
> > +     csi_fmt = csi2rx_get_fmt_by_code(fmt->code);
> > +
> > +     /* Reduce requested PPC if it is too high */
> > +     *ppc = min(*ppc, csi_fmt->max_pixels);
> > +
> > +     v4l2_subdev_unlock_state(state);
> > +
> > +     csi2rx->num_pixels[pad - CSI2RX_PAD_SOURCE_STREAM0] =
> > +             CSI2RX_STREAM_CFG_NUM_PIXELS(*ppc);
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(cdns_csi2rx_negotiate_ppc);
> 
> EXPORT_SYMBOL_GPL(). Or maybe use a namespace?
> 

Ah I wasn't aware of different namespaces. I think a module-specific one as
documented here [1] might make the most sense in this case. Will try that,
else will use _GPL.

[1] https://docs.kernel.org/core-api/symbol-namespaces.html#using-the-export-symbol-gpl-for-modules-macro

> > +
> >  static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
> >       .enum_mbus_code = csi2rx_enum_mbus_code,
> >       .get_fmt        = v4l2_subdev_get_fmt,
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.h b/drivers/media/platform/cadence/cdns-csi2rx.h
> 
> I wonder if it'd be better to put this under include/media.
> 

I was unsure about it, as while these are two separate drivers it's
essentially the same "device".. but I don't see a harm in keeping this
under include/media. Will do in v3.

> > new file mode 100644
> > index 0000000000000000000000000000000000000000..128d47e8513c99c083f49e249e876be6d19389f6
> > --- /dev/null
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.h
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +#ifndef CDNS_CSI2RX_H
> > +#define CDNS_CSI2RX_H
> > +
> > +#include <media/v4l2-subdev.h>
> > +
> > +/**
> > + * cdns_csi2rx_negotiate_ppc - Negotiate pixel-per-clock on output interface
> > + *
> > + * @subdev: point to &struct v4l2_subdev
> > + * @pad: pad number of the source pad
> > + * @ppc: pointer to requested pixel-per-clock value
> > + *
> > + * Returns 0 on success, negative error code otherwise.
> > + */
> > +int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
> > +                           u8 *ppc);
> > +
> > +#endif
> > 
> 
> -- 
> Regards,
> 
> Sakari Ailus

Thanks,
Jai

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.