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@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
--
Regards,
Laurent Pinchart
