Re: [PATCH v4 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver

[Svyatoslav Ryhel]

вт, 11 лист. 2025 р. о 14:50 Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> пише:
>
> Hi Svyatoslav,
>
> A few comments below...
>
> On Mon, Nov 03, 2025 at 04:56:29PM +0200, Svyatoslav Ryhel wrote:
>
> ...
>
> > +static int imx111_set_format(struct v4l2_subdev *sd,
> > +                          struct v4l2_subdev_state *state,
> > +                          struct v4l2_subdev_format *format)
> > +{
> > +     struct imx111 *sensor = sd_to_imx111(sd);
> > +     struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
> > +     struct v4l2_mbus_framefmt *fmt;
> > +     const struct imx111_mode *mode;
> > +
> > +     mode = v4l2_find_nearest_size(imx111_modes, ARRAY_SIZE(imx111_modes),
> > +                                   width, height,
> > +                                   mbus_fmt->width, mbus_fmt->height);
> > +
> > +     fmt = v4l2_subdev_state_get_format(state, format->pad);
>
> You should set the fields below after changing the controls. Albeit... it
> won't be perfect in that case either, as only some controls may have been
> applied. How about just moving the lines below after changing the controls?
>
> > +
> > +     fmt->code = imx111_get_format_code(sensor, mbus_fmt->code, false);
> > +     fmt->width = mode->width;
> > +     fmt->height = mode->height;
> > +     fmt->colorspace = V4L2_COLORSPACE_RAW;
> > +
> > +     *mbus_fmt = *fmt;
> > +
> > +     if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > +             int ret;
> > +
> > +             sensor->cur_mode = mode;
> > +             sensor->data_depth = imx111_get_format_bpp(fmt);
> > +
> > +             ret = __v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate,
> > +                                            div_u64(sensor->pixel_clk_raw,
> > +                                                    2 *
> > +                                                    sensor->data_depth));
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = __v4l2_ctrl_modify_range(sensor->vblank,
> > +                                            IMX111_VBLANK_MIN,
> > +                                            IMX111_VTL_MAX - mode->height,
> > +                                            1,
> > +                                            mode->vtl_def - mode->height);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = __v4l2_ctrl_s_ctrl(sensor->vblank, mode->vtl_def -
> > +                                      mode->height);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = __v4l2_ctrl_modify_range(sensor->hblank,
> > +                                            IMX111_HBLANK_MIN,
> > +                                            IMX111_HTL_MAX - mode->width,
> > +                                            1,
> > +                                            mode->htl_def - mode->width);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = __v4l2_ctrl_s_ctrl(sensor->hblank, mode->htl_def -
> > +                                      mode->width);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> > +}
>
> ...
>
> > +static int imx111_probe(struct i2c_client *client)
> > +{
> > +     struct device *dev = &client->dev;
> > +     struct imx111 *sensor;
> > +     int ret;
> > +
> > +     sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> > +     if (!sensor)
> > +             return -ENOMEM;
> > +
> > +     sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> > +     if (IS_ERR(sensor->regmap))
> > +             return dev_err_probe(dev, PTR_ERR(sensor->regmap),
> > +                                  "Failed to allocate register map\n");
> > +
> > +     sensor->extclk = devm_v4l2_sensor_clk_get(dev, NULL);
> > +     if (IS_ERR(sensor->extclk))
> > +             return dev_err_probe(dev, PTR_ERR(sensor->extclk),
> > +                                  "Failed to get clock\n");
> > +
> > +     sensor->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > +     if (IS_ERR(sensor->reset))
> > +             return dev_err_probe(dev, PTR_ERR(sensor->reset),
> > +                                  "Failed to get reset GPIO\n");
> > +
> > +     ret = devm_regulator_bulk_get_const(dev, ARRAY_SIZE(imx111_supplies),
> > +                                         imx111_supplies,
> > +                                         &sensor->supplies);
> > +     if (ret < 0)
> > +             return dev_err_probe(dev, ret, "Failed to get regulators\n");
> > +
> > +     ret = imx111_parse_dt(sensor);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = imx111_clk_init(sensor);
> > +     if (ret < 0)
> > +             goto error_ep_free;
> > +
> > +     ret = imx111_power_on(sensor);
> > +     if (ret < 0) {
> > +             dev_err_probe(dev, ret, "Could not power on the device\n");
> > +             goto error_ep_free;
> > +     }
> > +
> > +     ret = imx111_identify_module(sensor);
> > +     if (ret < 0) {
> > +             dev_err_probe(dev, ret, "Could not identify module\n");
> > +             goto error_power_off;
> > +     }
> > +
> > +     sensor->cur_mode = &imx111_modes[IMX111_MODE_3280x2464];
> > +     sensor->data_depth = IMX111_DATA_DEPTH_RAW10;
> > +
> > +     ret = imx111_initialize(sensor);
> > +     if (ret < 0)
> > +             goto error_power_off;
> > +
> > +     ret = imx111_init_subdev(sensor, client);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to init controls: %d", ret);
> > +             goto error_v4l2_ctrl_handler_free;
> > +     }
> > +
> > +     ret = v4l2_subdev_init_finalize(&sensor->sd);
> > +     if (ret)
> > +             goto error_v4l2_ctrl_handler_free;
> > +
> > +     pm_runtime_set_active(dev);
> > +     pm_runtime_enable(dev);
> > +
> > +     ret = v4l2_async_register_subdev_sensor(&sensor->sd);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to register V4L2 subdev: %d", ret);
> > +             goto error_pm;
> > +     }
> > +
> > +     pm_runtime_idle(dev);
> > +     pm_runtime_set_autosuspend_delay(dev, 1000);
> > +     pm_runtime_use_autosuspend(dev);
>
> The autosuspend should be set before pm_runtime_idle() call(), shouldn't
> it?
>
> I can make the two changes before applying, too, if that's ok.

If you don't mind adjusting commit on your own before applying I have
no objections. Thank you very much!

Best regards,
Svyatoslav R.

>
> --
> Regards,
>
> Sakari Ailus