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

[Kate Hsuan]

Hi Tarang,

Thank you for reviewing it.

On Wed, Jun 10, 2026 at 3:11 PM Tarang Raval
<tarang.raval@xxxxxxxxxxxxxxxxx> wrote:
>
> 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?

ops. I didn't put them back in the cci_reg_sequence when I changed the
register names.
I'll check the register values.

>
> ...
>
> > +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?
I only found avdd on X1 Carbon G14, my X9-14 and 15. I only have this
one regulator to test and ensure it runs.
According to the datasheet, I found "VNAN, VDIG, and VIF". Should I
add them to the list?

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

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

I'll drop the comment.

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

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

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

There is only one mode for this driver. This can be dropped.

>
> > +
> > +       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.
OK. I'll check the rest of the rumtime PM operations.

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

OK

>
> > +       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.
I'll do it here as follows.
   v4l2_ctrl_handler_free(ctrl_hdlr);
   return ret;

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

sure.

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

I don't have the hardware to verify it. I think it is better if
someone needs it and adds it.



On Wed, Jun 10, 2026 at 3:11 PM Tarang Raval
<tarang.raval@xxxxxxxxxxxxxxxxx> wrote:
>
> 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
>


--
BR,
Kate