linux-kernel - Re: [PATCH RESEND v9 2/2] media: i2c: add os05b10 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aV7ZB40s7L2PdW21@kekkonen.localdomain>
Date: Thu, 8 Jan 2026 00:07:03 +0200
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Himanshu Bhavani <himanshu.bhavani@...iconsignals.io>
Cc: robh@...nel.org, krzk+dt@...nel.org,
	Elgin Perumbilly <elgin.perumbilly@...iconsignals.io>,
	Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
	Mehdi Djait <mehdi.djait@...ux.intel.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Hans Verkuil <hverkuil@...nel.org>,
	Hans de Goede <hansg@...nel.org>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	André Apitzsch <git@...tzsch.eu>,
	Benjamin Mugnier <benjamin.mugnier@...s.st.com>,
	Dongcheng Yan <dongcheng.yan@...el.com>,
	Sylvain Petinot <sylvain.petinot@...s.st.com>,
	Heimir Thor Sverrisson <heimir.sverrisson@...il.com>,
	Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>,
	Svyatoslav Ryhel <clamor95@...il.com>, linux-media@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND v9 2/2] media: i2c: add os05b10 image sensor driver

Hi Himanshu,

Thanks for the update.

A few comments below... please post a patch on top.

On Mon, Jan 05, 2026 at 03:04:14PM +0530, Himanshu Bhavani wrote:

...

> +static int os05b10_disable_streams(struct v4l2_subdev *sd,
> +				   struct v4l2_subdev_state *state,
> +				   u32 pad, u64 streams_mask)
> +{
> +	struct os05b10 *os05b10 = to_os05b10(sd);
> +	int ret;
> +
> +	ret = cci_write(os05b10->cci, OS05B10_REG_CTRL_MODE,
> +			OS05B10_MODE_STANDBY, NULL);
> +	if (ret)
> +		dev_err(os05b10->dev, "failed to set stream off\n");
> +
> +	pm_runtime_put(os05b10->dev);
> +
> +	return ret;

I'd return 0 here: there's nothing the caller can really do about this.


...

> +static u64 os05b10_pixel_rate(struct os05b10 *os05b10,
> +			      const struct os05b10_mode *mode)
> +{
> +	u64 link_freq = link_frequencies[os05b10->link_freq_index];
> +	const unsigned int lanes = os05b10->data_lanes;
> +	u64 numerator = link_freq * 2 * lanes;
> +	unsigned int bpp = mode->bpp;
> +
> +	do_div(numerator, bpp);
> +
> +	dev_info(os05b10->dev,
> +		 "link_freq=%llu bpp=%u lanes=%u pixel_rate=%llu\n",
> +		 link_freq, bpp, lanes, numerator);

Use dev_dbg() if you print this at all. I wouldn't: it looks like a
development time leftover.

You can also make this more simple by using div_u64(). Also, there's no
really point for have a local variable for everything.

...

> +static int os05b10_probe(struct i2c_client *client)
> +{
> +	struct os05b10 *os05b10;
> +	unsigned int xclk_freq;
> +	unsigned int i;
> +	int ret;
> +
> +	os05b10 = devm_kzalloc(&client->dev, sizeof(*os05b10), GFP_KERNEL);
> +	if (!os05b10)
> +		return -ENOMEM;
> +
> +	os05b10->client = client;
> +	os05b10->dev = &client->dev;
> +
> +	v4l2_i2c_subdev_init(&os05b10->sd, client, &os05b10_subdev_ops);
> +
> +	os05b10->cci = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(os05b10->cci))
> +		return dev_err_probe(os05b10->dev, PTR_ERR(os05b10->cci),
> +				     "failed to initialize CCI\n");
> +
> +	os05b10->xclk = devm_v4l2_sensor_clk_get(os05b10->dev, NULL);
> +	if (IS_ERR(os05b10->xclk))
> +		return dev_err_probe(os05b10->dev, PTR_ERR(os05b10->xclk),
> +				     "failed to get xclk\n");
> +
> +	xclk_freq = clk_get_rate(os05b10->xclk);
> +	if (xclk_freq != OS05B10_XCLK_FREQ)
> +		return dev_err_probe(os05b10->dev, -EINVAL,
> +				     "xclk frequency not supported: %d Hz\n",
> +				     xclk_freq);
> +
> +	for (i = 0; i < ARRAY_SIZE(os05b10_supply_name); i++)

You could declare i here -- as unsigned int.

> +		os05b10->supplies[i].supply = os05b10_supply_name[i];
> +

-- 
Kind regards,

Sakari Ailus