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

[Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>]

Hi Vladimir,

>please find just a few more review comments, overall the driver is in
>a good shape.
>
>On 12/31/25 09:06, Himanshu Bhavani wrote:
>> Add a v4l2 subdevice driver for the Omnivision OS05B10 sensor.
>>
>> The Omnivision OS05B10 image sensor with an active
>> array size of 2592 x 1944.
>>
>> The following features are supported:
>> - Manual exposure an gain control support
>> - vblank/hblank control support
>> - Supported resolution: 2592 x 1944 @ 60fps (SBGGR10)
>>
>> Co-developed-by: Elgin Perumbilly <elgin.perumbilly@siliconsignals.io>
>> Signed-off-by: Elgin Perumbilly <elgin.perumbilly@siliconsignals.io>
>> Signed-off-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
>> Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>
>> ---
>>   MAINTAINERS                 |    1 +
>>   drivers/media/i2c/Kconfig   |   10 +
>>   drivers/media/i2c/Makefile  |    1 +
>>   drivers/media/i2c/os05b10.c | 1115 +++++++++++++++++++++++++++++++++++
>>   4 files changed, 1127 insertions(+)
>>   create mode 100644 drivers/media/i2c/os05b10.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c85915d5d20e..c48d04ca38d1 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19240,6 +19240,7 @@ M:    Elgin Perumbilly <elgin.perumbilly@siliconsignals.io>
>>   L:  linux-media@vger.kernel.org
>>   S:  Maintained
>>   F:  Documentation/devicetree/bindings/media/i2c/ovti,os05b10.yaml
>> +F:   drivers/media/i2c/os05b10.c
>>
>>   OMNIVISION OV01A10 SENSOR DRIVER
>>   M:  Bingbu Cao <bingbu.cao@intel.com>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 4b4db8c4f496..9800ba50b9a6 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -371,6 +371,16 @@ config VIDEO_OG0VE1B
>>         To compile this driver as a module, choose M here: the
>>         module will be called og0ve1b.
>>
>> +config VIDEO_OS05B10
>> +        tristate "OmniVision OS05B10 sensor support"
>> +        select V4L2_CCI_I2C
>> +        help
>> +          This is a Video4Linux2 sensor driver for Omnivision
>> +          OS05B10 camera sensor.
>> +
>> +       To compile this driver as a module, choose M here: the
>> +          module will be called os05b10.
>> +
>>   config VIDEO_OV01A10
>>       tristate "OmniVision OV01A10 sensor support"
>>       help
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index c5f17602454f..561d37939875 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -84,6 +84,7 @@ obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o
>>   obj-$(CONFIG_VIDEO_MT9V111) += mt9v111.o
>>   obj-$(CONFIG_VIDEO_OG01A1B) += og01a1b.o
>>   obj-$(CONFIG_VIDEO_OG0VE1B) += og0ve1b.o
>> +obj-$(CONFIG_VIDEO_OS05B10) += os05b10.o
>>   obj-$(CONFIG_VIDEO_OV01A10) += ov01a10.o
>>   obj-$(CONFIG_VIDEO_OV02A10) += ov02a10.o
>>   obj-$(CONFIG_VIDEO_OV02C10) += ov02c10.o
>> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
>> new file mode 100644
>> index 000000000000..7fa6e60d5d58
>> --- /dev/null
>> +++ b/drivers/media/i2c/os05b10.c
>> @@ -0,0 +1,1115 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * V4L2 Support for the os05b10
>> + *
>> + * Copyright (C) 2025 Silicon Signals Pvt. Ltd.
>> + *
>> + * Inspired from imx219, ov2735 camera drivers.
>> + */
>> +
>> +#include <linux/array_size.h>
>> +#include <linux/clk.h>
>> +#include <linux/container_of.h>
>> +#include <linux/delay.h>
>> +#include <linux/device/devres.h>
>> +#include <linux/err.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/types.h>
>> +#include <linux/time.h>
>> +#include <linux/units.h>
>> +
>> +#include <media/v4l2-cci.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-fwnode.h>
>> +#include <media/v4l2-mediabus.h>
>> +
>> +#define OS05B10_XCLK_FREQ            (24 * HZ_PER_MHZ)
>> +
>> +#define OS05B10_REG_CHIP_ID          CCI_REG24(0x300A)
>
>In the CCI register write sequence all hex values are lower case, please
>convert this and other register addresses/values to lower case as well.
>
>> +#define OS05B10_CHIP_ID                      0x530641
>> +
>> +#define OS05B10_REG_CTRL_MODE                CCI_REG8(0x0100)
>> +#define OS05B10_MODE_STANDBY         0x00
>> +#define OS05B10_MODE_STREAMING               0x01
>> +
>> +#define OS05B10_REG_VTS                      CCI_REG16(0x380E)
>> +#define OS05B10_VTS_MAX                      0xFFFF
>> +
>> +#define OS05B10_REG_HTS                      CCI_REG16(0x380C)
>> +
>> +#define OS05B10_REG_ANALOG_GAIN              CCI_REG16(0x3508)
>> +#define OS05B10_ANALOG_GAIN_MIN              0x80
>> +#define OS05B10_ANALOG_GAIN_MAX              0x7C0
>> +#define OS05B10_ANALOG_GAIN_STEP     1
>> +#define OS05B10_ANALOG_GAIN_DEFAULT     0x80
>> +
>> +#define OS05B10_REG_EXPOSURE         CCI_REG24(0x3500)
>
>For sake of better reading let me ask you to reorder the list of register
>macro in this order:
>- OS05B10_REG_EXPOSURE
>- OS05B10_REG_ANALOG_GAIN
>- OS05B10_REG_HTS
>- OS05B10_REG_VTS
>
>It would be clear which position to insert another added register description,
>since it follows an ascending register value order, only OS05B10_REG_CHIP_ID
>is outstanding.
>
>> +#define OS05B10_EXPOSURE_MIN         2
>> +#define OS05B10_EXPOSURE_STEP                1
>> +#define OS05B10_EXPOSURE_MARGIN         8
>> +
>> +#define OS05B10_PIXEL_RATE           (480 * HZ_PER_MHZ)
>
>Here pixel rate should not be hardcoded, it'd be computed in runtime
>as 600MHz * 2 * 4 lanes / 10 bpp = 480MHz.
>
>The sensor may be wired over 2 lanes or (presumably) it can stream 8 bpp data.

We are not supporting 2 lanes right now, driver supports only for 4 lanes and 
10 bpp data. So technically OS05B10_PIXEL_RATE will never change.
For reference I will add OS05B10_PIXEL_RATE calculation in comments.
I would prefer the hardcoded at the moment. Other than this comment 
I will resolve and send new version.

Thanks for reviewing the driver.

Regards,
Himanshu Bhavani