Re: [PATCH v4 3/3] media: i2c: imx471: Add Sony IMX471 image sensor driver

[Tarang Raval]

Hi Kate,

I noticed a few more issues. Could you please check the comments below?

Sorry, I missed these in my first review.

> Add a new driver for Sony imx471 camera sensor. It is based on
> Jimmy Su <jimmy.su@xxxxxxxxx> implementation and the driver can be found
> in the following URL.
> https://github.com/intel/ipu6-drivers/commits/master/drivers/media/i2c/imx471.c
>
> This sensor can be found on Lenovo X1 Carbon G14, X9-14 and X9-15 laptops
> and it is a part of IPU7 solution. The driver was tested on Lenovo X1
> Carbon G14, X9-14 and X9-15 laptops.
>
> Signed-off-by: Kate Hsuan <hpa@xxxxxxxxxx>

...

> +#define IMX471_REG_CSI_DATA_FORMAT             CCI_REG16(0x0112)
> +#define IMX471_CSI_DATA_FORMAT_RAW10           0x0a0a
> +
> +#define IMX471_REG_CSI_LANE_MODE               CCI_REG8(0x0114)
> +#define IMX471_CSI_2_LANE_MODE                 1
> +#define IMX471_CSI_4_LANE_MODE                 3

The CSI data format (0x0112) and lane mode (0x0114) registers are defined but
never programmed.

The out-of-tree driver mentioned in the cover letter configures these registers
during initialization. Shouldn't the same settings be applied here as part of
the global register configuration?

...

> +static const char * const imx471_supply_name[] = {
> +       "avdd",
> +};

Only avdd is defined as a regulator supply.

According to the datasheet, are there any additional power rails
(e.g. dvdd or iovdd) required by the sensor?


...

> +       { CCI_REG8(0x0307), 0x79 },
> +       { CCI_REG8(0x030b), 0x01 },
> +       { CCI_REG8(0x030d), 0x02 },
> +       { CCI_REG8(0x030e), 0x00 },
> +       { CCI_REG8(0x030f), 0x53 },
> +       { CCI_REG8(0x0310), 0x01 },
> +       { IMX471_REG_EXPOSURE, IMX471_EXPOSURE_DEFAULT },

drop this setting above.

> +       { CCI_REG8(0x3f4c), 0x81 },
> +       { CCI_REG8(0x3f4d), 0x81 },
> +       { CCI_REG8(0x3f78), 0x01 },
> +       { CCI_REG8(0x3f79), 0x31 },
> +       { CCI_REG8(0x3ffe), 0x00 },
> +       { CCI_REG8(0x3fff), 0x8a },
> +       { CCI_REG8(0x5f0a), 0xb6 },
> +};

...

> +static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +       struct imx471 *sensor = container_of_const(ctrl->handler,
> +                                                  struct imx471,
> +                                                  ctrl_handler);
> +       struct v4l2_subdev_state *state =
> +                       v4l2_subdev_get_locked_active_state(&sensor->sd);
> +       const struct v4l2_mbus_framefmt *format =
> +                       v4l2_subdev_state_get_format(state, 0);
> +       s64 exposure_max;
> +       int ret;
> +
> +       if (ctrl->id == V4L2_CID_VBLANK) {
> +               exposure_max =
> +                       format->height + ctrl->val - IMX471_EXPOSURE_MARGIN;
> +               ret = __v4l2_ctrl_modify_range(sensor->exposure,
> +                                              sensor->exposure->minimum,
> +                                              exposure_max,
> +                                              sensor->exposure->step,
> +                                              exposure_max);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       /* V4L2 controls values will be applied only when power is already up */
> +       if (!pm_runtime_get_if_in_use(sensor->dev))

Use pm_runtime_get_if_active() or update the comment. With pm_runtime_get_if_in_use(),
the comment should say "applied only when the device is in use".

> +               return 0;
> +
> +       switch (ctrl->id) {
> +       case V4L2_CID_ANALOGUE_GAIN:
> +               ret = cci_write(sensor->regmap, IMX471_REG_ANALOG_GAIN,
> +                               ctrl->val, NULL);
> +               break;
> +       case V4L2_CID_DIGITAL_GAIN:
> +               ret = cci_write(sensor->regmap, IMX471_REG_DIG_GAIN_GLOBAL,
> +                               ctrl->val, NULL);
> +               break;
> +       case V4L2_CID_EXPOSURE:
> +               ret = cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
> +                               ctrl->val, &ret);
> +               break;
> +       case V4L2_CID_VBLANK:
> +               /* Update FLL that meets expected vertical blanking */
> +               ret = cci_write(sensor->regmap, IMX471_REG_FLL,
> +                               format->height + ctrl->val, &ret);
> +               break;
> +       case V4L2_CID_TEST_PATTERN:
> +               ret = cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
> +                               ctrl->val, NULL);
> +               break;
> +       case V4L2_CID_HFLIP:
> +       case V4L2_CID_VFLIP:
> +               ret = cci_write(sensor->regmap, IMX471_REG_ORIENTATION,
> +                               sensor->hflip->val | sensor->vflip->val << 1, NULL);
> +               break;
> +       default:
> +               ret = -EINVAL;
> +               dev_info(sensor->dev, "ctrl(id:0x%x,val:0x%x) is not handled",

Use dev_err.

> +                        ctrl->id, ctrl->val);
> +               break;
> +       }
> +
> +       pm_runtime_put(sensor->dev);
> +
> +       return ret;
> +}

...

> +static int imx471_set_pad_format(struct v4l2_subdev *sd,
> +                                struct v4l2_subdev_state *sd_state,
> +                                struct v4l2_subdev_format *fmt)
> +{
> +       struct imx471 *sensor = to_imx471(sd);
> +       const struct imx471_mode *mode;
> +       u64 pixel_rate;
> +       int h_blank;
> +       int ret;

int h_blank, ret;

> +
> +       mode = v4l2_find_nearest_size(imx471_modes,
> +                                     ARRAY_SIZE(imx471_modes),
> +                                     width, height,
> +                                     fmt->format.width, fmt->format.height);
> +
> +       imx471_update_pad_format(sensor, mode, fmt);
> +
> +       *v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
> +
> +       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> +               return 0;
> +
> +       if (media_entity_is_streaming(&sensor->sd.entity))
> +               return -EBUSY;
> +
> +       pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> +       ret = __v4l2_ctrl_modify_range(sensor->pixel_rate,
> +                                      V4L2_CID_PIXEL_RATE,
> +                                      pixel_rate, 1, pixel_rate);
> +       if (ret)
> +               return ret;

Why do we need this? You have a fixed pixel_rate. Drop the settings above.

> +
> +       ret = __v4l2_ctrl_modify_range(sensor->vblank,
> +                                      mode->fll_min - mode->height,
> +                                      IMX471_FLL_MAX - mode->height,
> +                                      1,
> +                                      mode->fll_def - mode->height);
> +       if (ret)
> +               return ret;
> +
> +       h_blank = mode->llp - mode->width;
> +       /*
> +        * Currently hblank is not changeable.
> +        * So FPS control is done only by vblank.
> +        */
> +       return __v4l2_ctrl_modify_range(sensor->hblank, h_blank,
> +                                       h_blank, 1, h_blank);
> +}

...

> +static int imx471_enable_stream(struct v4l2_subdev *sd,
> +                               struct v4l2_subdev_state *state,
> +                               u32 pad, u64 streams_mask)
> +{
> +       struct imx471 *sensor = to_imx471(sd);
> +       const struct imx471_mode *mode;
> +       struct v4l2_mbus_framefmt *fmt;
> +       int ret;
> +
> +       ret = pm_runtime_resume_and_get(sensor->dev);
> +       if (ret)
> +               return ret;
> +
> +       ret = imx471_identify_module(sensor);
> +       if (ret)
> +               return ret;

Runtime PM leak. Please call error_powerdown at return path.

> +
> +       ret = cci_multi_reg_write(sensor->regmap, imx471_global_regs,
> +                                 ARRAY_SIZE(imx471_global_regs), NULL);
> +       if (ret) {
> +               dev_err(sensor->dev, "failed to set global settings: %d", ret);
> +               goto error_powerdown;
> +       }
> +
> +       state = v4l2_subdev_get_locked_active_state(&sensor->sd);

Why is the active state retrieved again here? state is already passed to enable_stream()

Drop this line.

> +       fmt = v4l2_subdev_state_get_format(state, 0);
> +       mode = v4l2_find_nearest_size(imx471_modes, ARRAY_SIZE(imx471_modes),
> +                                     width, height, fmt->width, fmt->height);
> +
> +       ret = cci_multi_reg_write(sensor->regmap, mode->default_mode_regs,
> +                                 mode->default_mode_regs_length, NULL);
> +       if (ret) {
> +               dev_err(sensor->dev, "failed to set mode: %d", ret);
> +               goto error_powerdown;
> +       }
> +
> +       ret = cci_write(sensor->regmap, IMX471_REG_DPGA_USE_GLOBAL_GAIN, 1, NULL);
> +       if (ret)
> +               goto error_powerdown;
> +
> +       ret = __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
> +       if (ret)
> +               goto error_powerdown;
> +
> +       ret = cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
> +                       IMX471_MODE_STREAMING, NULL);
> +       if (ret)
> +               goto error_powerdown;
> +
> +       __v4l2_ctrl_grab(sensor->vflip, true);
> +       __v4l2_ctrl_grab(sensor->hflip, true);
> +
> +       return ret;
> +
> +error_powerdown:
> +       pm_runtime_put(sensor->dev);
> +
> +       return ret;

...

> +static int imx471_init_controls(struct imx471 *sensor)
> +{
> +       const struct imx471_mode *mode = &imx471_modes[0];
> +       struct v4l2_fwnode_device_properties props;
> +       struct v4l2_ctrl_handler *ctrl_hdlr;
> +       struct v4l2_ctrl *link_freq;
> +       s64 exposure_max, hblank;
> +       u64 pixel_rate;
> +       int ret;
> +
> +       ctrl_hdlr = &sensor->ctrl_handler;
> +       v4l2_ctrl_handler_init(ctrl_hdlr, 12);
> +
> +       ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> +       if (ret) {
> +               dev_err(sensor->dev, "failed to parse fwnode: %d", ret);
> +               return ret;

Memory leak, Use goto error here.

> +       }
> +
> +       v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx471_ctrl_ops, &props);
> +
> +       link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> +                                          &imx471_ctrl_ops,
> +                                          V4L2_CID_LINK_FREQ,
> +                                          ARRAY_SIZE(link_freq_menu_items) - 1,
> +                                          0,
> +                                          link_freq_menu_items);
> +
> +       /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> +       pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> +
> +       sensor->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +                                              V4L2_CID_PIXEL_RATE, pixel_rate,
> +                                              pixel_rate, 1, pixel_rate);

You can drop pixel_rate from struct sensor. it is not used anywhere in the driver.

> +
> +       sensor->vblank = v4l2_ctrl_new_std(ctrl_hdlr,
> +                                          &imx471_ctrl_ops,
> +                                          V4L2_CID_VBLANK,
> +                                          mode->fll_min - mode->height,
> +                                          IMX471_FLL_MAX - mode->height,
> +                                          1,
> +                                          mode->fll_def - mode->height);
> +
> +       hblank = mode->llp - mode->width;
> +       sensor->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +                                          V4L2_CID_HBLANK, hblank, hblank,
> +                                          1, hblank);
> +
> +       /* fll >= exposure time + adjust parameter (default value is 18) */
> +       exposure_max = mode->fll_def - IMX471_EXPOSURE_MARGIN;
> +       sensor->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +                                            V4L2_CID_EXPOSURE,
> +                                            IMX471_EXPOSURE_MIN, exposure_max,
> +                                            IMX471_EXPOSURE_STEP,
> +                                            IMX471_EXPOSURE_DEFAULT);
> +
> +       v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> +                         IMX471_ANA_GAIN_MIN, IMX471_ANA_GAIN_MAX,
> +                         IMX471_ANA_GAIN_STEP, IMX471_ANA_GAIN_DEFAULT);
> +
> +       v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
> +                         IMX471_DGTL_GAIN_MIN, IMX471_DGTL_GAIN_MAX,
> +                         IMX471_DGTL_GAIN_STEP, IMX471_DGTL_GAIN_DEFAULT);
> +
> +       v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx471_ctrl_ops,
> +                                    V4L2_CID_TEST_PATTERN,
> +                                    ARRAY_SIZE(imx471_test_pattern_menu) - 1,
> +                                    0, 0, imx471_test_pattern_menu);
> +
> +       sensor->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +                                         V4L2_CID_HFLIP, 0, 1, 1, 0);
> +
> +       sensor->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +                                         V4L2_CID_VFLIP, 0, 1, 1, 0);
> +
> +       if (ctrl_hdlr->error) {
> +               dev_err(sensor->dev, "%s control init failed: %d",
> +                       __func__, ctrl_hdlr->error);
> +               goto error;
> +       }
> +
> +       link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +       sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +       sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +       sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> +       sensor->sd.ctrl_handler = ctrl_hdlr;
> +
> +       return 0;
> +
> +error:
> +       v4l2_ctrl_handler_free(ctrl_hdlr);
> +
> +       return ctrl_hdlr->error;
> +}
> +
> +static int imx471_check_hwcfg(struct imx471 *sensor)
> +{
> +       struct v4l2_fwnode_endpoint bus_cfg = {
> +               .bus_type = V4L2_MBUS_CSI2_DPHY,
> +       };
> +       struct fwnode_handle *ep, *fwnode = dev_fwnode(sensor->dev);
> +       unsigned long link_freq_bitmap;
> +       struct clk *clk;
> +       int ret;
> +
> +       clk = devm_v4l2_sensor_clk_get(sensor->dev, NULL);
> +       if (IS_ERR(clk))
> +               return dev_err_probe(sensor->dev, PTR_ERR(clk),
> +                                    "can't get clock frequency\n");
> +
> +       if (clk_get_rate(clk) != IMX471_EXT_CLK)
> +               return dev_err_probe(sensor->dev, -EINVAL,
> +                                    "external clock %lu is not supported\n",
> +                                    clk_get_rate(clk));
> +
> +       ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
> +       ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> +       fwnode_handle_put(ep);
> +       if (ret)
> +               return dev_err_probe(sensor->dev, ret,
> +                                    "parsing endpoint failed");
> +
> +       ret = v4l2_link_freq_to_bitmap(sensor->dev, bus_cfg.link_frequencies,
> +                                      bus_cfg.nr_of_link_frequencies,
> +                                      link_freq_menu_items,
> +                                      ARRAY_SIZE(link_freq_menu_items),
> +                                      &link_freq_bitmap);
> +
> +       v4l2_fwnode_endpoint_free(&bus_cfg);

The sensor supports both 2 and 4 CSI-2 data lanes, but this driver only
implements the 4-lane configuration. Please validate the data-lanes property
and reject anything other than 4 lanes.

> +
> +       return ret;
> +}

...

> +static const struct acpi_device_id imx471_acpi_ids[] __maybe_unused = {
> +       { "SONY471A" },
> +       { "TBE20A0" },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(acpi, imx471_acpi_ids);
> +
> +static struct i2c_driver imx471_i2c_driver = {
> +       .driver = {
> +               .name = "imx471",
> +               .acpi_match_table = ACPI_PTR(imx471_acpi_ids),

Could you please add .of_match_table as well? The driver can also be used
on DT-based systems.

> +               .pm = pm_sleep_ptr(&imx471_pm_ops),
> +       },
> +       .probe = imx471_probe,
> +       .remove = imx471_remove,
> +};
> +module_i2c_driver(imx471_i2c_driver);
> +
> +MODULE_AUTHOR("Jimmy Su <jimmy.su@xxxxxxxxx>");
> +MODULE_AUTHOR("Serin Yeh <serin.yeh@xxxxxxxxx>");
> +MODULE_AUTHOR("Kate Hsuan <hpa@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Sony imx471 sensor driver");
> +MODULE_LICENSE("GPL");
> --
> 2.54.0

Best Regards,
Tarang