linux-kernel - Re: [PATCH v2 2/2] media: i2c: Add ov2732 image sensor driver

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: <2846f26f-57cc-4fa1-b316-63f94991cd62@kernel.org>
Date: Thu, 18 Dec 2025 21:05:19 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Walter Werner Schneider <contact@...nwalter.eu>,
 linux-media@...r.kernel.org, Jacopo Mondi <jacopo.mondi@...asonboard.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: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] media: i2c: Add ov2732 image sensor driver

On 18/12/2025 19:15, Walter Werner Schneider wrote:
> +static int ov2732_probe(struct i2c_client *client)
> +{
> +	struct ov2732 *ov2732;
> +	int ret;
> +
> +	ov2732 = devm_kzalloc(&client->dev, sizeof(*ov2732), GFP_KERNEL);
> +	if (!ov2732)
> +		return -ENOMEM;
> +
> +	ov2732->dev = &client->dev;
> +
> +	ret = ov2632_probe_dt(ov2732);
> +	if (ret)
> +		return dev_err_probe(ov2732->dev, ret,
> +				     "failed to probe device tree\n");
> +
> +	ov2732->xvclk = devm_v4l2_sensor_clk_get(ov2732->dev, "xvclk");
> +	if (IS_ERR(ov2732->xvclk))
> +		return dev_err_probe(ov2732->dev, PTR_ERR(ov2732->xvclk),
> +				     "failed to get xvclk\n");
> +
> +	ov2732->xvclk_freq = clk_get_rate(ov2732->xvclk);
> +	if (ov2732->xvclk_freq != OV2732_XVCLK_FREQ)
> +		return dev_err_probe(ov2732->dev, -EINVAL,
> +				     "xvclk frequency not supported: %dHz\n",
> +				      ov2732->xvclk_freq);
> +
> +	ov2732->pwdn_gpio = devm_gpiod_get_optional(ov2732->dev, "pwdn",
> +						    GPIOD_OUT_HIGH);
> +	gpiod_set_value_cansleep(ov2732->pwdn_gpio, 1);

No need to set the same value twice. It's already 1.

> +
> +	ov2732->reset_gpio = devm_gpiod_get_optional(ov2732->dev, "reset",
> +						     GPIOD_OUT_HIGH);
> +	gpiod_set_value_cansleep(ov2732->reset_gpio, 1);

Same here

> +
> +	ov2732->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(ov2732->regmap))
> +		return dev_err_probe(ov2732->dev, PTR_ERR(ov2732->regmap),
> +				     "failed to init CCI\n");
> +
> +	ret = ov2732_get_regulators(ov2732);
> +	if (ret)
> +		return dev_err_probe(ov2732->dev, ret,
> +				     "failed to get regulators\n");
> +
> +	v4l2_i2c_subdev_init(&ov2732->sd, client, &ov2732_subdev_ops);
> +
> +	/* Device must be powered on for ov2732_identify_chip(). */
> +	ret = ov2732_power_on(ov2732->dev);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_set_active(ov2732->dev);
> +	pm_runtime_enable(ov2732->dev);
> +
> +	/* Wait 1ms to avoid intermittent communication errors at startup. */
> +	fsleep(USEC_PER_MSEC);
> +
> +	ret = ov2732_identify_chip(ov2732);
> +	if (ret)
> +		goto err_power_off;
> +
> +	ret = ov2732_init_controls(ov2732);
> +	if (ret)
> +		goto err_power_off;
> +
> +	/* Initialize subdev */
> +	ov2732->sd.internal_ops = &ov2732_internal_ops;
> +	ov2732->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	ov2732->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +	/* Initialize source pad */
> +	ov2732->pad.flags = MEDIA_PAD_FL_SOURCE;
> +
> +	ret = media_entity_pads_init(&ov2732->sd.entity, 1, &ov2732->pad);
> +	if (ret) {
> +		dev_err_probe(ov2732->dev, ret, "failed to init entity pads\n");
> +		goto error_handler_free;
> +	}
> +
> +	ov2732->sd.state_lock = ov2732->ctrl_handler.lock;
> +	ret = v4l2_subdev_init_finalize(&ov2732->sd);
> +	if (ret < 0) {
> +		dev_err_probe(ov2732->dev, ret, "subdev init error\n");
> +		goto err_media_entity;
> +	}
> +
> +	ret = v4l2_async_register_subdev_sensor(&ov2732->sd);
> +	if (ret < 0) {
> +		dev_err_probe(ov2732->dev, ret,
> +			      "failed to register sensor sub-device\n");

So you print errors twice? Once here and second time at the end. Why?
> +		goto err_subdev_cleanup;
> +	}
> +
> +	pm_runtime_set_autosuspend_delay(ov2732->dev, 1000);
> +	pm_runtime_use_autosuspend(ov2732->dev);
> +	pm_runtime_idle(ov2732->dev);
> +
> +	return 0;
> +
> +err_subdev_cleanup:
> +	v4l2_subdev_cleanup(&ov2732->sd);
> +
> +err_media_entity:
> +	media_entity_cleanup(&ov2732->sd.entity);
> +
> +error_handler_free:
> +	v4l2_ctrl_handler_free(&ov2732->ctrl_handler);
> +
> +err_power_off:
> +	pm_runtime_disable(ov2732->dev);
> +	pm_runtime_put_noidle(ov2732->dev);
> +	ov2732_power_off(ov2732->dev);
> +
> +	return dev_err_probe(ov2732->dev, ret, "probing failed\n");

This is useless message. Print informative messages for specific errors
or do not print at all.

> +}
> +

...

> +
> +static const struct of_device_id ov2732_of_match[] = {
> +	{ .compatible = "ovti,ov2732", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ov2732_of_match);
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(ov2732_pm_ops, ov2732_power_off,
> +				 ov2732_power_on, NULL);
> +
> +static struct i2c_driver ov2732_i2c_driver = {
> +	.driver = {
> +		.name = "ov2732",
> +		.of_match_table = of_match_ptr(ov2732_of_match),

Drop of_match_ptr, you have here warning.

> +		.pm = pm_sleep_ptr(&ov2732_pm_ops),
> +	},
> +	.probe = ov2732_probe,
> +	.remove = ov2732_remove,
> +};
Best regards,
Krzysztof