Hi Kieran, Sakari, Shravan, and Tarang
On Wed, 5 Mar 2025 at 12:17, Kieran Bingham
<kieran.bingham@xxxxxxxxxxxxxxxx> wrote:
>
> Quoting Sakari Ailus (2025-03-05 11:50:53)
> > Hi Kieran, Shravan,
> >
> > On Wed, Mar 05, 2025 at 10:45:42AM +0000, Kieran Bingham wrote:
> > > Quoting Shravan.Chippa@xxxxxxxxxxxxx (2025-03-05 10:22:12)
> > > > Hi Kieran
> > > >
> > > > > -----Original Message-----
> > > > > From: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> > > > > Sent: Wednesday, March 5, 2025 3:05 PM
> > > > > To: mchehab@xxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx; shravan Chippa -
> > > > > I35088 <Shravan.Chippa@xxxxxxxxxxxxx>
> > > > > Cc: linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Conor Dooley
> > > > > - M52691 <Conor.Dooley@xxxxxxxxxxxxx>; Valentina Fernandez Alanis -
> > > > > M63239 <Valentina.FernandezAlanis@xxxxxxxxxxxxx>; Praveen Kumar -
> > > > > I30718 <Praveen.Kumar@xxxxxxxxxxxxx>; shravan Chippa - I35088
> > > > > <Shravan.Chippa@xxxxxxxxxxxxx>
> > > > > Subject: Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p
> > > > > resolutions
> > > > >
> > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > > > > content is safe
> > > > >
> > > > > Quoting shravan kumar (2025-03-05 05:14:42)
> > > > > > From: Shravan Chippa <shravan.chippa@xxxxxxxxxxxxx>
> > > > > >
> > > > > > Added support for 1280x720@30 and 640x480@30 resolutions
Silly question - why is the frame rate specified here? Is that the
minimum, maximum, or default framerate, as we have V4L2_CID_VBLANK for
varying the frame rate.
> > > > > >
> > > > > > Signed-off-by: Shravan Chippa <shravan.chippa@xxxxxxxxxxxxx>
> > > > > > ---
> > > > > > drivers/media/i2c/imx334.c | 66
> > > > > > ++++++++++++++++++++++++++++++++++++++
> > > > > > 1 file changed, 66 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > > > > index a7c0bd38c9b8..8cd1eecd0143 100644
> > > > > > --- a/drivers/media/i2c/imx334.c
> > > > > > +++ b/drivers/media/i2c/imx334.c
> > > > > > @@ -314,6 +314,46 @@ static const struct imx334_reg
> > > > > common_mode_regs[] = {
> > > > > > {0x3002, 0x00},
> > > > > > };
> > > > > >
> > > > > > +/* Sensor mode registers for 640x480@30fps */ static const struct
> > > > > > +imx334_reg mode_640x480_regs[] = {
> > > > > > + {0x302c, 0x70},
> > > > > > + {0x302d, 0x06},
> > > > >
> > > > > These two are a single 16 bit register HTRIMMING_START = 1648
> > > > >
> > > > > > + {0x302e, 0x80},
> > > > > > + {0x302f, 0x02},
> > > > >
> > > > > These two are a single 16 bit register HNUM = 640
> > > > >
> > > > > > + {0x3074, 0x48},
> > > > > > + {0x3075, 0x07},
> > > > >
> > > > > These two are a single 16 bit (well, 12 bit value) AREA3_ST_ADR_1 = 1864
> > > > >
> > > > > > + {0x308e, 0x49},
> > > > > > + {0x308f, 0x07},
> > > > >
> > > > > These two are a single 16 bit register AREA3_ST_ADR_2 = 1865
> > > > >
> > > > > > + {0x3076, 0xe0},
> > > > > > + {0x3077, 0x01},
> > > > >
> > > > > These two are a single 16 bit register AREA3_WIDTH_1 = 480
> > > > >
> > > > > > + {0x3090, 0xe0},
> > > > > > + {0x3091, 0x01},
> > > > >
> > > > > These two are a single 16 bit register AREA3_WIDTH_2 = 480
> > > > >
> > > > > > + {0x3308, 0xe0},
> > > > > > + {0x3309, 0x01},
> > > > >
> > > > > These two are a single 16 bit register Y_OUT_SIZE
> > > > >
> > > > > Don't you think
> > > > > { Y_OUT_SIZE, 480 },
> > > > >
> > > > > Is so much more readable and easier to comprehend and maintain?
> > > > >
> > > > >
> > > > > > + {0x30d8, 0x30},
> > > > > > + {0x30d9, 0x0b},
> > > > >
> > > > > These two are a single 16 bit register UNREAD_ED_ADR = 2864
> > > > >
> > > > > > +};
> > > > >
> > > > > I'm still sad that we can all know the names of all these registers and yet this
> > > > > is writing new tables of hex values.
> > > >
> > > > Do you want me use call like bellow API with register names:
> > > > CCI_REG16_LE(0x30d8);
> > > > cci_write();
> > > > cci_multi_reg_write();
> > > > devm_cci_regmap_init_i2c();
> > >
> > >
> > > Yes please, I would want that very much. I'm not very good at reading
> > > and storing hex values in my head! That's why I broke down the above
> > > registers to strings and decimal values.
> >
> > I agree, it'd be good to do these while changing the driver now. I think it
> > could be done after adding the new modes, now that the patches already
> > exist.
>
> That's ok with me! Either way I'm happy to see the drivers are getting
> cleaned up!
>
> >
> > >
> > >
> > > The discussions at
> > > https://lore.kernel.org/all/PH0PR11MB5611FD22CF6E12F7226FA9C081E12@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> > > reports the full datasheet, and I stated:
> > >
> > >
> > > > > > This is an enormous amount of duplicated data that could be factored
> > > > > > out.
> > > > > >
> > > > > > These are also /very/ common against the existing mode register
> > > > > > tables too.
> > > > > >
> > > > > > I think several things need to happen in this driver:
> > > > > >
> > > > > > 1. It should be converted to use the CCI helpers.
> > > > > > 2. Whereever identifiable, the register names should be used
> > > > > > instead of
> > > > > > just the addresses.
> > > > > > 3. The common factors of these tables should be de-duplicated.
> > > > > >
> > > > > > In your additions you only have differences in the following
> > > > > > registers from those entire tables:
> > >
> > > You replied with
> > >
> > > > >
> > > > > I will try to optimize camera resolution array register value usage by
> > > > > writing common register array values.
> > >
> > > which you have done (point 3), and is great progress in the series.
> > >
> > > Further down in the thread I stated:
> > >
> > >
> > > > > > 4. And ideally - the differences which determine the modes should
> > > > > > be
> > > > > > factored out to calculations so that we are not writing out
> > > > > > large
> > > > > > tables just to write a parameterised frame size.
> > > > > >
> > > > > >
> > > > > > I would beleive that at least steps 1 and 3 would be achievable, 2
> > > > > > and 4 would depend upon access to the datasheet.
> > > > > >
> > >
> > >
> > > I still believe steps 1 is important to this development. You have done
> > > step 3 I think. And now both step 2 and (later) step 4 are possible as we
> > > have the datasheet.
> >
> > In this case the datasheet doesn't appear to be documenting the PLL.
>
> :-(
>
> But at least we will have enough to handle all the vmax/and cropping
> more directly.
I haven't fully analysed the datasheet, but isn't this just another
Sony Starvis sensor, working in the same way as imx415 and imx290?
I don't think you can reasonably treat it as generic PLLs without
effectively reverse engineering the hardware.
INCK gets handled by the registers as specified in "INCK Setting" to
get the source clocks consistent regardless of INCK.
Source clock vs link frequency registers need to be teased out of the
"INCK Setting Register" table. Quick analysis:
- BCWAIT_TIME, CP_WAIT, INCKSEL2, INCKSEL3, and INCKSEL4 only change with INCK.
- PLL_IF_GC and SYS_MODE only change with link frequency.
- INCKSEL1 is the awkward one if there is a need to support the
1188Mbit/s data rate, otherwise it only changes with INCK.
The pixel rate stays the same for all modes (binning may be the exception here)
HMAX (V4L2_CID_HBLANK) is changed to vary the line time.
Whilst VMAX is listed as a constant for each mode, it can be changed
to alter frame rate.
Link frequency is independent of resolution, but HMAX has to be set to
ensure that we don't overflow the FIFO between array and CSI block.
Is there anyone selling an IMX334 module for use with SBCs at a
sensible price? I'm always up for tinkering with sensors, even if I'm
not meant to be!
Dave
> --
> Kieran
>
>
> >
> > --
> > Regards,
> >
> > Sakari Ailus
>
