Re: [PATCH 2/3] media: i2c: add imx576 image sensor driver

[Tarang Raval]

Hi Himanshu,

I noticed a few issues. Please check.

> Add a v4l2 subdevice driver for the Sony imx576 sensor.
>
> The Sony IMX576 image sensor with an active
> array size of 5760 x 4312
>
> The following features are supported:
> - Manual exposure an gain control support
> - vblank/hblank control support
> - Supported resolution: 2880 x 2156 30fps (SRGGB10)
>
> Signed-off-by: Himanshu Bhavani <himanshu.bhavani@xxxxxxxxxxxxxxxxx>
> ---
>  MAINTAINERS                |    1 +
>  drivers/media/i2c/Kconfig  |   10 +
>  drivers/media/i2c/Makefile |    1 +
>  drivers/media/i2c/imx576.c | 1029 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 1041 insertions(+)
>  create mode 100644 drivers/media/i2c/imx576.c

...

> +static const struct imx576_mode supported_modes_10bit[] = {
> +       {
> +               .width = 2880,
> +               .height = 2156,
> +               .hts = 3165,
> +               .vts = 2172,
> +               .reg_list = {
> +                       .num_of_regs = ARRAY_SIZE(mode_2880x2156_regs),
> +                       .regs = mode_2880x2156_regs,
> +               },
> +       },
> +};
> +
> +static const s64 link_freq[] = {

your control name is also link_freq, so could you please use a
more meaningful name here to avoid misunderstanding?

> +       IMX576_LINK_FREQ_600MHZ,
> +};
> +
> +static const u32 imx576_mbus_codes[] = {
> +       MEDIA_BUS_FMT_SRGGB10_1X10,
> +};
> +
> +static inline struct imx576 *to_imx576(struct v4l2_subdev *sd)
> +{
> +       return container_of_const(sd, struct imx576, sd);
> +}
> +
> +static int imx576_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +       struct imx576 *imx576 = container_of_const(ctrl->handler,
> +                                                  struct imx576, handler);
> +       struct v4l2_subdev_state *state;
> +       struct v4l2_mbus_framefmt *fmt;
> +       int ret = 0;
> +
> +       state = v4l2_subdev_get_locked_active_state(&imx576->sd);
> +       fmt = v4l2_subdev_state_get_format(state, 0);
> +
> +       if (ctrl->id == V4L2_CID_VBLANK) {
> +               /* Honour the VBLANK limits when setting exposure */
> +               ret = __v4l2_ctrl_modify_range(imx576->exposure,
> +                                              IMX576_EXPOSURE_MIN,
> +                                              fmt->height + ctrl->val -
> +                                              IMX576_EXPOSURE_OFFSET,
> +                                              1, IMX576_EXPOSURE_DEFAULT);

Please use IMX576_EXPOSURE_STEP instead of 1.

> +               if (ret)
> +                       return ret;
> +       }
> +
> +       if (pm_runtime_get_if_active(imx576->dev) == 0)
> +               return 0;
> +
> +       cci_write(imx576->regmap, IMX576_REG_HOLD, 1, &ret);
> +
> +       switch (ctrl->id) {
> +       case V4L2_CID_VBLANK: {
> +               u64 vmax = fmt->height + ctrl->val;
> +
> +               ret = cci_write(imx576->regmap, IMX576_REG_VTS, vmax, &ret);

You are already passing &ret to cci_write() for error handling, so why are
you assigning the return value again using ret = ?

Please avoid this redundant assignment here and in all other controls as well.

> +               break;
> +       }
> +       case V4L2_CID_EXPOSURE:
> +               ret = cci_write(imx576->regmap, IMX576_REG_EXPOSURE, ctrl->val, &ret);
> +               break;
> +       case V4L2_CID_ANALOGUE_GAIN:
> +               ret = cci_write(imx576->regmap, IMX576_REG_ANALOG_GAIN,
> +                               ctrl->val, &ret);
> +               break;
> +       default:
> +               dev_err(imx576->dev, "Invalid control %d\n", ctrl->id);
> +               ret = -EINVAL;
> +               break;
> +       }
> +
> +       cci_write(imx576->regmap, IMX576_REG_HOLD, 0, &ret);
> +
> +       pm_runtime_put(imx576->dev);
> +
> +       return ret;
> +}

...

> +static int imx576_enable_streams(struct v4l2_subdev *sd,
> +                                struct v4l2_subdev_state *state,
> +                                u32 pad, u64 streams_mask)
> +{
> +       const struct imx576_reg_list *reg_list;
> +       struct imx576 *imx576 = to_imx576(sd);
> +       const struct v4l2_mbus_framefmt *fmt;
> +       const struct imx576_mode *mode;
> +       int ret;
> +
> +       fmt = v4l2_subdev_state_get_format(state, 0);
> +       mode = v4l2_find_nearest_size(supported_modes_10bit,
> +                                     ARRAY_SIZE(supported_modes_10bit), width,
> +                                     height, fmt->width, fmt->height);
> +
> +       ret = pm_runtime_resume_and_get(imx576->dev);
> +       if (ret < 0)
> +               return ret;
> +
> +       /* Write common registers */
> +       ret = cci_multi_reg_write(imx576->regmap, imx576_common_regs,
> +                                 ARRAY_SIZE(imx576_common_regs), NULL);
> +       if (ret) {
> +               dev_err(imx576->dev, "failed to write common registers\n");
> +               goto err_rpm_put;
> +       }
> +
> +       /* Write sensor mode registers */
> +       reg_list = &mode->reg_list;
> +       ret = cci_multi_reg_write(imx576->regmap, reg_list->regs,
> +                                 reg_list->num_of_regs, NULL);
> +       if (ret) {
> +               dev_err(imx576->dev, "fail to write initial registers\n");
> +               goto err_rpm_put;
> +       }
> +
> +       /* Apply customized user controls */
> +       ret =  __v4l2_ctrl_handler_setup(imx576->sd.ctrl_handler);
> +       if (ret) {
> +               dev_err(imx576->dev, "fail to setup handler\n");
> +               goto err_rpm_put;
> +       }
> +
> +       /* T7: delay before sending stream command */
> +       usleep_range(8000, 9000);
> +
> +       /* Start streaming */
> +       cci_write(imx576->regmap, IMX576_REG_MODE_SELECT, IMX576_MODE_STREAMING, &ret);
> +       if (ret) {
> +               dev_err(imx576->dev, "fail to start streaming\n");
> +               goto err_rpm_put;
> +       }

Please be consistent everywhere. Here, use ret = cci_write(...).

> +
> +       return 0;
> +
> +err_rpm_put:
> +       pm_runtime_put(imx576->dev);
> +
> +       return ret;
> +}
> +
> +static int imx576_disable_streams(struct v4l2_subdev *sd,
> +                                 struct v4l2_subdev_state *state,
> +                                 u32 pad, u64 streams_mask)
> +{
> +       struct imx576 *imx576 = to_imx576(sd);
> +       int ret;
> +
> +       ret = cci_write(imx576->regmap, IMX576_REG_MODE_SELECT,
> +                       IMX576_MODE_STANDBY, NULL);
> +       if (ret)
> +               dev_err(imx576->dev, "failed to set stream off\n");
> +
> +       pm_runtime_put(imx576->dev);
> +
> +       return 0;
> +}
> +
> +static int imx576_init_state(struct v4l2_subdev *sd,
> +                            struct v4l2_subdev_state *sd_state)
> +{
> +       struct v4l2_subdev_format fmt = {
> +               .which = V4L2_SUBDEV_FORMAT_TRY,
> +               .pad = 0,
> +               .format = {
> +                       .code = imx576_mbus_codes[0],
> +                       .width = supported_modes_10bit[0].width,
> +                       .height = supported_modes_10bit[0].height,
> +               },
> +       };
> +
> +       imx576_set_pad_format(sd, sd_state, &fmt);
> +
> +       return 0;

imx576_set_pad_format() can fail.

return imx576_set_pad_format(sd, sd_state, &fmt);

> +}

...

> +static int imx576_init_controls(struct imx576 *imx576)
> +{
> +       const struct imx576_mode *mode = &supported_modes_10bit[0];
> +       struct v4l2_fwnode_device_properties props;
> +       u64 vblank_def, hblank_def, pixel_rate;
> +       struct v4l2_ctrl_handler *ctrl_hdlr;
> +       u32 lpfr;
> +       int ret;
> +
> +       ctrl_hdlr = &imx576->handler;
> +       v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> +
> +       /* Initialize exposure and gain */

Please remove this comment.

> +       vblank_def = mode->vts - mode->height;
> +       lpfr = vblank_def + mode->height;
> +       imx576->exposure = v4l2_ctrl_new_std(ctrl_hdlr,
> +                                            &imx576_ctrl_ops,
> +                                            V4L2_CID_EXPOSURE,
> +                                            IMX576_EXPOSURE_MIN,
> +                                            lpfr - IMX576_EXPOSURE_OFFSET,
> +                                            IMX576_EXPOSURE_STEP,
> +                                            IMX576_EXPOSURE_DEFAULT);
> +
> +       imx576->gain = v4l2_ctrl_new_std(ctrl_hdlr, &imx576_ctrl_ops,
> +                                        V4L2_CID_ANALOGUE_GAIN,
> +                                        IMX576_ANA_GAIN_MIN,
> +                                        IMX576_ANA_GAIN_MAX,
> +                                        IMX576_ANA_GAIN_STEP,
> +                                        IMX576_ANA_GAIN_DEFAULT);
> +
> +       imx576->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx576_ctrl_ops,
> +                                          V4L2_CID_VBLANK, vblank_def,
> +                                          IMX576_VBLANK_MAX, 1,
> +                                          vblank_def);
> +
> +       /* pixel_rate = link_frequency * 2 * nr_of_lanes / bits_per_sample */
> +       pixel_rate = div_u64(IMX576_LINK_FREQ_600MHZ * 2 * IMX576_NUM_DATA_LANES, 10);
> +       v4l2_ctrl_new_std(ctrl_hdlr, &imx576_ctrl_ops, V4L2_CID_PIXEL_RATE,
> +                         pixel_rate, pixel_rate, 1, pixel_rate);
> +
> +       imx576->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx576_ctrl_ops,
> +                                                  V4L2_CID_LINK_FREQ,
> +                                                  ARRAY_SIZE(link_freq) - 1,
> +                                                  imx576->link_freq_index,

Something looks wrong here. You are not initializing link_freq_index anywhere,
so why are you using imx576->link_freq_index as the default value?

Please use 0 as the default value here and remove link_freq_index from the
imx576 structure.

> +                                                  link_freq);
> +       if (imx576->link_freq)
> +               imx576->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +       hblank_def = mode->hts - mode->width;
> +       imx576->hblank = v4l2_ctrl_new_std(ctrl_hdlr,
> +                                          &imx576_ctrl_ops,
> +                                          V4L2_CID_HBLANK,
> +                                          hblank_def,
> +                                          hblank_def,
> +                                          1, hblank_def);
> +       if (imx576->hblank)
> +               imx576->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +       ret = v4l2_fwnode_device_parse(imx576->dev, &props);
> +       if (ret)
> +               goto err_handler_free;
> +
> +       ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx576_ctrl_ops, &props);
> +       if (ret)
> +               goto err_handler_free;
> +
> +       imx576->sd.ctrl_handler = ctrl_hdlr;
> +
> +       return 0;
> +
> +err_handler_free:
> +       v4l2_ctrl_handler_free(ctrl_hdlr);
> +
> +       return ret;
> +}
> +
> +static int imx576_probe(struct i2c_client *client)
> +{
> +       struct imx576 *imx576;
> +       unsigned int inclk_freq;
> +       int ret;
> +
> +       imx576 = devm_kzalloc(&client->dev, sizeof(*imx576), GFP_KERNEL);
> +       if (!imx576)
> +               return -ENOMEM;
> +
> +       imx576->dev = &client->dev;
> +
> +       /* Initialize subdev */
> +       v4l2_i2c_subdev_init(&imx576->sd, client, &imx576_subdev_ops);
> +       imx576->sd.internal_ops = &imx576_internal_ops;
> +
> +       imx576->regmap = devm_cci_regmap_init_i2c(client, 16);
> +       if (IS_ERR(imx576->regmap))
> +               return dev_err_probe(imx576->dev, PTR_ERR(imx576->regmap),
> +                                    "failed to initialize CCI\n");
> +
> +       ret = imx576_parse_endpoint(imx576);
> +       if (ret)
> +               return dev_err_probe(imx576->dev, ret,
> +                                    "failed to parse endpoint configuration\n");
> +
> +       /* Get sensor input clock */
> +       imx576->inclk = devm_v4l2_sensor_clk_get(imx576->dev, NULL);
> +       if (IS_ERR(imx576->inclk))
> +               return dev_err_probe(imx576->dev, PTR_ERR(imx576->inclk),
> +                                    "failed to get inclk\n");
> +
> +       inclk_freq = clk_get_rate(imx576->inclk);
> +       if (inclk_freq != IMX576_INCLK_RATE)
> +               return dev_err_probe(imx576->dev, -EINVAL,
> +                                    "inclk frequency not supported: %u Hz\n",
> +                                    inclk_freq);
> +
> +       for (unsigned int i = 0; i < ARRAY_SIZE(imx576_supply_names); i++)
> +               imx576->supplies[i].supply = imx576_supply_names[i];
> +
> +       ret = devm_regulator_bulk_get(imx576->dev,
> +                                     ARRAY_SIZE(imx576_supply_names),
> +                                     imx576->supplies);
> +       if (ret)
> +               return dev_err_probe(imx576->dev, ret,
> +                                    "failed to get regulators\n");
> +
> +       imx576->reset_gpio = devm_gpiod_get_optional(imx576->dev, "reset",
> +                                                    GPIOD_OUT_HIGH);
> +       if (IS_ERR(imx576->reset_gpio))
> +               return dev_err_probe(imx576->dev, PTR_ERR(imx576->reset_gpio),
> +                                    "failed to get reset GPIO\n");
> +
> +       ret = imx576_power_on(imx576->dev);
> +       if (ret)
> +               return ret;
> +
> +       ret = imx576_detect(imx576);
> +       if (ret)
> +               goto error_power_off;
> +
> +       ret = imx576_init_controls(imx576);
> +       if (ret) {
> +               dev_err(imx576->dev, "failed to init controls: %d\n", ret);

Use dev_err_probe.

> +               goto error_power_off;
> +       }
> +
> +       /* Initialize subdev */
> +       imx576->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +       imx576->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +       imx576->pad.flags = MEDIA_PAD_FL_SOURCE;
> +
> +       ret = media_entity_pads_init(&imx576->sd.entity, 1, &imx576->pad);
> +       if (ret) {
> +               dev_err(imx576->dev, "failed to init entity pads: %d\n", ret);

Use dev_err_probe.

> +               goto error_handler_free;
> +       }
> +
> +       imx576->sd.state_lock = imx576->handler.lock;
> +       ret = v4l2_subdev_init_finalize(&imx576->sd);
> +       if (ret < 0) {
> +               dev_err_probe(imx576->dev, ret, "subdev init error\n");
> +               goto error_media_entity;
> +       }
> +
> +       pm_runtime_set_active(imx576->dev);
> +       pm_runtime_enable(imx576->dev);
> +
> +       ret = v4l2_async_register_subdev_sensor(&imx576->sd);
> +       if (ret < 0) {
> +               dev_err_probe(imx576->dev, ret,
> +                             "failed to register imx576 sub-device\n");
> +               goto error_subdev_cleanup;
> +       }
> +
> +       pm_runtime_idle(imx576->dev);
> +
> +       return 0;
> +
> +error_subdev_cleanup:
> +       v4l2_subdev_cleanup(&imx576->sd);
> +       pm_runtime_disable(imx576->dev);
> +       pm_runtime_set_suspended(imx576->dev);
> +
> +error_media_entity:
> +       media_entity_cleanup(&imx576->sd.entity);
> +
> +error_handler_free:
> +       v4l2_ctrl_handler_free(imx576->sd.ctrl_handler);
> +
> +error_power_off:
> +       imx576_power_off(imx576->dev);
> +
> +       return ret;
> +}
> +
> +static void imx576_remove(struct i2c_client *client)
> +{
> +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +       struct imx576 *imx576 = to_imx576(sd);
> +
> +       v4l2_async_unregister_subdev(sd);
> +       v4l2_subdev_cleanup(&imx576->sd);
> +       media_entity_cleanup(&sd->entity);
> +       v4l2_ctrl_handler_free(imx576->sd.ctrl_handler);
> +
> +       pm_runtime_disable(&client->dev);
> +       if (!pm_runtime_status_suspended(&client->dev)) {
> +               imx576_power_off(&client->dev);
> +               pm_runtime_set_suspended(&client->dev);
> +       }
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(imx576_pm_ops,
> +                                imx576_power_off, imx576_power_on, NULL);
> +
> +static const struct of_device_id imx576_of_match[] = {
> +       { .compatible = "sony,imx576" },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx576_of_match);
> +
> +static struct i2c_driver imx576_driver = {
> +       .driver = {
> +               .name = "imx576",
> +               .pm = &imx576_pm_ops,
> +               .of_match_table = imx576_of_match,
> +       },
> +       .probe = imx576_probe,
> +       .remove = imx576_remove,
> +};
> +module_i2c_driver(imx576_driver);
> +
> +MODULE_DESCRIPTION("IMX576 Camera Sensor Driver");
> +MODULE_AUTHOR("Himanshu Bhavani <himanshu.bhavani@xxxxxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Hardevsinh Palaniya <hardevsinh.palaniya@xxxxxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1


Best Regards,
Tarang