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
