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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Tarang, Kate,

On Fri, May 22, 2026 at 11:12:53AM +0000, Tarang Raval wrote:
> > +/* Exposure control */
> > +#define IMX471_REG_EXPOSURE                    CCI_REG16(0x0202)
> > +#define IMX471_EXPOSURE_MIN                    1
> > +#define IMX471_EXPOSURE_STEP                   1
> > +#define IMX471_EXPOSURE_DEFAULT                        0x04f6
> 
> Better to use a decimal value here.

I'd rather just initialise this to the maximum instead of a fixed value.


...

> > +static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +       struct imx471 *sensor = container_of(ctrl->handler,
> > +                                                 struct imx471,
> > +                                                 ctrl_handler);
> 
> Use container_of_const.
> 
> > +       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;
> 
> ret = 0;

Or assign the return value to ret below. Either works.

> 
> > +
> > +       /* Propagate change of current control to all related controls */
> > +       if (ctrl->id == V4L2_CID_VBLANK) {
> > +               /* Update max exposure while meeting expected vblanking */
> > +               exposure_max =
> > +                       format->height + ctrl->val - IMX471_EXPOSURE_MARGIN;
> > +               __v4l2_ctrl_modify_range(sensor->exposure,
> > +                                        sensor->exposure->minimum,
> > +                                        exposure_max,
> > +                                        sensor->exposure->step,
> > +                                        exposure_max);
> 
> This control operation can fail. Please check the return value.
> 
> > +       }
> > +
> > +       /* V4L2 controls values will be applied only when power is already up */
> > +       if (!pm_runtime_get_if_in_use(sensor->dev))
> > +               return 0;
> > +
> > +       switch (ctrl->id) {
> > +       case V4L2_CID_ANALOGUE_GAIN:
> > +               cci_write(sensor->regmap, IMX471_REG_ANALOG_GAIN,
> > +                         ctrl->val, &ret);
> 
> You are using ret for the first time here, Please initialize ret with 0 when
> declaring it.
> 
> cci_write() uses the value pointed by &ret to determine whether a previous
> error has already occurred, and an uninitialized ret may contain a garbage
> value, causing the write operation to fail unexpectedly.
> 
> > +               break;
> > +       case V4L2_CID_DIGITAL_GAIN:
> > +               cci_write(sensor->regmap, IMX471_REG_DIG_GAIN_GLOBAL,
> > +                         ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_EXPOSURE:
> > +               cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
> > +                         ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_VBLANK:
> > +               /* Update FLL that meets expected vertical blanking */
> > +               cci_write(sensor->regmap, IMX471_REG_FLL,
> > +                         format->height + ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_TEST_PATTERN:
> > +               cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
> > +                         ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_HFLIP:
> > +       case V4L2_CID_VFLIP:
> > +               cci_write(sensor->regmap, IMX471_REG_ORIENTATION,
> > +                         sensor->hflip->val | sensor->vflip->val << 1, &ret);
> > +               break;
> > +       default:
> > +               ret = -EINVAL;
> > +               dev_info(sensor->dev, "ctrl(id:0x%x,val:0x%x) is not handled",
> > +                        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;
> > +       int h_blank;
> > +       u64 pixel_rate;
> > +
> > +       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 = IMX471_LINK_FREQ_DEFAULT * 2 * 4;
> > +       div_u64(pixel_rate, 10);
> 
> You need to store the return value, as the above operation does not update
> pixel_rate.
> 
> Please use:
> pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> 
> > +       __v4l2_ctrl_modify_range(sensor->pixel_rate,
> > +                                V4L2_CID_PIXEL_RATE,
> > +                                pixel_rate, 1, pixel_rate);
> > +
> > +       __v4l2_ctrl_modify_range(sensor->vblank,
> > +                                mode->fll_min - mode->height,
> > +                                IMX471_FLL_MAX - mode->height,
> > +                                1,
> > +                                mode->fll_def - mode->height);
> > +
> > +       h_blank = mode->llp - mode->width;
> > +       /*
> > +        * Currently hblank is not changeable.
> > +        * So FPS control is done only by vblank.
> > +        */
> > +       __v4l2_ctrl_modify_range(sensor->hblank, h_blank,
> > +                                h_blank, 1, h_blank);
> 
> All the above control operations can fail. Please add proper error checks for them.
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int imx471_get_selection(struct v4l2_subdev *sd,
> > +                               struct v4l2_subdev_state *sd_state,
> > +                               struct v4l2_subdev_selection *sel)
> > +{
> > +       switch (sel->target) {
> > +       case V4L2_SEL_TGT_CROP:
> > +               sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
> > +               break;
> > +
> > +       case V4L2_SEL_TGT_NATIVE_SIZE:
> > +               sel->r.top = 0;
> > +               sel->r.left = 0;
> > +               sel->r.width = IMX471_NATIVE_WIDTH;
> > +               sel->r.height = IMX471_NATIVE_HEIGHT;
> > +               return 0;
> > +
> > +       case V4L2_SEL_TGT_CROP_DEFAULT:
> > +       case V4L2_SEL_TGT_CROP_BOUNDS:
> > +               sel->r.top = IMX471_PIXEL_ARRAY_TOP;
> > +               sel->r.left = IMX471_PIXEL_ARRAY_LEFT;
> > +               sel->r.width = IMX471_PIXEL_ARRAY_WIDTH;
> > +               sel->r.height = IMX471_PIXEL_ARRAY_HEIGHT;
> > +               return 0;
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> > +
> > +static int imx471_init_state(struct v4l2_subdev *sd,
> > +                            struct v4l2_subdev_state *sd_state)
> > +{
> > +       struct v4l2_subdev_format fmt = {
> > +               .which = V4L2_SUBDEV_FORMAT_ACTIVE,

You shouldn't be setting the active format here.

> > +               .format = {
> > +                       .code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > +                       .width = imx471_modes[0].width,
> > +                       .height = imx471_modes[0].height,
> > +               },
> > +       };
> > +
> > +       imx471_set_pad_format(sd, sd_state, &fmt);
> > +
> > +       return 0;

...

> > +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);
> > +       struct clk *clk;
> > +       unsigned long link_freq_bitmap;
> > +       int ret;
> 
> If you want, you can sort the variable declarations throughout the code, where
> appropriate, by length to make them more readable.
> 
> > +
> > +       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);
> 
> This can fail silently. Please add an error message before returning the failure.

v4l2_link_freq_to_bitmap() does print errors already, don't do it here.

> 
> > +
> > +       v4l2_fwnode_endpoint_free(&bus_cfg);
> > +
> > +       return ret;
> > +}

-- 
Regards,

Sakari Ailus