Re: [PATCH v7 2/2] media: i2c: add os05b10 image sensor driver
From: Himanshu Bhavani
Date: Thu Jan 01 2026 - 01:19:15 EST
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@xxxxxxxxxxxxxxxxx>
>> Signed-off-by: Elgin Perumbilly <elgin.perumbilly@xxxxxxxxxxxxxxxxx>
>> Signed-off-by: Himanshu Bhavani <himanshu.bhavani@xxxxxxxxxxxxxxxxx>
>> Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@xxxxxxxxxx>
>> Reviewed-by: Mehdi Djait <mehdi.djait@xxxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxx>
>> L: linux-media@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxx>
>> 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