Re: [PATCH v3 3/3] media: i2c: og0ve1b: Add support for OmniVision OG0VA1B

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

Hi Wenmeng.

On 7/8/26 17:33, Wenmeng Liu wrote:
> The OmniVision OG0VA1B is a monochrome image sensor closely related to
> the OG0VE1B. It shares the SCCB control interface, power supplies and
> the single-lane MIPI D-PHY description, and differs in its chip id, the
> test pattern register, the register programming and the output format
> (10-bit RAW instead of 8-bit).
> 
> Add an og0ve1b_sensor_data entry describing the OG0VA1B together with
> its 640x480 60fps register sequence.
> 
> Signed-off-by: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>
> ---
>   drivers/media/i2c/og0ve1b.c | 278 ++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 266 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/i2c/og0ve1b.c b/drivers/media/i2c/og0ve1b.c
> index 041342fbe3c822400388f58a03e6057e186f060f..c558cdd43314931db35e463641dd28e10b94ec8a 100644
> --- a/drivers/media/i2c/og0ve1b.c
> +++ b/drivers/media/i2c/og0ve1b.c
> @@ -14,10 +14,14 @@
>   #include <media/v4l2-device.h>
>   #include <media/v4l2-fwnode.h>
>   
> +#define OG0VA1B_LINK_FREQ_480MHZ	(480 * HZ_PER_MHZ)
> +#define OG0VA1B_MCLK_FREQ_19_2MHZ	(19200 * HZ_PER_KHZ)
> +
>   #define OG0VE1B_LINK_FREQ_500MHZ	(500 * HZ_PER_MHZ)
>   #define OG0VE1B_MCLK_FREQ_24MHZ		(24 * HZ_PER_MHZ)
>   
> -#define OG0VE1B_REG_CHIP_ID		CCI_REG24(0x300a)
> +#define OG0V_REG_CHIP_ID		CCI_REG24(0x300a)
> +#define OG0VA1B_CHIP_ID			0xc75641
>   #define OG0VE1B_CHIP_ID			0xc75645
>   
>   #define OG0VE1B_REG_MODE_SELECT		CCI_REG8(0x0100)
> @@ -45,12 +49,18 @@
>   #define OG0VE1B_REG_VTS			CCI_REG16(0x380e)
>   #define OG0VE1B_VTS_MAX			0xffff
>   
> -/* Test pattern */
> +/* Test pattern - OG0VA1B uses 0x5100, OG0VE1B uses 0x5e00 */
> +#define OG0VA1B_REG_TEST_PATTERN	CCI_REG8(0x5100)
> +#define OG0VA1B_TEST_PATTERN_BAR_SHIFT	2
>   #define OG0VE1B_REG_PRE_ISP		CCI_REG8(0x5e00)
>   #define OG0VE1B_TEST_PATTERN_ENABLE	BIT(7)
>   
>   #define to_og0ve1b(_sd)			container_of(_sd, struct og0ve1b, sd)
>   
> +static const s64 og0va1b_link_freq_menu[] = {
> +	OG0VA1B_LINK_FREQ_480MHZ,
> +};
> +
>   static const s64 og0ve1b_link_freq_menu[] = {
>   	OG0VE1B_LINK_FREQ_500MHZ,
>   };
> @@ -73,15 +83,31 @@ struct og0ve1b_mode {
>   struct og0ve1b;
>   
>   struct og0ve1b_sensor_data {
> +	const char *name;
>   	u64 chip_id;
>   	unsigned long mclk_freq;
>   	int (*enable_test_pattern)(struct og0ve1b *og0ve1b, u32 pattern);
> +	const char * const *test_pattern_menu;
> +	int num_test_patterns;
> +	bool cache_test_pattern_reg;
> +	/* Exposure register unit: OG0VE1B 1/16 line (4), OG0VA1B whole lines (0). */
> +	unsigned int exposure_shift;
> +	/* Pixel rate multiplier: OG0VA1B uses CSI-2 DDR (2), OG0VE1B keeps 1. */
> +	unsigned int pixel_rate_mul;
>   	const s64 *link_freq_menu;
>   	int num_link_freqs;
>   	const struct og0ve1b_mode *modes;
>   	int num_modes;
>   };
>   
> +static const char * const og0va1b_test_pattern_menu[] = {
> +	"Disabled",
> +	"Standard Color Bar",
> +	"Top-Bottom Darker Color Bar",
> +	"Right-Left Darker Color Bar",
> +	"Bottom-Top Darker Color Bar",
> +};
> +

In the original og0ve1b_test_pattern_menu[] I copied a pretty regular
test pattern name "Vertical Colour Bars" inapproptiately, and here
the references to "Colour Bars" are also present... Due to quite
an obvious reason of sensor specifics would you consider to change
the test pattern names to something else?.. Sorry for late comment.

-- 
Best wishes,
Vladimir