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

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

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.

-- 
Regards,

Sakari Ailus