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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3cdc3771-c61e-427f-ad59-12bcb41e7e8d@foss.st.com>
Date: Mon, 31 Mar 2025 14:34:09 +0200
From: Benjamin Mugnier <benjamin.mugnier@...s.st.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
        Sylvain Petinot
	<sylvain.petinot@...s.st.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Sakari Ailus
	<sakari.ailus@...ux.intel.com>
CC: <linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] media: i2c: Add driver for ST VD55G1 camera sensor

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