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

[Himanshu Bhavani]

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