linux-kernel - Re: [PATCH 4/6] media: i2c: imx334: Support 4 or 8 lane operation modes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z-Uj1VnLKQH09__5@kekkonen.localdomain>
Date: Thu, 27 Mar 2025 10:09:25 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Tarang Raval <tarang.raval@...iconsignals.io>
Cc: kieran.bingham@...asonboard.com, Shravan.Chippa@...rochip.com,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Hans Verkuil <hverkuil@...all.nl>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Umang Jain <umang.jain@...asonboard.com>,
	Zhi Mao <zhi.mao@...iatek.com>,
	Julien Massot <julien.massot@...labora.com>,
	Luis Garcia <git@...gi311.com>,
	Benjamin Mugnier <benjamin.mugnier@...s.st.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/6] media: i2c: imx334: Support 4 or 8 lane operation
 modes

Hi Tarang,

On Mon, Mar 10, 2025 at 12:47:46PM +0530, Tarang Raval wrote:
> imx334 can support both 4 and 8 lane configurations.
> Extend the driver to configure the lane mode accordingly.
> 
> Signed-off-by: Tarang Raval <tarang.raval@...iconsignals.io>
> ---
>  drivers/media/i2c/imx334.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index 24ccfd1d0986..23bfc64969cc 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -47,6 +47,8 @@
>  #define IMX334_EXPOSURE_DEFAULT	0x0648
>  
>  #define IMX334_REG_LANEMODE            CCI_REG8(0x3a01)
> +#define IMX334_CSI_4_LANE_MODE         3
> +#define IMX334_CSI_8_LANE_MODE         7
>  
>  /* Window cropping Settings */
>  #define IMX334_REG_AREA3_ST_ADR_1      CCI_REG16_LE(0x3074)
> @@ -107,7 +109,6 @@
>  /* CSI2 HW configuration */
>  #define IMX334_LINK_FREQ_891M	891000000
>  #define IMX334_LINK_FREQ_445M	445500000
> -#define IMX334_NUM_DATA_LANES	4
>  
>  #define IMX334_REG_MIN		0x00
>  #define IMX334_REG_MAX		0xfffff
> @@ -181,6 +182,7 @@ struct imx334_mode {
>   * @exp_ctrl: Pointer to exposure control
>   * @again_ctrl: Pointer to analog gain control
>   * @vblank: Vertical blanking in lines
> + * @lane_mode: Mode for number of connected data lanes
>   * @cur_mode: Pointer to current selected sensor mode
>   * @mutex: Mutex for serializing sensor controls
>   * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
> @@ -204,6 +206,7 @@ struct imx334 {
>  		struct v4l2_ctrl *again_ctrl;
>  	};
>  	u32 vblank;
> +	u32 lane_mode;
>  	const struct imx334_mode *cur_mode;
>  	struct mutex mutex;
>  	unsigned long link_freq_bitmap;
> @@ -240,7 +243,6 @@ static const struct cci_reg_sequence common_mode_regs[] = {
>  	{ IMX334_REG_HADD_VADD, 0x00},
>  	{ IMX334_REG_VALID_EXPAND, 0x03},
>  	{ IMX334_REG_TCYCLE, 0x00},
> -	{ IMX334_REG_LANEMODE, 0x03},

Not a fault of this patch but also the closing brace should have a space
before it. Could you address it in the earlier patches?

>  	{ IMX334_REG_TCLKPOST, 0x007f},
>  	{ IMX334_REG_TCLKPREPARE, 0x0037},
>  	{ IMX334_REG_TCLKTRAIL, 0x0037},
> @@ -876,6 +878,13 @@ static int imx334_start_streaming(struct imx334 *imx334)
>  		return ret;
>  	}
>  
> +	ret = cci_write(imx334->cci, IMX334_REG_LANEMODE,
> +			imx334->lane_mode, NULL);
> +	if (ret) {
> +		dev_err(imx334->dev, "failed to configure lanes\n");
> +		return ret;
> +	}
> +
>  	ret = imx334_set_framefmt(imx334);
>  	if (ret) {
>  		dev_err(imx334->dev, "%s failed to set frame format: %d\n",
> @@ -1022,7 +1031,14 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
>  	if (ret)
>  		return ret;
>  
> -	if (bus_cfg.bus.mipi_csi2.num_data_lanes != IMX334_NUM_DATA_LANES) {
> +	switch (bus_cfg.bus.mipi_csi2.num_data_lanes) {
> +	case 4:
> +		imx334->lane_mode = IMX334_CSI_4_LANE_MODE;
> +		break;
> +	case 8:
> +		imx334->lane_mode = IMX334_CSI_8_LANE_MODE;

Doesn't this affect the PLL configuration? Presumably higher frame rates
could be achieved at least.

> +		break;
> +	default:
>  		dev_err(imx334->dev,
>  			"number of CSI2 data lanes %d is not supported\n",
>  			bus_cfg.bus.mipi_csi2.num_data_lanes);

-- 
Regards,

Sakari Ailus