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

[Benjamin Mugnier <benjamin.mugnier@foss.st.com>]

Hi Krzysztof,

Thank you for your review.

On 3/29/25 06:46, Krzysztof Kozlowski wrote:
> 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.
> 

While it should, checkpatch does not complain about that. Maybe because
the function call is on two lines ? Anyway thanks a lot for pointing
this. Fixed.

>> +		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.

Interesting, do you know how to enable this kind of tracing ?

> 
> Best regards,
> Krzysztof

-- 
Regards,
Benjamin