Re: [PATCH 2/2] media: i2c: imx290: Add support for imx327 variant

[Dave Stevenson <dave.stevenson@raspberrypi.com>]

On Fri, 3 Feb 2023 at 11:10, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi Dave,
>
> Am Freitag, 3. Februar 2023, 11:55:55 CET schrieb Dave Stevenson:
> > Hi Alexander
> >
> > On Fri, 3 Feb 2023 at 10:24, Alexander Stein
> >
> > <alexander.stein@ew.tq-group.com> wrote:
> > > Both sensors are quite similar. Their specs only differ regarding LVDS
> > > and parallel output but are identical regarding MIPI-CSI-2 interface.
> > > But they use a different init setting of hard-coded values, taken from
> > > the datasheet.
> > >
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > ---
> > >
> > >  drivers/media/i2c/imx290.c | 88 +++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 77 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index e642e1df520d..337252b2ec15 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -164,6 +164,36 @@
> > >
> > >  #define CLK_74_25      1
> > >  #define NUM_CLK                2
> > >
> > > +enum imx290_model {
> > > +       IMX290,
> > > +       IMX290_MONO,
> > > +       IMX327,
> > > +};
> > > +
> > > +struct imx290_device_data {
> > > +       enum imx290_model model;
> > > +       const char *name;
> > > +       u8 mono;
> > > +};
> > > +
> > > +static const struct imx290_device_data imx290_models[] = {
> > > +       [IMX290] = {
> > > +               .model = IMX290,
> > > +               .name = "imx290",
> > > +               .mono = 0,
> > > +       },
> > > +       [IMX290_MONO] = {
> > > +               .model = IMX290_MONO,
> > > +               .name = "imx290-mono",
> > > +               .mono = 1,
> > > +       },
> > > +       [IMX327] = {
> > > +               .model = IMX327,
> > > +               .name = "imx327",
> > > +               .mono = 0,
> > > +       },
> > > +};
> > > +
> > >
> > >  struct imx290_regval {
> > >
> > >         u32 reg;
> > >         u32 val;
> > >
> > > @@ -210,9 +240,9 @@ struct imx290 {
> > >
> > >         struct device *dev;
> > >         struct clk *xclk;
> > >         struct regmap *regmap;
> > >
> > > +       const struct imx290_device_data *devdata;
> > >
> > >         u32 xclk_freq;
> > >         u8 nlanes;
> > >
> > > -       u8 mono;
> > >
> > >         struct v4l2_subdev sd;
> > >         struct media_pad pad;
> > >
> > > @@ -240,7 +270,7 @@ static inline struct imx290 *to_imx290(struct
> > > v4l2_subdev *_sd)>
> > >   * Modes and formats
> > >   */
> > >
> > > -static const struct imx290_regval imx290_global_init_settings[] = {
> > > +static const struct imx290_regval imx290_global_init_settings_290[] = {
> > >
> > >         { IMX290_WINWV_OB, 12 },
> > >         { IMX290_WINPH, 0 },
> > >         { IMX290_WINPV, 0 },
> > >
> > > @@ -292,6 +322,23 @@ static const struct imx290_regval
> > > imx290_global_init_settings[] = {>
> > >         { IMX290_REG_8BIT(0x33b3), 0x04 },
> > >
> > >  };
> > >
> > > +static const struct imx290_regval imx290_global_init_settings_327[] = {
> > > +       { IMX290_WINWV_OB, 12 },
> > > +       { IMX290_WINPH, 0 },
> > > +       { IMX290_WINPV, 0 },
> > > +       { IMX290_WINWH, 1948 },
> > > +       { IMX290_WINWV, 1097 },
> > > +       { IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC |
> > > +                          IMX290_XSOUTSEL_XHSOUTSEL_HSYNC },
> > > +       { IMX290_REG_8BIT(0x3011), 0x0A },
> >
> > What datasheet are you working from? Mine (2019/03/25) has a
> > correction listed at v0.3 of:
> > Register 3011h setting 0Ah -> 02h
>
> I've got a v0.2 (2017/05/25). As you have v0.3 I am not really surprised you
> have different/additional values.

Revision history appears to be
0.1 2017/02/23
0.2 2017/05/25
0.3 2017/09/04
E17Z06 2017/12/18
E17Z06A81 2018/02/01
E1706B93 2019/03/25
so you are a way behind.

>
> > > +       { IMX290_REG_8BIT(0x3012), 0x64 },
> > > +       { IMX290_REG_8BIT(0x3013), 0x00 },
> > > +       { IMX290_REG_8BIT(0x309e), 0x4A },
> > > +       { IMX290_REG_8BIT(0x309f), 0x4A },
> >
> > 309e/f undocumented in my datasheet beyond "default value 5Ah, set to
> > "4Ah"". Not documented in imx290 or imx462 datasheets either. I'll read it
> > back from IMX290 and IMX462 when I get to the office and see if 0x4a is the
> > default anyway, in which case it can be generic.

I can't read back from my IMX290LLR as Vision Components use a
secondary MCU to stop you reading registers.

IMX462LQR default 0x5a 0x5a :-(

> Exactly, they are not documented at all, just marked red indicating it's
> different from reset default.
>
> > > +       { IMX290_REG_8BIT(0x3128), 0x04 },
> >
> > Correction v0.3 - register address 3128h deleted.
> >
> > > +       { IMX290_REG_8BIT(0x313b), 0x41 },
> >
> > Correction v0.3 - Register address 313Bh setting 41h -> 61h.

IMX462LQR default 0x51

> >
> >
> > I'll check the defaults on imx290 and imx462, because there is no harm
> > in adding those register writes if they happen to be the defaults.
> > There is also a fair amount of duplication between
> > imx290_global_init_settings_290 and imx290_global_init_settings_327 -
> > it'd be nice to reduce it down to the minimum set of diffs.
>
> Thanks, I'll update to the values you provided and split the common settings.

So annoyingly that does mean we need a sensor specific table, but I
think it is only 3 registers (0x309e, 0x309ef, and 0x313b). None of
those need to be set for IMX290 or 462.

  Dave

> > > +};
> > > +
> > >
> > >  static const struct imx290_regval imx290_37_125mhz_clock[] = {
> > >
> > >         { IMX290_EXTCK_FREQ, 0x2520 },
> > >         { IMX290_INCKSEL7, 0x49 },
> > >
> > > @@ -558,7 +605,7 @@ imx290_format_info(const struct imx290 *imx290, u32
> > > code)>
> > >         for (i = 0; i < ARRAY_SIZE(imx290_formats); ++i) {
> > >
> > >                 const struct imx290_format_info *info =
> > >                 &imx290_formats[i];
> > >
> > > -               if (info->code[imx290->mono] == code)
> > > +               if (info->code[imx290->devdata->mono] == code)
> > >
> > >                         return info;
> > >
> > >         }
> > >
> > > @@ -957,11 +1004,27 @@ static int imx290_start_streaming(struct imx290
> > > *imx290,>
> > >                                   struct v4l2_subdev_state *state)
> > >
> > >  {
> > >
> > >         const struct v4l2_mbus_framefmt *format;
> > >
> > > +       const struct imx290_regval *regs;
> > > +       unsigned int reg_num;
> > >
> > >         int ret;
> > >
> > > +       switch (imx290->devdata->model) {
> > > +       case IMX290:
> > > +       case IMX290_MONO:
> > > +               regs = imx290_global_init_settings_290;
> > > +               reg_num = ARRAY_SIZE(imx290_global_init_settings_290);
> > > +               break;
> > > +       case IMX327:
> > > +               regs = imx290_global_init_settings_327;
> > > +               reg_num = ARRAY_SIZE(imx290_global_init_settings_327);
> > > +               break;
> > > +       default:
> > > +               dev_err(imx290->dev, "Invalid model: %u\n",
> > > imx290->devdata->model); +               return -EINVAL;
> > > +       }
> > > +
> > >
> > >         /* Set init register settings */
> > >
> > > -       ret = imx290_set_register_array(imx290,
> > > imx290_global_init_settings, -
> > > ARRAY_SIZE(imx290_global_init_settings)); +       ret =
> > > imx290_set_register_array(imx290, regs, reg_num);
> > >
> > >         if (ret < 0) {
> > >
> > >                 dev_err(imx290->dev, "Could not set init registers\n");
> > >                 return ret;
> > >
> > > @@ -1072,7 +1135,7 @@ static int imx290_enum_mbus_code(struct v4l2_subdev
> > > *sd,>
> > >         if (code->index >= ARRAY_SIZE(imx290_formats))
> > >
> > >                 return -EINVAL;
> > >
> > > -       code->code = imx290_formats[code->index].code[imx290->mono];
> > > +       code->code =
> > > imx290_formats[code->index].code[imx290->devdata->mono];>
> > >         return 0;
> > >
> > >  }
> > >
> > > @@ -1114,7 +1177,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> > >
> > >         fmt->format.height = mode->height;
> > >
> > >         if (!imx290_format_info(imx290, fmt->format.code))
> > >
> > > -               fmt->format.code = imx290_formats[0].code[imx290->mono];
> > > +               fmt->format.code =
> > > imx290_formats[0].code[imx290->devdata->mono];>
> > >         fmt->format.field = V4L2_FIELD_NONE;
> > >         fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> > >
> > > @@ -1422,8 +1485,9 @@ static s64 imx290_check_link_freqs(const struct
> > > imx290 *imx290,>
> > >  }
> > >
> > >  static const struct of_device_id imx290_of_match[] = {
> > >
> > > -       { .compatible = "sony,imx290" },
> > > -       { .compatible = "sony,imx290-mono", .data = (void *)1 },
> > > +       { .compatible = "sony,imx290", .data = &imx290_models[IMX290] },
> > > +       { .compatible = "sony,imx290-mono", .data =
> > > &imx290_models[IMX290_MONO] }, +       { .compatible = "sony,imx327",
> > > .data = &imx290_models[IMX327] },
> > Based on Laurent's requests my parent to this set will be switching to
> > imx290 (as legacy), imx290lqr and imx290llr as the compatible strings.
> > imx327 ought to follow the same pattern.
>
> Okay thanks. I'll update to imx327lqr as well. Put me on CC so I'll notice the
> update & conflict.
>
> Best regards,
> Alexander
>
> >   Dave
> >
> > >         { /* sentinel */ }
> > >
> > >  };
> > >  MODULE_DEVICE_TABLE(of, imx290_of_match);
> > >
> > > @@ -1441,8 +1505,7 @@ static int imx290_parse_dt(struct imx290 *imx290)
> > >
> > >         s64 fq;
> > >
> > >         match = i2c_of_match_device(imx290_of_match, client);
> > >
> > > -       if (match)
> > > -               imx290->mono = match->data ? 1 : 0;
> > > +       imx290->devdata = match->data;
> > >
> > >         endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev),
> > >         NULL);
> > >         if (!endpoint) {
> > >
> > > @@ -1561,6 +1624,9 @@ static int imx290_probe(struct i2c_client *client)
> > >
> > >         if (ret)
> > >
> > >                 goto err_pm;
> > >
> > > +       v4l2_i2c_subdev_set_name(&imx290->sd, client,
> > > +                                imx290->devdata->name, NULL);
> > > +
> > >
> > >         /*
> > >
> > >          * Finally, register the V4L2 subdev. This must be done after
> > >          * initializing everything as the subdev can be used immediately
> > >          after
> > >
> > > --
> > > 2.34.1
>
>
>
>