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

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

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

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

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

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

  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
>