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

[Tarang Raval]

Hi Sakari,

Thanks for the review.

> 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?

Okay, I will correct it.

> >       { 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.

Sorry, my commit message is misleading. The intention of this patch is to
configure the lane mode dynamically from the streaming function instead 
of using a hardcoded value.

You are correct that supporting an 8-lane mode requires changes to the PLL
configuration. This patch does not address that aspect yet.

Best Regards,
Tarang
> > +             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