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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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:

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Friday, June 14, 2024 10:25 PM
> To: Yan, Dongcheng <dongcheng.yan@xxxxxxxxx>
> Cc: sakari.ailus@xxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx;
> tomi.valkeinen@xxxxxxxxxxxxxxxx; jacopo.mondi@xxxxxxxxxxxxxxxx;
> bingbu.cao@xxxxxxxxxxxxxxx; dave.stevenson@xxxxxxxxxxxxxxx; Li, Daxing
> <daxing.li@xxxxxxxxx>; Yao, Hao <hao.yao@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Dongcheng Yan <dongcheng.yan@xxxxxxxxx>
> > ---
> > 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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux