Re: Re: Re: [PATCH 16/19] media: i2c: imx290: Move registers with fixed value to init array

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hello Dave,

Am Donnerstag, 21. Juli 2022, 18:19:56 CEST schrieb Dave Stevenson:
> Hi Laurent and Alexander
> 
> On Thu, 21 Jul 2022 at 12:08, Alexander Stein
> 
> <alexander.stein@ew.tq-group.com> wrote:
> > Hello Laurent,
> > 
> > Am Donnerstag, 21. Juli 2022, 12:40:36 CEST schrieb Laurent Pinchart:
> > > Hi Alexander,
> > > 
> > > On Thu, Jul 21, 2022 at 12:08:50PM +0200, Alexander Stein wrote:
> > > > Am Donnerstag, 21. Juli 2022, 10:35:37 CEST schrieb Laurent Pinchart:
> > > > > Registers 0x3012, 0x3013 and 0x3480 are not documented and are set
> > > > > in
> > > > > the per-mode register arrays with values indentical for all modes.
> > > > > Move
> > > > > them to the common array.
> > > > > 
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > ---
> > > > > 
> > > > >  drivers/media/i2c/imx290.c | 8 ++------
> > > > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > > index 78772c6327a2..fc6e87fada1c 100644
> > > > > --- a/drivers/media/i2c/imx290.c
> > > > > +++ b/drivers/media/i2c/imx290.c
> > > > > @@ -192,6 +192,7 @@ static const struct imx290_regval
> > > > > imx290_global_init_settings[] = { { IMX290_REG_8BIT(0x300f), 0x00 },
> > > > > 
> > > > >   { IMX290_REG_8BIT(0x3010), 0x21 },
> > > > >   { IMX290_REG_8BIT(0x3012), 0x64 },
> > > > > 
> > > > > + { IMX290_REG_8BIT(0x3013), 0x00 },
> > > > > 
> > > > >   { IMX290_REG_8BIT(0x3016), 0x09 },
> > > > >   { IMX290_REG_8BIT(0x3070), 0x02 },
> > > > >   { IMX290_REG_8BIT(0x3071), 0x11 },
> > > > > 
> > > > > @@ -230,6 +231,7 @@ static const struct imx290_regval
> > > > > imx290_global_init_settings[] = { { IMX290_REG_8BIT(0x33b0), 0x50 },
> > > > > 
> > > > >   { IMX290_REG_8BIT(0x33b2), 0x1a },
> > > > >   { IMX290_REG_8BIT(0x33b3), 0x04 },
> > > > > 
> > > > > + { IMX290_REG_8BIT(0x3480), 0x49 },
> > > > > 
> > > > >  };
> > > > >  
> > > > >  static const struct imx290_regval imx290_1080p_settings[] = {
> > > > > 
> > > > > @@ -239,15 +241,12 @@ static const struct imx290_regval
> > > > > imx290_1080p_settings[] = { { IMX290_OPB_SIZE_V, 10 },
> > > > > 
> > > > >   { IMX290_X_OUT_SIZE, 1920 },
> > > > >   { IMX290_Y_OUT_SIZE, 1080 },
> > > > > 
> > > > > - { IMX290_REG_8BIT(0x3012), 0x64 },
> > > > > - { IMX290_REG_8BIT(0x3013), 0x00 },
> > > > > 
> > > > >   { IMX290_INCKSEL1, 0x18 },
> > > > >   { IMX290_INCKSEL2, 0x03 },
> > > > >   { IMX290_INCKSEL3, 0x20 },
> > > > >   { IMX290_INCKSEL4, 0x01 },
> > > > >   { IMX290_INCKSEL5, 0x1a },
> > > > >   { IMX290_INCKSEL6, 0x1a },
> > > > > 
> > > > > - { IMX290_REG_8BIT(0x3480), 0x49 },
> > > > > 
> > > > >   /* data rate settings */
> > > > >   { IMX290_REPETITION, 0x10 },
> > > > >   { IMX290_TCLKPOST, 87 },
> > > > > 
> > > > > @@ -267,15 +266,12 @@ static const struct imx290_regval
> > > > > imx290_720p_settings[] = { { IMX290_OPB_SIZE_V, 4 },
> > > > > 
> > > > >   { IMX290_X_OUT_SIZE, 1280 },
> > > > >   { IMX290_Y_OUT_SIZE, 720 },
> > > > > 
> > > > > - { IMX290_REG_8BIT(0x3012), 0x64 },
> > > > > - { IMX290_REG_8BIT(0x3013), 0x00 },
> > > > > 
> > > > >   { IMX290_INCKSEL1, 0x20 },
> > > > >   { IMX290_INCKSEL2, 0x00 },
> > > > >   { IMX290_INCKSEL3, 0x20 },
> > > > >   { IMX290_INCKSEL4, 0x01 },
> > > > >   { IMX290_INCKSEL5, 0x1a },
> > > > >   { IMX290_INCKSEL6, 0x1a },
> > > > > 
> > > > > - { IMX290_REG_8BIT(0x3480), 0x49 },
> > > > > 
> > > > >   /* data rate settings */
> > > > >   { IMX290_REPETITION, 0x10 },
> > > > >   { IMX290_TCLKPOST, 79 },
> > > > 
> > > > 0x3480 is INCKSEL7 for imx327, not sure if that should be set yet for
> > > > imx290 (only) driver, without proper imx327 support.
> > > 
> > > Do you mean the register doesn't exist on the IMX290 ? We could make it
> > > conditional on the sensor model, but it's not added by this patch, it
> > > has been there since the first version of the driver, so I'd rather do
> > > that on top.
> > 
> > As far as I know INCKSEL7 is only valid on imx327. On imx290 the whole
> > 0x300-0x34ff range is reserved.
> 
> IMX290_CSI_LANE_MODE to select the number of CSI2 data lanes is
> 0x3443, so clearly the whole range is not reserved.

You are right, I was looking at the wrong table, my bad.

> > I agree this should be conditional on the sensor model. If you want to
> > keep
> > it, because it is not new, I'm fine with that.
> 
> 0x3840 is documented in my IMX290 datasheet as being INCKSEL7. 0x49
> for 37.125MHz clock. 0x92 for 74.25MHz (default).
> Removing it *will* break this driver for existing platforms as the
> rest of the code configures a 37.125MHz clock.

I guess you mean 0x3480, just a small typo. Just to be sure.
Interesting, the datasheet for imx290 I found doesn't contain INCKSEL7 at all. 
But good to know, that this register applies to imx290 as well.
Thanks for that information.

Best regards,
Alexander

> See [1] for adding 74.25MHz clock support.
> 
>   Dave
> 
> [1]
> https://github.com/raspberrypi/linux/commit/125f7e7ef1194d4849c74b25c87d18a
> ea9de2de7