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

[Sakari Ailus]

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@xxxxxxxxxxxxxxxxx>
> ---
>  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