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

[Krzysztof Kozlowski]

On 28/03/2025 14:40, Benjamin Mugnier wrote:
> +
> +static void vd55g1_subdev_cleanup(struct vd55g1 *sensor)
> +{
> +	v4l2_async_unregister_subdev(&sensor->sd);
> +	v4l2_subdev_cleanup(&sensor->sd);
> +	media_entity_cleanup(&sensor->sd.entity);
> +	v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
> +}
> +
> +static int vd55g1_err_probe(struct device *dev, int ret, char *msg)

Drop, it's really useless. Don't create own abstraction layers for other
systems.

> +{
> +	return dev_err_probe(dev, ret, msg);
> +}
> +
> +static int vd55g1_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct vd55g1 *sensor;
> +	int ret;
> +
> +	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> +	if (!sensor)
> +		return -ENOMEM;
> +
> +	v4l2_i2c_subdev_init(&sensor->sd, client, &vd55g1_subdev_ops);
> +	sensor->i2c_client = client;
> +
> +	ret = vd55g1_parse_dt(sensor);
> +	if (ret)
> +		return vd55g1_err_probe(dev, ret,
> +					"Failed to parse Device Tree.");
> +
> +	/* Get (and check) resources : power regs, ext clock, reset gpio */
> +	ret = vd55g1_get_regulators(sensor);
> +	if (ret)
> +		return vd55g1_err_probe(dev, ret, "Failed to get regulators.");
> +
> +	sensor->xclk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(sensor->xclk)) {

Drop {}

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.

> +		return vd55g1_err_probe(dev, PTR_ERR(sensor->xclk),

No. Syntax is return dev_err_probe, not some custom wrappers over single
function.

> +					"Failed to get xclk.");
> +	}
> +	sensor->xclk_freq = clk_get_rate(sensor->xclk);
> +	ret = vd55g1_prepare_clock_tree(sensor);
> +	if (ret)
> +		return ret;
> +
> +	sensor->reset_gpio =
> +		devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);

Odd wrapping. This should be one line with optionally wrapped last argument.

> +	if (IS_ERR(sensor->reset_gpio))
> +		return vd55g1_err_probe(dev, PTR_ERR(sensor->reset_gpio),
> +					"Failed to get reset gpio.");
> +
> +	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(sensor->regmap))
> +		return vd55g1_err_probe(dev, PTR_ERR(sensor->regmap),
> +					"Failed to init regmap.");
> +
> +	/* Detect if sensor is present and if its revision is supported */
> +	ret = vd55g1_power_on(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = vd55g1_subdev_init(sensor);
> +	if (ret) {
> +		dev_err(dev, "V4l2 init failed : %d", ret);
> +		goto err_power_off;
> +	}
> +
> +	ret = v4l2_async_register_subdev(&sensor->sd);
> +	if (ret) {
> +		dev_err(dev, "async subdev register failed %d", ret);
> +		goto err_subdev;
> +	}
> +
> +	/* Enable pm_runtime and power off the sensor */
> +	pm_runtime_set_active(dev);
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 4000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_mark_last_busy(dev);
> +
> +	dev_dbg(dev, "vd55g1 probe successfully");

Drop. Kernel already provides you such debugging/tracing.

Best regards,
Krzysztof