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

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

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.

> > +       { 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.

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.
> 
> 
> 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.

> > +};
> > +
> > 
> >  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