On Fri, 22 Jul 2022 at 06:53, Alexander Stein
<alexander.stein@xxxxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> > > > > > ---
> > > > > >
> > > > > > 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.
Yes, typo of 0x3840 vs 0x3480.
I did spend a fair amount of time looking at IMX327 / IMX290 a while
back, and do have datasheets, so happy to help out with reviews. I
really ought to clean up my patches and send them to the list, but
I'll wait for Laurent's set here to be merged first (assuming that
doesn't become too drawn out).
Dave
> Best regards,
> Alexander
>
> > See [1] for adding 74.25MHz clock support.
> >
> > Dave
> >
> > [1]
> > https://github.com/raspberrypi/linux/commit/125f7e7ef1194d4849c74b25c87d18a
> > ea9de2de7
>
>
>
>
