Re: [PATCH RESEND v9 2/2] media: i2c: add os05b10 image sensor driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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