Re: [PATCH 4/5] media: imx335: Fix active area height discrepency

[Umang Jain]

Hi Kieran,

On 06/03/24 10:35 pm, Kieran Bingham wrote:
> Quoting Umang Jain (2024-03-06 08:10:37)
> > The imx335 reports a recommended pixel area of - 2592x1944.
> > The driver supported mode however limits it to height=1940.
> Hrm, I think I would convert widths and sizes to decimal as a patch
> before this patch so the effect is clearer in this diff.
>
> > Fix the height discrepency by correctly the value of height
> > (with updates to vblank and mode registers).
> >
> > Signed-off-by: Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/media/i2c/imx335.c | 18 +++++++++---------
> >   1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > index 6ea09933e47b..c00e0c2be3f3 100644
> > --- a/drivers/media/i2c/imx335.c
> > +++ b/drivers/media/i2c/imx335.c
> > @@ -246,13 +246,13 @@ static const int imx335_tpg_val[] = {
> >   };
> >   
> >   /* Sensor mode registers */
> > -static const struct cci_reg_sequence mode_2592x1940_regs[] = {
> > +static const struct cci_reg_sequence mode_2592x1944_regs[] = {
> >          {IMX335_REG_MODE_SELECT, 0x01},
> >          {IMX335_REG_MASTER_MODE, 0x00},
> > -       {IMX335_REG_WINMODE, 0x04},
> > -       {IMX335_REG_HTRIMMING_START, 0x0180},
> > +       {IMX335_REG_WINMODE, 0x00},
> What's the distinction of the winmode here. What is 0x04 vs 0x00?
>
> Is this something that could be a defined value? Or is that not worth
> the effort?

It can be split out as a define in later parts if we introduce more 
modes. currently this change fixes the sensor to be in 'all-pixel scan 
mode' hence you see the change in value of WINMODE.
> > +       {IMX335_REG_HTRIMMING_START, 0x30},
> HTRIMMING_START has moved a lot more than I would expect there.
> Is there a visual impact of any concern here?

Value as per mentioned in the datasheet
> >          {IMX335_REG_HNUM, 0x0a20},
> > -       {IMX335_REG_Y_OUT_SIZE, 0x0794},
> > +       {IMX335_REG_Y_OUT_SIZE, 0x0798},
> This bit looks expected ;-)
>
> >          {IMX335_REG_VCROP_POS, 0x00b0},
> >          {IMX335_REG_VCROP_SIZE, 0x0f58},
> 0x0f58 = 3928. Does that correspond to anything on the pixel array size?
> We're modifying the vertical size, so I'm curious if the 'vcrop' is or
> should be impacted?

Probably I have not named this correctly, as VCROP_

This register (0x3076) is denotes the size of cropping rectangle
Named as 'AREA3_WIDTH_1' with explaination 'cropping size designation 
(Vertical direction)'

The value 0x0f58 is as per mentioned in the datasheet. I don't find any 
relation except 3928 is twice the IMX335_NATIVE_HEIGHT
> >          {IMX335_REG_OPB_SIZE_V, 0x00},
> > @@ -403,14 +403,14 @@ static const u32 imx335_mbus_codes[] = {
> >   /* Supported sensor mode configurations */
> >   static const struct imx335_mode supported_mode = {
> >          .width = 2592,
> > -       .height = 1940,
> > +       .height = 1944,
> >          .hblank = 342,
> > -       .vblank = 2560,
> > -       .vblank_min = 2560,
> > +       .vblank = 2556,
> > +       .vblank_min = 2556,
> >          .vblank_max = 133060,
> >          .reg_list = {
> > -               .num_of_regs = ARRAY_SIZE(mode_2592x1940_regs),
> > -               .regs = mode_2592x1940_regs,
> > +               .num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
> > +               .regs = mode_2592x1944_regs,
> >          },
> >   };
> >   
> > --
> > 2.43.0