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:
 <PN0P287MB20198835408FA94EDE9660139A85A@PN0P287MB2019.INDP287.PROD.OUTLOOK.COM>
Date: Thu, 8 Jan 2026 05:30:04 +0000
From: Himanshu Bhavani <himanshu.bhavani@...iconsignals.io>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
CC: "robh@...nel.org" <robh@...nel.org>, "krzk+dt@...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"
	<linux-media@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RESEND v9 2/2] media: i2c: add os05b10 image sensor driver

Hi Sakari,

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

Will resolve your comment and will send patch on top this
driver

>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];
>> +

Best Regards,
Himanshu Bhavani