Re: [PATCH v3] media: i2c: Add ar0234 camera sensor driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello Dongcheng,

On Mon, Jul 01, 2024 at 05:22:22AM +0000, Yan, Dongcheng wrote:
> Hi Larent,
> 
> Thanks for your review and meaningful suggestions. I have contacted
> the vendor and optimized the code related to the register settings.
> The response is as follows:

I think you forgot the response below :-)

> > -----Original Message-----
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Sent: Friday, June 14, 2024 10:25 PM
> > To: Yan, Dongcheng <dongcheng.yan@intel.com>
> > Cc: sakari.ailus@linux.intel.com; linux-media@vger.kernel.org;
> > tomi.valkeinen@ideasonboard.com; jacopo.mondi@ideasonboard.com;
> > bingbu.cao@linux.intel.com; dave.stevenson@raspberrypi.com; Li, Daxing
> > <daxing.li@intel.com>; Yao, Hao <hao.yao@intel.com>
> > Subject: Re: [PATCH v3] media: i2c: Add ar0234 camera sensor driver
> > 
> > Hi Dongcheng,
> > 
> > Thank you for the patch.
> > 
> > On Fri, Jun 14, 2024 at 04:09:41PM +0800, Dongcheng Yan wrote:
> > > The driver is implemented with V4L2 framework, and supports following
> > > features:
> > >
> > >     - manual exposure and analog/digital gain control
> > >     - vblank/hblank control
> > >     - vflip/hflip control
> > >     - runtime PM support
> > >     - 1280x960 at 30FPS
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > Signed-off-by: Dongcheng Yan <dongcheng.yan@intel.com>
> > > ---
> > > v2 --> v3:
> > >     - remove unused reg setting
> > >     - add vflip/hflip control
> > >     - add external clock check & lanes check
> > >
> > > ---
> > >  drivers/media/i2c/Kconfig  |   11 +
> > >  drivers/media/i2c/Makefile |    1 +
> > >  drivers/media/i2c/ar0234.c | 1077
> > > ++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 1089 insertions(+)
> > >  create mode 100644 drivers/media/i2c/ar0234.c
> > >
> > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > index c6d3ee472d81..7108d194c975 100644
> > > --- a/drivers/media/i2c/Kconfig
> > > +++ b/drivers/media/i2c/Kconfig
> > > @@ -51,6 +51,17 @@ config VIDEO_ALVIUM_CSI2
> > >  	  To compile this driver as a module, choose M here: the
> > >  	  module will be called alvium-csi2.
> > >
> > > +config VIDEO_AR0234
> > > +        tristate "ON Semiconductor AR0234 sensor support"
> > > +        depends on ACPI || COMPILE_TEST
> > > +        select V4L2_CCI_I2C
> > > +        help
> > > +          This is a Video4Linux2 sensor driver for the ON Semiconductor
> > > +          AR0234 camera.
> > > +
> > > +          To compile this driver as a module, choose M here: the
> > > +          module will be called ar0234.
> > > +
> > >  config VIDEO_AR0521
> > >  	tristate "ON Semiconductor AR0521 sensor support"
> > >  	help
> > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > > index dfbe6448b549..57b4f62106d9 100644
> > > --- a/drivers/media/i2c/Makefile
> > > +++ b/drivers/media/i2c/Makefile
> > > @@ -19,6 +19,7 @@ obj-$(CONFIG_VIDEO_AK7375) += ak7375.o
> > >  obj-$(CONFIG_VIDEO_AK881X) += ak881x.o
> > >  obj-$(CONFIG_VIDEO_ALVIUM_CSI2) += alvium-csi2.o
> > >  obj-$(CONFIG_VIDEO_APTINA_PLL) += aptina-pll.o
> > > +obj-$(CONFIG_VIDEO_AR0234) += ar0234.o
> > >  obj-$(CONFIG_VIDEO_AR0521) += ar0521.o
> > >  obj-$(CONFIG_VIDEO_BT819) += bt819.o
> > >  obj-$(CONFIG_VIDEO_BT856) += bt856.o
> > > diff --git a/drivers/media/i2c/ar0234.c b/drivers/media/i2c/ar0234.c
> > > new file mode 100644 index 000000000000..80fe5ffd1c64
> > > --- /dev/null
> > > +++ b/drivers/media/i2c/ar0234.c
> > > @@ -0,0 +1,1077 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +// Copyright (c) 2019 - 2024 Intel Corporation.
> > > +
> > > +#include <linux/acpi.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <asm/unaligned.h>
> > > +
> > > +#include <media/v4l2-cci.h>
> > > +#include <media/v4l2-ctrls.h>
> > > +#include <media/v4l2-event.h>
> > > +#include <media/v4l2-device.h>
> > > +#include <media/v4l2-fwnode.h>
> > > +
> > > +/* Chip ID */
> > > +#define AR0234_REG_CHIP_ID		CCI_REG16(0x3000)
> > > +#define AR0234_CHIP_ID			0x0a56
> > > +
> > > +#define AR0234_REG_MODE_SELECT		CCI_REG16(0x301a)
> > > +#define AR0234_REG_VTS			CCI_REG16(0x300a)
> > > +#define AR0234_REG_EXPOSURE		CCI_REG16(0x3012)
> > > +#define AR0234_REG_ANALOG_GAIN		CCI_REG16(0x3060)
> > > +#define AR0234_REG_GLOBAL_GAIN		CCI_REG16(0x305e)
> > > +#define AR0234_REG_ORIENTATION		CCI_REG16(0x3040)
> > > +#define AR0234_REG_TEST_PATTERN		CCI_REG16(0x0600)
> > > +
> > > +#define AR0234_EXPOSURE_MIN		0
> > > +#define AR0234_EXPOSURE_MAX_MARGIN	80
> > > +#define AR0234_EXPOSURE_STEP		1
> > > +
> > > +#define AR0234_ANALOG_GAIN_MIN		0
> > > +#define AR0234_ANALOG_GAIN_MAX		0x7f
> > > +#define AR0234_ANALOG_GAIN_STEP		1
> > > +#define AR0234_ANALOG_GAIN_DEFAULT	0xe
> > > +
> > > +#define AR0234_GLOBAL_GAIN_MIN		0
> > > +#define AR0234_GLOBAL_GAIN_MAX		0x7ff
> > > +#define AR0234_GLOBAL_GAIN_STEP		1
> > > +#define AR0234_GLOBAL_GAIN_DEFAULT	0x80
> > > +
> > > +#define AR0234_NATIVE_WIDTH		1920
> > > +#define AR0234_NATIVE_HEIGHT		1080
> > > +#define AR0234_COMMON_WIDTH		1280
> > > +#define AR0234_COMMON_HEIGHT		960
> > > +#define AR0234_PIXEL_ARRAY_LEFT		320
> > > +#define AR0234_PIXEL_ARRAY_TOP		60
> > > +#define AR0234_ORIENTATION_HFLIP	BIT(14)
> > > +#define AR0234_ORIENTATION_VFLIP	BIT(15)
> > > +
> > > +#define AR0234_VTS_DEFAULT		0x04c4
> > > +#define AR0234_VTS_MAX			0xffff
> > > +#define AR0234_HTS_DEFAULT		0x04c4
> > > +#define AR0234_PPL_DEFAULT		3498
> > > +
> > > +#define AR0234_MODE_RESET		0x00d9
> > > +#define AR0234_MODE_STANDBY		0x2058
> > > +#define AR0234_MODE_STREAMING		0x205c
> > > +
> > > +#define AR0234_PIXEL_RATE		128000000ULL
> > > +#define AR0234_XCLK_FREQ		19200000ULL
> > > +
> > > +#define AR0234_TEST_PATTERN_DISABLE	0
> > > +#define AR0234_TEST_PATTERN_SOLID_COLOR	1
> > > +#define AR0234_TEST_PATTERN_COLOR_BARS	2
> > > +#define AR0234_TEST_PATTERN_GREY_COLOR	3
> > > +#define AR0234_TEST_PATTERN_WALKING	256
> > > +
> > > +#define to_ar0234(_sd)	container_of(_sd, struct ar0234, sd)
> > > +
> > > +struct ar0234_reg_list {
> > > +	u32 num_of_regs;
> > > +	const struct cci_reg_sequence *regs; };
> > > +
> > > +struct ar0234_mode {
> > > +	u32 width;
> > > +	u32 height;
> > > +	u32 hts;
> > > +	u32 vts_def;
> > > +	u32 code;
> > > +	/* Sensor register settings for this mode */
> > > +	const struct ar0234_reg_list reg_list; };
> > > +
> > > +static const struct cci_reg_sequence mode_1280x960_10bit_2lane[] = {
> > > +	{ CCI_REG16(0x3f4c), 0x121f },
> > > +	{ CCI_REG16(0x3f4e), 0x121f },
> > > +	{ CCI_REG16(0x3f50), 0x0b81 },
> > > +	{ CCI_REG16(0x31e0), 0x0003 },
> > > +	{ CCI_REG16(0x30b0), 0x0028 },
> > > +	/* R0x3088 specify the sequencer RAM access address. */
> > > +	{ CCI_REG16(0x3088), 0x8000 },
> > > +	/* R0x3086 write the sequencer RAM. */
> > > +	{ CCI_REG16(0x3086), 0xc1ae },
> > > +	{ CCI_REG16(0x3086), 0x327f },
> > > +	{ CCI_REG16(0x3086), 0x5780 },
> > > +	{ CCI_REG16(0x3086), 0x272f },
> > > +	{ CCI_REG16(0x3086), 0x7416 },
> > 
> > Storing the sequencer data in this table wastes lots of memory and CPU cycles.
> > Please move the data out to a
> > 
> > static const u16 ar0234_sequencer[] = {
> > 	0xc1ae, 0x327f, 0x5780, 0x272f, 0x7416, 0x7e13, 0x8000, 0x307e,
> > 	...
> > };
> > 
> > table, and program it with
> > 
> > 	/* Program the sequencer. */
> > 	cci_write(ar0234->regmap, CCI_REG16(0x3088), 0x8000, &ret);
> > 	for (i = 0; i < ARRAY_SIZE(ar0234_sequencer); ++i)
> > 		cci_write(ar0234->regmap, CCI_REG16(0x3086),
> > 			  ar0234_sequencer[i], &ret);
> > 
> > And please define macros for the sequencer access registers 0x3086 and
> > 0x3088, as well as for the bits of the 0x3088 register.
> > 
> > [snip]
>
> [Yan, Dongcheng

-- 
Regards,

Laurent Pinchart