Re: [PATCH v5 2/2] media: i2c: Add driver for ST VD55G1 camera sensor

[Christophe JAILLET]

Le 04/04/2025 à 16:50, Benjamin Mugnier a écrit :
> The VD55G1 is a monochrome global shutter camera with a 804x704 maximum
> resolution with RAW8 and RAW10 bytes per pixel.
> The driver supports :
> - Auto exposure from the sensor, or manual exposure mode
> - HDR subtraction mode, allowing edge detection and background removal
> - Auto exposure cold start, using configuration values from last stream
> to start the next one
> - LED GPIOs for illumination
> - Most standard camera sensor features (hblank, vblank, test patterns,
> again, dgain, hflip, vflip, auto exposure bias, etc.)
> Add driver source code to MAINTAINERS file.

Hi, a few nitpicks below, should they make sense.

...

> +static int vd55g1_prepare_clock_tree(struct vd55g1 *sensor)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	/* Double data rate */
> +	u32 mipi_freq = sensor->link_freq * 2;
> +	u32 sys_clk, mipi_div, pixel_div;
> +	int ret = 0;
> +
> +	if (sensor->xclk_freq < 6 * HZ_PER_MHZ ||
> +	    sensor->xclk_freq > 27 * HZ_PER_MHZ) {
> +		dev_err(&client->dev,
> +			"Only 6Mhz-27Mhz clock range supported. Provided %lu MHz\n",
> +			sensor->xclk_freq / HZ_PER_MHZ);
> +		return -EINVAL;
> +	}
> +
> +	if (mipi_freq < 250 * HZ_PER_MHZ ||
> +	    mipi_freq > 1200 * HZ_PER_MHZ) {
> +		dev_err(&client->dev,
> +			"Only 250Mhz-1200Mhz link frequency range supported. Provided %lu MHz\n",
> +			mipi_freq / HZ_PER_MHZ);
> +		return -EINVAL;
> +	}
> +
> +	if (mipi_freq <= 300 * HZ_PER_MHZ)
> +		mipi_div = 4;
> +	else if (mipi_freq <= 600 * HZ_PER_MHZ)
> +		mipi_div = 2;
> +	else
> +		mipi_div = 1;
> +
> +	sys_clk = mipi_freq * mipi_div;
> +
> +	if (sys_clk <= 780 * HZ_PER_MHZ)
> +		pixel_div = 5;
> +	else if (sys_clk <= 900 * HZ_PER_MHZ)
> +		pixel_div = 6;
> +	else
> +		pixel_div = 8;
> +
> +	sensor->pixel_clock = sys_clk / pixel_div;
> +	/* Frequency to data rate is 1:1 ratio for MIPI */
> +	sensor->data_rate_in_mbps = mipi_freq;
> +
> +	return ret;

Could be return 0, and ret could be removed.

> +}

...

> +static int vd55g1_enable_streams(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_state *state, u32 pad,
> +				 u64 streams_mask)
> +{
> +	struct vd55g1 *sensor = to_vd55g1(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
> +	int ret = 0;

Un-needed init, it is set just the line after.

> +
> +	ret = pm_runtime_resume_and_get(&client->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	vd55g1_write(sensor, VD55G1_REG_EXT_CLOCK, sensor->xclk_freq, &ret);
> +
> +	/* configure output */
> +	vd55g1_write(sensor, VD55G1_REG_MIPI_DATA_RATE,
> +		     sensor->data_rate_in_mbps, &ret);
> +	vd55g1_write(sensor, VD55G1_REG_OIF_CTRL, sensor->oif_ctrl, &ret);
> +	vd55g1_write(sensor, VD55G1_REG_ISL_ENABLE, 0, &ret);
> +	if (ret)
> +		goto err_rpm_put;
> +
> +	ret = vd55g1_set_framefmt(sensor);
> +	if (ret)
> +		goto err_rpm_put;
> +
> +	/* Setup default GPIO values; could be overridden by V4L2 ctrl setup */
> +	ret = vd55g1_update_gpios(sensor, GENMASK(VD55G1_NB_GPIOS - 1, 0));
> +	if (ret)
> +		goto err_rpm_put;
> +
> +	ret = vd55g1_apply_cold_start(sensor);
> +	if (ret)
> +		goto err_rpm_put;
> +
> +	/* Apply settings from V4L2 ctrls */
> +	ret = __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
> +	if (ret)
> +		goto err_rpm_put;
> +
> +	/* Also apply settings from read-only V4L2 ctrls */
> +	ret = vd55g1_ro_ctrls_setup(sensor);
> +	if (ret)
> +		goto err_rpm_put;
> +
> +	/* Start streaming */
> +	vd55g1_write(sensor, VD55G1_REG_STBY, VD55G1_STBY_START_STREAM, &ret);
> +	vd55g1_poll_reg(sensor, VD55G1_REG_STBY, 0, &ret);
> +	vd55g1_wait_state(sensor, VD55G1_SYSTEM_FSM_STREAMING, &ret);
> +	if (ret)
> +		goto err_rpm_put;
> +
> +	vd55g1_lock_ctrls(sensor, true);
> +
> +	return ret;

return 0?

> +
> +err_rpm_put:
> +	pm_runtime_put(&client->dev);
> +	return ret;
> +}


...

> +static int vd55g1_check_csi_conf(struct vd55g1 *sensor,
> +				 struct fwnode_handle *endpoint)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_CSI2_DPHY };
> +	u8 n_lanes;
> +	int ret = 0;

Un-needed init, it is set just the line after.

> +
> +	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
> +	if (ret)
> +		return -EINVAL;
> +
> +	/* Check lanes number */
> +	n_lanes = ep.bus.mipi_csi2.num_data_lanes;
> +	if (n_lanes != 1) {
> +		dev_err(&client->dev, "Sensor only supports 1 lane, found %d\n",
> +			n_lanes);
> +		ret = -EINVAL;
> +		goto done;
> +	}
> +
> +	/* Clock lane must be first */
> +	if (ep.bus.mipi_csi2.clock_lane != 0) {
> +		dev_err(&client->dev, "Clock lane must be mapped to lane 0\n");
> +		ret = -EINVAL;
> +		goto done;
> +	}
> +
> +	/* Handle polarities in sensor configuration */
> +	sensor->oif_ctrl = (ep.bus.mipi_csi2.lane_polarities[0] << 3) |
> +			   (ep.bus.mipi_csi2.lane_polarities[1] << 6);
> +
> +	/* Check the link frequency set in device tree */
> +	if (!ep.nr_of_link_frequencies) {
> +		dev_err(&client->dev, "link-frequency property not found in DT\n");
> +		ret = -EINVAL;
> +		goto done;
> +	}
> +	if (ep.nr_of_link_frequencies != 1) {
> +		dev_err(&client->dev, "Multiple link frequencies not supported\n");
> +		ret = -EINVAL;
> +		goto done;
> +	}
> +	sensor->link_freq = ep.link_frequencies[0];
> +
> +done:
> +	v4l2_fwnode_endpoint_free(&ep);
> +
> +	return ret;
> +}
...

> +static int vd55g1_parse_dt(struct vd55g1 *sensor)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	struct device *dev = &client->dev;
> +	struct fwnode_handle *endpoint;
> +	int ret;
> +
> +	endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0);
> +	if (!endpoint) {
> +		dev_err(dev, "Endpoint node not found\n");

The usage of trailing \n with dev_err() and dev_err_probe() is not 
consistant in this driver.

I would go for \n everywhere, but some people argue that it is no more 
necessary.

> +		return -EINVAL;
> +	}
> +
> +	ret = vd55g1_check_csi_conf(sensor, endpoint);
> +	fwnode_handle_put(endpoint);
> +	if (ret)
> +		return ret;
> +
> +	return vd55g1_parse_dt_gpios(sensor);
> +}
> +
> +static int vd55g1_subdev_init(struct vd55g1 *sensor)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	int ret;
> +
> +	/* Init sub device */
> +	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	sensor->sd.internal_ops = &vd55g1_internal_ops;
> +
> +	/* Init source pad */
> +	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to init media entity : %d", ret);

Unneeded space before : (to be consitant with code below)

> +		return ret;
> +	}
> +
> +	sensor->sd.state_lock = sensor->ctrl_handler.lock;
> +	ret = v4l2_subdev_init_finalize(&sensor->sd);
> +	if (ret) {
> +		dev_err(&client->dev, "Subdev init error: %d", ret);
> +		goto err_ctrls;
> +	}
> +
> +	/*
> +	 * Initiliaze controls after v4l2_subdev_init_finalize() to make sure

Initialize?

> +	 * default values are set.
> +	 */
> +	ret = vd55g1_init_ctrls(sensor);
> +	if (ret) {
> +		dev_err(&client->dev, "Controls initialization failed %d", ret);
> +		goto err_media;
> +	}
> +
> +	return ret;

return 0?

> +
> +err_ctrls:
> +	v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
> +
> +err_media:
> +	media_entity_cleanup(&sensor->sd.entity);
> +	return ret;
> +}

...

CJ