| Message ID | 20240204061538.2105-3-zhi.mao@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: Add support for GC08A3 sensor | expand |
Hi Zhi, Thank you for the patch. On Sun, Feb 04, 2024 at 02:15:38PM +0800, Zhi Mao wrote: > Add a V4L2 sub-device driver for Galaxycore GC08A3 image sensor. > > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com> > --- > drivers/media/i2c/Kconfig | 10 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/gc08a3.c | 1448 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 1459 insertions(+) > create mode 100644 drivers/media/i2c/gc08a3.c > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 56f276b920ab..e4da68835683 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -70,6 +70,16 @@ config VIDEO_GC0308 > To compile this driver as a module, choose M here: the > module will be called gc0308. > > +config VIDEO_GC08A3 > + tristate "GalaxyCore gc08a3 sensor support" > + select V4L2_CCI_I2C > + help > + This is a Video4Linux2 sensor driver for the GalaxyCore gc08a3 > + camera. > + > + To compile this driver as a module, choose M here: the > + module will be called gc08a3. > + > config VIDEO_GC2145 > select V4L2_CCI_I2C > tristate "GalaxyCore GC2145 sensor support" > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index dfbe6448b549..b82e99ca7578 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -38,6 +38,7 @@ obj-$(CONFIG_VIDEO_DW9768) += dw9768.o > obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o > obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/ > obj-$(CONFIG_VIDEO_GC0308) += gc0308.o > +obj-$(CONFIG_VIDEO_GC08A3) += gc08a3.o > obj-$(CONFIG_VIDEO_GC2145) += gc2145.o > obj-$(CONFIG_VIDEO_HI556) += hi556.o > obj-$(CONFIG_VIDEO_HI846) += hi846.o > diff --git a/drivers/media/i2c/gc08a3.c b/drivers/media/i2c/gc08a3.c > new file mode 100644 > index 000000000000..3fc7fffb815c > --- /dev/null > +++ b/drivers/media/i2c/gc08a3.c > @@ -0,0 +1,1448 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * gc08a3.c - gc08a3 sensor driver > + * > + * Copyright 2023 MediaTek > + * > + * Zhi Mao <zhi.mao@mediatek.com> > + */ > + > +#include <asm/unaligned.h> > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/pm_runtime.h> > +#include <linux/regulator/consumer.h> > +#include <media/media-entity.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-fwnode.h> > +#include <media/v4l2-subdev.h> > +#include <media/v4l2-cci.h> > +#include <media/v4l2-event.h> Please sort the headers alphabetically. > + > +#define GC08A3_REG_CHIP_ID CCI_REG16(0x03f0) > +#define GC08A3_CHIP_ID 0x08a3 > + > +#define GC08A3_NATIVE_WIDTH 3264 > +#define GC08A3_NATIVE_HEIGHT 2448 > + > +#define GC08A3_REG_TEST_PATTERN_EN CCI_REG8(0x008c) > +#define GC08A3_REG_TEST_PATTERN_IDX CCI_REG8(0x008d) > +#define GC08A3_TEST_PATTERN_EN 0x01 > + > +#define GC08A3_STRAEMING_REG CCI_REG8(0x0100) s/STRAEMING/STREAMING/ > + > +#define GC08A3_DEFAULT_CLK_FREQ 24000000 > +#define GC08A3_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10 > +#define GC08A3_DATA_LANES 4 Let's not mix registers and other macros. Please define the register and register field macros first (sorted by register address in numerical order), and then all other macros. > + > +/* for 1920*1080 */ > +#define GC08A3_LINK_FREQ_207MHZ 207000000ULL > +/* for 3264*2448 */ > +#define GC08A3_LINK_FREQ_336MHZ 336000000ULL Use the values directly in the single location where the macros are used below, and drop the macros. > + > +#define GC08A3_RGB_DEPTH 10 > + > +#define GC08A3_FRAME_LENGTH_REG CCI_REG16(0x0340) > +#define GC08A3_VTS_30FPS 2548 > +#define GC08A3_VTS_30FPS_MIN 2548 > +#define GC08A3_VTS_60FPS 1276 > +#define GC08A3_VTS_60FPS_MIN 1276 These four macros don't improve readability, you can use the values directly in the single location where they are used. > +#define GC08A3_VTS_MAX 0xfff0 > + > +#define GC08A3_HTS_30FPS 3640 > +#define GC08A3_HTS_60FPS 3640 Same for these two macros. > + > +#define GC08A3_EXP_REG CCI_REG16(0x0202) > +#define GC08A3_EXP_MARGIN 16 > +#define GC08A3_EXP_MIN 4 > +#define GC08A3_EXP_STEP 1 > + > +#define GC08A3_FLIP_REG CCI_REG8(0x0101) > +#define GC08A3_FLIP_H_MASK 0x1 BIT(0) > +#define GC08A3_FLIP_V_MASK 0x2 BIT(1) > + > +#define GC08A3_AGAIN_REG CCI_REG16(0x0204) > +#define GC08A3_AGAIN_MIN 1024 > +#define GC08A3_AGAIN_MAX (1024 * 16) > +#define GC08A3_AGAIN_STEP 1 > + > +#define GC08A3_MIN_SLEEP_US 2000 > +#define GC08A3_MAX_SLEEP_US 3000 > + > +static const char *const gc08a3_test_pattern_menu[] = { > + "No Pattern", "Solid Black", "Colour Bar", "Solid White", > + "Solid Red", "Solid Green", "Solid Blue", "Solid Yellow", > +}; > + > +enum { > + GC08A3_LINK_FREQ_336MHZ_INDEX, > + GC08A3_LINK_FREQ_207MHZ_INDEX, The second value is never used, and the first one is used in gc08a3_init_controls() only. I would drop both, and pass link_freq_menu_items[0] to the to_pixel_rate() function instead of passing an index. > +}; > + > +static const s64 link_freq_menu_items[] = { s/link_freq_menu_items/gc08a3_link_freq_menu_items/ > + GC08A3_LINK_FREQ_336MHZ, > + GC08A3_LINK_FREQ_207MHZ, > +}; > + > +static const char *const gc08a3_supply_name[] = { > + "avdd", > + "dvdd", > + "dovdd", > +}; > + > +struct gc08a3 { > + struct device *dev; > + struct v4l2_subdev sd; > + struct media_pad pad; > + struct i2c_client *client; Not used, you can drop this field. > + > + struct clk *xclk; > + struct regulator_bulk_data supplies[ARRAY_SIZE(gc08a3_supply_name)]; > + struct gpio_desc *reset_gpio; > + > + struct v4l2_ctrl_handler ctrls; > + struct v4l2_ctrl *pixel_rate; > + struct v4l2_ctrl *link_freq; > + struct v4l2_ctrl *exposure; > + struct v4l2_ctrl *vblank; > + struct v4l2_ctrl *hblank; > + > + struct regmap *regmap; > + > + bool streaming; Set but never used, you can drop this field. > + unsigned long link_freq_bitmap; > + > + /* Current mode */ > + const struct gc08a3_mode *cur_mode; > +}; > + > +struct gc08a3_reg_list { > + u32 num_of_regs; > + const struct cci_reg_sequence *regs; > +}; > + > +struct gc08a3_link_freq_config { > + const struct gc08a3_reg_list reg_list; > +}; > + > +static const struct cci_reg_sequence mode_3264x2448[] = { > + /* system */ > + { CCI_REG8(0x0336), 0x70 }, > + { CCI_REG8(0x0383), 0xbb }, > + { CCI_REG8(0x0344), 0x00 }, > + { CCI_REG8(0x0345), 0x06 }, > + { CCI_REG8(0x0346), 0x00 }, > + { CCI_REG8(0x0347), 0x04 }, > + { CCI_REG8(0x0348), 0x0c }, > + { CCI_REG8(0x0349), 0xd0 }, > + { CCI_REG8(0x034a), 0x09 }, > + { CCI_REG8(0x034b), 0x9c }, > + { CCI_REG8(0x0202), 0x09 }, > + { CCI_REG8(0x0203), 0x04 }, > + { CCI_REG8(0x0340), 0x09 }, > + { CCI_REG8(0x0341), 0xf4 }, > + { CCI_REG8(0x0342), 0x07 }, > + { CCI_REG8(0x0343), 0x1c }, > + > + { CCI_REG8(0x0226), 0x00 }, > + { CCI_REG8(0x0227), 0x28 }, > + { CCI_REG8(0x0e38), 0x49 }, > + { CCI_REG8(0x0210), 0x13 }, > + { CCI_REG8(0x0218), 0x00 }, > + { CCI_REG8(0x0241), 0x88 }, > + { CCI_REG8(0x0392), 0x60 }, > + > + /* ISP */ > + { CCI_REG8(0x00a2), 0x00 }, > + { CCI_REG8(0x00a3), 0x00 }, > + { CCI_REG8(0x00ab), 0x00 }, > + { CCI_REG8(0x00ac), 0x00 }, > + > + /* GAIN */ > + { CCI_REG8(0x0204), 0x04 }, > + { CCI_REG8(0x0205), 0x00 }, > + { CCI_REG8(0x0050), 0x5c }, > + { CCI_REG8(0x0051), 0x44 }, > + > + /* out window */ > + { CCI_REG8(0x009a), 0x66 }, > + { CCI_REG8(0x0351), 0x00 }, > + { CCI_REG8(0x0352), 0x06 }, > + { CCI_REG8(0x0353), 0x00 }, > + { CCI_REG8(0x0354), 0x08 }, > + { CCI_REG8(0x034c), 0x0c }, > + { CCI_REG8(0x034d), 0xc0 }, > + { CCI_REG8(0x034e), 0x09 }, > + { CCI_REG8(0x034f), 0x90 }, > + > + /* MIPI */ > + { CCI_REG8(0x0114), 0x03 }, > + { CCI_REG8(0x0180), 0x65 }, > + { CCI_REG8(0x0181), 0xf0 }, > + { CCI_REG8(0x0185), 0x01 }, > + { CCI_REG8(0x0115), 0x30 }, > + { CCI_REG8(0x011b), 0x12 }, > + { CCI_REG8(0x011c), 0x12 }, > + { CCI_REG8(0x0121), 0x06 }, > + { CCI_REG8(0x0122), 0x06 }, > + { CCI_REG8(0x0123), 0x15 }, > + { CCI_REG8(0x0124), 0x01 }, > + { CCI_REG8(0x0125), 0x0b }, > + { CCI_REG8(0x0126), 0x08 }, > + { CCI_REG8(0x0129), 0x06 }, > + { CCI_REG8(0x012a), 0x08 }, > + { CCI_REG8(0x012b), 0x08 }, > + > + { CCI_REG8(0x0a73), 0x60 }, > + { CCI_REG8(0x0a70), 0x11 }, > + { CCI_REG8(0x0313), 0x80 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0aff), 0x00 }, Does this register really need to be written 8 times ? Same below. > + { CCI_REG8(0x0a70), 0x00 }, > + { CCI_REG8(0x00a4), 0x80 }, > + { CCI_REG8(0x0316), 0x01 }, > + { CCI_REG8(0x0a67), 0x00 }, > + { CCI_REG8(0x0084), 0x10 }, > + { CCI_REG8(0x0102), 0x09 }, > +}; > + > +static const struct cci_reg_sequence mode_1920x1080[] = { > + /* system */ > + { CCI_REG8(0x0336), 0x45 }, > + { CCI_REG8(0x0383), 0x8b }, > + { CCI_REG8(0x0344), 0x02 }, > + { CCI_REG8(0x0345), 0xa6 }, > + { CCI_REG8(0x0346), 0x02 }, > + { CCI_REG8(0x0347), 0xb0 }, > + { CCI_REG8(0x0348), 0x07 }, > + { CCI_REG8(0x0349), 0x90 }, > + { CCI_REG8(0x034a), 0x04 }, > + { CCI_REG8(0x034b), 0x44 }, > + { CCI_REG8(0x0202), 0x03 }, > + { CCI_REG8(0x0203), 0x00 }, > + { CCI_REG8(0x0340), 0x04 }, > + { CCI_REG8(0x0341), 0xfc }, > + { CCI_REG8(0x0342), 0x07 }, > + { CCI_REG8(0x0343), 0x1c }, > + { CCI_REG8(0x0226), 0x00 }, > + { CCI_REG8(0x0227), 0x88 }, > + { CCI_REG8(0x0e38), 0x49 }, > + { CCI_REG8(0x0210), 0x13 }, > + { CCI_REG8(0x0218), 0x00 }, > + { CCI_REG8(0x0241), 0x88 }, > + { CCI_REG8(0x0392), 0x60 }, > + > + /* ISP */ > + { CCI_REG8(0x00a2), 0xac }, > + { CCI_REG8(0x00a3), 0x02 }, > + { CCI_REG8(0x00ab), 0xa0 }, > + { CCI_REG8(0x00ac), 0x02 }, > + > + /* GAIN */ > + { CCI_REG8(0x0204), 0x04 }, > + { CCI_REG8(0x0205), 0x00 }, > + { CCI_REG8(0x0050), 0x38 }, > + { CCI_REG8(0x0051), 0x20 }, > + > + /* out window */ > + { CCI_REG8(0x009a), 0x66 }, > + { CCI_REG8(0x0351), 0x00 }, > + { CCI_REG8(0x0352), 0x06 }, > + { CCI_REG8(0x0353), 0x00 }, > + { CCI_REG8(0x0354), 0x08 }, > + { CCI_REG8(0x034c), 0x07 }, > + { CCI_REG8(0x034d), 0x80 }, > + { CCI_REG8(0x034e), 0x04 }, > + { CCI_REG8(0x034f), 0x38 }, > + > + /* MIPI */ > + { CCI_REG8(0x0114), 0x03 }, > + { CCI_REG8(0x0180), 0x65 }, > + { CCI_REG8(0x0181), 0xf0 }, > + { CCI_REG8(0x0185), 0x01 }, > + { CCI_REG8(0x0115), 0x30 }, > + { CCI_REG8(0x011b), 0x12 }, > + { CCI_REG8(0x011c), 0x12 }, > + { CCI_REG8(0x0121), 0x02 }, > + { CCI_REG8(0x0122), 0x03 }, > + { CCI_REG8(0x0123), 0x0c }, > + { CCI_REG8(0x0124), 0x00 }, > + { CCI_REG8(0x0125), 0x09 }, > + { CCI_REG8(0x0126), 0x06 }, > + { CCI_REG8(0x0129), 0x04 }, > + { CCI_REG8(0x012a), 0x03 }, > + { CCI_REG8(0x012b), 0x06 }, > + > + { CCI_REG8(0x0a73), 0x60 }, > + { CCI_REG8(0x0a70), 0x11 }, > + { CCI_REG8(0x0313), 0x80 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0aff), 0x00 }, > + { CCI_REG8(0x0a70), 0x00 }, > + { CCI_REG8(0x00a4), 0x80 }, > + { CCI_REG8(0x0316), 0x01 }, > + { CCI_REG8(0x0a67), 0x00 }, > + { CCI_REG8(0x0084), 0x10 }, > + { CCI_REG8(0x0102), 0x09 }, > +}; > + > +static const struct cci_reg_sequence mode_table_common[] = { > + { GC08A3_STRAEMING_REG, 0x00 }, > + /* system */ > + { CCI_REG8(0x031c), 0x60 }, > + { CCI_REG8(0x0337), 0x04 }, > + { CCI_REG8(0x0335), 0x51 }, > + { CCI_REG8(0x0336), 0x70 }, > + { CCI_REG8(0x0383), 0xbb }, > + { CCI_REG8(0x031a), 0x00 }, > + { CCI_REG8(0x0321), 0x10 }, > + { CCI_REG8(0x0327), 0x03 }, > + { CCI_REG8(0x0325), 0x40 }, > + { CCI_REG8(0x0326), 0x23 }, > + { CCI_REG8(0x0314), 0x11 }, > + { CCI_REG8(0x0315), 0xd6 }, > + { CCI_REG8(0x0316), 0x01 }, > + { CCI_REG8(0x0334), 0x40 }, > + { CCI_REG8(0x0324), 0x42 }, > + { CCI_REG8(0x031c), 0x00 }, > + { CCI_REG8(0x031c), 0x9f }, > + { CCI_REG8(0x039a), 0x13 }, > + { CCI_REG8(0x0084), 0x30 }, > + { CCI_REG8(0x02b3), 0x08 }, > + { CCI_REG8(0x0057), 0x0c }, > + { CCI_REG8(0x05c3), 0x50 }, > + { CCI_REG8(0x0311), 0x90 }, > + { CCI_REG8(0x05a0), 0x02 }, > + { CCI_REG8(0x0074), 0x0a }, > + { CCI_REG8(0x0059), 0x11 }, > + { CCI_REG8(0x0070), 0x05 }, > + { CCI_REG8(0x0101), 0x00 }, > + > + /* analog */ > + { CCI_REG8(0x0344), 0x00 }, > + { CCI_REG8(0x0345), 0x06 }, > + { CCI_REG8(0x0346), 0x00 }, > + { CCI_REG8(0x0347), 0x04 }, > + { CCI_REG8(0x0348), 0x0c }, > + { CCI_REG8(0x0349), 0xd0 }, > + { CCI_REG8(0x034a), 0x09 }, > + { CCI_REG8(0x034b), 0x9c }, > + { CCI_REG8(0x0202), 0x09 }, > + { CCI_REG8(0x0203), 0x04 }, > + > + { CCI_REG8(0x0219), 0x05 }, > + { CCI_REG8(0x0226), 0x00 }, > + { CCI_REG8(0x0227), 0x28 }, > + { CCI_REG8(0x0e0a), 0x00 }, > + { CCI_REG8(0x0e0b), 0x00 }, > + { CCI_REG8(0x0e24), 0x04 }, > + { CCI_REG8(0x0e25), 0x04 }, > + { CCI_REG8(0x0e26), 0x00 }, > + { CCI_REG8(0x0e27), 0x10 }, > + { CCI_REG8(0x0e01), 0x74 }, > + { CCI_REG8(0x0e03), 0x47 }, > + { CCI_REG8(0x0e04), 0x33 }, > + { CCI_REG8(0x0e05), 0x44 }, > + { CCI_REG8(0x0e06), 0x44 }, > + { CCI_REG8(0x0e0c), 0x1e }, > + { CCI_REG8(0x0e17), 0x3a }, > + { CCI_REG8(0x0e18), 0x3c }, > + { CCI_REG8(0x0e19), 0x40 }, > + { CCI_REG8(0x0e1a), 0x42 }, > + { CCI_REG8(0x0e28), 0x21 }, > + { CCI_REG8(0x0e2b), 0x68 }, > + { CCI_REG8(0x0e2c), 0x0d }, > + { CCI_REG8(0x0e2d), 0x08 }, > + { CCI_REG8(0x0e34), 0xf4 }, > + { CCI_REG8(0x0e35), 0x44 }, > + { CCI_REG8(0x0e36), 0x07 }, > + { CCI_REG8(0x0e38), 0x49 }, > + { CCI_REG8(0x0210), 0x13 }, > + { CCI_REG8(0x0218), 0x00 }, > + { CCI_REG8(0x0241), 0x88 }, > + { CCI_REG8(0x0e32), 0x00 }, > + { CCI_REG8(0x0e33), 0x18 }, > + { CCI_REG8(0x0e42), 0x03 }, > + { CCI_REG8(0x0e43), 0x80 }, > + { CCI_REG8(0x0e44), 0x04 }, > + { CCI_REG8(0x0e45), 0x00 }, > + { CCI_REG8(0x0e4f), 0x04 }, > + { CCI_REG8(0x057a), 0x20 }, > + { CCI_REG8(0x0381), 0x7c }, > + { CCI_REG8(0x0382), 0x9b }, > + { CCI_REG8(0x0384), 0xfb }, > + { CCI_REG8(0x0389), 0x38 }, > + { CCI_REG8(0x038a), 0x03 }, > + { CCI_REG8(0x0390), 0x6a }, > + { CCI_REG8(0x0391), 0x0b }, > + { CCI_REG8(0x0392), 0x60 }, > + { CCI_REG8(0x0393), 0xc1 }, > + { CCI_REG8(0x0396), 0xff }, > + { CCI_REG8(0x0398), 0x62 }, > + > + /* cisctl reset */ > + { CCI_REG8(0x031c), 0x80 }, > + { CCI_REG8(0x03fe), 0x10 }, > + { CCI_REG8(0x03fe), 0x00 }, > + { CCI_REG8(0x031c), 0x9f }, > + { CCI_REG8(0x03fe), 0x00 }, > + { CCI_REG8(0x03fe), 0x00 }, > + { CCI_REG8(0x03fe), 0x00 }, > + { CCI_REG8(0x03fe), 0x00 }, > + { CCI_REG8(0x031c), 0x80 }, > + { CCI_REG8(0x03fe), 0x10 }, > + { CCI_REG8(0x03fe), 0x00 }, > + { CCI_REG8(0x031c), 0x9f }, > + { CCI_REG8(0x0360), 0x01 }, > + { CCI_REG8(0x0360), 0x00 }, > + { CCI_REG8(0x0316), 0x09 }, > + { CCI_REG8(0x0a67), 0x80 }, > + { CCI_REG8(0x0313), 0x00 }, > + { CCI_REG8(0x0a53), 0x0e }, > + { CCI_REG8(0x0a65), 0x17 }, > + { CCI_REG8(0x0a68), 0xa1 }, > + { CCI_REG8(0x0a58), 0x00 }, > + { CCI_REG8(0x0ace), 0x0c }, > + { CCI_REG8(0x00a4), 0x00 }, > + { CCI_REG8(0x00a5), 0x01 }, > + { CCI_REG8(0x00a7), 0x09 }, > + { CCI_REG8(0x00a8), 0x9c }, > + { CCI_REG8(0x00a9), 0x0c }, > + { CCI_REG8(0x00aa), 0xd0 }, > + { CCI_REG8(0x0a8a), 0x00 }, > + { CCI_REG8(0x0a8b), 0xe0 }, > + { CCI_REG8(0x0a8c), 0x13 }, > + { CCI_REG8(0x0a8d), 0xe8 }, > + { CCI_REG8(0x0a90), 0x0a }, > + { CCI_REG8(0x0a91), 0x10 }, > + { CCI_REG8(0x0a92), 0xf8 }, > + { CCI_REG8(0x0a71), 0xf2 }, > + { CCI_REG8(0x0a72), 0x12 }, > + { CCI_REG8(0x0a73), 0x64 }, > + { CCI_REG8(0x0a75), 0x41 }, > + { CCI_REG8(0x0a70), 0x07 }, > + { CCI_REG8(0x0313), 0x80 }, > + > + /* ISP */ > + { CCI_REG8(0x00a0), 0x01 }, > + { CCI_REG8(0x0080), 0xd2 }, > + { CCI_REG8(0x0081), 0x3f }, > + { CCI_REG8(0x0087), 0x51 }, > + { CCI_REG8(0x0089), 0x03 }, > + { CCI_REG8(0x009b), 0x40 }, > + { CCI_REG8(0x05a0), 0x82 }, > + { CCI_REG8(0x05ac), 0x00 }, > + { CCI_REG8(0x05ad), 0x01 }, > + { CCI_REG8(0x05ae), 0x00 }, > + { CCI_REG8(0x0800), 0x0a }, > + { CCI_REG8(0x0801), 0x14 }, > + { CCI_REG8(0x0802), 0x28 }, > + { CCI_REG8(0x0803), 0x34 }, > + { CCI_REG8(0x0804), 0x0e }, > + { CCI_REG8(0x0805), 0x33 }, > + { CCI_REG8(0x0806), 0x03 }, > + { CCI_REG8(0x0807), 0x8a }, > + { CCI_REG8(0x0808), 0x50 }, > + { CCI_REG8(0x0809), 0x00 }, > + { CCI_REG8(0x080a), 0x34 }, > + { CCI_REG8(0x080b), 0x03 }, > + { CCI_REG8(0x080c), 0x26 }, > + { CCI_REG8(0x080d), 0x03 }, > + { CCI_REG8(0x080e), 0x18 }, > + { CCI_REG8(0x080f), 0x03 }, > + { CCI_REG8(0x0810), 0x10 }, > + { CCI_REG8(0x0811), 0x03 }, > + { CCI_REG8(0x0812), 0x00 }, > + { CCI_REG8(0x0813), 0x00 }, > + { CCI_REG8(0x0814), 0x01 }, > + { CCI_REG8(0x0815), 0x00 }, > + { CCI_REG8(0x0816), 0x01 }, > + { CCI_REG8(0x0817), 0x00 }, > + { CCI_REG8(0x0818), 0x00 }, > + { CCI_REG8(0x0819), 0x0a }, > + { CCI_REG8(0x081a), 0x01 }, > + { CCI_REG8(0x081b), 0x6c }, > + { CCI_REG8(0x081c), 0x00 }, > + { CCI_REG8(0x081d), 0x0b }, > + { CCI_REG8(0x081e), 0x02 }, > + { CCI_REG8(0x081f), 0x00 }, > + { CCI_REG8(0x0820), 0x00 }, > + { CCI_REG8(0x0821), 0x0c }, > + { CCI_REG8(0x0822), 0x02 }, > + { CCI_REG8(0x0823), 0xd9 }, > + { CCI_REG8(0x0824), 0x00 }, > + { CCI_REG8(0x0825), 0x0d }, > + { CCI_REG8(0x0826), 0x03 }, > + { CCI_REG8(0x0827), 0xf0 }, > + { CCI_REG8(0x0828), 0x00 }, > + { CCI_REG8(0x0829), 0x0e }, > + { CCI_REG8(0x082a), 0x05 }, > + { CCI_REG8(0x082b), 0x94 }, > + { CCI_REG8(0x082c), 0x09 }, > + { CCI_REG8(0x082d), 0x6e }, > + { CCI_REG8(0x082e), 0x07 }, > + { CCI_REG8(0x082f), 0xe6 }, > + { CCI_REG8(0x0830), 0x10 }, > + { CCI_REG8(0x0831), 0x0e }, > + { CCI_REG8(0x0832), 0x0b }, > + { CCI_REG8(0x0833), 0x2c }, > + { CCI_REG8(0x0834), 0x14 }, > + { CCI_REG8(0x0835), 0xae }, > + { CCI_REG8(0x0836), 0x0f }, > + { CCI_REG8(0x0837), 0xc4 }, > + { CCI_REG8(0x0838), 0x18 }, > + { CCI_REG8(0x0839), 0x0e }, > + { CCI_REG8(0x05ac), 0x01 }, > + { CCI_REG8(0x059a), 0x00 }, > + { CCI_REG8(0x059b), 0x00 }, > + { CCI_REG8(0x059c), 0x01 }, > + { CCI_REG8(0x0598), 0x00 }, > + { CCI_REG8(0x0597), 0x14 }, > + { CCI_REG8(0x05ab), 0x09 }, > + { CCI_REG8(0x05a4), 0x02 }, > + { CCI_REG8(0x05a3), 0x05 }, > + { CCI_REG8(0x05a0), 0xc2 }, > + { CCI_REG8(0x0207), 0xc4 }, > + > + /* GAIN */ > + { CCI_REG8(0x0208), 0x01 }, > + { CCI_REG8(0x0209), 0x72 }, > + { CCI_REG8(0x0204), 0x04 }, > + { CCI_REG8(0x0205), 0x00 }, > + > + { CCI_REG8(0x0040), 0x22 }, > + { CCI_REG8(0x0041), 0x20 }, > + { CCI_REG8(0x0043), 0x10 }, > + { CCI_REG8(0x0044), 0x00 }, > + { CCI_REG8(0x0046), 0x08 }, > + { CCI_REG8(0x0047), 0xf0 }, > + { CCI_REG8(0x0048), 0x0f }, > + { CCI_REG8(0x004b), 0x0f }, > + { CCI_REG8(0x004c), 0x00 }, > + { CCI_REG8(0x0050), 0x5c }, > + { CCI_REG8(0x0051), 0x44 }, > + { CCI_REG8(0x005b), 0x03 }, > + { CCI_REG8(0x00c0), 0x00 }, > + { CCI_REG8(0x00c1), 0x80 }, > + { CCI_REG8(0x00c2), 0x31 }, > + { CCI_REG8(0x00c3), 0x00 }, > + { CCI_REG8(0x0460), 0x04 }, > + { CCI_REG8(0x0462), 0x08 }, > + { CCI_REG8(0x0464), 0x0e }, > + { CCI_REG8(0x0466), 0x0a }, > + { CCI_REG8(0x0468), 0x12 }, > + { CCI_REG8(0x046a), 0x12 }, > + { CCI_REG8(0x046c), 0x10 }, > + { CCI_REG8(0x046e), 0x0c }, > + { CCI_REG8(0x0461), 0x03 }, > + { CCI_REG8(0x0463), 0x03 }, > + { CCI_REG8(0x0465), 0x03 }, > + { CCI_REG8(0x0467), 0x03 }, > + { CCI_REG8(0x0469), 0x04 }, > + { CCI_REG8(0x046b), 0x04 }, > + { CCI_REG8(0x046d), 0x04 }, > + { CCI_REG8(0x046f), 0x04 }, > + { CCI_REG8(0x0470), 0x04 }, > + { CCI_REG8(0x0472), 0x10 }, > + { CCI_REG8(0x0474), 0x26 }, > + { CCI_REG8(0x0476), 0x38 }, > + { CCI_REG8(0x0478), 0x20 }, > + { CCI_REG8(0x047a), 0x30 }, > + { CCI_REG8(0x047c), 0x38 }, > + { CCI_REG8(0x047e), 0x60 }, > + { CCI_REG8(0x0471), 0x05 }, > + { CCI_REG8(0x0473), 0x05 }, > + { CCI_REG8(0x0475), 0x05 }, > + { CCI_REG8(0x0477), 0x05 }, > + { CCI_REG8(0x0479), 0x04 }, > + { CCI_REG8(0x047b), 0x04 }, > + { CCI_REG8(0x047d), 0x04 }, > + { CCI_REG8(0x047f), 0x04 }, > +}; > + > +static const struct gc08a3_link_freq_config gc08a3_link_freq_336m_configs = { > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_table_common), > + .regs = mode_table_common, > + } > +}; > + > +static const struct gc08a3_link_freq_config gc08a3_link_freq_207m_configs = { > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_table_common), > + .regs = mode_table_common, > + } > +}; The registers are the same for both, so I think you can simplify this and drop the gc08a3_link_freq_config structure. > + > +struct gc08a3_mode { > + u32 width; > + u32 height; > + const struct gc08a3_reg_list reg_list; > + > + u32 hts; /* Horizontal timining size */ > + u32 vts_def; /* Default vertical timining size */ > + u32 vts_min; /* Min vertical timining size */ > + u32 max_framerate; > + const struct gc08a3_link_freq_config *link_freq_configs; > +}; > + > +/* > + * Declare modes in order, from biggest > + * to smallest height. > + */ > +static const struct gc08a3_mode gc08a3_modes[] = { > + { > + .width = GC08A3_NATIVE_WIDTH, > + .height = GC08A3_NATIVE_HEIGHT, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_3264x2448), > + .regs = mode_3264x2448, > + }, > + .link_freq_configs = &gc08a3_link_freq_336m_configs, > + > + .hts = GC08A3_HTS_30FPS, > + .vts_def = GC08A3_VTS_30FPS, > + .vts_min = GC08A3_VTS_30FPS_MIN, > + .max_framerate = 300, > + }, > + { > + .width = 1920, > + .height = 1080, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_1920x1080), > + .regs = mode_1920x1080, > + }, > + .link_freq_configs = &gc08a3_link_freq_207m_configs, > + > + .hts = GC08A3_HTS_60FPS, > + .vts_def = GC08A3_VTS_60FPS, > + .vts_min = GC08A3_VTS_60FPS_MIN, > + .max_framerate = 600, > + }, > +}; > + > +static u64 to_pixel_rate(u32 f_index) Move this function just before gc08a3_init_controls() where it is used, and rename it with a gc08a3_ prefix. > +{ > + u64 pixel_rate = link_freq_menu_items[f_index] * 2 * GC08A3_DATA_LANES; > + > + do_div(pixel_rate, GC08A3_RGB_DEPTH); > + > + return pixel_rate; > +} > + > +static int gc08a3_identify_module(struct gc08a3 *gc08a3) I would also move this down, before the probe function. > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&gc08a3->sd); > + u64 val = 0; No need to initialize val to 0. > + int ret; > + > + ret = cci_read(gc08a3->regmap, GC08A3_REG_CHIP_ID, &val, NULL); > + if (ret) { > + dev_err(&client->dev, Use gc08a3->dev and drop the local client variable. > + "failed to read chip id: 0x%x", GC08A3_CHIP_ID); "failed to read chip id\n"); Printing GC08A3_CHIP_ID isn't useful. > + return ret; > + } > + > + if (val != GC08A3_CHIP_ID) { > + dev_err(&client->dev, "chip id mismatch: 0x%x!=0x%llx", > + GC08A3_CHIP_ID, val); > + return -ENXIO; > + } > + > + return 0; > +} > + > +static inline struct gc08a3 *to_gc08a3(struct v4l2_subdev *sd) > +{ > + return container_of(sd, struct gc08a3, sd); > +} > + > +static int gc08a3_power_on(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); You can use struct v4l2_subdev *sd = dev_get_drvdata(dev); Same below. > + struct gc08a3 *gc08a3 = to_gc08a3(sd); > + int ret; > + > + ret = regulator_bulk_enable(ARRAY_SIZE(gc08a3_supply_name), > + gc08a3->supplies); > + if (ret < 0) { > + dev_err(gc08a3->dev, "failed to enable regulators: %d\n", ret); > + return ret; > + } > + > + ret = clk_prepare_enable(gc08a3->xclk); > + if (ret < 0) { > + regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name), > + gc08a3->supplies); > + dev_err(gc08a3->dev, "clk prepare enable failed\n"); > + return ret; > + } > + > + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); fsleep() is a good candidate to replace the usleep_range() calls. > + > + gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); Are you asserting reset when powering on ? That sounds wrong, you should de-assert reset here (and acquire the reset gpio in probe() with GPIOD_OUT_HIGH). Drivers should use logical levels for GPIOs, setting a GPIO named "reset" to 1 should assert the reset signal, even if the physical signal is active low. You may have the wrong polarity in the device tree. > + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > + > + return 0; > +} > + > +static int gc08a3_power_off(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct gc08a3 *gc08a3 = to_gc08a3(sd); > + > + clk_disable_unprepare(gc08a3->xclk); > + > + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > + > + gpiod_set_value_cansleep(gc08a3->reset_gpio, 0); > + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > + > + regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name), > + gc08a3->supplies); > + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); Do you need all those sleeps, especially the last one ? > + > + return 0; > +} > + > +static int gc08a3_enum_mbus_code(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_mbus_code_enum *code) > +{ > + if (code->index > 0) > + return -EINVAL; > + > + code->code = GC08A3_MBUS_CODE; > + > + return 0; > +} > + > +static int gc08a3_enum_frame_size(struct v4l2_subdev *subdev, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_frame_size_enum *fse) > +{ > + if (fse->code != GC08A3_MBUS_CODE) > + return -EINVAL; > + > + if (fse->index >= ARRAY_SIZE(gc08a3_modes)) > + return -EINVAL; > + > + fse->min_width = gc08a3_modes[fse->index].width; > + fse->max_width = gc08a3_modes[fse->index].width; > + fse->min_height = gc08a3_modes[fse->index].height; > + fse->max_height = gc08a3_modes[fse->index].height; > + > + return 0; > +} > + > +static int gc08a3_update_cur_mode_controls(struct gc08a3 *gc08a3, > + const struct gc08a3_mode *mode) > +{ > + s64 exposure_max, h_blank; > + int ret = 0; > + > + ret = __v4l2_ctrl_modify_range(gc08a3->vblank, > + mode->vts_min - mode->height, > + GC08A3_VTS_MAX - mode->height, 1, > + mode->vts_def - mode->height); > + if (ret) > + dev_err(gc08a3->dev, "VB ctrl range update failed\n"); return ret; } Same below. > + > + h_blank = mode->hts - mode->width; > + ret = __v4l2_ctrl_modify_range(gc08a3->hblank, h_blank, h_blank, 1, > + h_blank); > + if (ret) > + dev_err(gc08a3->dev, "HB ctrl range update failed\n"); > + > + exposure_max = mode->vts_def - GC08A3_EXP_MARGIN; > + ret = __v4l2_ctrl_modify_range(gc08a3->exposure, GC08A3_EXP_MIN, > + exposure_max, GC08A3_EXP_STEP, > + exposure_max); > + if (ret) > + dev_err(gc08a3->dev, "exposure ctrl range update failed\n"); > + > + return ret; > +} > + > +static void gc08a3_update_pad_format(struct gc08a3 *gc08a3, > + const struct gc08a3_mode *mode, > + struct v4l2_mbus_framefmt *fmt) > +{ > + fmt->width = mode->width; > + fmt->height = mode->height; > + fmt->code = GC08A3_MBUS_CODE; > + fmt->field = V4L2_FIELD_NONE; > + fmt->colorspace = V4L2_COLORSPACE_SRGB; It's a raw sensor, isn't V4L2_COLORSPACE_RAW more appropriate, or does the sensor really use the sRGB primaries ? > + fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace); > + fmt->quantization = > + V4L2_MAP_QUANTIZATION_DEFAULT(true, > + fmt->colorspace, > + fmt->ycbcr_enc); I think you can write fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; here, it's more explicit. > + fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace); Sensors usually produce linear output, V4L2_XFER_FUNC_NONE is likely what you need here. > +} > + > +static int gc08a3_set_format(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_format *fmt) > +{ > + struct gc08a3 *gc08a3 = to_gc08a3(sd); > + struct v4l2_mbus_framefmt *mbus_fmt; > + struct v4l2_rect *crop; > + const struct gc08a3_mode *mode; > + > + mode = v4l2_find_nearest_size(gc08a3_modes, ARRAY_SIZE(gc08a3_modes), > + width, height, fmt->format.width, > + fmt->format.height); > + > + /*update crop info to subdev state*/ Missing spaces after /* and before */. Same below. > + crop = v4l2_subdev_state_get_crop(state, 0); > + crop->width = mode->width; > + crop->height = mode->height; > + > + /*update fmt info to subdev state*/ > + gc08a3_update_pad_format(gc08a3, mode, &fmt->format); > + mbus_fmt = v4l2_subdev_state_get_format(state, 0); > + *mbus_fmt = fmt->format; > + > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) > + return 0; > + > + gc08a3->cur_mode = mode; > + gc08a3_update_cur_mode_controls(gc08a3, mode); > + > + return 0; > +} > + > +static int gc08a3_get_selection(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_selection *sel) > +{ > + struct gc08a3 *gc08a3 = to_gc08a3(sd); > + > + switch (sel->target) { > + case V4L2_SEL_TGT_CROP: > + sel->r = *v4l2_subdev_state_get_crop(state, 0); > + break; > + case V4L2_SEL_TGT_CROP_BOUNDS: > + sel->r.top = 0; > + sel->r.left = 0; > + sel->r.width = GC08A3_NATIVE_WIDTH; > + sel->r.height = GC08A3_NATIVE_HEIGHT; > + break; > + case V4L2_SEL_TGT_CROP_DEFAULT: > + if (gc08a3->cur_mode->width == GC08A3_NATIVE_WIDTH) { > + sel->r.top = 0; > + sel->r.left = 0; > + sel->r.width = gc08a3_modes[0].width; > + sel->r.height = gc08a3_modes[0].height; > + } else { > + sel->r.top = 0; > + sel->r.left = 0; > + sel->r.width = gc08a3_modes[1].width; > + sel->r.height = gc08a3_modes[1].height; > + } You store the crop rectangle in the gc08a3_set_format() function, you can simplify this with sel->r = *v4l2_subdev_state_get_crop(state, 0); > + break; > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int gc08a3_init_state(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state) > +{ > + struct v4l2_subdev_format fmt = { > + .which = V4L2_SUBDEV_FORMAT_TRY, > + .pad = 0, > + .format = { > + .code = GC08A3_MBUS_CODE, > + .width = gc08a3_modes[0].width, > + .height = gc08a3_modes[0].height, > + }, > + }; > + > + gc08a3_set_format(sd, state, &fmt); > + > + return 0; > +} > + > +static int gc08a3_set_ctrl_hflip(struct gc08a3 *gc08a3, u32 ctrl_val) > +{ > + int ret; > + u64 val; > + > + ret = cci_read(gc08a3->regmap, GC08A3_FLIP_REG, &val, NULL); > + if (ret) { > + dev_err(gc08a3->dev, "read hflip register failed: %d\n", ret); > + return ret; > + } > + > + val = ctrl_val ? (val | GC08A3_FLIP_H_MASK) : > + (val & ~GC08A3_FLIP_H_MASK); > + ret = cci_write(gc08a3->regmap, GC08A3_FLIP_REG, val, NULL); > + if (ret < 0) > + dev_err(gc08a3->dev, "Error %d\n", ret); You can use cci_update_bits() return cci_update_bits(gc08a3->regmap, GC08A3_FLIP_REG, GC08A3_FLIP_H_MASK, ctrl_val ? GC08A3_FLIP_H_MASK : 0, NULL); Same below. > + > + return ret; > +} > + > +static int gc08a3_set_ctrl_vflip(struct gc08a3 *gc08a3, u32 ctrl_val) > +{ > + int ret; > + u64 val; > + > + ret = cci_read(gc08a3->regmap, GC08A3_FLIP_REG, &val, NULL); > + if (ret) { > + dev_err(gc08a3->dev, "read vflip register failed: %d\n", ret); > + return ret; > + } > + > + val = ctrl_val ? (val | GC08A3_FLIP_V_MASK) : > + (val & ~GC08A3_FLIP_V_MASK); > + ret = cci_write(gc08a3->regmap, GC08A3_FLIP_REG, val, NULL); > + if (ret < 0) > + dev_err(gc08a3->dev, "Error %d\n", ret); > + > + return ret; > +} > + > +static int gc08a3_test_pattern(struct gc08a3 *gc08a3, u32 pattern_menu) > +{ > + u32 pattern = 0; > + int ret; int ret = 0; > + > + if (pattern_menu) { > + switch (pattern_menu) { > + case 1: > + pattern = 0x00; > + break; > + case 2: > + pattern = 0x10; > + break; > + case 3: > + case 4: > + case 5: > + case 6: > + case 7: > + pattern = pattern_menu + 1; > + break; > + } > + > + ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN, > + GC08A3_TEST_PATTERN_EN, NULL); > + if (ret) > + return ret; > + > + ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_IDX, > + pattern, NULL); > + cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN, GC08A3_TEST_PATTERN_EN, &ret); cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_IDX, pattern, &ret); And unless the sensor requires writing the GC08A3_REG_TEST_PATTERN_EN register first, I would write the test pattern index first, to avoid generating one frame with the wrong pattern. > + } else { > + ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN, > + 0x00, NULL); > + } > + > + return ret; > +} > + > +static int gc08a3_set_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct gc08a3 *gc08a3 = > + container_of(ctrl->handler, struct gc08a3, ctrls); > + int ret = 0; > + s64 exposure_max; > + > + if (ctrl->id == V4L2_CID_VBLANK) { > + /* Update max exposure while meeting expected vblanking */ > + exposure_max = gc08a3->cur_mode->height + ctrl->val - > + GC08A3_EXP_MARGIN; Let's not use cur_mode here, but retrieve the height from the state. This makes the driver implementation less dependent on the modes, paving the way to make the sensor fully configurable. You can look at the imx219 driver for an example. > + __v4l2_ctrl_modify_range(gc08a3->exposure, > + gc08a3->exposure->minimum, > + exposure_max, gc08a3->exposure->step, > + exposure_max); > + } > + > + /* > + * Applying V4L2 control value only happens > + * when power is on for streaming > + */ > + if (!pm_runtime_get_if_in_use(gc08a3->dev)) > + return 0; > + > + switch (ctrl->id) { > + case V4L2_CID_EXPOSURE: > + ret = cci_write(gc08a3->regmap, GC08A3_EXP_REG, > + ctrl->val, NULL); > + break; > + > + case V4L2_CID_ANALOGUE_GAIN: > + ret = cci_write(gc08a3->regmap, GC08A3_AGAIN_REG, > + ctrl->val, NULL); > + break; > + > + case V4L2_CID_VBLANK: > + ret = cci_write(gc08a3->regmap, GC08A3_FRAME_LENGTH_REG, > + gc08a3->cur_mode->height + ctrl->val, NULL); > + break; > + > + case V4L2_CID_HFLIP: > + ret = gc08a3_set_ctrl_hflip(gc08a3, ctrl->val); > + break; > + > + case V4L2_CID_VFLIP: > + ret = gc08a3_set_ctrl_vflip(gc08a3, ctrl->val); > + break; > + > + case V4L2_CID_TEST_PATTERN: > + ret = gc08a3_test_pattern(gc08a3, ctrl->val); > + break; > + > + default: > + break; > + } > + > + pm_runtime_put(gc08a3->dev); > + > + return ret; > +} > + > +static const struct v4l2_ctrl_ops gc08a3_ctrl_ops = { > + .s_ctrl = gc08a3_set_ctrl, > +}; > + > +static int gc08a3_start_streaming(struct gc08a3 *gc08a3) > +{ > + const struct gc08a3_mode *mode; > + const struct gc08a3_reg_list *reg_list; > + const struct gc08a3_link_freq_config *link_freq_cfg; > + int ret; > + > + ret = pm_runtime_resume_and_get(gc08a3->dev); > + if (ret < 0) > + return ret; > + > + link_freq_cfg = gc08a3->cur_mode->link_freq_configs; > + > + reg_list = &link_freq_cfg->reg_list; > + ret = cci_multi_reg_write(gc08a3->regmap, > + reg_list->regs, reg_list->num_of_regs, NULL); > + if (ret) { > + dev_err(gc08a3->dev, "could not sent common table %d\n", ret); The cci helpers already write error messages, I think you can drop this one, as well as similar messages below. > + goto err_rpm_put; > + } > + > + mode = gc08a3->cur_mode; > + reg_list = &mode->reg_list; > + ret = cci_multi_reg_write(gc08a3->regmap, > + reg_list->regs, reg_list->num_of_regs, NULL); > + if (ret < 0) { > + dev_err(gc08a3->dev, "could not sent mode table %d\n", ret); > + goto err_rpm_put; > + } > + > + ret = __v4l2_ctrl_handler_setup(&gc08a3->ctrls); > + if (ret < 0) { > + dev_err(gc08a3->dev, "could not sync v4l2 controls\n"); > + goto err_rpm_put; > + } > + > + ret = cci_write(gc08a3->regmap, GC08A3_STRAEMING_REG, 1, NULL); > + if (ret < 0) { > + dev_err(gc08a3->dev, "write STRAEMING_REG failed: %d\n", ret); > + goto err_rpm_put; > + } > + > + return 0; > + > +err_rpm_put: > + pm_runtime_put(gc08a3->dev); > + return ret; > +} > + > +static int gc08a3_stop_streaming(struct gc08a3 *gc08a3) > +{ > + int ret; > + > + ret = cci_write(gc08a3->regmap, GC08A3_STRAEMING_REG, 0, NULL); > + if (ret < 0) > + dev_err(gc08a3->dev, "could not sent stop streaming %d\n", ret); > + > + pm_runtime_put(gc08a3->dev); > + return ret; > +} > + > +static int gc08a3_s_stream(struct v4l2_subdev *subdev, int enable) > +{ > + struct gc08a3 *gc08a3 = to_gc08a3(subdev); > + struct v4l2_subdev_state *state; > + int ret; > + > + state = v4l2_subdev_lock_and_get_active_state(subdev); > + > + if (enable) > + ret = gc08a3_start_streaming(gc08a3); > + else > + ret = gc08a3_stop_streaming(gc08a3); > + > + gc08a3->streaming = enable; > + v4l2_subdev_unlock_state(state); > + > + return ret; > +} > + > +static int > +gc08a3_enum_frame_interval(struct v4l2_subdev *subdev, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_frame_interval_enum *fie) Frame intervals on sensors should be controlled through the pixel rate and blanking. You can drop this function, as well as the gc08a3_mode.max_framerate field. > +{ > + unsigned int index = fie->index; > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(gc08a3_modes); i++) { > + if (fie->width != gc08a3_modes[i].width || > + fie->height != gc08a3_modes[i].height) > + continue; > + > + if (index-- == 0) { > + fie->interval.numerator = 1; > + fie->interval.denominator = > + gc08a3_modes[i].max_framerate / 10; > + return 0; > + } > + } > + > + return -EINVAL; > +} > + > +static const struct v4l2_subdev_video_ops gc08a3_video_ops = { > + .s_stream = gc08a3_s_stream, > +}; > + > +static const struct v4l2_subdev_pad_ops gc08a3_subdev_pad_ops = { > + .enum_mbus_code = gc08a3_enum_mbus_code, > + .enum_frame_size = gc08a3_enum_frame_size, > + .enum_frame_interval = gc08a3_enum_frame_interval, > + .get_fmt = v4l2_subdev_get_fmt, > + .set_fmt = gc08a3_set_format, > + .get_selection = gc08a3_get_selection, > +}; > + > +static const struct v4l2_subdev_core_ops gc08a3_core_ops = { > + .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > + .unsubscribe_event = v4l2_event_subdev_unsubscribe, > +}; > + > +static const struct v4l2_subdev_ops gc08a3_subdev_ops = { > + .core = &gc08a3_core_ops, > + .video = &gc08a3_video_ops, > + .pad = &gc08a3_subdev_pad_ops, > +}; > + > +static const struct v4l2_subdev_internal_ops gc08a3_internal_ops = { > + .init_state = gc08a3_init_state, > +}; > + > +static int gc08a3_get_regulators(struct device *dev, struct gc08a3 *gc08a3) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(gc08a3_supply_name); i++) > + gc08a3->supplies[i].supply = gc08a3_supply_name[i]; > + > + return devm_regulator_bulk_get(dev, ARRAY_SIZE(gc08a3_supply_name), > + gc08a3->supplies); > +} > + > +static int gc08a3_parse_fwnode(struct device *dev, struct gc08a3 *gc08a3) You can drop the first argument and use gc08a3->dev instead. > +{ > + struct fwnode_handle *endpoint; > + struct v4l2_fwnode_endpoint bus_cfg = { > + .bus_type = V4L2_MBUS_CSI2_DPHY, > + }; > + int ret; > + > + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL); Please use endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(gc08a3->dev), 0, 0, FWNODE_GRAPH_ENDPOINT_NEXT); > + if (!endpoint) { > + dev_err(dev, "endpoint node not found\n"); > + return -EINVAL; > + } > + > + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg); > + if (ret) { > + dev_err(dev, "parsing endpoint node failed\n"); > + goto done; > + } > + > + ret = v4l2_link_freq_to_bitmap(dev, bus_cfg.link_frequencies, > + bus_cfg.nr_of_link_frequencies, > + link_freq_menu_items, > + ARRAY_SIZE(link_freq_menu_items), > + &gc08a3->link_freq_bitmap); > + if (ret) > + goto done; > + > +done: > + v4l2_fwnode_endpoint_free(&bus_cfg); > + fwnode_handle_put(endpoint); > + return ret; > +} > + > +static int gc08a3_init_controls(struct gc08a3 *gc08a3) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&gc08a3->sd); > + const struct gc08a3_mode *mode = &gc08a3_modes[0]; > + const struct v4l2_ctrl_ops *ops = &gc08a3_ctrl_ops; > + struct v4l2_fwnode_device_properties props; > + struct v4l2_ctrl_handler *ctrl_hdlr; > + s64 exposure_max, h_blank; > + int ret; > + > + ctrl_hdlr = &gc08a3->ctrls; > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10); > + if (ret) > + return ret; > + > + gc08a3->link_freq = > + v4l2_ctrl_new_int_menu(ctrl_hdlr, > + &gc08a3_ctrl_ops, > + V4L2_CID_LINK_FREQ, > + ARRAY_SIZE(link_freq_menu_items) - 1, > + 0, link_freq_menu_items); > + if (gc08a3->link_freq) > + gc08a3->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + gc08a3->pixel_rate = > + v4l2_ctrl_new_std(ctrl_hdlr, > + &gc08a3_ctrl_ops, > + V4L2_CID_PIXEL_RATE, 0, > + to_pixel_rate(GC08A3_LINK_FREQ_336MHZ_INDEX), > + 1, > + to_pixel_rate(GC08A3_LINK_FREQ_336MHZ_INDEX)); > + > + gc08a3->vblank = > + v4l2_ctrl_new_std(ctrl_hdlr, > + &gc08a3_ctrl_ops, V4L2_CID_VBLANK, > + mode->vts_min - mode->height, > + GC08A3_VTS_MAX - mode->height, 1, > + mode->vts_def - mode->height); > + > + h_blank = mode->hts - mode->width; > + gc08a3->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, > + V4L2_CID_HBLANK, h_blank, h_blank, 1, > + h_blank); > + if (gc08a3->hblank) > + gc08a3->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, > + V4L2_CID_ANALOGUE_GAIN, GC08A3_AGAIN_MIN, > + GC08A3_AGAIN_MAX, GC08A3_AGAIN_STEP, > + GC08A3_AGAIN_MIN); > + > + exposure_max = mode->vts_def - GC08A3_EXP_MARGIN; > + gc08a3->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, > + V4L2_CID_EXPOSURE, GC08A3_EXP_MIN, > + exposure_max, GC08A3_EXP_STEP, > + exposure_max); > + > + v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &gc08a3_ctrl_ops, > + V4L2_CID_TEST_PATTERN, > + ARRAY_SIZE(gc08a3_test_pattern_menu) - 1, > + 0, 0, gc08a3_test_pattern_menu); > + > + v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, V4L2_CID_HFLIP, 0, > + 1, 1, 0); > + > + v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, V4L2_CID_VFLIP, 0, > + 1, 1, 0); Let's put the HFLIP and VFLIP controls in a cluster to make it possible to efficiently set them in an atomic operation. See the imx219 driver for an example. > + > + /* register properties to fwnode (e.g. rotation, orientation) */ > + ret = v4l2_fwnode_device_parse(&client->dev, &props); > + if (ret) > + goto error_ctrls; > + > + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, ops, &props); > + if (ret) > + goto error_ctrls; > + > + if (ctrl_hdlr->error) { > + ret = ctrl_hdlr->error; > + goto error_ctrls; > + } > + > + gc08a3->sd.ctrl_handler = ctrl_hdlr; > + > + return 0; > + > +error_ctrls: > + v4l2_ctrl_handler_free(ctrl_hdlr); > + > + return ret; > +} > + > +static int gc08a3_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct gc08a3 *gc08a3; > + int ret; > + > + gc08a3 = devm_kzalloc(dev, sizeof(*gc08a3), GFP_KERNEL); > + if (!gc08a3) > + return -ENOMEM; > + > + gc08a3->dev = dev; > + > + ret = gc08a3_parse_fwnode(dev, gc08a3); > + if (ret) > + return ret; > + > + gc08a3->regmap = devm_cci_regmap_init_i2c(client, 16); > + if (IS_ERR(gc08a3->regmap)) > + return dev_err_probe(dev, PTR_ERR(gc08a3->regmap), > + "failed to init CCI\n"); > + > + gc08a3->xclk = devm_clk_get(dev, NULL); > + if (IS_ERR(gc08a3->xclk)) > + return dev_err_probe(dev, PTR_ERR(gc08a3->xclk), > + "failed to get xclk\n"); > + > + ret = clk_set_rate(gc08a3->xclk, GC08A3_DEFAULT_CLK_FREQ); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to set xclk frequency\n"); > + > + ret = gc08a3_get_regulators(dev, gc08a3); > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "failed to get regulators\n"); > + > + gc08a3->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > + if (IS_ERR(gc08a3->reset_gpio)) > + return dev_err_probe(dev, PTR_ERR(gc08a3->reset_gpio), > + "failed to get gpio\n"); > + > + v4l2_i2c_subdev_init(&gc08a3->sd, client, &gc08a3_subdev_ops); > + gc08a3->sd.internal_ops = &gc08a3_internal_ops; > + gc08a3->cur_mode = &gc08a3_modes[0]; > + > + ret = gc08a3_power_on(gc08a3->dev); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to sensor power on\n"); > + > + ret = gc08a3_identify_module(gc08a3); > + if (ret) { > + dev_err(&client->dev, "failed to find sensor: %d\n", ret); > + goto err_power_off; > + } > + > + ret = gc08a3_init_controls(gc08a3); > + if (ret) { > + dev_err(&client->dev, "failed to init controls: %d", ret); > + goto err_power_off; > + } > + > + gc08a3->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | > + V4L2_SUBDEV_FL_HAS_EVENTS; > + gc08a3->pad.flags = MEDIA_PAD_FL_SOURCE; > + gc08a3->sd.dev = &client->dev; > + gc08a3->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > + > + ret = media_entity_pads_init(&gc08a3->sd.entity, 1, &gc08a3->pad); > + if (ret < 0) { > + dev_err(dev, "could not register media entity\n"); > + goto err_v4l2_ctrl_handler_free; > + } > + > + gc08a3->sd.state_lock = gc08a3->ctrls.lock; > + ret = v4l2_subdev_init_finalize(&gc08a3->sd); > + if (ret < 0) { > + dev_err(dev, "v4l2 subdev init error: %d\n", ret); > + goto err_media_entity_cleanup; > + } > + > + pm_runtime_set_active(gc08a3->dev); > + pm_runtime_enable(gc08a3->dev); > + pm_runtime_idle(gc08a3->dev); Could you enable autosuspend ? > + > + ret = v4l2_async_register_subdev_sensor(&gc08a3->sd); > + if (ret < 0) { > + dev_err(dev, "could not register v4l2 device\n"); > + goto err_rpm; > + } > + > + return 0; > + > +err_rpm: > + pm_runtime_disable(gc08a3->dev); > + v4l2_subdev_cleanup(&gc08a3->sd); > + > +err_media_entity_cleanup: > + media_entity_cleanup(&gc08a3->sd.entity); > + > +err_v4l2_ctrl_handler_free: > + v4l2_ctrl_handler_free(gc08a3->sd.ctrl_handler); > + > +err_power_off: > + gc08a3_power_off(gc08a3->dev); > + > + return ret; > +} > + > +static void gc08a3_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct gc08a3 *gc08a3 = to_gc08a3(sd); > + > + v4l2_async_unregister_subdev(&gc08a3->sd); > + v4l2_subdev_cleanup(sd); > + media_entity_cleanup(&gc08a3->sd.entity); > + v4l2_ctrl_handler_free(&gc08a3->ctrls); > + > + pm_runtime_disable(&client->dev); > + if (!pm_runtime_status_suspended(&client->dev)) > + gc08a3_power_off(gc08a3->dev); > + pm_runtime_set_suspended(&client->dev); > +} > + > +static const struct of_device_id gc08a3_of_match[] = { > + { .compatible = "galaxycore,gc08a3" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, gc08a3_of_match); > + > +static const struct dev_pm_ops gc08a3_pm_ops = { > + SET_RUNTIME_PM_OPS(gc08a3_power_off, gc08a3_power_on, NULL) > +}; > + > +static struct i2c_driver gc08a3_i2c_driver = { > + .driver = { > + .of_match_table = gc08a3_of_match, > + .pm = &gc08a3_pm_ops, > + .name = "gc08a3", > + }, > + .probe = gc08a3_probe, > + .remove = gc08a3_remove, > +}; > + > +module_i2c_driver(gc08a3_i2c_driver); > + > +MODULE_DESCRIPTION("GalaxyCore gc08a3 Camera driver"); > +MODULE_AUTHOR("Zhi Mao <zhi.mao@mediatek.com>"); > +MODULE_LICENSE("GPL");
Il 04/02/24 07:15, Zhi Mao ha scritto: > Add a V4L2 sub-device driver for Galaxycore GC08A3 image sensor. > > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com> > --- > drivers/media/i2c/Kconfig | 10 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/gc08a3.c | 1448 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 1459 insertions(+) > create mode 100644 drivers/media/i2c/gc08a3.c > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 56f276b920ab..e4da68835683 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -70,6 +70,16 @@ config VIDEO_GC0308 > To compile this driver as a module, choose M here: the > module will be called gc0308. > > +config VIDEO_GC08A3 > + tristate "GalaxyCore gc08a3 sensor support" > + select V4L2_CCI_I2C > + help > + This is a Video4Linux2 sensor driver for the GalaxyCore gc08a3 > + camera. > + > + To compile this driver as a module, choose M here: the > + module will be called gc08a3. > + > config VIDEO_GC2145 > select V4L2_CCI_I2C > tristate "GalaxyCore GC2145 sensor support" > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index dfbe6448b549..b82e99ca7578 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -38,6 +38,7 @@ obj-$(CONFIG_VIDEO_DW9768) += dw9768.o > obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o > obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/ > obj-$(CONFIG_VIDEO_GC0308) += gc0308.o > +obj-$(CONFIG_VIDEO_GC08A3) += gc08a3.o > obj-$(CONFIG_VIDEO_GC2145) += gc2145.o > obj-$(CONFIG_VIDEO_HI556) += hi556.o > obj-$(CONFIG_VIDEO_HI846) += hi846.o > diff --git a/drivers/media/i2c/gc08a3.c b/drivers/media/i2c/gc08a3.c > new file mode 100644 > index 000000000000..3fc7fffb815c > --- /dev/null > +++ b/drivers/media/i2c/gc08a3.c > @@ -0,0 +1,1448 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * gc08a3.c - gc08a3 sensor driver > + * > + * Copyright 2023 MediaTek > + * > + * Zhi Mao <zhi.mao@mediatek.com> > + */ > + ..snip.. > + > +static const struct gc08a3_link_freq_config gc08a3_link_freq_336m_configs = { > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_table_common), > + .regs = mode_table_common, > + } > +}; > + > +static const struct gc08a3_link_freq_config gc08a3_link_freq_207m_configs = { > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_table_common), > + .regs = mode_table_common, > + } > +}; > + Since you're documenting this structure anyway, why not kerneldoc? :-) > +struct gc08a3_mode { > + u32 width; > + u32 height; > + const struct gc08a3_reg_list reg_list; > + > + u32 hts; /* Horizontal timining size */ > + u32 vts_def; /* Default vertical timining size */ > + u32 vts_min; /* Min vertical timining size */ > + u32 max_framerate; > + const struct gc08a3_link_freq_config *link_freq_configs; > +}; > + > +/* > + * Declare modes in order, from biggest > + * to smallest height. > + */ one line is enough for this comment. > +static const struct gc08a3_mode gc08a3_modes[] = { > + { > + .width = GC08A3_NATIVE_WIDTH, > + .height = GC08A3_NATIVE_HEIGHT, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_3264x2448), > + .regs = mode_3264x2448, > + }, > + .link_freq_configs = &gc08a3_link_freq_336m_configs, > + > + .hts = GC08A3_HTS_30FPS, > + .vts_def = GC08A3_VTS_30FPS, > + .vts_min = GC08A3_VTS_30FPS_MIN, > + .max_framerate = 300, > + }, > + { > + .width = 1920, > + .height = 1080, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_1920x1080), > + .regs = mode_1920x1080, > + }, > + .link_freq_configs = &gc08a3_link_freq_207m_configs, > + > + .hts = GC08A3_HTS_60FPS, > + .vts_def = GC08A3_VTS_60FPS, > + .vts_min = GC08A3_VTS_60FPS_MIN, > + .max_framerate = 600, > + }, > +}; > + > +static u64 to_pixel_rate(u32 f_index) > +{ > + u64 pixel_rate = link_freq_menu_items[f_index] * 2 * GC08A3_DATA_LANES; > + > + do_div(pixel_rate, GC08A3_RGB_DEPTH); The divisor is (less than) 32 bits and the dividend is always 64 bits: that will break on builds for 32-bits CPUs. Just do.... return div_u64(pixel_rate, GB08A3_RGB_DEPTH); > + > + return pixel_rate; > +} > + > +static int gc08a3_identify_module(struct gc08a3 *gc08a3) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&gc08a3->sd); > + u64 val = 0; u64 val; > + int ret; > + Either log here or in the probe function, otherwise it's just redudant. > + ret = cci_read(gc08a3->regmap, GC08A3_REG_CHIP_ID, &val, NULL); > + if (ret) { > + dev_err(&client->dev, > + "failed to read chip id: 0x%x", GC08A3_CHIP_ID); > + return ret; > + } > + > + if (val != GC08A3_CHIP_ID) { > + dev_err(&client->dev, "chip id mismatch: 0x%x!=0x%llx", > + GC08A3_CHIP_ID, val); > + return -ENXIO; > + } > + > + return 0; > +} > + > +static inline struct gc08a3 *to_gc08a3(struct v4l2_subdev *sd) > +{ > + return container_of(sd, struct gc08a3, sd); > +} > + ..snip.. > + > +static int gc08a3_update_cur_mode_controls(struct gc08a3 *gc08a3, > + const struct gc08a3_mode *mode) > +{ > + s64 exposure_max, h_blank; > + int ret = 0; > + > + ret = __v4l2_ctrl_modify_range(gc08a3->vblank, > + mode->vts_min - mode->height, > + GC08A3_VTS_MAX - mode->height, 1, > + mode->vts_def - mode->height); > + if (ret) > + dev_err(gc08a3->dev, "VB ctrl range update failed\n"); > + > + h_blank = mode->hts - mode->width; > + ret = __v4l2_ctrl_modify_range(gc08a3->hblank, h_blank, h_blank, 1, > + h_blank); > + if (ret) > + dev_err(gc08a3->dev, "HB ctrl range update failed\n"); > + > + exposure_max = mode->vts_def - GC08A3_EXP_MARGIN; > + ret = __v4l2_ctrl_modify_range(gc08a3->exposure, GC08A3_EXP_MIN, > + exposure_max, GC08A3_EXP_STEP, > + exposure_max); > + if (ret) > + dev_err(gc08a3->dev, "exposure ctrl range update failed\n"); No. You're not returning anywhere for error. That's not okay. Besides... if (ret) { dev_err.. return ret; } return 0; > + > + return ret; > +} > + > +static void gc08a3_update_pad_format(struct gc08a3 *gc08a3, > + const struct gc08a3_mode *mode, > + struct v4l2_mbus_framefmt *fmt) > +{ > + fmt->width = mode->width; > + fmt->height = mode->height; > + fmt->code = GC08A3_MBUS_CODE; > + fmt->field = V4L2_FIELD_NONE; > + fmt->colorspace = V4L2_COLORSPACE_SRGB; > + fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace); > + fmt->quantization = > + V4L2_MAP_QUANTIZATION_DEFAULT(true, > + fmt->colorspace, > + fmt->ycbcr_enc); > + fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace); > +} > + > +static int gc08a3_set_format(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_format *fmt) > +{ > + struct gc08a3 *gc08a3 = to_gc08a3(sd); > + struct v4l2_mbus_framefmt *mbus_fmt; > + struct v4l2_rect *crop; > + const struct gc08a3_mode *mode; > + > + mode = v4l2_find_nearest_size(gc08a3_modes, ARRAY_SIZE(gc08a3_modes), > + width, height, fmt->format.width, > + fmt->format.height); > + > + /*update crop info to subdev state*/ > + crop = v4l2_subdev_state_get_crop(state, 0); > + crop->width = mode->width; > + crop->height = mode->height; > + > + /*update fmt info to subdev state*/ > + gc08a3_update_pad_format(gc08a3, mode, &fmt->format); > + mbus_fmt = v4l2_subdev_state_get_format(state, 0); > + *mbus_fmt = fmt->format; > + > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) > + return 0; > + > + gc08a3->cur_mode = mode; > + gc08a3_update_cur_mode_controls(gc08a3, mode); > + > + return 0; > +} > + > +static int gc08a3_get_selection(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_selection *sel) > +{ > + struct gc08a3 *gc08a3 = to_gc08a3(sd); > + > + switch (sel->target) { > + case V4L2_SEL_TGT_CROP: > + sel->r = *v4l2_subdev_state_get_crop(state, 0); > + break; > + case V4L2_SEL_TGT_CROP_BOUNDS: > + sel->r.top = 0; > + sel->r.left = 0; > + sel->r.width = GC08A3_NATIVE_WIDTH; > + sel->r.height = GC08A3_NATIVE_HEIGHT; > + break; > + case V4L2_SEL_TGT_CROP_DEFAULT: > + if (gc08a3->cur_mode->width == GC08A3_NATIVE_WIDTH) { > + sel->r.top = 0; > + sel->r.left = 0; > + sel->r.width = gc08a3_modes[0].width; > + sel->r.height = gc08a3_modes[0].height; > + } else { > + sel->r.top = 0; > + sel->r.left = 0; > + sel->r.width = gc08a3_modes[1].width; > + sel->r.height = gc08a3_modes[1].height; > + } > + break; > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int gc08a3_init_state(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state) > +{ > + struct v4l2_subdev_format fmt = { > + .which = V4L2_SUBDEV_FORMAT_TRY, > + .pad = 0, > + .format = { > + .code = GC08A3_MBUS_CODE, > + .width = gc08a3_modes[0].width, > + .height = gc08a3_modes[0].height, > + }, > + }; > + > + gc08a3_set_format(sd, state, &fmt); > + > + return 0; > +} > + > +static int gc08a3_set_ctrl_hflip(struct gc08a3 *gc08a3, u32 ctrl_val) > +{ > + int ret; > + u64 val; > + > + ret = cci_read(gc08a3->regmap, GC08A3_FLIP_REG, &val, NULL); > + if (ret) { > + dev_err(gc08a3->dev, "read hflip register failed: %d\n", ret); > + return ret; > + } > + > + val = ctrl_val ? (val | GC08A3_FLIP_H_MASK) : > + (val & ~GC08A3_FLIP_H_MASK); > + ret = cci_write(gc08a3->regmap, GC08A3_FLIP_REG, val, NULL); > + if (ret < 0) > + dev_err(gc08a3->dev, "Error %d\n", ret); > + > + return ret; > +} > + > +static int gc08a3_set_ctrl_vflip(struct gc08a3 *gc08a3, u32 ctrl_val) > +{ > + int ret; > + u64 val; > + > + ret = cci_read(gc08a3->regmap, GC08A3_FLIP_REG, &val, NULL); > + if (ret) { > + dev_err(gc08a3->dev, "read vflip register failed: %d\n", ret); > + return ret; > + } > + > + val = ctrl_val ? (val | GC08A3_FLIP_V_MASK) : > + (val & ~GC08A3_FLIP_V_MASK); > + ret = cci_write(gc08a3->regmap, GC08A3_FLIP_REG, val, NULL); > + if (ret < 0) > + dev_err(gc08a3->dev, "Error %d\n", ret); > + > + return ret; > +} > + > +static int gc08a3_test_pattern(struct gc08a3 *gc08a3, u32 pattern_menu) > +{ > + u32 pattern = 0; > + int ret; ret not initialized is ok here; > + > + if (pattern_menu) { > + switch (pattern_menu) { > + case 1: > + pattern = 0x00; > + break; > + case 2: > + pattern = 0x10; > + break; > + case 3: > + case 4: > + case 5: > + case 6: > + case 7: > + pattern = pattern_menu + 1; > + break; > + } > + > + ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN, > + GC08A3_TEST_PATTERN_EN, NULL); > + if (ret) > + return ret; > + > + ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_IDX, > + pattern, NULL); if (ret) return ret; > + > + } else { > + ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN, > + 0x00, NULL); if (ret) return ret; > + } > + return 0; > + return ret; > +} > + > +static int gc08a3_set_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct gc08a3 *gc08a3 = > + container_of(ctrl->handler, struct gc08a3, ctrls); > + int ret = 0; int ret; > + s64 exposure_max; > + > + if (ctrl->id == V4L2_CID_VBLANK) { > + /* Update max exposure while meeting expected vblanking */ > + exposure_max = gc08a3->cur_mode->height + ctrl->val - > + GC08A3_EXP_MARGIN; > + __v4l2_ctrl_modify_range(gc08a3->exposure, > + gc08a3->exposure->minimum, > + exposure_max, gc08a3->exposure->step, > + exposure_max); > + } > + > + /* > + * Applying V4L2 control value only happens > + * when power is on for streaming > + */ > + if (!pm_runtime_get_if_in_use(gc08a3->dev)) > + return 0; > + > + switch (ctrl->id) { > + case V4L2_CID_EXPOSURE: > + ret = cci_write(gc08a3->regmap, GC08A3_EXP_REG, > + ctrl->val, NULL); > + break; > + > + case V4L2_CID_ANALOGUE_GAIN: > + ret = cci_write(gc08a3->regmap, GC08A3_AGAIN_REG, > + ctrl->val, NULL); > + break; > + > + case V4L2_CID_VBLANK: > + ret = cci_write(gc08a3->regmap, GC08A3_FRAME_LENGTH_REG, > + gc08a3->cur_mode->height + ctrl->val, NULL); > + break; > + > + case V4L2_CID_HFLIP: > + ret = gc08a3_set_ctrl_hflip(gc08a3, ctrl->val); > + break; > + > + case V4L2_CID_VFLIP: > + ret = gc08a3_set_ctrl_vflip(gc08a3, ctrl->val); > + break; > + > + case V4L2_CID_TEST_PATTERN: > + ret = gc08a3_test_pattern(gc08a3, ctrl->val); > + break; > + > + default: > + break; > + } > + > + pm_runtime_put(gc08a3->dev); if (ret) return ret; return 0; > + > + return ret; > +} > + ...and I've ignored some more stuff as other reviewers already gave you feedback. Regards, Angelo
Hi Laurent, Thanks for your review. On Tue, 2024-02-06 at 20:45 +0200, Laurent Pinchart wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Hi Zhi, > > Thank you for the patch. > > On Sun, Feb 04, 2024 at 02:15:38PM +0800, Zhi Mao wrote: > > Add a V4L2 sub-device driver for Galaxycore GC08A3 image sensor. > > > > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com> > > --- > > drivers/media/i2c/Kconfig | 10 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/gc08a3.c | 1448 > ++++++++++++++++++++++++++++++++++++ > > 3 files changed, 1459 insertions(+) > > create mode 100644 drivers/media/i2c/gc08a3.c > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > index 56f276b920ab..e4da68835683 100644 > > --- a/drivers/media/i2c/Kconfig > > +++ b/drivers/media/i2c/Kconfig > > @@ -70,6 +70,16 @@ config VIDEO_GC0308 > > To compile this driver as a module, choose M here: the > > module will be called gc0308. > > > > +config VIDEO_GC08A3 > > +tristate "GalaxyCore gc08a3 sensor support" > > +select V4L2_CCI_I2C > > +help > > + This is a Video4Linux2 sensor driver for the GalaxyCore gc08a3 > > + camera. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called gc08a3. > > + > > config VIDEO_GC2145 > > select V4L2_CCI_I2C > > tristate "GalaxyCore GC2145 sensor support" > > diff --git a/drivers/media/i2c/Makefile > b/drivers/media/i2c/Makefile > > index dfbe6448b549..b82e99ca7578 100644 > > --- a/drivers/media/i2c/Makefile > > +++ b/drivers/media/i2c/Makefile > > @@ -38,6 +38,7 @@ obj-$(CONFIG_VIDEO_DW9768) += dw9768.o > > obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o > > obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/ > > obj-$(CONFIG_VIDEO_GC0308) += gc0308.o > > +obj-$(CONFIG_VIDEO_GC08A3) += gc08a3.o > > obj-$(CONFIG_VIDEO_GC2145) += gc2145.o > > obj-$(CONFIG_VIDEO_HI556) += hi556.o > > obj-$(CONFIG_VIDEO_HI846) += hi846.o > > diff --git a/drivers/media/i2c/gc08a3.c > b/drivers/media/i2c/gc08a3.c > > new file mode 100644 > > index 000000000000..3fc7fffb815c > > --- /dev/null > > +++ b/drivers/media/i2c/gc08a3.c > > @@ -0,0 +1,1448 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * gc08a3.c - gc08a3 sensor driver > > + * > > + * Copyright 2023 MediaTek > > + * > > + * Zhi Mao <zhi.mao@mediatek.com> > > + */ > > + > > +#include <asm/unaligned.h> > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/i2c.h> > > +#include <linux/module.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/regulator/consumer.h> > > +#include <media/media-entity.h> > > +#include <media/v4l2-ctrls.h> > > +#include <media/v4l2-fwnode.h> > > +#include <media/v4l2-subdev.h> > > +#include <media/v4l2-cci.h> > > +#include <media/v4l2-event.h> > > Please sort the headers alphabetically. > fixed in patch:v5. > > + > > +#define GC08A3_REG_CHIP_ID CCI_REG16(0x03f0) > > +#define GC08A3_CHIP_ID 0x08a3 > > + > > +#define GC08A3_NATIVE_WIDTH 3264 > > +#define GC08A3_NATIVE_HEIGHT 2448 > > + > > +#define GC08A3_REG_TEST_PATTERN_EN CCI_REG8(0x008c) > > +#define GC08A3_REG_TEST_PATTERN_IDX CCI_REG8(0x008d) > > +#define GC08A3_TEST_PATTERN_EN 0x01 > > + > > +#define GC08A3_STRAEMING_REG CCI_REG8(0x0100) > > s/STRAEMING/STREAMING/ > fixed in patch:v5. > > + > > +#define GC08A3_DEFAULT_CLK_FREQ 24000000 > > +#define GC08A3_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10 > > +#define GC08A3_DATA_LANES 4 > > Let's not mix registers and other macros. Please define the register > and > register field macros first (sorted by register address in numerical > order), and then all other macros. > fixed in patch:v5. > > + > > +/* for 1920*1080 */ > > +#define GC08A3_LINK_FREQ_207MHZ 207000000ULL > > +/* for 3264*2448 */ > > +#define GC08A3_LINK_FREQ_336MHZ 336000000ULL > > Use the values directly in the single location where the macros are > used > below, and drop the macros. > fixed in patch:v5. > > + > > +#define GC08A3_RGB_DEPTH 10 > > + > > +#define GC08A3_FRAME_LENGTH_REG CCI_REG16(0x0340) > > +#define GC08A3_VTS_30FPS 2548 > > +#define GC08A3_VTS_30FPS_MIN 2548 > > +#define GC08A3_VTS_60FPS 1276 > > +#define GC08A3_VTS_60FPS_MIN 1276 > > These four macros don't improve readability, you can use the values > directly in the single location where they are used. > fixed in patch:v5. > > +#define GC08A3_VTS_MAX 0xfff0 > > + > > +#define GC08A3_HTS_30FPS 3640 > > +#define GC08A3_HTS_60FPS 3640 > > Same for these two macros. > fixed in patch:v5. > > + > > +#define GC08A3_EXP_REG CCI_REG16(0x0202) > > +#define GC08A3_EXP_MARGIN 16 > > +#define GC08A3_EXP_MIN 4 > > +#define GC08A3_EXP_STEP 1 > > + > > +#define GC08A3_FLIP_REG CCI_REG8(0x0101) > > +#define GC08A3_FLIP_H_MASK 0x1 > > BIT(0) > > > +#define GC08A3_FLIP_V_MASK 0x2 > > BIT(1) > fixed in patch:v5. > > + > > +#define GC08A3_AGAIN_REG CCI_REG16(0x0204) > > +#define GC08A3_AGAIN_MIN 1024 > > +#define GC08A3_AGAIN_MAX (1024 * 16) > > +#define GC08A3_AGAIN_STEP 1 > > + > > +#define GC08A3_MIN_SLEEP_US 2000 > > +#define GC08A3_MAX_SLEEP_US 3000 > > + > > +static const char *const gc08a3_test_pattern_menu[] = { > > +"No Pattern", "Solid Black", "Colour Bar", "Solid White", > > +"Solid Red", "Solid Green", "Solid Blue", "Solid Yellow", > > +}; > > + > > +enum { > > +GC08A3_LINK_FREQ_336MHZ_INDEX, > > +GC08A3_LINK_FREQ_207MHZ_INDEX, > > The second value is never used, and the first one is used in > gc08a3_init_controls() only. I would drop both, and pass > link_freq_menu_items[0] to the to_pixel_rate() function instead of > passing an index. > fixed in patch:v5. > > +}; > > + > > +static const s64 link_freq_menu_items[] = { > > s/link_freq_menu_items/gc08a3_link_freq_menu_items/ > > > +GC08A3_LINK_FREQ_336MHZ, > > +GC08A3_LINK_FREQ_207MHZ, > > +}; > > + > > +static const char *const gc08a3_supply_name[] = { > > +"avdd", > > +"dvdd", > > +"dovdd", > > +}; > > + > > +struct gc08a3 { > > +struct device *dev; > > +struct v4l2_subdev sd; > > +struct media_pad pad; > > +struct i2c_client *client; > > Not used, you can drop this field. > fixed in patch:v5. > > + > > +struct clk *xclk; > > +struct regulator_bulk_data > supplies[ARRAY_SIZE(gc08a3_supply_name)]; > > +struct gpio_desc *reset_gpio; > > + > > +struct v4l2_ctrl_handler ctrls; > > +struct v4l2_ctrl *pixel_rate; > > +struct v4l2_ctrl *link_freq; > > +struct v4l2_ctrl *exposure; > > +struct v4l2_ctrl *vblank; > > +struct v4l2_ctrl *hblank; > > + > > +struct regmap *regmap; > > + > > +bool streaming; > > Set but never used, you can drop this field. > fixed in patch:v5. > > +unsigned long link_freq_bitmap; > > + > > +/* Current mode */ > > +const struct gc08a3_mode *cur_mode; > > +}; > > + > > +struct gc08a3_reg_list { > > +u32 num_of_regs; > > +const struct cci_reg_sequence *regs; > > +}; > > + > > +struct gc08a3_link_freq_config { > > +const struct gc08a3_reg_list reg_list; > > +}; > > + > > +static const struct cci_reg_sequence mode_3264x2448[] = { > > +/* system */ > > +{ CCI_REG8(0x0336), 0x70 }, > > +{ CCI_REG8(0x0383), 0xbb }, > > +{ CCI_REG8(0x0344), 0x00 }, > > +{ CCI_REG8(0x0345), 0x06 }, > > +{ CCI_REG8(0x0346), 0x00 }, > > +{ CCI_REG8(0x0347), 0x04 }, > > +{ CCI_REG8(0x0348), 0x0c }, > > +{ CCI_REG8(0x0349), 0xd0 }, > > +{ CCI_REG8(0x034a), 0x09 }, > > +{ CCI_REG8(0x034b), 0x9c }, > > +{ CCI_REG8(0x0202), 0x09 }, > > +{ CCI_REG8(0x0203), 0x04 }, > > +{ CCI_REG8(0x0340), 0x09 }, > > +{ CCI_REG8(0x0341), 0xf4 }, > > +{ CCI_REG8(0x0342), 0x07 }, > > +{ CCI_REG8(0x0343), 0x1c }, > > + > > +{ CCI_REG8(0x0226), 0x00 }, > > +{ CCI_REG8(0x0227), 0x28 }, > > +{ CCI_REG8(0x0e38), 0x49 }, > > +{ CCI_REG8(0x0210), 0x13 }, > > +{ CCI_REG8(0x0218), 0x00 }, > > +{ CCI_REG8(0x0241), 0x88 }, > > +{ CCI_REG8(0x0392), 0x60 }, > > + > > +/* ISP */ > > +{ CCI_REG8(0x00a2), 0x00 }, > > +{ CCI_REG8(0x00a3), 0x00 }, > > +{ CCI_REG8(0x00ab), 0x00 }, > > +{ CCI_REG8(0x00ac), 0x00 }, > > + > > +/* GAIN */ > > +{ CCI_REG8(0x0204), 0x04 }, > > +{ CCI_REG8(0x0205), 0x00 }, > > +{ CCI_REG8(0x0050), 0x5c }, > > +{ CCI_REG8(0x0051), 0x44 }, > > + > > +/* out window */ > > +{ CCI_REG8(0x009a), 0x66 }, > > +{ CCI_REG8(0x0351), 0x00 }, > > +{ CCI_REG8(0x0352), 0x06 }, > > +{ CCI_REG8(0x0353), 0x00 }, > > +{ CCI_REG8(0x0354), 0x08 }, > > +{ CCI_REG8(0x034c), 0x0c }, > > +{ CCI_REG8(0x034d), 0xc0 }, > > +{ CCI_REG8(0x034e), 0x09 }, > > +{ CCI_REG8(0x034f), 0x90 }, > > + > > +/* MIPI */ > > +{ CCI_REG8(0x0114), 0x03 }, > > +{ CCI_REG8(0x0180), 0x65 }, > > +{ CCI_REG8(0x0181), 0xf0 }, > > +{ CCI_REG8(0x0185), 0x01 }, > > +{ CCI_REG8(0x0115), 0x30 }, > > +{ CCI_REG8(0x011b), 0x12 }, > > +{ CCI_REG8(0x011c), 0x12 }, > > +{ CCI_REG8(0x0121), 0x06 }, > > +{ CCI_REG8(0x0122), 0x06 }, > > +{ CCI_REG8(0x0123), 0x15 }, > > +{ CCI_REG8(0x0124), 0x01 }, > > +{ CCI_REG8(0x0125), 0x0b }, > > +{ CCI_REG8(0x0126), 0x08 }, > > +{ CCI_REG8(0x0129), 0x06 }, > > +{ CCI_REG8(0x012a), 0x08 }, > > +{ CCI_REG8(0x012b), 0x08 }, > > + > > +{ CCI_REG8(0x0a73), 0x60 }, > > +{ CCI_REG8(0x0a70), 0x11 }, > > +{ CCI_REG8(0x0313), 0x80 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > Does this register really need to be written 8 times ? Same below. > fixed in patch:v5. > > +{ CCI_REG8(0x0a70), 0x00 }, > > +{ CCI_REG8(0x00a4), 0x80 }, > > +{ CCI_REG8(0x0316), 0x01 }, > > +{ CCI_REG8(0x0a67), 0x00 }, > > +{ CCI_REG8(0x0084), 0x10 }, > > +{ CCI_REG8(0x0102), 0x09 }, > > +}; > > + > > +static const struct cci_reg_sequence mode_1920x1080[] = { > > +/* system */ > > +{ CCI_REG8(0x0336), 0x45 }, > > +{ CCI_REG8(0x0383), 0x8b }, > > +{ CCI_REG8(0x0344), 0x02 }, > > +{ CCI_REG8(0x0345), 0xa6 }, > > +{ CCI_REG8(0x0346), 0x02 }, > > +{ CCI_REG8(0x0347), 0xb0 }, > > +{ CCI_REG8(0x0348), 0x07 }, > > +{ CCI_REG8(0x0349), 0x90 }, > > +{ CCI_REG8(0x034a), 0x04 }, > > +{ CCI_REG8(0x034b), 0x44 }, > > +{ CCI_REG8(0x0202), 0x03 }, > > +{ CCI_REG8(0x0203), 0x00 }, > > +{ CCI_REG8(0x0340), 0x04 }, > > +{ CCI_REG8(0x0341), 0xfc }, > > +{ CCI_REG8(0x0342), 0x07 }, > > +{ CCI_REG8(0x0343), 0x1c }, > > +{ CCI_REG8(0x0226), 0x00 }, > > +{ CCI_REG8(0x0227), 0x88 }, > > +{ CCI_REG8(0x0e38), 0x49 }, > > +{ CCI_REG8(0x0210), 0x13 }, > > +{ CCI_REG8(0x0218), 0x00 }, > > +{ CCI_REG8(0x0241), 0x88 }, > > +{ CCI_REG8(0x0392), 0x60 }, > > + > > +/* ISP */ > > +{ CCI_REG8(0x00a2), 0xac }, > > +{ CCI_REG8(0x00a3), 0x02 }, > > +{ CCI_REG8(0x00ab), 0xa0 }, > > +{ CCI_REG8(0x00ac), 0x02 }, > > + > > +/* GAIN */ > > +{ CCI_REG8(0x0204), 0x04 }, > > +{ CCI_REG8(0x0205), 0x00 }, > > +{ CCI_REG8(0x0050), 0x38 }, > > +{ CCI_REG8(0x0051), 0x20 }, > > + > > +/* out window */ > > +{ CCI_REG8(0x009a), 0x66 }, > > +{ CCI_REG8(0x0351), 0x00 }, > > +{ CCI_REG8(0x0352), 0x06 }, > > +{ CCI_REG8(0x0353), 0x00 }, > > +{ CCI_REG8(0x0354), 0x08 }, > > +{ CCI_REG8(0x034c), 0x07 }, > > +{ CCI_REG8(0x034d), 0x80 }, > > +{ CCI_REG8(0x034e), 0x04 }, > > +{ CCI_REG8(0x034f), 0x38 }, > > + > > +/* MIPI */ > > +{ CCI_REG8(0x0114), 0x03 }, > > +{ CCI_REG8(0x0180), 0x65 }, > > +{ CCI_REG8(0x0181), 0xf0 }, > > +{ CCI_REG8(0x0185), 0x01 }, > > +{ CCI_REG8(0x0115), 0x30 }, > > +{ CCI_REG8(0x011b), 0x12 }, > > +{ CCI_REG8(0x011c), 0x12 }, > > +{ CCI_REG8(0x0121), 0x02 }, > > +{ CCI_REG8(0x0122), 0x03 }, > > +{ CCI_REG8(0x0123), 0x0c }, > > +{ CCI_REG8(0x0124), 0x00 }, > > +{ CCI_REG8(0x0125), 0x09 }, > > +{ CCI_REG8(0x0126), 0x06 }, > > +{ CCI_REG8(0x0129), 0x04 }, > > +{ CCI_REG8(0x012a), 0x03 }, > > +{ CCI_REG8(0x012b), 0x06 }, > > + > > +{ CCI_REG8(0x0a73), 0x60 }, > > +{ CCI_REG8(0x0a70), 0x11 }, > > +{ CCI_REG8(0x0313), 0x80 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0aff), 0x00 }, > > +{ CCI_REG8(0x0a70), 0x00 }, > > +{ CCI_REG8(0x00a4), 0x80 }, > > +{ CCI_REG8(0x0316), 0x01 }, > > +{ CCI_REG8(0x0a67), 0x00 }, > > +{ CCI_REG8(0x0084), 0x10 }, > > +{ CCI_REG8(0x0102), 0x09 }, > > +}; > > + > > +static const struct cci_reg_sequence mode_table_common[] = { > > +{ GC08A3_STRAEMING_REG, 0x00 }, > > +/* system */ > > +{ CCI_REG8(0x031c), 0x60 }, > > +{ CCI_REG8(0x0337), 0x04 }, > > +{ CCI_REG8(0x0335), 0x51 }, > > +{ CCI_REG8(0x0336), 0x70 }, > > +{ CCI_REG8(0x0383), 0xbb }, > > +{ CCI_REG8(0x031a), 0x00 }, > > +{ CCI_REG8(0x0321), 0x10 }, > > +{ CCI_REG8(0x0327), 0x03 }, > > +{ CCI_REG8(0x0325), 0x40 }, > > +{ CCI_REG8(0x0326), 0x23 }, > > +{ CCI_REG8(0x0314), 0x11 }, > > +{ CCI_REG8(0x0315), 0xd6 }, > > +{ CCI_REG8(0x0316), 0x01 }, > > +{ CCI_REG8(0x0334), 0x40 }, > > +{ CCI_REG8(0x0324), 0x42 }, > > +{ CCI_REG8(0x031c), 0x00 }, > > +{ CCI_REG8(0x031c), 0x9f }, > > +{ CCI_REG8(0x039a), 0x13 }, > > +{ CCI_REG8(0x0084), 0x30 }, > > +{ CCI_REG8(0x02b3), 0x08 }, > > +{ CCI_REG8(0x0057), 0x0c }, > > +{ CCI_REG8(0x05c3), 0x50 }, > > +{ CCI_REG8(0x0311), 0x90 }, > > +{ CCI_REG8(0x05a0), 0x02 }, > > +{ CCI_REG8(0x0074), 0x0a }, > > +{ CCI_REG8(0x0059), 0x11 }, > > +{ CCI_REG8(0x0070), 0x05 }, > > +{ CCI_REG8(0x0101), 0x00 }, > > + > > +/* analog */ > > +{ CCI_REG8(0x0344), 0x00 }, > > +{ CCI_REG8(0x0345), 0x06 }, > > +{ CCI_REG8(0x0346), 0x00 }, > > +{ CCI_REG8(0x0347), 0x04 }, > > +{ CCI_REG8(0x0348), 0x0c }, > > +{ CCI_REG8(0x0349), 0xd0 }, > > +{ CCI_REG8(0x034a), 0x09 }, > > +{ CCI_REG8(0x034b), 0x9c }, > > +{ CCI_REG8(0x0202), 0x09 }, > > +{ CCI_REG8(0x0203), 0x04 }, > > + > > +{ CCI_REG8(0x0219), 0x05 }, > > +{ CCI_REG8(0x0226), 0x00 }, > > +{ CCI_REG8(0x0227), 0x28 }, > > +{ CCI_REG8(0x0e0a), 0x00 }, > > +{ CCI_REG8(0x0e0b), 0x00 }, > > +{ CCI_REG8(0x0e24), 0x04 }, > > +{ CCI_REG8(0x0e25), 0x04 }, > > +{ CCI_REG8(0x0e26), 0x00 }, > > +{ CCI_REG8(0x0e27), 0x10 }, > > +{ CCI_REG8(0x0e01), 0x74 }, > > +{ CCI_REG8(0x0e03), 0x47 }, > > +{ CCI_REG8(0x0e04), 0x33 }, > > +{ CCI_REG8(0x0e05), 0x44 }, > > +{ CCI_REG8(0x0e06), 0x44 }, > > +{ CCI_REG8(0x0e0c), 0x1e }, > > +{ CCI_REG8(0x0e17), 0x3a }, > > +{ CCI_REG8(0x0e18), 0x3c }, > > +{ CCI_REG8(0x0e19), 0x40 }, > > +{ CCI_REG8(0x0e1a), 0x42 }, > > +{ CCI_REG8(0x0e28), 0x21 }, > > +{ CCI_REG8(0x0e2b), 0x68 }, > > +{ CCI_REG8(0x0e2c), 0x0d }, > > +{ CCI_REG8(0x0e2d), 0x08 }, > > +{ CCI_REG8(0x0e34), 0xf4 }, > > +{ CCI_REG8(0x0e35), 0x44 }, > > +{ CCI_REG8(0x0e36), 0x07 }, > > +{ CCI_REG8(0x0e38), 0x49 }, > > +{ CCI_REG8(0x0210), 0x13 }, > > +{ CCI_REG8(0x0218), 0x00 }, > > +{ CCI_REG8(0x0241), 0x88 }, > > +{ CCI_REG8(0x0e32), 0x00 }, > > +{ CCI_REG8(0x0e33), 0x18 }, > > +{ CCI_REG8(0x0e42), 0x03 }, > > +{ CCI_REG8(0x0e43), 0x80 }, > > +{ CCI_REG8(0x0e44), 0x04 }, > > +{ CCI_REG8(0x0e45), 0x00 }, > > +{ CCI_REG8(0x0e4f), 0x04 }, > > +{ CCI_REG8(0x057a), 0x20 }, > > +{ CCI_REG8(0x0381), 0x7c }, > > +{ CCI_REG8(0x0382), 0x9b }, > > +{ CCI_REG8(0x0384), 0xfb }, > > +{ CCI_REG8(0x0389), 0x38 }, > > +{ CCI_REG8(0x038a), 0x03 }, > > +{ CCI_REG8(0x0390), 0x6a }, > > +{ CCI_REG8(0x0391), 0x0b }, > > +{ CCI_REG8(0x0392), 0x60 }, > > +{ CCI_REG8(0x0393), 0xc1 }, > > +{ CCI_REG8(0x0396), 0xff }, > > +{ CCI_REG8(0x0398), 0x62 }, > > + > > +/* cisctl reset */ > > +{ CCI_REG8(0x031c), 0x80 }, > > +{ CCI_REG8(0x03fe), 0x10 }, > > +{ CCI_REG8(0x03fe), 0x00 }, > > +{ CCI_REG8(0x031c), 0x9f }, > > +{ CCI_REG8(0x03fe), 0x00 }, > > +{ CCI_REG8(0x03fe), 0x00 }, > > +{ CCI_REG8(0x03fe), 0x00 }, > > +{ CCI_REG8(0x03fe), 0x00 }, > > +{ CCI_REG8(0x031c), 0x80 }, > > +{ CCI_REG8(0x03fe), 0x10 }, > > +{ CCI_REG8(0x03fe), 0x00 }, > > +{ CCI_REG8(0x031c), 0x9f }, > > +{ CCI_REG8(0x0360), 0x01 }, > > +{ CCI_REG8(0x0360), 0x00 }, > > +{ CCI_REG8(0x0316), 0x09 }, > > +{ CCI_REG8(0x0a67), 0x80 }, > > +{ CCI_REG8(0x0313), 0x00 }, > > +{ CCI_REG8(0x0a53), 0x0e }, > > +{ CCI_REG8(0x0a65), 0x17 }, > > +{ CCI_REG8(0x0a68), 0xa1 }, > > +{ CCI_REG8(0x0a58), 0x00 }, > > +{ CCI_REG8(0x0ace), 0x0c }, > > +{ CCI_REG8(0x00a4), 0x00 }, > > +{ CCI_REG8(0x00a5), 0x01 }, > > +{ CCI_REG8(0x00a7), 0x09 }, > > +{ CCI_REG8(0x00a8), 0x9c }, > > +{ CCI_REG8(0x00a9), 0x0c }, > > +{ CCI_REG8(0x00aa), 0xd0 }, > > +{ CCI_REG8(0x0a8a), 0x00 }, > > +{ CCI_REG8(0x0a8b), 0xe0 }, > > +{ CCI_REG8(0x0a8c), 0x13 }, > > +{ CCI_REG8(0x0a8d), 0xe8 }, > > +{ CCI_REG8(0x0a90), 0x0a }, > > +{ CCI_REG8(0x0a91), 0x10 }, > > +{ CCI_REG8(0x0a92), 0xf8 }, > > +{ CCI_REG8(0x0a71), 0xf2 }, > > +{ CCI_REG8(0x0a72), 0x12 }, > > +{ CCI_REG8(0x0a73), 0x64 }, > > +{ CCI_REG8(0x0a75), 0x41 }, > > +{ CCI_REG8(0x0a70), 0x07 }, > > +{ CCI_REG8(0x0313), 0x80 }, > > + > > +/* ISP */ > > +{ CCI_REG8(0x00a0), 0x01 }, > > +{ CCI_REG8(0x0080), 0xd2 }, > > +{ CCI_REG8(0x0081), 0x3f }, > > +{ CCI_REG8(0x0087), 0x51 }, > > +{ CCI_REG8(0x0089), 0x03 }, > > +{ CCI_REG8(0x009b), 0x40 }, > > +{ CCI_REG8(0x05a0), 0x82 }, > > +{ CCI_REG8(0x05ac), 0x00 }, > > +{ CCI_REG8(0x05ad), 0x01 }, > > +{ CCI_REG8(0x05ae), 0x00 }, > > +{ CCI_REG8(0x0800), 0x0a }, > > +{ CCI_REG8(0x0801), 0x14 }, > > +{ CCI_REG8(0x0802), 0x28 }, > > +{ CCI_REG8(0x0803), 0x34 }, > > +{ CCI_REG8(0x0804), 0x0e }, > > +{ CCI_REG8(0x0805), 0x33 }, > > +{ CCI_REG8(0x0806), 0x03 }, > > +{ CCI_REG8(0x0807), 0x8a }, > > +{ CCI_REG8(0x0808), 0x50 }, > > +{ CCI_REG8(0x0809), 0x00 }, > > +{ CCI_REG8(0x080a), 0x34 }, > > +{ CCI_REG8(0x080b), 0x03 }, > > +{ CCI_REG8(0x080c), 0x26 }, > > +{ CCI_REG8(0x080d), 0x03 }, > > +{ CCI_REG8(0x080e), 0x18 }, > > +{ CCI_REG8(0x080f), 0x03 }, > > +{ CCI_REG8(0x0810), 0x10 }, > > +{ CCI_REG8(0x0811), 0x03 }, > > +{ CCI_REG8(0x0812), 0x00 }, > > +{ CCI_REG8(0x0813), 0x00 }, > > +{ CCI_REG8(0x0814), 0x01 }, > > +{ CCI_REG8(0x0815), 0x00 }, > > +{ CCI_REG8(0x0816), 0x01 }, > > +{ CCI_REG8(0x0817), 0x00 }, > > +{ CCI_REG8(0x0818), 0x00 }, > > +{ CCI_REG8(0x0819), 0x0a }, > > +{ CCI_REG8(0x081a), 0x01 }, > > +{ CCI_REG8(0x081b), 0x6c }, > > +{ CCI_REG8(0x081c), 0x00 }, > > +{ CCI_REG8(0x081d), 0x0b }, > > +{ CCI_REG8(0x081e), 0x02 }, > > +{ CCI_REG8(0x081f), 0x00 }, > > +{ CCI_REG8(0x0820), 0x00 }, > > +{ CCI_REG8(0x0821), 0x0c }, > > +{ CCI_REG8(0x0822), 0x02 }, > > +{ CCI_REG8(0x0823), 0xd9 }, > > +{ CCI_REG8(0x0824), 0x00 }, > > +{ CCI_REG8(0x0825), 0x0d }, > > +{ CCI_REG8(0x0826), 0x03 }, > > +{ CCI_REG8(0x0827), 0xf0 }, > > +{ CCI_REG8(0x0828), 0x00 }, > > +{ CCI_REG8(0x0829), 0x0e }, > > +{ CCI_REG8(0x082a), 0x05 }, > > +{ CCI_REG8(0x082b), 0x94 }, > > +{ CCI_REG8(0x082c), 0x09 }, > > +{ CCI_REG8(0x082d), 0x6e }, > > +{ CCI_REG8(0x082e), 0x07 }, > > +{ CCI_REG8(0x082f), 0xe6 }, > > +{ CCI_REG8(0x0830), 0x10 }, > > +{ CCI_REG8(0x0831), 0x0e }, > > +{ CCI_REG8(0x0832), 0x0b }, > > +{ CCI_REG8(0x0833), 0x2c }, > > +{ CCI_REG8(0x0834), 0x14 }, > > +{ CCI_REG8(0x0835), 0xae }, > > +{ CCI_REG8(0x0836), 0x0f }, > > +{ CCI_REG8(0x0837), 0xc4 }, > > +{ CCI_REG8(0x0838), 0x18 }, > > +{ CCI_REG8(0x0839), 0x0e }, > > +{ CCI_REG8(0x05ac), 0x01 }, > > +{ CCI_REG8(0x059a), 0x00 }, > > +{ CCI_REG8(0x059b), 0x00 }, > > +{ CCI_REG8(0x059c), 0x01 }, > > +{ CCI_REG8(0x0598), 0x00 }, > > +{ CCI_REG8(0x0597), 0x14 }, > > +{ CCI_REG8(0x05ab), 0x09 }, > > +{ CCI_REG8(0x05a4), 0x02 }, > > +{ CCI_REG8(0x05a3), 0x05 }, > > +{ CCI_REG8(0x05a0), 0xc2 }, > > +{ CCI_REG8(0x0207), 0xc4 }, > > + > > +/* GAIN */ > > +{ CCI_REG8(0x0208), 0x01 }, > > +{ CCI_REG8(0x0209), 0x72 }, > > +{ CCI_REG8(0x0204), 0x04 }, > > +{ CCI_REG8(0x0205), 0x00 }, > > + > > +{ CCI_REG8(0x0040), 0x22 }, > > +{ CCI_REG8(0x0041), 0x20 }, > > +{ CCI_REG8(0x0043), 0x10 }, > > +{ CCI_REG8(0x0044), 0x00 }, > > +{ CCI_REG8(0x0046), 0x08 }, > > +{ CCI_REG8(0x0047), 0xf0 }, > > +{ CCI_REG8(0x0048), 0x0f }, > > +{ CCI_REG8(0x004b), 0x0f }, > > +{ CCI_REG8(0x004c), 0x00 }, > > +{ CCI_REG8(0x0050), 0x5c }, > > +{ CCI_REG8(0x0051), 0x44 }, > > +{ CCI_REG8(0x005b), 0x03 }, > > +{ CCI_REG8(0x00c0), 0x00 }, > > +{ CCI_REG8(0x00c1), 0x80 }, > > +{ CCI_REG8(0x00c2), 0x31 }, > > +{ CCI_REG8(0x00c3), 0x00 }, > > +{ CCI_REG8(0x0460), 0x04 }, > > +{ CCI_REG8(0x0462), 0x08 }, > > +{ CCI_REG8(0x0464), 0x0e }, > > +{ CCI_REG8(0x0466), 0x0a }, > > +{ CCI_REG8(0x0468), 0x12 }, > > +{ CCI_REG8(0x046a), 0x12 }, > > +{ CCI_REG8(0x046c), 0x10 }, > > +{ CCI_REG8(0x046e), 0x0c }, > > +{ CCI_REG8(0x0461), 0x03 }, > > +{ CCI_REG8(0x0463), 0x03 }, > > +{ CCI_REG8(0x0465), 0x03 }, > > +{ CCI_REG8(0x0467), 0x03 }, > > +{ CCI_REG8(0x0469), 0x04 }, > > +{ CCI_REG8(0x046b), 0x04 }, > > +{ CCI_REG8(0x046d), 0x04 }, > > +{ CCI_REG8(0x046f), 0x04 }, > > +{ CCI_REG8(0x0470), 0x04 }, > > +{ CCI_REG8(0x0472), 0x10 }, > > +{ CCI_REG8(0x0474), 0x26 }, > > +{ CCI_REG8(0x0476), 0x38 }, > > +{ CCI_REG8(0x0478), 0x20 }, > > +{ CCI_REG8(0x047a), 0x30 }, > > +{ CCI_REG8(0x047c), 0x38 }, > > +{ CCI_REG8(0x047e), 0x60 }, > > +{ CCI_REG8(0x0471), 0x05 }, > > +{ CCI_REG8(0x0473), 0x05 }, > > +{ CCI_REG8(0x0475), 0x05 }, > > +{ CCI_REG8(0x0477), 0x05 }, > > +{ CCI_REG8(0x0479), 0x04 }, > > +{ CCI_REG8(0x047b), 0x04 }, > > +{ CCI_REG8(0x047d), 0x04 }, > > +{ CCI_REG8(0x047f), 0x04 }, > > +}; > > + > > +static const struct gc08a3_link_freq_config > gc08a3_link_freq_336m_configs = { > > +.reg_list = { > > +.num_of_regs = ARRAY_SIZE(mode_table_common), > > +.regs = mode_table_common, > > +} > > +}; > > + > > +static const struct gc08a3_link_freq_config > gc08a3_link_freq_207m_configs = { > > +.reg_list = { > > +.num_of_regs = ARRAY_SIZE(mode_table_common), > > +.regs = mode_table_common, > > +} > > +}; > > The registers are the same for both, so I think you can simplify this > and drop the gc08a3_link_freq_config structure. > fixed in patch:v5. > > + > > +struct gc08a3_mode { > > +u32 width; > > +u32 height; > > +const struct gc08a3_reg_list reg_list; > > + > > +u32 hts; /* Horizontal timining size */ > > +u32 vts_def; /* Default vertical timining size */ > > +u32 vts_min; /* Min vertical timining size */ > > +u32 max_framerate; > > +const struct gc08a3_link_freq_config *link_freq_configs; > > +}; > > + > > +/* > > + * Declare modes in order, from biggest > > + * to smallest height. > > + */ > > +static const struct gc08a3_mode gc08a3_modes[] = { > > +{ > > +.width = GC08A3_NATIVE_WIDTH, > > +.height = GC08A3_NATIVE_HEIGHT, > > +.reg_list = { > > +.num_of_regs = ARRAY_SIZE(mode_3264x2448), > > +.regs = mode_3264x2448, > > +}, > > +.link_freq_configs = &gc08a3_link_freq_336m_configs, > > + > > +.hts = GC08A3_HTS_30FPS, > > +.vts_def = GC08A3_VTS_30FPS, > > +.vts_min = GC08A3_VTS_30FPS_MIN, > > +.max_framerate = 300, > > +}, > > +{ > > +.width = 1920, > > +.height = 1080, > > +.reg_list = { > > +.num_of_regs = ARRAY_SIZE(mode_1920x1080), > > +.regs = mode_1920x1080, > > +}, > > +.link_freq_configs = &gc08a3_link_freq_207m_configs, > > + > > +.hts = GC08A3_HTS_60FPS, > > +.vts_def = GC08A3_VTS_60FPS, > > +.vts_min = GC08A3_VTS_60FPS_MIN, > > +.max_framerate = 600, > > +}, > > +}; > > + > > +static u64 to_pixel_rate(u32 f_index) > > Move this function just before gc08a3_init_controls() where it is > used, > and rename it with a gc08a3_ prefix. > fixed in patch:v5. > > +{ > > +u64 pixel_rate = link_freq_menu_items[f_index] * 2 * > GC08A3_DATA_LANES; > > + > > +do_div(pixel_rate, GC08A3_RGB_DEPTH); > > + > > +return pixel_rate; > > +} > > + > > +static int gc08a3_identify_module(struct gc08a3 *gc08a3) > > I would also move this down, before the probe function. > fixed in patch:v5. > > +{ > > +struct i2c_client *client = v4l2_get_subdevdata(&gc08a3->sd); > > +u64 val = 0; > > No need to initialize val to 0. > fixed in patch:v5. > > +int ret; > > + > > +ret = cci_read(gc08a3->regmap, GC08A3_REG_CHIP_ID, &val, NULL); > > +if (ret) { > > +dev_err(&client->dev, > > Use gc08a3->dev and drop the local client variable. > fixed in patch:v5. > > +"failed to read chip id: 0x%x", GC08A3_CHIP_ID); > > "failed to read chip id\n"); > > Printing GC08A3_CHIP_ID isn't useful. > fixed in patch:v5. > > +return ret; > > +} > > + > > +if (val != GC08A3_CHIP_ID) { > > +dev_err(&client->dev, "chip id mismatch: 0x%x!=0x%llx", > > +GC08A3_CHIP_ID, val); > > +return -ENXIO; > > +} > > + > > +return 0; > > +} > > + > > +static inline struct gc08a3 *to_gc08a3(struct v4l2_subdev *sd) > > +{ > > +return container_of(sd, struct gc08a3, sd); > > +} > > + > > +static int gc08a3_power_on(struct device *dev) > > +{ > > +struct i2c_client *client = to_i2c_client(dev); > > +struct v4l2_subdev *sd = i2c_get_clientdata(client); > > You can use > > struct v4l2_subdev *sd = dev_get_drvdata(dev); > > Same below. > fixed in patch:v5. > > +struct gc08a3 *gc08a3 = to_gc08a3(sd); > > +int ret; > > + > > +ret = regulator_bulk_enable(ARRAY_SIZE(gc08a3_supply_name), > > + gc08a3->supplies); > > +if (ret < 0) { > > +dev_err(gc08a3->dev, "failed to enable regulators: %d\n", ret); > > +return ret; > > +} > > + > > +ret = clk_prepare_enable(gc08a3->xclk); > > +if (ret < 0) { > > +regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name), > > + gc08a3->supplies); > > +dev_err(gc08a3->dev, "clk prepare enable failed\n"); > > +return ret; > > +} > > + > > +usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > > fsleep() is a good candidate to replace the usleep_range() calls. > fixed in patch:v5. > > + > > +gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > Are you asserting reset when powering on ? That sounds wrong, you > should > de-assert reset here (and acquire the reset gpio in probe() with > GPIOD_OUT_HIGH). Drivers should use logical levels for GPIOs, setting > a > GPIO named "reset" to 1 should assert the reset signal, even if the > physical signal is active low. You may have the wrong polarity in the > device tree. > According to the sensor power sequence sepc, "reset" pin should be pull from low to high after "dovdd/dvdd/avdd" power on, so I follow this power sequece to pull "reset" pin high in software flow. > > +usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > > + > > +return 0; > > +} > > + > > +static int gc08a3_power_off(struct device *dev) > > +{ > > +struct i2c_client *client = to_i2c_client(dev); > > +struct v4l2_subdev *sd = i2c_get_clientdata(client); > > +struct gc08a3 *gc08a3 = to_gc08a3(sd); > > + > > +clk_disable_unprepare(gc08a3->xclk); > > + > > +usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > > + > > +gpiod_set_value_cansleep(gc08a3->reset_gpio, 0); > > +usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > > + > > +regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name), > > + gc08a3->supplies); > > +usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > > Do you need all those sleeps, especially the last one ? > fixed in patch:v5. > > + > > +return 0; > > +} > > + > > +static int gc08a3_enum_mbus_code(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *sd_state, > > + struct v4l2_subdev_mbus_code_enum *code) > > +{ > > +if (code->index > 0) > > +return -EINVAL; > > + > > +code->code = GC08A3_MBUS_CODE; > > + > > +return 0; > > +} > > + > > +static int gc08a3_enum_frame_size(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_state *sd_state, > > + struct v4l2_subdev_frame_size_enum *fse) > > +{ > > +if (fse->code != GC08A3_MBUS_CODE) > > +return -EINVAL; > > + > > +if (fse->index >= ARRAY_SIZE(gc08a3_modes)) > > +return -EINVAL; > > + > > +fse->min_width = gc08a3_modes[fse->index].width; > > +fse->max_width = gc08a3_modes[fse->index].width; > > +fse->min_height = gc08a3_modes[fse->index].height; > > +fse->max_height = gc08a3_modes[fse->index].height; > > + > > +return 0; > > +} > > + > > +static int gc08a3_update_cur_mode_controls(struct gc08a3 *gc08a3, > > + const struct gc08a3_mode *mode) > > +{ > > +s64 exposure_max, h_blank; > > +int ret = 0; > > + > > +ret = __v4l2_ctrl_modify_range(gc08a3->vblank, > > + mode->vts_min - mode->height, > > + GC08A3_VTS_MAX - mode->height, 1, > > + mode->vts_def - mode->height); > > +if (ret) > > +dev_err(gc08a3->dev, "VB ctrl range update failed\n"); > > return ret; > } > > Same below. > fixed in patch:v5. > > + > > +h_blank = mode->hts - mode->width; > > +ret = __v4l2_ctrl_modify_range(gc08a3->hblank, h_blank, h_blank, > 1, > > + h_blank); > > +if (ret) > > +dev_err(gc08a3->dev, "HB ctrl range update failed\n"); > > + > > +exposure_max = mode->vts_def - GC08A3_EXP_MARGIN; > > +ret = __v4l2_ctrl_modify_range(gc08a3->exposure, GC08A3_EXP_MIN, > > + exposure_max, GC08A3_EXP_STEP, > > + exposure_max); > > +if (ret) > > +dev_err(gc08a3->dev, "exposure ctrl range update failed\n"); > > + > > +return ret; > > +} > > + > > +static void gc08a3_update_pad_format(struct gc08a3 *gc08a3, > > + const struct gc08a3_mode *mode, > > + struct v4l2_mbus_framefmt *fmt) > > +{ > > +fmt->width = mode->width; > > +fmt->height = mode->height; > > +fmt->code = GC08A3_MBUS_CODE; > > +fmt->field = V4L2_FIELD_NONE; > > +fmt->colorspace = V4L2_COLORSPACE_SRGB; > > It's a raw sensor, isn't V4L2_COLORSPACE_RAW more appropriate, or > does > the sensor really use the sRGB primaries ? > fixed in patch:v5. > > +fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace); > > +fmt->quantization = > > +V4L2_MAP_QUANTIZATION_DEFAULT(true, > > + fmt->colorspace, > > + fmt->ycbcr_enc); > > I think you can write > > fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; > > here, it's more explicit. > > > +fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace); > > Sensors usually produce linear output, V4L2_XFER_FUNC_NONE is likely > what you need here. > fixed in patch:v5. > > +} > > + > > +static int gc08a3_set_format(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state, > > + struct v4l2_subdev_format *fmt) > > +{ > > +struct gc08a3 *gc08a3 = to_gc08a3(sd); > > +struct v4l2_mbus_framefmt *mbus_fmt; > > +struct v4l2_rect *crop; > > +const struct gc08a3_mode *mode; > > + > > +mode = v4l2_find_nearest_size(gc08a3_modes, > ARRAY_SIZE(gc08a3_modes), > > + width, height, fmt->format.width, > > + fmt->format.height); > > + > > +/*update crop info to subdev state*/ > > Missing spaces after /* and before */. Same below. > fixed in patch:v5. > > +crop = v4l2_subdev_state_get_crop(state, 0); > > +crop->width = mode->width; > > +crop->height = mode->height; > > + > > +/*update fmt info to subdev state*/ > > +gc08a3_update_pad_format(gc08a3, mode, &fmt->format); > > +mbus_fmt = v4l2_subdev_state_get_format(state, 0); > > +*mbus_fmt = fmt->format; > > + > > +if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) > > +return 0; > > + > > +gc08a3->cur_mode = mode; > > +gc08a3_update_cur_mode_controls(gc08a3, mode); > > + > > +return 0; > > +} > > + > > +static int gc08a3_get_selection(struct v4l2_subdev *sd, > > +struct v4l2_subdev_state *state, > > +struct v4l2_subdev_selection *sel) > > +{ > > +struct gc08a3 *gc08a3 = to_gc08a3(sd); > > + > > +switch (sel->target) { > > +case V4L2_SEL_TGT_CROP: > > +sel->r = *v4l2_subdev_state_get_crop(state, 0); > > +break; > > +case V4L2_SEL_TGT_CROP_BOUNDS: > > +sel->r.top = 0; > > +sel->r.left = 0; > > +sel->r.width = GC08A3_NATIVE_WIDTH; > > +sel->r.height = GC08A3_NATIVE_HEIGHT; > > +break; > > +case V4L2_SEL_TGT_CROP_DEFAULT: > > +if (gc08a3->cur_mode->width == GC08A3_NATIVE_WIDTH) { > > +sel->r.top = 0; > > +sel->r.left = 0; > > +sel->r.width = gc08a3_modes[0].width; > > +sel->r.height = gc08a3_modes[0].height; > > +} else { > > +sel->r.top = 0; > > +sel->r.left = 0; > > +sel->r.width = gc08a3_modes[1].width; > > +sel->r.height = gc08a3_modes[1].height; > > +} > > You store the crop rectangle in the gc08a3_set_format() function, you > can simplify this with > > sel->r = *v4l2_subdev_state_get_crop(state, 0); > fixed in patch:v5. > > +break; > > +default: > > +return -EINVAL; > > +} > > + > > +return 0; > > +} > > + > > +static int gc08a3_init_state(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state) > > +{ > > +struct v4l2_subdev_format fmt = { > > +.which = V4L2_SUBDEV_FORMAT_TRY, > > +.pad = 0, > > +.format = { > > +.code = GC08A3_MBUS_CODE, > > +.width = gc08a3_modes[0].width, > > +.height = gc08a3_modes[0].height, > > +}, > > +}; > > + > > +gc08a3_set_format(sd, state, &fmt); > > + > > +return 0; > > +} > > + > > +static int gc08a3_set_ctrl_hflip(struct gc08a3 *gc08a3, u32 > ctrl_val) > > +{ > > +int ret; > > +u64 val; > > + > > +ret = cci_read(gc08a3->regmap, GC08A3_FLIP_REG, &val, NULL); > > +if (ret) { > > +dev_err(gc08a3->dev, "read hflip register failed: %d\n", ret); > > +return ret; > > +} > > + > > +val = ctrl_val ? (val | GC08A3_FLIP_H_MASK) : > > + (val & ~GC08A3_FLIP_H_MASK); > > +ret = cci_write(gc08a3->regmap, GC08A3_FLIP_REG, val, NULL); > > +if (ret < 0) > > +dev_err(gc08a3->dev, "Error %d\n", ret); > > You can use cci_update_bits() > > return cci_update_bits(gc08a3->regmap, GC08A3_FLIP_REG, > GC08A3_FLIP_H_MASK, > ctrl_val ? GC08A3_FLIP_H_MASK : 0, NULL); > > Same below. > fixed in patch:v5. > > + > > +return ret; > > +} > > + > > +static int gc08a3_set_ctrl_vflip(struct gc08a3 *gc08a3, u32 > ctrl_val) > > +{ > > +int ret; > > +u64 val; > > + > > +ret = cci_read(gc08a3->regmap, GC08A3_FLIP_REG, &val, NULL); > > +if (ret) { > > +dev_err(gc08a3->dev, "read vflip register failed: %d\n", ret); > > +return ret; > > +} > > + > > +val = ctrl_val ? (val | GC08A3_FLIP_V_MASK) : > > + (val & ~GC08A3_FLIP_V_MASK); > > +ret = cci_write(gc08a3->regmap, GC08A3_FLIP_REG, val, NULL); > > +if (ret < 0) > > +dev_err(gc08a3->dev, "Error %d\n", ret); > > + > > +return ret; > > +} > > + > > +static int gc08a3_test_pattern(struct gc08a3 *gc08a3, u32 > pattern_menu) > > +{ > > +u32 pattern = 0; > > +int ret; > > int ret = 0; > According to Mr.Angelo's comments, I modified this function in patch:v5. > > + > > +if (pattern_menu) { > > +switch (pattern_menu) { > > +case 1: > > +pattern = 0x00; > > +break; > > +case 2: > > +pattern = 0x10; > > +break; > > +case 3: > > +case 4: > > +case 5: > > +case 6: > > +case 7: > > +pattern = pattern_menu + 1; > > +break; > > +} > > + > > +ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN, > > +GC08A3_TEST_PATTERN_EN, NULL); > > +if (ret) > > +return ret; > > + > > +ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_IDX, > > +pattern, NULL); > > + > > cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN, > GC08A3_TEST_PATTERN_EN, &ret); > cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_IDX, > pattern, &ret); > > And unless the sensor requires writing the GC08A3_REG_TEST_PATTERN_EN > register first, I would write the test pattern index first, to avoid > generating one frame with the wrong pattern. > fixed in patch:v5. > > +} else { > > +ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN, > > +0x00, NULL); > > +} > > + > > +return ret; > > +} > > + > > +static int gc08a3_set_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > +struct gc08a3 *gc08a3 = > > +container_of(ctrl->handler, struct gc08a3, ctrls); > > +int ret = 0; > > +s64 exposure_max; > > + > > +if (ctrl->id == V4L2_CID_VBLANK) { > > +/* Update max exposure while meeting expected vblanking */ > > +exposure_max = gc08a3->cur_mode->height + ctrl->val - > > + GC08A3_EXP_MARGIN; > > Let's not use cur_mode here, but retrieve the height from the state. > This makes the driver implementation less dependent on the modes, > paving > the way to make the sensor fully configurable. You can look at the > imx219 driver for an example. > fixed in patch:v5. > > +__v4l2_ctrl_modify_range(gc08a3->exposure, > > + gc08a3->exposure->minimum, > > + exposure_max, gc08a3->exposure->step, > > + exposure_max); > > +} > > + > > +/* > > + * Applying V4L2 control value only happens > > + * when power is on for streaming > > + */ > > +if (!pm_runtime_get_if_in_use(gc08a3->dev)) > > +return 0; > > + > > +switch (ctrl->id) { > > +case V4L2_CID_EXPOSURE: > > +ret = cci_write(gc08a3->regmap, GC08A3_EXP_REG, > > +ctrl->val, NULL); > > +break; > > + > > +case V4L2_CID_ANALOGUE_GAIN: > > +ret = cci_write(gc08a3->regmap, GC08A3_AGAIN_REG, > > +ctrl->val, NULL); > > +break; > > + > > +case V4L2_CID_VBLANK: > > +ret = cci_write(gc08a3->regmap, GC08A3_FRAME_LENGTH_REG, > > +gc08a3->cur_mode->height + ctrl->val, NULL); > > +break; > > + > > +case V4L2_CID_HFLIP: > > +ret = gc08a3_set_ctrl_hflip(gc08a3, ctrl->val); > > +break; > > + > > +case V4L2_CID_VFLIP: > > +ret = gc08a3_set_ctrl_vflip(gc08a3, ctrl->val); > > +break; > > + > > +case V4L2_CID_TEST_PATTERN: > > +ret = gc08a3_test_pattern(gc08a3, ctrl->val); > > +break; > > + > > +default: > > +break; > > +} > > + > > +pm_runtime_put(gc08a3->dev); > > + > > +return ret; > > +} > > + > > +static const struct v4l2_ctrl_ops gc08a3_ctrl_ops = { > > +.s_ctrl = gc08a3_set_ctrl, > > +}; > > + > > +static int gc08a3_start_streaming(struct gc08a3 *gc08a3) > > +{ > > +const struct gc08a3_mode *mode; > > +const struct gc08a3_reg_list *reg_list; > > +const struct gc08a3_link_freq_config *link_freq_cfg; > > +int ret; > > + > > +ret = pm_runtime_resume_and_get(gc08a3->dev); > > +if (ret < 0) > > +return ret; > > + > > +link_freq_cfg = gc08a3->cur_mode->link_freq_configs; > > + > > +reg_list = &link_freq_cfg->reg_list; > > +ret = cci_multi_reg_write(gc08a3->regmap, > > + reg_list->regs, reg_list->num_of_regs, NULL); > > +if (ret) { > > +dev_err(gc08a3->dev, "could not sent common table %d\n", ret); > > The cci helpers already write error messages, I think you can drop > this > one, as well as similar messages below. > fixed in patch:v5. > > +goto err_rpm_put; > > +} > > + > > +mode = gc08a3->cur_mode; > > +reg_list = &mode->reg_list; > > +ret = cci_multi_reg_write(gc08a3->regmap, > > + reg_list->regs, reg_list->num_of_regs, NULL); > > +if (ret < 0) { > > +dev_err(gc08a3->dev, "could not sent mode table %d\n", ret); > > +goto err_rpm_put; > > +} > > + > > +ret = __v4l2_ctrl_handler_setup(&gc08a3->ctrls); > > +if (ret < 0) { > > +dev_err(gc08a3->dev, "could not sync v4l2 controls\n"); > > +goto err_rpm_put; > > +} > > + > > +ret = cci_write(gc08a3->regmap, GC08A3_STRAEMING_REG, 1, NULL); > > +if (ret < 0) { > > +dev_err(gc08a3->dev, "write STRAEMING_REG failed: %d\n", ret); > > +goto err_rpm_put; > > +} > > + > > +return 0; > > + > > +err_rpm_put: > > +pm_runtime_put(gc08a3->dev); > > +return ret; > > +} > > + > > +static int gc08a3_stop_streaming(struct gc08a3 *gc08a3) > > +{ > > +int ret; > > + > > +ret = cci_write(gc08a3->regmap, GC08A3_STRAEMING_REG, 0, NULL); > > +if (ret < 0) > > +dev_err(gc08a3->dev, "could not sent stop streaming %d\n", ret); > > + > > +pm_runtime_put(gc08a3->dev); > > +return ret; > > +} > > + > > +static int gc08a3_s_stream(struct v4l2_subdev *subdev, int enable) > > +{ > > +struct gc08a3 *gc08a3 = to_gc08a3(subdev); > > +struct v4l2_subdev_state *state; > > +int ret; > > + > > +state = v4l2_subdev_lock_and_get_active_state(subdev); > > + > > +if (enable) > > +ret = gc08a3_start_streaming(gc08a3); > > +else > > +ret = gc08a3_stop_streaming(gc08a3); > > + > > +gc08a3->streaming = enable; > > +v4l2_subdev_unlock_state(state); > > + > > +return ret; > > +} > > + > > +static int > > +gc08a3_enum_frame_interval(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_state *sd_state, > > + struct v4l2_subdev_frame_interval_enum *fie) > > Frame intervals on sensors should be controlled through the pixel > rate > and blanking. You can drop this function, as well as the > gc08a3_mode.max_framerate field. > fixed in patch:v5. > > +{ > > +unsigned int index = fie->index; > > +unsigned int i; > > + > > +for (i = 0; i < ARRAY_SIZE(gc08a3_modes); i++) { > > +if (fie->width != gc08a3_modes[i].width || > > + fie->height != gc08a3_modes[i].height) > > +continue; > > + > > +if (index-- == 0) { > > +fie->interval.numerator = 1; > > +fie->interval.denominator = > > +gc08a3_modes[i].max_framerate / 10; > > +return 0; > > +} > > +} > > + > > +return -EINVAL; > > +} > > + > > +static const struct v4l2_subdev_video_ops gc08a3_video_ops = { > > +.s_stream = gc08a3_s_stream, > > +}; > > + > > +static const struct v4l2_subdev_pad_ops gc08a3_subdev_pad_ops = { > > +.enum_mbus_code = gc08a3_enum_mbus_code, > > +.enum_frame_size = gc08a3_enum_frame_size, > > +.enum_frame_interval = gc08a3_enum_frame_interval, > > +.get_fmt = v4l2_subdev_get_fmt, > > +.set_fmt = gc08a3_set_format, > > +.get_selection = gc08a3_get_selection, > > +}; > > + > > +static const struct v4l2_subdev_core_ops gc08a3_core_ops = { > > +.subscribe_event = v4l2_ctrl_subdev_subscribe_event, > > +.unsubscribe_event = v4l2_event_subdev_unsubscribe, > > +}; > > + > > +static const struct v4l2_subdev_ops gc08a3_subdev_ops = { > > +.core = &gc08a3_core_ops, > > +.video = &gc08a3_video_ops, > > +.pad = &gc08a3_subdev_pad_ops, > > +}; > > + > > +static const struct v4l2_subdev_internal_ops gc08a3_internal_ops = > { > > +.init_state = gc08a3_init_state, > > +}; > > + > > +static int gc08a3_get_regulators(struct device *dev, struct gc08a3 > *gc08a3) > > +{ > > +unsigned int i; > > + > > +for (i = 0; i < ARRAY_SIZE(gc08a3_supply_name); i++) > > +gc08a3->supplies[i].supply = gc08a3_supply_name[i]; > > + > > +return devm_regulator_bulk_get(dev, > ARRAY_SIZE(gc08a3_supply_name), > > + gc08a3->supplies); > > +} > > + > > +static int gc08a3_parse_fwnode(struct device *dev, struct gc08a3 > *gc08a3) > > You can drop the first argument and use gc08a3->dev instead. > fixed in patch:v5. > > +{ > > +struct fwnode_handle *endpoint; > > +struct v4l2_fwnode_endpoint bus_cfg = { > > +.bus_type = V4L2_MBUS_CSI2_DPHY, > > +}; > > +int ret; > > + > > +endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL); > > Please use > > endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(gc08a3->dev), > 0, 0, > FWNODE_GRAPH_ENDPOINT_NEXT); > fixed in patch:v5. > > +if (!endpoint) { > > +dev_err(dev, "endpoint node not found\n"); > > +return -EINVAL; > > +} > > + > > +ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg); > > +if (ret) { > > +dev_err(dev, "parsing endpoint node failed\n"); > > +goto done; > > +} > > + > > +ret = v4l2_link_freq_to_bitmap(dev, bus_cfg.link_frequencies, > > + bus_cfg.nr_of_link_frequencies, > > + link_freq_menu_items, > > + ARRAY_SIZE(link_freq_menu_items), > > + &gc08a3->link_freq_bitmap); > > +if (ret) > > +goto done; > > + > > +done: > > +v4l2_fwnode_endpoint_free(&bus_cfg); > > +fwnode_handle_put(endpoint); > > +return ret; > > +} > > + > > +static int gc08a3_init_controls(struct gc08a3 *gc08a3) > > +{ > > +struct i2c_client *client = v4l2_get_subdevdata(&gc08a3->sd); > > +const struct gc08a3_mode *mode = &gc08a3_modes[0]; > > +const struct v4l2_ctrl_ops *ops = &gc08a3_ctrl_ops; > > +struct v4l2_fwnode_device_properties props; > > +struct v4l2_ctrl_handler *ctrl_hdlr; > > +s64 exposure_max, h_blank; > > +int ret; > > + > > +ctrl_hdlr = &gc08a3->ctrls; > > +ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10); > > +if (ret) > > +return ret; > > + > > +gc08a3->link_freq = > > +v4l2_ctrl_new_int_menu(ctrl_hdlr, > > + &gc08a3_ctrl_ops, > > + V4L2_CID_LINK_FREQ, > > + ARRAY_SIZE(link_freq_menu_items) - 1, > > + 0, link_freq_menu_items); > > +if (gc08a3->link_freq) > > +gc08a3->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > +gc08a3->pixel_rate = > > +v4l2_ctrl_new_std(ctrl_hdlr, > > + &gc08a3_ctrl_ops, > > + V4L2_CID_PIXEL_RATE, 0, > > + to_pixel_rate(GC08A3_LINK_FREQ_336MHZ_INDEX), > > + 1, > > + to_pixel_rate(GC08A3_LINK_FREQ_336MHZ_INDEX)); > > + > > +gc08a3->vblank = > > +v4l2_ctrl_new_std(ctrl_hdlr, > > + &gc08a3_ctrl_ops, V4L2_CID_VBLANK, > > + mode->vts_min - mode->height, > > + GC08A3_VTS_MAX - mode->height, 1, > > + mode->vts_def - mode->height); > > + > > +h_blank = mode->hts - mode->width; > > +gc08a3->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, > > + V4L2_CID_HBLANK, h_blank, h_blank, 1, > > + h_blank); > > +if (gc08a3->hblank) > > +gc08a3->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > +v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, > > + V4L2_CID_ANALOGUE_GAIN, GC08A3_AGAIN_MIN, > > + GC08A3_AGAIN_MAX, GC08A3_AGAIN_STEP, > > + GC08A3_AGAIN_MIN); > > + > > +exposure_max = mode->vts_def - GC08A3_EXP_MARGIN; > > +gc08a3->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, > > + V4L2_CID_EXPOSURE, GC08A3_EXP_MIN, > > + exposure_max, GC08A3_EXP_STEP, > > + exposure_max); > > + > > +v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &gc08a3_ctrl_ops, > > + V4L2_CID_TEST_PATTERN, > > + ARRAY_SIZE(gc08a3_test_pattern_menu) - 1, > > + 0, 0, gc08a3_test_pattern_menu); > > + > > +v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, V4L2_CID_HFLIP, 0, > > + 1, 1, 0); > > + > > +v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, V4L2_CID_VFLIP, 0, > > + 1, 1, 0); > > Let's put the HFLIP and VFLIP controls in a cluster to make it > possible > to efficiently set them in an atomic operation. See the imx219 driver > for an example. > fixed in patch:v5. > > + > > +/* register properties to fwnode (e.g. rotation, orientation) */ > > +ret = v4l2_fwnode_device_parse(&client->dev, &props); > > +if (ret) > > +goto error_ctrls; > > + > > +ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, ops, &props); > > +if (ret) > > +goto error_ctrls; > > + > > +if (ctrl_hdlr->error) { > > +ret = ctrl_hdlr->error; > > +goto error_ctrls; > > +} > > + > > +gc08a3->sd.ctrl_handler = ctrl_hdlr; > > + > > +return 0; > > + > > +error_ctrls: > > +v4l2_ctrl_handler_free(ctrl_hdlr); > > + > > +return ret; > > +} > > + > > +static int gc08a3_probe(struct i2c_client *client) > > +{ > > +struct device *dev = &client->dev; > > +struct gc08a3 *gc08a3; > > +int ret; > > + > > +gc08a3 = devm_kzalloc(dev, sizeof(*gc08a3), GFP_KERNEL); > > +if (!gc08a3) > > +return -ENOMEM; > > + > > +gc08a3->dev = dev; > > + > > +ret = gc08a3_parse_fwnode(dev, gc08a3); > > +if (ret) > > +return ret; > > + > > +gc08a3->regmap = devm_cci_regmap_init_i2c(client, 16); > > +if (IS_ERR(gc08a3->regmap)) > > +return dev_err_probe(dev, PTR_ERR(gc08a3->regmap), > > + "failed to init CCI\n"); > > + > > +gc08a3->xclk = devm_clk_get(dev, NULL); > > +if (IS_ERR(gc08a3->xclk)) > > +return dev_err_probe(dev, PTR_ERR(gc08a3->xclk), > > + "failed to get xclk\n"); > > + > > +ret = clk_set_rate(gc08a3->xclk, GC08A3_DEFAULT_CLK_FREQ); > > +if (ret) > > +return dev_err_probe(dev, ret, > > + "failed to set xclk frequency\n"); > > + > > +ret = gc08a3_get_regulators(dev, gc08a3); > > +if (ret < 0) > > +return dev_err_probe(dev, ret, > > + "failed to get regulators\n"); > > + > > +gc08a3->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > > +if (IS_ERR(gc08a3->reset_gpio)) > > +return dev_err_probe(dev, PTR_ERR(gc08a3->reset_gpio), > > + "failed to get gpio\n"); > > + > > +v4l2_i2c_subdev_init(&gc08a3->sd, client, &gc08a3_subdev_ops); > > +gc08a3->sd.internal_ops = &gc08a3_internal_ops; > > +gc08a3->cur_mode = &gc08a3_modes[0]; > > + > > +ret = gc08a3_power_on(gc08a3->dev); > > +if (ret) > > +return dev_err_probe(dev, ret, > > + "failed to sensor power on\n"); > > + > > +ret = gc08a3_identify_module(gc08a3); > > +if (ret) { > > +dev_err(&client->dev, "failed to find sensor: %d\n", ret); > > +goto err_power_off; > > +} > > + > > +ret = gc08a3_init_controls(gc08a3); > > +if (ret) { > > +dev_err(&client->dev, "failed to init controls: %d", ret); > > +goto err_power_off; > > +} > > + > > +gc08a3->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | > > + V4L2_SUBDEV_FL_HAS_EVENTS; > > +gc08a3->pad.flags = MEDIA_PAD_FL_SOURCE; > > +gc08a3->sd.dev = &client->dev; > > +gc08a3->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > > + > > +ret = media_entity_pads_init(&gc08a3->sd.entity, 1, &gc08a3->pad); > > +if (ret < 0) { > > +dev_err(dev, "could not register media entity\n"); > > +goto err_v4l2_ctrl_handler_free; > > +} > > + > > +gc08a3->sd.state_lock = gc08a3->ctrls.lock; > > +ret = v4l2_subdev_init_finalize(&gc08a3->sd); > > +if (ret < 0) { > > +dev_err(dev, "v4l2 subdev init error: %d\n", ret); > > +goto err_media_entity_cleanup; > > +} > > + > > +pm_runtime_set_active(gc08a3->dev); > > +pm_runtime_enable(gc08a3->dev); > > +pm_runtime_idle(gc08a3->dev); > > Could you enable autosuspend ? > fixed in patch:v5. > > + > > +ret = v4l2_async_register_subdev_sensor(&gc08a3->sd); > > +if (ret < 0) { > > +dev_err(dev, "could not register v4l2 device\n"); > > +goto err_rpm; > > +} > > + > > +return 0; > > + > > +err_rpm: > > +pm_runtime_disable(gc08a3->dev); > > +v4l2_subdev_cleanup(&gc08a3->sd); > > + > > +err_media_entity_cleanup: > > +media_entity_cleanup(&gc08a3->sd.entity); > > + > > +err_v4l2_ctrl_handler_free: > > +v4l2_ctrl_handler_free(gc08a3->sd.ctrl_handler); > > + > > +err_power_off: > > +gc08a3_power_off(gc08a3->dev); > > + > > +return ret; > > +} > > + > > +static void gc08a3_remove(struct i2c_client *client) > > +{ > > +struct v4l2_subdev *sd = i2c_get_clientdata(client); > > +struct gc08a3 *gc08a3 = to_gc08a3(sd); > > + > > +v4l2_async_unregister_subdev(&gc08a3->sd); > > +v4l2_subdev_cleanup(sd); > > +media_entity_cleanup(&gc08a3->sd.entity); > > +v4l2_ctrl_handler_free(&gc08a3->ctrls); > > + > > +pm_runtime_disable(&client->dev); > > +if (!pm_runtime_status_suspended(&client->dev)) > > +gc08a3_power_off(gc08a3->dev); > > +pm_runtime_set_suspended(&client->dev); > > +} > > + > > +static const struct of_device_id gc08a3_of_match[] = { > > +{ .compatible = "galaxycore,gc08a3" }, > > +{} > > +}; > > +MODULE_DEVICE_TABLE(of, gc08a3_of_match); > > + > > +static const struct dev_pm_ops gc08a3_pm_ops = { > > +SET_RUNTIME_PM_OPS(gc08a3_power_off, gc08a3_power_on, NULL) > > +}; > > + > > +static struct i2c_driver gc08a3_i2c_driver = { > > +.driver = { > > +.of_match_table = gc08a3_of_match, > > +.pm = &gc08a3_pm_ops, > > +.name = "gc08a3", > > +}, > > +.probe = gc08a3_probe, > > +.remove = gc08a3_remove, > > +}; > > + > > +module_i2c_driver(gc08a3_i2c_driver); > > + > > +MODULE_DESCRIPTION("GalaxyCore gc08a3 Camera driver"); > > +MODULE_AUTHOR("Zhi Mao <zhi.mao@mediatek.com>"); > > +MODULE_LICENSE("GPL"); > > -- > Regards, > > Laurent Pinchart
Hi AngeloGioacchino, Thanks for your review. On Wed, 2024-02-07 at 15:42 +0100, AngeloGioacchino Del Regno wrote: > Il 04/02/24 07:15, Zhi Mao ha scritto: > > Add a V4L2 sub-device driver for Galaxycore GC08A3 image sensor. > > > > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com> > > --- > > drivers/media/i2c/Kconfig | 10 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/gc08a3.c | 1448 > > ++++++++++++++++++++++++++++++++++++ > > 3 files changed, 1459 insertions(+) > > create mode 100644 drivers/media/i2c/gc08a3.c > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > index 56f276b920ab..e4da68835683 100644 > > --- a/drivers/media/i2c/Kconfig > > +++ b/drivers/media/i2c/Kconfig > > @@ -70,6 +70,16 @@ config VIDEO_GC0308 > > To compile this driver as a module, choose M here: the > > module will be called gc0308. > > > > +config VIDEO_GC08A3 > > + tristate "GalaxyCore gc08a3 sensor support" > > + select V4L2_CCI_I2C > > + help > > + This is a Video4Linux2 sensor driver for the GalaxyCore > > gc08a3 > > + camera. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called gc08a3. > > + > > config VIDEO_GC2145 > > select V4L2_CCI_I2C > > tristate "GalaxyCore GC2145 sensor support" > > diff --git a/drivers/media/i2c/Makefile > > b/drivers/media/i2c/Makefile > > index dfbe6448b549..b82e99ca7578 100644 > > --- a/drivers/media/i2c/Makefile > > +++ b/drivers/media/i2c/Makefile > > @@ -38,6 +38,7 @@ obj-$(CONFIG_VIDEO_DW9768) += dw9768.o > > obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o > > obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/ > > obj-$(CONFIG_VIDEO_GC0308) += gc0308.o > > +obj-$(CONFIG_VIDEO_GC08A3) += gc08a3.o > > obj-$(CONFIG_VIDEO_GC2145) += gc2145.o > > obj-$(CONFIG_VIDEO_HI556) += hi556.o > > obj-$(CONFIG_VIDEO_HI846) += hi846.o > > diff --git a/drivers/media/i2c/gc08a3.c > > b/drivers/media/i2c/gc08a3.c > > new file mode 100644 > > index 000000000000..3fc7fffb815c > > --- /dev/null > > +++ b/drivers/media/i2c/gc08a3.c > > @@ -0,0 +1,1448 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * gc08a3.c - gc08a3 sensor driver > > + * > > + * Copyright 2023 MediaTek > > + * > > + * Zhi Mao <zhi.mao@mediatek.com> > > + */ > > + > > ..snip.. > fixed in patch:v5. > > + > > +static const struct gc08a3_link_freq_config > > gc08a3_link_freq_336m_configs = { > > + .reg_list = { > > + .num_of_regs = ARRAY_SIZE(mode_table_common), > > + .regs = mode_table_common, > > + } > > +}; > > + > > +static const struct gc08a3_link_freq_config > > gc08a3_link_freq_207m_configs = { > > + .reg_list = { > > + .num_of_regs = ARRAY_SIZE(mode_table_common), > > + .regs = mode_table_common, > > + } > > +}; > > + > > Since you're documenting this structure anyway, why not kerneldoc? :- > ) > As these registers are the same, I followed Mr.Laurent's comments and droped this "gc08a3_link_freq_config" structure. > > +struct gc08a3_mode { > > + u32 width; > > + u32 height; > > + const struct gc08a3_reg_list reg_list; > > + > > + u32 hts; /* Horizontal timining size */ > > + u32 vts_def; /* Default vertical timining size */ > > + u32 vts_min; /* Min vertical timining size */ > > + u32 max_framerate; > > + const struct gc08a3_link_freq_config *link_freq_configs; > > +}; > > + > > +/* > > + * Declare modes in order, from biggest > > + * to smallest height. > > + */ > > one line is enough for this comment. > fixed in patch:v5. > > +static const struct gc08a3_mode gc08a3_modes[] = { > > + { > > + .width = GC08A3_NATIVE_WIDTH, > > + .height = GC08A3_NATIVE_HEIGHT, > > + .reg_list = { > > + .num_of_regs = ARRAY_SIZE(mode_3264x2448), > > + .regs = mode_3264x2448, > > + }, > > + .link_freq_configs = &gc08a3_link_freq_336m_configs, > > + > > + .hts = GC08A3_HTS_30FPS, > > + .vts_def = GC08A3_VTS_30FPS, > > + .vts_min = GC08A3_VTS_30FPS_MIN, > > + .max_framerate = 300, > > + }, > > + { > > + .width = 1920, > > + .height = 1080, > > + .reg_list = { > > + .num_of_regs = ARRAY_SIZE(mode_1920x1080), > > + .regs = mode_1920x1080, > > + }, > > + .link_freq_configs = &gc08a3_link_freq_207m_configs, > > + > > + .hts = GC08A3_HTS_60FPS, > > + .vts_def = GC08A3_VTS_60FPS, > > + .vts_min = GC08A3_VTS_60FPS_MIN, > > + .max_framerate = 600, > > + }, > > +}; > > + > > +static u64 to_pixel_rate(u32 f_index) > > +{ > > + u64 pixel_rate = link_freq_menu_items[f_index] * 2 * > > GC08A3_DATA_LANES; > > + > > + do_div(pixel_rate, GC08A3_RGB_DEPTH); > > The divisor is (less than) 32 bits and the dividend is always 64 > bits: that will > break on builds for 32-bits CPUs. > > Just do.... > > return div_u64(pixel_rate, GB08A3_RGB_DEPTH); > fixed in patch:v5. > > + > > + return pixel_rate; > > +} > > + > > +static int gc08a3_identify_module(struct gc08a3 *gc08a3) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&gc08a3->sd); > > + u64 val = 0; > > u64 val; > > > + int ret; > > + > > Either log here or in the probe function, otherwise it's just > redudant. > please review patch:v5. > > + ret = cci_read(gc08a3->regmap, GC08A3_REG_CHIP_ID, &val, NULL); > > + if (ret) { > > + dev_err(&client->dev, > > + "failed to read chip id: 0x%x", > > GC08A3_CHIP_ID); > > + return ret; > > + } > > + > > + if (val != GC08A3_CHIP_ID) { > > + dev_err(&client->dev, "chip id mismatch: 0x%x!=0x%llx", > > + GC08A3_CHIP_ID, val); > > + return -ENXIO; > > + } > > + > > + return 0; > > +} > > + > > +static inline struct gc08a3 *to_gc08a3(struct v4l2_subdev *sd) > > +{ > > + return container_of(sd, struct gc08a3, sd); > > +} > > + > > ..snip.. > fixed in patch:v5. > > + > > +static int gc08a3_update_cur_mode_controls(struct gc08a3 *gc08a3, > > + const struct gc08a3_mode > > *mode) > > +{ > > + s64 exposure_max, h_blank; > > + int ret = 0; > > + > > + ret = __v4l2_ctrl_modify_range(gc08a3->vblank, > > + mode->vts_min - mode->height, > > + GC08A3_VTS_MAX - mode->height, > > 1, > > + mode->vts_def - mode->height); > > + if (ret) > > + dev_err(gc08a3->dev, "VB ctrl range update failed\n"); > > + > > + h_blank = mode->hts - mode->width; > > + ret = __v4l2_ctrl_modify_range(gc08a3->hblank, h_blank, > > h_blank, 1, > > + h_blank); > > + if (ret) > > + dev_err(gc08a3->dev, "HB ctrl range update failed\n"); > > + > > + exposure_max = mode->vts_def - GC08A3_EXP_MARGIN; > > + ret = __v4l2_ctrl_modify_range(gc08a3->exposure, > > GC08A3_EXP_MIN, > > + exposure_max, GC08A3_EXP_STEP, > > + exposure_max); > > + if (ret) > > + dev_err(gc08a3->dev, "exposure ctrl range update > > failed\n"); > > No. You're not returning anywhere for error. That's not okay. > Besides... > > if (ret) { > dev_err.. > return ret; > } > > return 0; > fixed in patch:v5. > > + > > + return ret; > > +} > > + > > +static void gc08a3_update_pad_format(struct gc08a3 *gc08a3, > > + const struct gc08a3_mode *mode, > > + struct v4l2_mbus_framefmt *fmt) > > +{ > > + fmt->width = mode->width; > > + fmt->height = mode->height; > > + fmt->code = GC08A3_MBUS_CODE; > > + fmt->field = V4L2_FIELD_NONE; > > + fmt->colorspace = V4L2_COLORSPACE_SRGB; > > + fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace); > > + fmt->quantization = > > + V4L2_MAP_QUANTIZATION_DEFAULT(true, > > + fmt->colorspace, > > + fmt->ycbcr_enc); > > + fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace); > > +} > > + > > +static int gc08a3_set_format(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct gc08a3 *gc08a3 = to_gc08a3(sd); > > + struct v4l2_mbus_framefmt *mbus_fmt; > > + struct v4l2_rect *crop; > > + const struct gc08a3_mode *mode; > > + > > + mode = v4l2_find_nearest_size(gc08a3_modes, > > ARRAY_SIZE(gc08a3_modes), > > + width, height, fmt->format.width, > > + fmt->format.height); > > + > > + /*update crop info to subdev state*/ > > + crop = v4l2_subdev_state_get_crop(state, 0); > > + crop->width = mode->width; > > + crop->height = mode->height; > > + > > + /*update fmt info to subdev state*/ > > + gc08a3_update_pad_format(gc08a3, mode, &fmt->format); > > + mbus_fmt = v4l2_subdev_state_get_format(state, 0); > > + *mbus_fmt = fmt->format; > > + > > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) > > + return 0; > > + > > + gc08a3->cur_mode = mode; > > + gc08a3_update_cur_mode_controls(gc08a3, mode); > > + > > + return 0; > > +} > > + > > +static int gc08a3_get_selection(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state, > > + struct v4l2_subdev_selection *sel) > > +{ > > + struct gc08a3 *gc08a3 = to_gc08a3(sd); > > + > > + switch (sel->target) { > > + case V4L2_SEL_TGT_CROP: > > + sel->r = *v4l2_subdev_state_get_crop(state, 0); > > + break; > > + case V4L2_SEL_TGT_CROP_BOUNDS: > > + sel->r.top = 0; > > + sel->r.left = 0; > > + sel->r.width = GC08A3_NATIVE_WIDTH; > > + sel->r.height = GC08A3_NATIVE_HEIGHT; > > + break; > > + case V4L2_SEL_TGT_CROP_DEFAULT: > > + if (gc08a3->cur_mode->width == GC08A3_NATIVE_WIDTH) { > > + sel->r.top = 0; > > + sel->r.left = 0; > > + sel->r.width = gc08a3_modes[0].width; > > + sel->r.height = gc08a3_modes[0].height; > > + } else { > > + sel->r.top = 0; > > + sel->r.left = 0; > > + sel->r.width = gc08a3_modes[1].width; > > + sel->r.height = gc08a3_modes[1].height; > > + } > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +static int gc08a3_init_state(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state) > > +{ > > + struct v4l2_subdev_format fmt = { > > + .which = V4L2_SUBDEV_FORMAT_TRY, > > + .pad = 0, > > + .format = { > > + .code = GC08A3_MBUS_CODE, > > + .width = gc08a3_modes[0].width, > > + .height = gc08a3_modes[0].height, > > + }, > > + }; > > + > > + gc08a3_set_format(sd, state, &fmt); > > + > > + return 0; > > +} > > + > > +static int gc08a3_set_ctrl_hflip(struct gc08a3 *gc08a3, u32 > > ctrl_val) > > +{ > > + int ret; > > + u64 val; > > + > > + ret = cci_read(gc08a3->regmap, GC08A3_FLIP_REG, &val, NULL); > > + if (ret) { > > + dev_err(gc08a3->dev, "read hflip register failed: > > %d\n", ret); > > + return ret; > > + } > > + > > + val = ctrl_val ? (val | GC08A3_FLIP_H_MASK) : > > + (val & ~GC08A3_FLIP_H_MASK); > > + ret = cci_write(gc08a3->regmap, GC08A3_FLIP_REG, val, NULL); > > + if (ret < 0) > > + dev_err(gc08a3->dev, "Error %d\n", ret); > > + > > + return ret; > > +} > > + > > +static int gc08a3_set_ctrl_vflip(struct gc08a3 *gc08a3, u32 > > ctrl_val) > > +{ > > + int ret; > > + u64 val; > > + > > + ret = cci_read(gc08a3->regmap, GC08A3_FLIP_REG, &val, NULL); > > + if (ret) { > > + dev_err(gc08a3->dev, "read vflip register failed: > > %d\n", ret); > > + return ret; > > + } > > + > > + val = ctrl_val ? (val | GC08A3_FLIP_V_MASK) : > > + (val & ~GC08A3_FLIP_V_MASK); > > + ret = cci_write(gc08a3->regmap, GC08A3_FLIP_REG, val, NULL); > > + if (ret < 0) > > + dev_err(gc08a3->dev, "Error %d\n", ret); > > + > > + return ret; > > +} > > + > > +static int gc08a3_test_pattern(struct gc08a3 *gc08a3, u32 > > pattern_menu) > > +{ > > + u32 pattern = 0; > > + int ret; > > ret not initialized is ok here; > > > + > > + if (pattern_menu) { > > + switch (pattern_menu) { > > + case 1: > > + pattern = 0x00; > > + break; > > + case 2: > > + pattern = 0x10; > > + break; > > + case 3: > > + case 4: > > + case 5: > > + case 6: > > + case 7: > > + pattern = pattern_menu + 1; > > + break; > > + } > > + > > + ret = cci_write(gc08a3->regmap, > > GC08A3_REG_TEST_PATTERN_EN, > > + GC08A3_TEST_PATTERN_EN, NULL); > > + if (ret) > > + return ret; > > + > > + ret = cci_write(gc08a3->regmap, > > GC08A3_REG_TEST_PATTERN_IDX, > > + pattern, NULL); > > if (ret) > return ret; > > > + > > + } else { > > + ret = cci_write(gc08a3->regmap, > > GC08A3_REG_TEST_PATTERN_EN, > > + 0x00, NULL); > > if (ret) > return ret; > > > + } > > + > > return 0; > fixed in patch:v5. > > + return ret; > > +} > > + > > +static int gc08a3_set_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct gc08a3 *gc08a3 = > > + container_of(ctrl->handler, struct gc08a3, ctrls); > > + int ret = 0; > > int ret; > > > + s64 exposure_max; > > + > > + if (ctrl->id == V4L2_CID_VBLANK) { > > + /* Update max exposure while meeting expected vblanking > > */ > > + exposure_max = gc08a3->cur_mode->height + ctrl->val - > > + GC08A3_EXP_MARGIN; > > + __v4l2_ctrl_modify_range(gc08a3->exposure, > > + gc08a3->exposure->minimum, > > + exposure_max, gc08a3- > > >exposure->step, > > + exposure_max); > > + } > > + > > + /* > > + * Applying V4L2 control value only happens > > + * when power is on for streaming > > + */ > > + if (!pm_runtime_get_if_in_use(gc08a3->dev)) > > + return 0; > > + > > + switch (ctrl->id) { > > + case V4L2_CID_EXPOSURE: > > + ret = cci_write(gc08a3->regmap, GC08A3_EXP_REG, > > + ctrl->val, NULL); > > + break; > > + > > + case V4L2_CID_ANALOGUE_GAIN: > > + ret = cci_write(gc08a3->regmap, GC08A3_AGAIN_REG, > > + ctrl->val, NULL); > > + break; > > + > > + case V4L2_CID_VBLANK: > > + ret = cci_write(gc08a3->regmap, > > GC08A3_FRAME_LENGTH_REG, > > + gc08a3->cur_mode->height + ctrl->val, > > NULL); > > + break; > > + > > + case V4L2_CID_HFLIP: > > + ret = gc08a3_set_ctrl_hflip(gc08a3, ctrl->val); > > + break; > > + > > + case V4L2_CID_VFLIP: > > + ret = gc08a3_set_ctrl_vflip(gc08a3, ctrl->val); > > + break; > > + > > + case V4L2_CID_TEST_PATTERN: > > + ret = gc08a3_test_pattern(gc08a3, ctrl->val); > > + break; > > + > > + default: > > + break; > > + } > > + > > + pm_runtime_put(gc08a3->dev); > > if (ret) > return ret; > > return 0; > As "default" case is not assign the variable "ret", this return value is not expected, so we need initialize the variable "ret=0". Can we keep this coding style? > > + > > + return ret; > > +} > > + > > ...and I've ignored some more stuff as other reviewers already gave > you feedback. > > Regards, > Angelo >
Hi Zhi, On Tue, Feb 20, 2024 at 02:12:26AM +0000, Zhi Mao (毛智) wrote: > On Tue, 2024-02-06 at 20:45 +0200, Laurent Pinchart wrote: > > On Sun, Feb 04, 2024 at 02:15:38PM +0800, Zhi Mao wrote: > > > Add a V4L2 sub-device driver for Galaxycore GC08A3 image sensor. > > > > > > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com> > > > --- > > > drivers/media/i2c/Kconfig | 10 + > > > drivers/media/i2c/Makefile | 1 + > > > drivers/media/i2c/gc08a3.c | 1448 ++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 1459 insertions(+) > > > create mode 100644 drivers/media/i2c/gc08a3.c [snip] > > > diff --git a/drivers/media/i2c/gc08a3.c b/drivers/media/i2c/gc08a3.c > > > new file mode 100644 > > > index 000000000000..3fc7fffb815c > > > --- /dev/null > > > +++ b/drivers/media/i2c/gc08a3.c > > > @@ -0,0 +1,1448 @@ [snip] > > > +static int gc08a3_power_on(struct device *dev) > > > +{ > > > + struct i2c_client *client = to_i2c_client(dev); > > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > > + struct gc08a3 *gc08a3 = to_gc08a3(sd); > > > + int ret; > > > + > > > + ret = regulator_bulk_enable(ARRAY_SIZE(gc08a3_supply_name), > > > + gc08a3->supplies); > > > + if (ret < 0) { > > > + dev_err(gc08a3->dev, "failed to enable regulators: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = clk_prepare_enable(gc08a3->xclk); > > > + if (ret < 0) { > > > + regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name), > > > + gc08a3->supplies); > > > + dev_err(gc08a3->dev, "clk prepare enable failed\n"); > > > + return ret; > > > + } > > > + > > > + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > > > + > > > + gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > > > Are you asserting reset when powering on ? That sounds wrong, you should > > de-assert reset here (and acquire the reset gpio in probe() with > > GPIOD_OUT_HIGH). Drivers should use logical levels for GPIOs, setting a > > GPIO named "reset" to 1 should assert the reset signal, even if the > > physical signal is active low. You may have the wrong polarity in the > > device tree. > > According to the sensor power sequence sepc, "reset" pin should be pull > from low to high after "dovdd/dvdd/avdd" power on, so I follow this > power sequece to pull "reset" pin high in software flow. From a hardware point of view that's right, but the Linux kernel handles logical level of GPIOs. If a GPIO is named "reset", it is expected that calling gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); will "assert" the reset signal, setting it to a logical "reset = true" level. This maps to the hardware 0V output level, as the signal is active-low. To achieve this, define the reset GPIO as active low in DT, and the GPIO framework will invert the signal for you. You should then call gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); in the driver when you want to assert reset (set it to 0V), and gpiod_set_value_cansleep(gc08a3->reset_gpio, 0); when you want to deassert it (set it to 3.3V, or whatever the I/O voltage for the signal is). This way all driver use logical states, and the inversion is handled in DT. > > > + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > > > + > > > + return 0; > > > +} [snip]
Hi Laurent, Thanks for you reply. I'd like to ask for advice about how to contrl "reset-pin", please check the below comments. On Tue, 2024-02-20 at 05:01 +0200, Laurent Pinchart wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Hi Zhi, > > On Tue, Feb 20, 2024 at 02:12:26AM +0000, Zhi Mao (毛智) wrote: > > On Tue, 2024-02-06 at 20:45 +0200, Laurent Pinchart wrote: > > > On Sun, Feb 04, 2024 at 02:15:38PM +0800, Zhi Mao wrote: > > > > Add a V4L2 sub-device driver for Galaxycore GC08A3 image > sensor. > > > > > > > > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com> > > > > --- > > > > drivers/media/i2c/Kconfig | 10 + > > > > drivers/media/i2c/Makefile | 1 + > > > > drivers/media/i2c/gc08a3.c | 1448 > ++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 1459 insertions(+) > > > > create mode 100644 drivers/media/i2c/gc08a3.c > > [snip] > > > > > diff --git a/drivers/media/i2c/gc08a3.c > b/drivers/media/i2c/gc08a3.c > > > > new file mode 100644 > > > > index 000000000000..3fc7fffb815c > > > > --- /dev/null > > > > +++ b/drivers/media/i2c/gc08a3.c > > > > @@ -0,0 +1,1448 @@ > > [snip] > > > > > +static int gc08a3_power_on(struct device *dev) > > > > +{ > > > > +struct i2c_client *client = to_i2c_client(dev); > > > > +struct v4l2_subdev *sd = i2c_get_clientdata(client); > > > > +struct gc08a3 *gc08a3 = to_gc08a3(sd); > > > > +int ret; > > > > + > > > > +ret = regulator_bulk_enable(ARRAY_SIZE(gc08a3_supply_name), > > > > + gc08a3->supplies); > > > > +if (ret < 0) { > > > > +dev_err(gc08a3->dev, "failed to enable regulators: %d\n", > ret); > > > > +return ret; > > > > +} > > > > + > > > > +ret = clk_prepare_enable(gc08a3->xclk); > > > > +if (ret < 0) { > > > > +regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name), > > > > + gc08a3->supplies); > > > > +dev_err(gc08a3->dev, "clk prepare enable failed\n"); > > > > +return ret; > > > > +} > > > > + > > > > +usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > > > > + > > > > +gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > > > > > Are you asserting reset when powering on ? That sounds wrong, you > should > > > de-assert reset here (and acquire the reset gpio in probe() with > > > GPIOD_OUT_HIGH). Drivers should use logical levels for GPIOs, > setting a > > > GPIO named "reset" to 1 should assert the reset signal, even if > the > > > physical signal is active low. You may have the wrong polarity in > the > > > device tree. > > > > According to the sensor power sequence sepc, "reset" pin should be > pull > > from low to high after "dovdd/dvdd/avdd" power on, so I follow this > > power sequece to pull "reset" pin high in software flow. > > From a hardware point of view that's right, but the Linux kernel > handles > logical level of GPIOs. If a GPIO is named "reset", it is expected > that > calling > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > will "assert" the reset signal, setting it to a logical "reset = > true" > level. This maps to the hardware 0V output level, as the signal is > active-low. To achieve this, define the reset GPIO as active low in > DT, > and the GPIO framework will invert the signal for you. You should > then > call > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > in the driver when you want to assert reset (set it to 0V), and > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 0); > > when you want to deassert it (set it to 3.3V, or whatever the I/O > voltage for the signal is). > > This way all driver use logical states, and the inversion is handled > in > DT. > Sensor power sequence as below: ------------------ | | | | | | dvdd/avdd/dovdd -------- --------- | | reset-pin ------------- In order to match this power sequece, "reset-pin" contrl flow is below: 1. config the "reset-pin" is "active-high" in DTS: reset-gpios = <&pio 19 GPIO_ACTIVE_HIGH>; 2. image sensor driver probe function: gc08a3->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); //init "reset-pin" is low 3. image sensor driver power_on function: gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); //pull "reset-pin" high so, the expect state of "reset-pin" is from low to high. If I am wrong, please correct me. > > > > +usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > > > > + > > > > +return 0; > > > > +} > > [snip] > > -- > Regards, > > Laurent Pinchart
Hi Zhi, On Tue, Feb 20, 2024 at 05:45:54AM +0000, Zhi Mao (毛智) wrote: > Hi Laurent, > > Thanks for you reply. > I'd like to ask for advice about how to contrl "reset-pin", please > check the below comments. > > On Tue, 2024-02-20 at 05:01 +0200, Laurent Pinchart wrote: > > > > External email : Please do not click links or open attachments until > > you have verified the sender or the content. > > Hi Zhi, > > > > On Tue, Feb 20, 2024 at 02:12:26AM +0000, Zhi Mao (毛智) wrote: > > > On Tue, 2024-02-06 at 20:45 +0200, Laurent Pinchart wrote: > > > > On Sun, Feb 04, 2024 at 02:15:38PM +0800, Zhi Mao wrote: > > > > > Add a V4L2 sub-device driver for Galaxycore GC08A3 image > > sensor. > > > > > > > > > > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com> > > > > > --- > > > > > drivers/media/i2c/Kconfig | 10 + > > > > > drivers/media/i2c/Makefile | 1 + > > > > > drivers/media/i2c/gc08a3.c | 1448 > > ++++++++++++++++++++++++++++++++++++ > > > > > 3 files changed, 1459 insertions(+) > > > > > create mode 100644 drivers/media/i2c/gc08a3.c > > > > [snip] > > > > > > > diff --git a/drivers/media/i2c/gc08a3.c > > b/drivers/media/i2c/gc08a3.c > > > > > new file mode 100644 > > > > > index 000000000000..3fc7fffb815c > > > > > --- /dev/null > > > > > +++ b/drivers/media/i2c/gc08a3.c > > > > > @@ -0,0 +1,1448 @@ > > > > [snip] > > > > > > > +static int gc08a3_power_on(struct device *dev) > > > > > +{ > > > > > +struct i2c_client *client = to_i2c_client(dev); > > > > > +struct v4l2_subdev *sd = i2c_get_clientdata(client); > > > > > +struct gc08a3 *gc08a3 = to_gc08a3(sd); > > > > > +int ret; > > > > > + > > > > > +ret = regulator_bulk_enable(ARRAY_SIZE(gc08a3_supply_name), > > > > > + gc08a3->supplies); > > > > > +if (ret < 0) { > > > > > +dev_err(gc08a3->dev, "failed to enable regulators: %d\n", > > ret); > > > > > +return ret; > > > > > +} > > > > > + > > > > > +ret = clk_prepare_enable(gc08a3->xclk); > > > > > +if (ret < 0) { > > > > > +regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name), > > > > > + gc08a3->supplies); > > > > > +dev_err(gc08a3->dev, "clk prepare enable failed\n"); > > > > > +return ret; > > > > > +} > > > > > + > > > > > +usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > > > > > + > > > > > +gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > > > > > > > Are you asserting reset when powering on ? That sounds wrong, you > > should > > > > de-assert reset here (and acquire the reset gpio in probe() with > > > > GPIOD_OUT_HIGH). Drivers should use logical levels for GPIOs, > > setting a > > > > GPIO named "reset" to 1 should assert the reset signal, even if > > the > > > > physical signal is active low. You may have the wrong polarity in > > the > > > > device tree. > > > > > > According to the sensor power sequence sepc, "reset" pin should be > > pull > > > from low to high after "dovdd/dvdd/avdd" power on, so I follow this > > > power sequece to pull "reset" pin high in software flow. > > > > From a hardware point of view that's right, but the Linux kernel > > handles > > logical level of GPIOs. If a GPIO is named "reset", it is expected > > that > > calling > > > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > > > will "assert" the reset signal, setting it to a logical "reset = > > true" > > level. This maps to the hardware 0V output level, as the signal is > > active-low. To achieve this, define the reset GPIO as active low in > > DT, > > and the GPIO framework will invert the signal for you. You should > > then > > call > > > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > > > in the driver when you want to assert reset (set it to 0V), and > > > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 0); > > > > when you want to deassert it (set it to 3.3V, or whatever the I/O > > voltage for the signal is). > > > > This way all driver use logical states, and the inversion is handled > > in > > DT. > > > > Sensor power sequence as below: > ------------------ > | | | > | | | > dvdd/avdd/dovdd -------- > --------- > | > | > reset-pin ------------- > > In order to match this power sequece, "reset-pin" contrl flow is below: > 1. config the "reset-pin" is "active-high" in DTS: > reset-gpios = <&pio 19 GPIO_ACTIVE_HIGH>; > > 2. image sensor driver probe function: > gc08a3->reset_gpio = devm_gpiod_get(dev, "reset", > GPIOD_OUT_LOW); //init "reset-pin" is low > > 3. image sensor driver power_on function: > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); //pull "reset-pin" > high > > so, the expect state of "reset-pin" is from low to high. > If I am wrong, please correct me. From Documentation/driver-api/gpio/consumer.rst: As a consumer should not have to care about the physical line level, all of the gpiod_set_value_xxx() or gpiod_set_array_value_xxx() functions operate with the *logical* value. With this they take the active low property into account. This means that they check whether the GPIO is configured to be active low, and if so, they manipulate the passed value before the physical line level is driven. I.e. when you want to enable reset, you set the value to 1 in the driver. I think you're now setting the value to 0 in that case. The opposite for disabling it of course.
Hi Laurent & sakari, Thanks for your feedback. On Tue, 2024-02-20 at 07:25 +0000, sakari.ailus@linux.intel.com wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Hi Zhi, > > On Tue, Feb 20, 2024 at 05:45:54AM +0000, Zhi Mao (毛智) wrote: > > Hi Laurent, > > > > Thanks for you reply. > > I'd like to ask for advice about how to contrl "reset-pin", please > > check the below comments. > > > > On Tue, 2024-02-20 at 05:01 +0200, Laurent Pinchart wrote: > > > > > > External email : Please do not click links or open attachments > until > > > you have verified the sender or the content. > > > Hi Zhi, > > > > > > On Tue, Feb 20, 2024 at 02:12:26AM +0000, Zhi Mao (毛智) wrote: > > > > On Tue, 2024-02-06 at 20:45 +0200, Laurent Pinchart wrote: > > > > > On Sun, Feb 04, 2024 at 02:15:38PM +0800, Zhi Mao wrote: > > > > > > Add a V4L2 sub-device driver for Galaxycore GC08A3 image > > > sensor. > > > > > > > > > > > > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com> > > > > > > --- > > > > > > drivers/media/i2c/Kconfig | 10 + > > > > > > drivers/media/i2c/Makefile | 1 + > > > > > > drivers/media/i2c/gc08a3.c | 1448 > > > ++++++++++++++++++++++++++++++++++++ > > > > > > 3 files changed, 1459 insertions(+) > > > > > > create mode 100644 drivers/media/i2c/gc08a3.c > > > > > > [snip] > > > > > > > > > diff --git a/drivers/media/i2c/gc08a3.c > > > b/drivers/media/i2c/gc08a3.c > > > > > > new file mode 100644 > > > > > > index 000000000000..3fc7fffb815c > > > > > > --- /dev/null > > > > > > +++ b/drivers/media/i2c/gc08a3.c > > > > > > @@ -0,0 +1,1448 @@ > > > > > > [snip] > > > > > > > > > +static int gc08a3_power_on(struct device *dev) > > > > > > +{ > > > > > > +struct i2c_client *client = to_i2c_client(dev); > > > > > > +struct v4l2_subdev *sd = i2c_get_clientdata(client); > > > > > > +struct gc08a3 *gc08a3 = to_gc08a3(sd); > > > > > > +int ret; > > > > > > + > > > > > > +ret = > regulator_bulk_enable(ARRAY_SIZE(gc08a3_supply_name), > > > > > > + gc08a3->supplies); > > > > > > +if (ret < 0) { > > > > > > +dev_err(gc08a3->dev, "failed to enable regulators: %d\n", > > > ret); > > > > > > +return ret; > > > > > > +} > > > > > > + > > > > > > +ret = clk_prepare_enable(gc08a3->xclk); > > > > > > +if (ret < 0) { > > > > > > +regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name), > > > > > > + gc08a3->supplies); > > > > > > +dev_err(gc08a3->dev, "clk prepare enable failed\n"); > > > > > > +return ret; > > > > > > +} > > > > > > + > > > > > > +usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > > > > > > + > > > > > > +gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > > > > > > > > > Are you asserting reset when powering on ? That sounds wrong, > you > > > should > > > > > de-assert reset here (and acquire the reset gpio in probe() > with > > > > > GPIOD_OUT_HIGH). Drivers should use logical levels for GPIOs, > > > setting a > > > > > GPIO named "reset" to 1 should assert the reset signal, even > if > > > the > > > > > physical signal is active low. You may have the wrong > polarity in > > > the > > > > > device tree. > > > > > > > > According to the sensor power sequence sepc, "reset" pin should > be > > > pull > > > > from low to high after "dovdd/dvdd/avdd" power on, so I follow > this > > > > power sequece to pull "reset" pin high in software flow. > > > > > > From a hardware point of view that's right, but the Linux kernel > > > handles > > > logical level of GPIOs. If a GPIO is named "reset", it is > expected > > > that > > > calling > > > > > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > > > > > will "assert" the reset signal, setting it to a logical "reset = > > > true" > > > level. This maps to the hardware 0V output level, as the signal > is > > > active-low. To achieve this, define the reset GPIO as active low > in > > > DT, > > > and the GPIO framework will invert the signal for you. You should > > > then > > > call > > > > > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > > > > > in the driver when you want to assert reset (set it to 0V), and > > > > > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 0); > > > > > > when you want to deassert it (set it to 3.3V, or whatever the I/O > > > voltage for the signal is). > > > > > > This way all driver use logical states, and the inversion is > handled > > > in > > > DT. > > > > > > > Sensor power sequence as below: > > ------------------ > > | | | > > | | | > > dvdd/avdd/dovdd -------- > > --------- > > | > > | > > reset-pin ------------- > > > > In order to match this power sequece, "reset-pin" contrl flow is > below: > > 1. config the "reset-pin" is "active-high" in DTS: > > reset-gpios = <&pio 19 GPIO_ACTIVE_HIGH>; > > > > 2. image sensor driver probe function: > > gc08a3->reset_gpio = devm_gpiod_get(dev, "reset", > > GPIOD_OUT_LOW); //init "reset-pin" is low > > > > 3. image sensor driver power_on function: > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); //pull "reset-pin" > > high > > > > so, the expect state of "reset-pin" is from low to high. > > If I am wrong, please correct me. > > > From Documentation/driver-api/gpio/consumer.rst: > > As a consumer should not have to care about the physical line > level, all of the gpiod_set_value_xxx() or > gpiod_set_array_value_xxx() functions operate with the *logical* > value. With this they take the active low property into account. > This means that they check whether the GPIO is configured to be > active low, and if so, they manipulate the passed value before the > physical line level is driven. > > I.e. when you want to enable reset, you set the value to 1 in the > driver. I > think you're now setting the value to 0 in that case. The opposite > for > disabling it of course. > After checking "Documentation/driver-api/gpio/consumer.rst": To summarize:: Function (example) line property physical line ... gpiod_set_value(desc, 1); default (active high) high gpiod_set_value(desc, 0); active low high ... From my understanding, it seems that "reset-pin" is using the following(active_high) case in current code: "gpiod_set_value(desc, 1); default (active high) high" Do you mean, we should use the "active_low" case: "gpiod_set_value(desc, 0); active low high" Code should be changed as below: 1. config the "reset-pin" is "active-low" in DTS: - reset-gpios = <&pio 19 GPIO_ACTIVE_LOW> 2. image sensor driver power_on function: - gpiod_set_value_cansleep(gc08a3->reset_gpio, 0); //pull high Is that so? > -- > Regards, > > Sakari Ailus
Hi Zhi, On Wed, Feb 21, 2024 at 02:37:27AM +0000, Zhi Mao (毛智) wrote: > Hi Laurent & sakari, > > Thanks for your feedback. > > On Tue, 2024-02-20 at 07:25 +0000, sakari.ailus@linux.intel.com wrote: > > > > External email : Please do not click links or open attachments until > > you have verified the sender or the content. > > Hi Zhi, > > > > On Tue, Feb 20, 2024 at 05:45:54AM +0000, Zhi Mao (毛智) wrote: > > > Hi Laurent, > > > > > > Thanks for you reply. > > > I'd like to ask for advice about how to contrl "reset-pin", please > > > check the below comments. > > > > > > On Tue, 2024-02-20 at 05:01 +0200, Laurent Pinchart wrote: > > > > > > > > External email : Please do not click links or open attachments > > until > > > > you have verified the sender or the content. > > > > Hi Zhi, > > > > > > > > On Tue, Feb 20, 2024 at 02:12:26AM +0000, Zhi Mao (毛智) wrote: > > > > > On Tue, 2024-02-06 at 20:45 +0200, Laurent Pinchart wrote: > > > > > > On Sun, Feb 04, 2024 at 02:15:38PM +0800, Zhi Mao wrote: > > > > > > > Add a V4L2 sub-device driver for Galaxycore GC08A3 image > > > > sensor. > > > > > > > > > > > > > > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com> > > > > > > > --- > > > > > > > drivers/media/i2c/Kconfig | 10 + > > > > > > > drivers/media/i2c/Makefile | 1 + > > > > > > > drivers/media/i2c/gc08a3.c | 1448 > > > > ++++++++++++++++++++++++++++++++++++ > > > > > > > 3 files changed, 1459 insertions(+) > > > > > > > create mode 100644 drivers/media/i2c/gc08a3.c > > > > > > > > [snip] > > > > > > > > > > > diff --git a/drivers/media/i2c/gc08a3.c > > > > b/drivers/media/i2c/gc08a3.c > > > > > > > new file mode 100644 > > > > > > > index 000000000000..3fc7fffb815c > > > > > > > --- /dev/null > > > > > > > +++ b/drivers/media/i2c/gc08a3.c > > > > > > > @@ -0,0 +1,1448 @@ > > > > > > > > [snip] > > > > > > > > > > > +static int gc08a3_power_on(struct device *dev) > > > > > > > +{ > > > > > > > +struct i2c_client *client = to_i2c_client(dev); > > > > > > > +struct v4l2_subdev *sd = i2c_get_clientdata(client); > > > > > > > +struct gc08a3 *gc08a3 = to_gc08a3(sd); > > > > > > > +int ret; > > > > > > > + > > > > > > > +ret = > > regulator_bulk_enable(ARRAY_SIZE(gc08a3_supply_name), > > > > > > > + gc08a3->supplies); > > > > > > > +if (ret < 0) { > > > > > > > +dev_err(gc08a3->dev, "failed to enable regulators: %d\n", > > > > ret); > > > > > > > +return ret; > > > > > > > +} > > > > > > > + > > > > > > > +ret = clk_prepare_enable(gc08a3->xclk); > > > > > > > +if (ret < 0) { > > > > > > > +regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name), > > > > > > > + gc08a3->supplies); > > > > > > > +dev_err(gc08a3->dev, "clk prepare enable failed\n"); > > > > > > > +return ret; > > > > > > > +} > > > > > > > + > > > > > > > +usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > > > > > > > + > > > > > > > +gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > > > > > > > > > > > Are you asserting reset when powering on ? That sounds wrong, > > you > > > > should > > > > > > de-assert reset here (and acquire the reset gpio in probe() > > with > > > > > > GPIOD_OUT_HIGH). Drivers should use logical levels for GPIOs, > > > > setting a > > > > > > GPIO named "reset" to 1 should assert the reset signal, even > > if > > > > the > > > > > > physical signal is active low. You may have the wrong > > polarity in > > > > the > > > > > > device tree. > > > > > > > > > > According to the sensor power sequence sepc, "reset" pin should > > be > > > > pull > > > > > from low to high after "dovdd/dvdd/avdd" power on, so I follow > > this > > > > > power sequece to pull "reset" pin high in software flow. > > > > > > > > From a hardware point of view that's right, but the Linux kernel > > > > handles > > > > logical level of GPIOs. If a GPIO is named "reset", it is > > expected > > > > that > > > > calling > > > > > > > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > > > > > > > will "assert" the reset signal, setting it to a logical "reset = > > > > true" > > > > level. This maps to the hardware 0V output level, as the signal > > is > > > > active-low. To achieve this, define the reset GPIO as active low > > in > > > > DT, > > > > and the GPIO framework will invert the signal for you. You should > > > > then > > > > call > > > > > > > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > > > > > > > in the driver when you want to assert reset (set it to 0V), and > > > > > > > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 0); > > > > > > > > when you want to deassert it (set it to 3.3V, or whatever the I/O > > > > voltage for the signal is). > > > > > > > > This way all driver use logical states, and the inversion is > > handled > > > > in > > > > DT. > > > > > > > > > > Sensor power sequence as below: > > > ------------------ > > > | | | > > > | | | > > > dvdd/avdd/dovdd -------- > > > --------- > > > | > > > | > > > reset-pin ------------- > > > > > > In order to match this power sequece, "reset-pin" contrl flow is > > below: > > > 1. config the "reset-pin" is "active-high" in DTS: > > > reset-gpios = <&pio 19 GPIO_ACTIVE_HIGH>; > > > > > > 2. image sensor driver probe function: > > > gc08a3->reset_gpio = devm_gpiod_get(dev, "reset", > > > GPIOD_OUT_LOW); //init "reset-pin" is low > > > > > > 3. image sensor driver power_on function: > > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); //pull "reset-pin" > > > high > > > > > > so, the expect state of "reset-pin" is from low to high. > > > If I am wrong, please correct me. > > > > > > From Documentation/driver-api/gpio/consumer.rst: > > > > As a consumer should not have to care about the physical line > > level, all of the gpiod_set_value_xxx() or > > gpiod_set_array_value_xxx() functions operate with the *logical* > > value. With this they take the active low property into account. > > This means that they check whether the GPIO is configured to be > > active low, and if so, they manipulate the passed value before the > > physical line level is driven. > > > > I.e. when you want to enable reset, you set the value to 1 in the > > driver. I > > think you're now setting the value to 0 in that case. The opposite > > for > > disabling it of course. > > > After checking "Documentation/driver-api/gpio/consumer.rst": > To summarize:: > Function (example) line property physical > line > ... > gpiod_set_value(desc, 1); default (active high) high > gpiod_set_value(desc, 0); active low high > ... > > > From my understanding, it seems that "reset-pin" is using the > following(active_high) case in current code: > "gpiod_set_value(desc, 1); default (active high) high" > > Do you mean, we should use the "active_low" case: > "gpiod_set_value(desc, 0); active low high" > Code should be changed as below: > 1. config the "reset-pin" is "active-low" in DTS: > - reset-gpios = <&pio 19 GPIO_ACTIVE_LOW> > 2. image sensor driver power_on function: > - gpiod_set_value_cansleep(gc08a3->reset_gpio, 0); //pull high > Is that so? Correct.
Hi Sakari, Thanks for your feedback. On Wed, 2024-02-21 at 07:43 +0000, sakari.ailus@linux.intel.com wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Hi Zhi, > > On Wed, Feb 21, 2024 at 02:37:27AM +0000, Zhi Mao (毛智) wrote: > > Hi Laurent & sakari, > > > > Thanks for your feedback. > > > > On Tue, 2024-02-20 at 07:25 +0000, sakari.ailus@linux.intel.com > wrote: > > > > > > External email : Please do not click links or open attachments > until > > > you have verified the sender or the content. > > > Hi Zhi, > > > > > > On Tue, Feb 20, 2024 at 05:45:54AM +0000, Zhi Mao (毛智) wrote: > > > > Hi Laurent, > > > > > > > > Thanks for you reply. > > > > I'd like to ask for advice about how to contrl "reset-pin", > please > > > > check the below comments. > > > > > > > > On Tue, 2024-02-20 at 05:01 +0200, Laurent Pinchart wrote: > > > > > > > > > > External email : Please do not click links or open > attachments > > > until > > > > > you have verified the sender or the content. > > > > > Hi Zhi, > > > > > > > > > > On Tue, Feb 20, 2024 at 02:12:26AM +0000, Zhi Mao (毛智) wrote: > > > > > > On Tue, 2024-02-06 at 20:45 +0200, Laurent Pinchart wrote: > > > > > > > On Sun, Feb 04, 2024 at 02:15:38PM +0800, Zhi Mao wrote: > > > > > > > > Add a V4L2 sub-device driver for Galaxycore GC08A3 > image > > > > > sensor. > > > > > > > > > > > > > > > > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com> > > > > > > > > --- > > > > > > > > drivers/media/i2c/Kconfig | 10 + > > > > > > > > drivers/media/i2c/Makefile | 1 + > > > > > > > > drivers/media/i2c/gc08a3.c | 1448 > > > > > ++++++++++++++++++++++++++++++++++++ > > > > > > > > 3 files changed, 1459 insertions(+) > > > > > > > > create mode 100644 drivers/media/i2c/gc08a3.c > > > > > > > > > > [snip] > > > > > > > > > > > > > diff --git a/drivers/media/i2c/gc08a3.c > > > > > b/drivers/media/i2c/gc08a3.c > > > > > > > > new file mode 100644 > > > > > > > > index 000000000000..3fc7fffb815c > > > > > > > > --- /dev/null > > > > > > > > +++ b/drivers/media/i2c/gc08a3.c > > > > > > > > @@ -0,0 +1,1448 @@ > > > > > > > > > > [snip] > > > > > > > > > > > > > +static int gc08a3_power_on(struct device *dev) > > > > > > > > +{ > > > > > > > > +struct i2c_client *client = to_i2c_client(dev); > > > > > > > > +struct v4l2_subdev *sd = i2c_get_clientdata(client); > > > > > > > > +struct gc08a3 *gc08a3 = to_gc08a3(sd); > > > > > > > > +int ret; > > > > > > > > + > > > > > > > > +ret = > > > regulator_bulk_enable(ARRAY_SIZE(gc08a3_supply_name), > > > > > > > > + gc08a3->supplies); > > > > > > > > +if (ret < 0) { > > > > > > > > +dev_err(gc08a3->dev, "failed to enable regulators: > %d\n", > > > > > ret); > > > > > > > > +return ret; > > > > > > > > +} > > > > > > > > + > > > > > > > > +ret = clk_prepare_enable(gc08a3->xclk); > > > > > > > > +if (ret < 0) { > > > > > > > > +regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name), > > > > > > > > + gc08a3->supplies); > > > > > > > > +dev_err(gc08a3->dev, "clk prepare enable failed\n"); > > > > > > > > +return ret; > > > > > > > > +} > > > > > > > > + > > > > > > > > +usleep_range(GC08A3_MIN_SLEEP_US, > GC08A3_MAX_SLEEP_US); > > > > > > > > + > > > > > > > > +gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > > > > > > > > > > > > > Are you asserting reset when powering on ? That sounds > wrong, > > > you > > > > > should > > > > > > > de-assert reset here (and acquire the reset gpio in > probe() > > > with > > > > > > > GPIOD_OUT_HIGH). Drivers should use logical levels for > GPIOs, > > > > > setting a > > > > > > > GPIO named "reset" to 1 should assert the reset signal, > even > > > if > > > > > the > > > > > > > physical signal is active low. You may have the wrong > > > polarity in > > > > > the > > > > > > > device tree. > > > > > > > > > > > > According to the sensor power sequence sepc, "reset" pin > should > > > be > > > > > pull > > > > > > from low to high after "dovdd/dvdd/avdd" power on, so I > follow > > > this > > > > > > power sequece to pull "reset" pin high in software flow. > > > > > > > > > > From a hardware point of view that's right, but the Linux > kernel > > > > > handles > > > > > logical level of GPIOs. If a GPIO is named "reset", it is > > > expected > > > > > that > > > > > calling > > > > > > > > > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > > > > > > > > > will "assert" the reset signal, setting it to a logical > "reset = > > > > > true" > > > > > level. This maps to the hardware 0V output level, as the > signal > > > is > > > > > active-low. To achieve this, define the reset GPIO as active > low > > > in > > > > > DT, > > > > > and the GPIO framework will invert the signal for you. You > should > > > > > then > > > > > call > > > > > > > > > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); > > > > > > > > > > in the driver when you want to assert reset (set it to 0V), > and > > > > > > > > > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 0); > > > > > > > > > > when you want to deassert it (set it to 3.3V, or whatever the > I/O > > > > > voltage for the signal is). > > > > > > > > > > This way all driver use logical states, and the inversion is > > > handled > > > > > in > > > > > DT. > > > > > > > > > > > > > Sensor power sequence as below: > > > > ------------------ > > > > | | | > > > > | | | > > > > dvdd/avdd/dovdd -------- > > > > --------- > > > > | > > > > | > > > > reset-pin ------------- > > > > > > > > In order to match this power sequece, "reset-pin" contrl flow > is > > > below: > > > > 1. config the "reset-pin" is "active-high" in DTS: > > > > reset-gpios = <&pio 19 GPIO_ACTIVE_HIGH>; > > > > > > > > 2. image sensor driver probe function: > > > > gc08a3->reset_gpio = devm_gpiod_get(dev, "reset", > > > > GPIOD_OUT_LOW); //init "reset-pin" is low > > > > > > > > 3. image sensor driver power_on function: > > > > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); //pull "reset- > pin" > > > > high > > > > > > > > so, the expect state of "reset-pin" is from low to high. > > > > If I am wrong, please correct me. > > > > > > > > > From Documentation/driver-api/gpio/consumer.rst: > > > > > > As a consumer should not have to care about the physical line > > > level, all of the gpiod_set_value_xxx() or > > > gpiod_set_array_value_xxx() functions operate with the *logical* > > > value. With this they take the active low property into account. > > > This means that they check whether the GPIO is configured to be > > > active low, and if so, they manipulate the passed value before > the > > > physical line level is driven. > > > > > > I.e. when you want to enable reset, you set the value to 1 in the > > > driver. I > > > think you're now setting the value to 0 in that case. The > opposite > > > for > > > disabling it of course. > > > > > After checking "Documentation/driver-api/gpio/consumer.rst": > > To summarize:: > > Function (example) line > property physical > > line > > ... > > gpiod_set_value(desc, 1); default (active high) high > > gpiod_set_value(desc, 0); active low high > > ... > > > > > > From my understanding, it seems that "reset-pin" is using the > > following(active_high) case in current code: > > "gpiod_set_value(desc, 1); default (active high) high" > > > > Do you mean, we should use the "active_low" case: > > "gpiod_set_value(desc, 0); active low high" > > Code should be changed as below: > > 1. config the "reset-pin" is "active-low" in DTS: > > - reset-gpios = <&pio 19 GPIO_ACTIVE_LOW> > > 2. image sensor driver power_on function: > > - gpiod_set_value_cansleep(gc08a3->reset_gpio, 0); //pull high > > Is that so? > > Correct. > fixed in patch:v6 > -- > Regards, > > Sakari Ailus
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 56f276b920ab..e4da68835683 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -70,6 +70,16 @@ config VIDEO_GC0308 To compile this driver as a module, choose M here: the module will be called gc0308. +config VIDEO_GC08A3 + tristate "GalaxyCore gc08a3 sensor support" + select V4L2_CCI_I2C + help + This is a Video4Linux2 sensor driver for the GalaxyCore gc08a3 + camera. + + To compile this driver as a module, choose M here: the + module will be called gc08a3. + config VIDEO_GC2145 select V4L2_CCI_I2C tristate "GalaxyCore GC2145 sensor support" diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index dfbe6448b549..b82e99ca7578 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -38,6 +38,7 @@ obj-$(CONFIG_VIDEO_DW9768) += dw9768.o obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/ obj-$(CONFIG_VIDEO_GC0308) += gc0308.o +obj-$(CONFIG_VIDEO_GC08A3) += gc08a3.o obj-$(CONFIG_VIDEO_GC2145) += gc2145.o obj-$(CONFIG_VIDEO_HI556) += hi556.o obj-$(CONFIG_VIDEO_HI846) += hi846.o diff --git a/drivers/media/i2c/gc08a3.c b/drivers/media/i2c/gc08a3.c new file mode 100644 index 000000000000..3fc7fffb815c --- /dev/null +++ b/drivers/media/i2c/gc08a3.c @@ -0,0 +1,1448 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * gc08a3.c - gc08a3 sensor driver + * + * Copyright 2023 MediaTek + * + * Zhi Mao <zhi.mao@mediatek.com> + */ + +#include <asm/unaligned.h> +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/pm_runtime.h> +#include <linux/regulator/consumer.h> +#include <media/media-entity.h> +#include <media/v4l2-ctrls.h> +#include <media/v4l2-fwnode.h> +#include <media/v4l2-subdev.h> +#include <media/v4l2-cci.h> +#include <media/v4l2-event.h> + +#define GC08A3_REG_CHIP_ID CCI_REG16(0x03f0) +#define GC08A3_CHIP_ID 0x08a3 + +#define GC08A3_NATIVE_WIDTH 3264 +#define GC08A3_NATIVE_HEIGHT 2448 + +#define GC08A3_REG_TEST_PATTERN_EN CCI_REG8(0x008c) +#define GC08A3_REG_TEST_PATTERN_IDX CCI_REG8(0x008d) +#define GC08A3_TEST_PATTERN_EN 0x01 + +#define GC08A3_STRAEMING_REG CCI_REG8(0x0100) + +#define GC08A3_DEFAULT_CLK_FREQ 24000000 +#define GC08A3_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10 +#define GC08A3_DATA_LANES 4 + +/* for 1920*1080 */ +#define GC08A3_LINK_FREQ_207MHZ 207000000ULL +/* for 3264*2448 */ +#define GC08A3_LINK_FREQ_336MHZ 336000000ULL + +#define GC08A3_RGB_DEPTH 10 + +#define GC08A3_FRAME_LENGTH_REG CCI_REG16(0x0340) +#define GC08A3_VTS_30FPS 2548 +#define GC08A3_VTS_30FPS_MIN 2548 +#define GC08A3_VTS_60FPS 1276 +#define GC08A3_VTS_60FPS_MIN 1276 +#define GC08A3_VTS_MAX 0xfff0 + +#define GC08A3_HTS_30FPS 3640 +#define GC08A3_HTS_60FPS 3640 + +#define GC08A3_EXP_REG CCI_REG16(0x0202) +#define GC08A3_EXP_MARGIN 16 +#define GC08A3_EXP_MIN 4 +#define GC08A3_EXP_STEP 1 + +#define GC08A3_FLIP_REG CCI_REG8(0x0101) +#define GC08A3_FLIP_H_MASK 0x1 +#define GC08A3_FLIP_V_MASK 0x2 + +#define GC08A3_AGAIN_REG CCI_REG16(0x0204) +#define GC08A3_AGAIN_MIN 1024 +#define GC08A3_AGAIN_MAX (1024 * 16) +#define GC08A3_AGAIN_STEP 1 + +#define GC08A3_MIN_SLEEP_US 2000 +#define GC08A3_MAX_SLEEP_US 3000 + +static const char *const gc08a3_test_pattern_menu[] = { + "No Pattern", "Solid Black", "Colour Bar", "Solid White", + "Solid Red", "Solid Green", "Solid Blue", "Solid Yellow", +}; + +enum { + GC08A3_LINK_FREQ_336MHZ_INDEX, + GC08A3_LINK_FREQ_207MHZ_INDEX, +}; + +static const s64 link_freq_menu_items[] = { + GC08A3_LINK_FREQ_336MHZ, + GC08A3_LINK_FREQ_207MHZ, +}; + +static const char *const gc08a3_supply_name[] = { + "avdd", + "dvdd", + "dovdd", +}; + +struct gc08a3 { + struct device *dev; + struct v4l2_subdev sd; + struct media_pad pad; + struct i2c_client *client; + + struct clk *xclk; + struct regulator_bulk_data supplies[ARRAY_SIZE(gc08a3_supply_name)]; + struct gpio_desc *reset_gpio; + + struct v4l2_ctrl_handler ctrls; + struct v4l2_ctrl *pixel_rate; + struct v4l2_ctrl *link_freq; + struct v4l2_ctrl *exposure; + struct v4l2_ctrl *vblank; + struct v4l2_ctrl *hblank; + + struct regmap *regmap; + + bool streaming; + unsigned long link_freq_bitmap; + + /* Current mode */ + const struct gc08a3_mode *cur_mode; +}; + +struct gc08a3_reg_list { + u32 num_of_regs; + const struct cci_reg_sequence *regs; +}; + +struct gc08a3_link_freq_config { + const struct gc08a3_reg_list reg_list; +}; + +static const struct cci_reg_sequence mode_3264x2448[] = { + /* system */ + { CCI_REG8(0x0336), 0x70 }, + { CCI_REG8(0x0383), 0xbb }, + { CCI_REG8(0x0344), 0x00 }, + { CCI_REG8(0x0345), 0x06 }, + { CCI_REG8(0x0346), 0x00 }, + { CCI_REG8(0x0347), 0x04 }, + { CCI_REG8(0x0348), 0x0c }, + { CCI_REG8(0x0349), 0xd0 }, + { CCI_REG8(0x034a), 0x09 }, + { CCI_REG8(0x034b), 0x9c }, + { CCI_REG8(0x0202), 0x09 }, + { CCI_REG8(0x0203), 0x04 }, + { CCI_REG8(0x0340), 0x09 }, + { CCI_REG8(0x0341), 0xf4 }, + { CCI_REG8(0x0342), 0x07 }, + { CCI_REG8(0x0343), 0x1c }, + + { CCI_REG8(0x0226), 0x00 }, + { CCI_REG8(0x0227), 0x28 }, + { CCI_REG8(0x0e38), 0x49 }, + { CCI_REG8(0x0210), 0x13 }, + { CCI_REG8(0x0218), 0x00 }, + { CCI_REG8(0x0241), 0x88 }, + { CCI_REG8(0x0392), 0x60 }, + + /* ISP */ + { CCI_REG8(0x00a2), 0x00 }, + { CCI_REG8(0x00a3), 0x00 }, + { CCI_REG8(0x00ab), 0x00 }, + { CCI_REG8(0x00ac), 0x00 }, + + /* GAIN */ + { CCI_REG8(0x0204), 0x04 }, + { CCI_REG8(0x0205), 0x00 }, + { CCI_REG8(0x0050), 0x5c }, + { CCI_REG8(0x0051), 0x44 }, + + /* out window */ + { CCI_REG8(0x009a), 0x66 }, + { CCI_REG8(0x0351), 0x00 }, + { CCI_REG8(0x0352), 0x06 }, + { CCI_REG8(0x0353), 0x00 }, + { CCI_REG8(0x0354), 0x08 }, + { CCI_REG8(0x034c), 0x0c }, + { CCI_REG8(0x034d), 0xc0 }, + { CCI_REG8(0x034e), 0x09 }, + { CCI_REG8(0x034f), 0x90 }, + + /* MIPI */ + { CCI_REG8(0x0114), 0x03 }, + { CCI_REG8(0x0180), 0x65 }, + { CCI_REG8(0x0181), 0xf0 }, + { CCI_REG8(0x0185), 0x01 }, + { CCI_REG8(0x0115), 0x30 }, + { CCI_REG8(0x011b), 0x12 }, + { CCI_REG8(0x011c), 0x12 }, + { CCI_REG8(0x0121), 0x06 }, + { CCI_REG8(0x0122), 0x06 }, + { CCI_REG8(0x0123), 0x15 }, + { CCI_REG8(0x0124), 0x01 }, + { CCI_REG8(0x0125), 0x0b }, + { CCI_REG8(0x0126), 0x08 }, + { CCI_REG8(0x0129), 0x06 }, + { CCI_REG8(0x012a), 0x08 }, + { CCI_REG8(0x012b), 0x08 }, + + { CCI_REG8(0x0a73), 0x60 }, + { CCI_REG8(0x0a70), 0x11 }, + { CCI_REG8(0x0313), 0x80 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0a70), 0x00 }, + { CCI_REG8(0x00a4), 0x80 }, + { CCI_REG8(0x0316), 0x01 }, + { CCI_REG8(0x0a67), 0x00 }, + { CCI_REG8(0x0084), 0x10 }, + { CCI_REG8(0x0102), 0x09 }, +}; + +static const struct cci_reg_sequence mode_1920x1080[] = { + /* system */ + { CCI_REG8(0x0336), 0x45 }, + { CCI_REG8(0x0383), 0x8b }, + { CCI_REG8(0x0344), 0x02 }, + { CCI_REG8(0x0345), 0xa6 }, + { CCI_REG8(0x0346), 0x02 }, + { CCI_REG8(0x0347), 0xb0 }, + { CCI_REG8(0x0348), 0x07 }, + { CCI_REG8(0x0349), 0x90 }, + { CCI_REG8(0x034a), 0x04 }, + { CCI_REG8(0x034b), 0x44 }, + { CCI_REG8(0x0202), 0x03 }, + { CCI_REG8(0x0203), 0x00 }, + { CCI_REG8(0x0340), 0x04 }, + { CCI_REG8(0x0341), 0xfc }, + { CCI_REG8(0x0342), 0x07 }, + { CCI_REG8(0x0343), 0x1c }, + { CCI_REG8(0x0226), 0x00 }, + { CCI_REG8(0x0227), 0x88 }, + { CCI_REG8(0x0e38), 0x49 }, + { CCI_REG8(0x0210), 0x13 }, + { CCI_REG8(0x0218), 0x00 }, + { CCI_REG8(0x0241), 0x88 }, + { CCI_REG8(0x0392), 0x60 }, + + /* ISP */ + { CCI_REG8(0x00a2), 0xac }, + { CCI_REG8(0x00a3), 0x02 }, + { CCI_REG8(0x00ab), 0xa0 }, + { CCI_REG8(0x00ac), 0x02 }, + + /* GAIN */ + { CCI_REG8(0x0204), 0x04 }, + { CCI_REG8(0x0205), 0x00 }, + { CCI_REG8(0x0050), 0x38 }, + { CCI_REG8(0x0051), 0x20 }, + + /* out window */ + { CCI_REG8(0x009a), 0x66 }, + { CCI_REG8(0x0351), 0x00 }, + { CCI_REG8(0x0352), 0x06 }, + { CCI_REG8(0x0353), 0x00 }, + { CCI_REG8(0x0354), 0x08 }, + { CCI_REG8(0x034c), 0x07 }, + { CCI_REG8(0x034d), 0x80 }, + { CCI_REG8(0x034e), 0x04 }, + { CCI_REG8(0x034f), 0x38 }, + + /* MIPI */ + { CCI_REG8(0x0114), 0x03 }, + { CCI_REG8(0x0180), 0x65 }, + { CCI_REG8(0x0181), 0xf0 }, + { CCI_REG8(0x0185), 0x01 }, + { CCI_REG8(0x0115), 0x30 }, + { CCI_REG8(0x011b), 0x12 }, + { CCI_REG8(0x011c), 0x12 }, + { CCI_REG8(0x0121), 0x02 }, + { CCI_REG8(0x0122), 0x03 }, + { CCI_REG8(0x0123), 0x0c }, + { CCI_REG8(0x0124), 0x00 }, + { CCI_REG8(0x0125), 0x09 }, + { CCI_REG8(0x0126), 0x06 }, + { CCI_REG8(0x0129), 0x04 }, + { CCI_REG8(0x012a), 0x03 }, + { CCI_REG8(0x012b), 0x06 }, + + { CCI_REG8(0x0a73), 0x60 }, + { CCI_REG8(0x0a70), 0x11 }, + { CCI_REG8(0x0313), 0x80 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0aff), 0x00 }, + { CCI_REG8(0x0a70), 0x00 }, + { CCI_REG8(0x00a4), 0x80 }, + { CCI_REG8(0x0316), 0x01 }, + { CCI_REG8(0x0a67), 0x00 }, + { CCI_REG8(0x0084), 0x10 }, + { CCI_REG8(0x0102), 0x09 }, +}; + +static const struct cci_reg_sequence mode_table_common[] = { + { GC08A3_STRAEMING_REG, 0x00 }, + /* system */ + { CCI_REG8(0x031c), 0x60 }, + { CCI_REG8(0x0337), 0x04 }, + { CCI_REG8(0x0335), 0x51 }, + { CCI_REG8(0x0336), 0x70 }, + { CCI_REG8(0x0383), 0xbb }, + { CCI_REG8(0x031a), 0x00 }, + { CCI_REG8(0x0321), 0x10 }, + { CCI_REG8(0x0327), 0x03 }, + { CCI_REG8(0x0325), 0x40 }, + { CCI_REG8(0x0326), 0x23 }, + { CCI_REG8(0x0314), 0x11 }, + { CCI_REG8(0x0315), 0xd6 }, + { CCI_REG8(0x0316), 0x01 }, + { CCI_REG8(0x0334), 0x40 }, + { CCI_REG8(0x0324), 0x42 }, + { CCI_REG8(0x031c), 0x00 }, + { CCI_REG8(0x031c), 0x9f }, + { CCI_REG8(0x039a), 0x13 }, + { CCI_REG8(0x0084), 0x30 }, + { CCI_REG8(0x02b3), 0x08 }, + { CCI_REG8(0x0057), 0x0c }, + { CCI_REG8(0x05c3), 0x50 }, + { CCI_REG8(0x0311), 0x90 }, + { CCI_REG8(0x05a0), 0x02 }, + { CCI_REG8(0x0074), 0x0a }, + { CCI_REG8(0x0059), 0x11 }, + { CCI_REG8(0x0070), 0x05 }, + { CCI_REG8(0x0101), 0x00 }, + + /* analog */ + { CCI_REG8(0x0344), 0x00 }, + { CCI_REG8(0x0345), 0x06 }, + { CCI_REG8(0x0346), 0x00 }, + { CCI_REG8(0x0347), 0x04 }, + { CCI_REG8(0x0348), 0x0c }, + { CCI_REG8(0x0349), 0xd0 }, + { CCI_REG8(0x034a), 0x09 }, + { CCI_REG8(0x034b), 0x9c }, + { CCI_REG8(0x0202), 0x09 }, + { CCI_REG8(0x0203), 0x04 }, + + { CCI_REG8(0x0219), 0x05 }, + { CCI_REG8(0x0226), 0x00 }, + { CCI_REG8(0x0227), 0x28 }, + { CCI_REG8(0x0e0a), 0x00 }, + { CCI_REG8(0x0e0b), 0x00 }, + { CCI_REG8(0x0e24), 0x04 }, + { CCI_REG8(0x0e25), 0x04 }, + { CCI_REG8(0x0e26), 0x00 }, + { CCI_REG8(0x0e27), 0x10 }, + { CCI_REG8(0x0e01), 0x74 }, + { CCI_REG8(0x0e03), 0x47 }, + { CCI_REG8(0x0e04), 0x33 }, + { CCI_REG8(0x0e05), 0x44 }, + { CCI_REG8(0x0e06), 0x44 }, + { CCI_REG8(0x0e0c), 0x1e }, + { CCI_REG8(0x0e17), 0x3a }, + { CCI_REG8(0x0e18), 0x3c }, + { CCI_REG8(0x0e19), 0x40 }, + { CCI_REG8(0x0e1a), 0x42 }, + { CCI_REG8(0x0e28), 0x21 }, + { CCI_REG8(0x0e2b), 0x68 }, + { CCI_REG8(0x0e2c), 0x0d }, + { CCI_REG8(0x0e2d), 0x08 }, + { CCI_REG8(0x0e34), 0xf4 }, + { CCI_REG8(0x0e35), 0x44 }, + { CCI_REG8(0x0e36), 0x07 }, + { CCI_REG8(0x0e38), 0x49 }, + { CCI_REG8(0x0210), 0x13 }, + { CCI_REG8(0x0218), 0x00 }, + { CCI_REG8(0x0241), 0x88 }, + { CCI_REG8(0x0e32), 0x00 }, + { CCI_REG8(0x0e33), 0x18 }, + { CCI_REG8(0x0e42), 0x03 }, + { CCI_REG8(0x0e43), 0x80 }, + { CCI_REG8(0x0e44), 0x04 }, + { CCI_REG8(0x0e45), 0x00 }, + { CCI_REG8(0x0e4f), 0x04 }, + { CCI_REG8(0x057a), 0x20 }, + { CCI_REG8(0x0381), 0x7c }, + { CCI_REG8(0x0382), 0x9b }, + { CCI_REG8(0x0384), 0xfb }, + { CCI_REG8(0x0389), 0x38 }, + { CCI_REG8(0x038a), 0x03 }, + { CCI_REG8(0x0390), 0x6a }, + { CCI_REG8(0x0391), 0x0b }, + { CCI_REG8(0x0392), 0x60 }, + { CCI_REG8(0x0393), 0xc1 }, + { CCI_REG8(0x0396), 0xff }, + { CCI_REG8(0x0398), 0x62 }, + + /* cisctl reset */ + { CCI_REG8(0x031c), 0x80 }, + { CCI_REG8(0x03fe), 0x10 }, + { CCI_REG8(0x03fe), 0x00 }, + { CCI_REG8(0x031c), 0x9f }, + { CCI_REG8(0x03fe), 0x00 }, + { CCI_REG8(0x03fe), 0x00 }, + { CCI_REG8(0x03fe), 0x00 }, + { CCI_REG8(0x03fe), 0x00 }, + { CCI_REG8(0x031c), 0x80 }, + { CCI_REG8(0x03fe), 0x10 }, + { CCI_REG8(0x03fe), 0x00 }, + { CCI_REG8(0x031c), 0x9f }, + { CCI_REG8(0x0360), 0x01 }, + { CCI_REG8(0x0360), 0x00 }, + { CCI_REG8(0x0316), 0x09 }, + { CCI_REG8(0x0a67), 0x80 }, + { CCI_REG8(0x0313), 0x00 }, + { CCI_REG8(0x0a53), 0x0e }, + { CCI_REG8(0x0a65), 0x17 }, + { CCI_REG8(0x0a68), 0xa1 }, + { CCI_REG8(0x0a58), 0x00 }, + { CCI_REG8(0x0ace), 0x0c }, + { CCI_REG8(0x00a4), 0x00 }, + { CCI_REG8(0x00a5), 0x01 }, + { CCI_REG8(0x00a7), 0x09 }, + { CCI_REG8(0x00a8), 0x9c }, + { CCI_REG8(0x00a9), 0x0c }, + { CCI_REG8(0x00aa), 0xd0 }, + { CCI_REG8(0x0a8a), 0x00 }, + { CCI_REG8(0x0a8b), 0xe0 }, + { CCI_REG8(0x0a8c), 0x13 }, + { CCI_REG8(0x0a8d), 0xe8 }, + { CCI_REG8(0x0a90), 0x0a }, + { CCI_REG8(0x0a91), 0x10 }, + { CCI_REG8(0x0a92), 0xf8 }, + { CCI_REG8(0x0a71), 0xf2 }, + { CCI_REG8(0x0a72), 0x12 }, + { CCI_REG8(0x0a73), 0x64 }, + { CCI_REG8(0x0a75), 0x41 }, + { CCI_REG8(0x0a70), 0x07 }, + { CCI_REG8(0x0313), 0x80 }, + + /* ISP */ + { CCI_REG8(0x00a0), 0x01 }, + { CCI_REG8(0x0080), 0xd2 }, + { CCI_REG8(0x0081), 0x3f }, + { CCI_REG8(0x0087), 0x51 }, + { CCI_REG8(0x0089), 0x03 }, + { CCI_REG8(0x009b), 0x40 }, + { CCI_REG8(0x05a0), 0x82 }, + { CCI_REG8(0x05ac), 0x00 }, + { CCI_REG8(0x05ad), 0x01 }, + { CCI_REG8(0x05ae), 0x00 }, + { CCI_REG8(0x0800), 0x0a }, + { CCI_REG8(0x0801), 0x14 }, + { CCI_REG8(0x0802), 0x28 }, + { CCI_REG8(0x0803), 0x34 }, + { CCI_REG8(0x0804), 0x0e }, + { CCI_REG8(0x0805), 0x33 }, + { CCI_REG8(0x0806), 0x03 }, + { CCI_REG8(0x0807), 0x8a }, + { CCI_REG8(0x0808), 0x50 }, + { CCI_REG8(0x0809), 0x00 }, + { CCI_REG8(0x080a), 0x34 }, + { CCI_REG8(0x080b), 0x03 }, + { CCI_REG8(0x080c), 0x26 }, + { CCI_REG8(0x080d), 0x03 }, + { CCI_REG8(0x080e), 0x18 }, + { CCI_REG8(0x080f), 0x03 }, + { CCI_REG8(0x0810), 0x10 }, + { CCI_REG8(0x0811), 0x03 }, + { CCI_REG8(0x0812), 0x00 }, + { CCI_REG8(0x0813), 0x00 }, + { CCI_REG8(0x0814), 0x01 }, + { CCI_REG8(0x0815), 0x00 }, + { CCI_REG8(0x0816), 0x01 }, + { CCI_REG8(0x0817), 0x00 }, + { CCI_REG8(0x0818), 0x00 }, + { CCI_REG8(0x0819), 0x0a }, + { CCI_REG8(0x081a), 0x01 }, + { CCI_REG8(0x081b), 0x6c }, + { CCI_REG8(0x081c), 0x00 }, + { CCI_REG8(0x081d), 0x0b }, + { CCI_REG8(0x081e), 0x02 }, + { CCI_REG8(0x081f), 0x00 }, + { CCI_REG8(0x0820), 0x00 }, + { CCI_REG8(0x0821), 0x0c }, + { CCI_REG8(0x0822), 0x02 }, + { CCI_REG8(0x0823), 0xd9 }, + { CCI_REG8(0x0824), 0x00 }, + { CCI_REG8(0x0825), 0x0d }, + { CCI_REG8(0x0826), 0x03 }, + { CCI_REG8(0x0827), 0xf0 }, + { CCI_REG8(0x0828), 0x00 }, + { CCI_REG8(0x0829), 0x0e }, + { CCI_REG8(0x082a), 0x05 }, + { CCI_REG8(0x082b), 0x94 }, + { CCI_REG8(0x082c), 0x09 }, + { CCI_REG8(0x082d), 0x6e }, + { CCI_REG8(0x082e), 0x07 }, + { CCI_REG8(0x082f), 0xe6 }, + { CCI_REG8(0x0830), 0x10 }, + { CCI_REG8(0x0831), 0x0e }, + { CCI_REG8(0x0832), 0x0b }, + { CCI_REG8(0x0833), 0x2c }, + { CCI_REG8(0x0834), 0x14 }, + { CCI_REG8(0x0835), 0xae }, + { CCI_REG8(0x0836), 0x0f }, + { CCI_REG8(0x0837), 0xc4 }, + { CCI_REG8(0x0838), 0x18 }, + { CCI_REG8(0x0839), 0x0e }, + { CCI_REG8(0x05ac), 0x01 }, + { CCI_REG8(0x059a), 0x00 }, + { CCI_REG8(0x059b), 0x00 }, + { CCI_REG8(0x059c), 0x01 }, + { CCI_REG8(0x0598), 0x00 }, + { CCI_REG8(0x0597), 0x14 }, + { CCI_REG8(0x05ab), 0x09 }, + { CCI_REG8(0x05a4), 0x02 }, + { CCI_REG8(0x05a3), 0x05 }, + { CCI_REG8(0x05a0), 0xc2 }, + { CCI_REG8(0x0207), 0xc4 }, + + /* GAIN */ + { CCI_REG8(0x0208), 0x01 }, + { CCI_REG8(0x0209), 0x72 }, + { CCI_REG8(0x0204), 0x04 }, + { CCI_REG8(0x0205), 0x00 }, + + { CCI_REG8(0x0040), 0x22 }, + { CCI_REG8(0x0041), 0x20 }, + { CCI_REG8(0x0043), 0x10 }, + { CCI_REG8(0x0044), 0x00 }, + { CCI_REG8(0x0046), 0x08 }, + { CCI_REG8(0x0047), 0xf0 }, + { CCI_REG8(0x0048), 0x0f }, + { CCI_REG8(0x004b), 0x0f }, + { CCI_REG8(0x004c), 0x00 }, + { CCI_REG8(0x0050), 0x5c }, + { CCI_REG8(0x0051), 0x44 }, + { CCI_REG8(0x005b), 0x03 }, + { CCI_REG8(0x00c0), 0x00 }, + { CCI_REG8(0x00c1), 0x80 }, + { CCI_REG8(0x00c2), 0x31 }, + { CCI_REG8(0x00c3), 0x00 }, + { CCI_REG8(0x0460), 0x04 }, + { CCI_REG8(0x0462), 0x08 }, + { CCI_REG8(0x0464), 0x0e }, + { CCI_REG8(0x0466), 0x0a }, + { CCI_REG8(0x0468), 0x12 }, + { CCI_REG8(0x046a), 0x12 }, + { CCI_REG8(0x046c), 0x10 }, + { CCI_REG8(0x046e), 0x0c }, + { CCI_REG8(0x0461), 0x03 }, + { CCI_REG8(0x0463), 0x03 }, + { CCI_REG8(0x0465), 0x03 }, + { CCI_REG8(0x0467), 0x03 }, + { CCI_REG8(0x0469), 0x04 }, + { CCI_REG8(0x046b), 0x04 }, + { CCI_REG8(0x046d), 0x04 }, + { CCI_REG8(0x046f), 0x04 }, + { CCI_REG8(0x0470), 0x04 }, + { CCI_REG8(0x0472), 0x10 }, + { CCI_REG8(0x0474), 0x26 }, + { CCI_REG8(0x0476), 0x38 }, + { CCI_REG8(0x0478), 0x20 }, + { CCI_REG8(0x047a), 0x30 }, + { CCI_REG8(0x047c), 0x38 }, + { CCI_REG8(0x047e), 0x60 }, + { CCI_REG8(0x0471), 0x05 }, + { CCI_REG8(0x0473), 0x05 }, + { CCI_REG8(0x0475), 0x05 }, + { CCI_REG8(0x0477), 0x05 }, + { CCI_REG8(0x0479), 0x04 }, + { CCI_REG8(0x047b), 0x04 }, + { CCI_REG8(0x047d), 0x04 }, + { CCI_REG8(0x047f), 0x04 }, +}; + +static const struct gc08a3_link_freq_config gc08a3_link_freq_336m_configs = { + .reg_list = { + .num_of_regs = ARRAY_SIZE(mode_table_common), + .regs = mode_table_common, + } +}; + +static const struct gc08a3_link_freq_config gc08a3_link_freq_207m_configs = { + .reg_list = { + .num_of_regs = ARRAY_SIZE(mode_table_common), + .regs = mode_table_common, + } +}; + +struct gc08a3_mode { + u32 width; + u32 height; + const struct gc08a3_reg_list reg_list; + + u32 hts; /* Horizontal timining size */ + u32 vts_def; /* Default vertical timining size */ + u32 vts_min; /* Min vertical timining size */ + u32 max_framerate; + const struct gc08a3_link_freq_config *link_freq_configs; +}; + +/* + * Declare modes in order, from biggest + * to smallest height. + */ +static const struct gc08a3_mode gc08a3_modes[] = { + { + .width = GC08A3_NATIVE_WIDTH, + .height = GC08A3_NATIVE_HEIGHT, + .reg_list = { + .num_of_regs = ARRAY_SIZE(mode_3264x2448), + .regs = mode_3264x2448, + }, + .link_freq_configs = &gc08a3_link_freq_336m_configs, + + .hts = GC08A3_HTS_30FPS, + .vts_def = GC08A3_VTS_30FPS, + .vts_min = GC08A3_VTS_30FPS_MIN, + .max_framerate = 300, + }, + { + .width = 1920, + .height = 1080, + .reg_list = { + .num_of_regs = ARRAY_SIZE(mode_1920x1080), + .regs = mode_1920x1080, + }, + .link_freq_configs = &gc08a3_link_freq_207m_configs, + + .hts = GC08A3_HTS_60FPS, + .vts_def = GC08A3_VTS_60FPS, + .vts_min = GC08A3_VTS_60FPS_MIN, + .max_framerate = 600, + }, +}; + +static u64 to_pixel_rate(u32 f_index) +{ + u64 pixel_rate = link_freq_menu_items[f_index] * 2 * GC08A3_DATA_LANES; + + do_div(pixel_rate, GC08A3_RGB_DEPTH); + + return pixel_rate; +} + +static int gc08a3_identify_module(struct gc08a3 *gc08a3) +{ + struct i2c_client *client = v4l2_get_subdevdata(&gc08a3->sd); + u64 val = 0; + int ret; + + ret = cci_read(gc08a3->regmap, GC08A3_REG_CHIP_ID, &val, NULL); + if (ret) { + dev_err(&client->dev, + "failed to read chip id: 0x%x", GC08A3_CHIP_ID); + return ret; + } + + if (val != GC08A3_CHIP_ID) { + dev_err(&client->dev, "chip id mismatch: 0x%x!=0x%llx", + GC08A3_CHIP_ID, val); + return -ENXIO; + } + + return 0; +} + +static inline struct gc08a3 *to_gc08a3(struct v4l2_subdev *sd) +{ + return container_of(sd, struct gc08a3, sd); +} + +static int gc08a3_power_on(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct gc08a3 *gc08a3 = to_gc08a3(sd); + int ret; + + ret = regulator_bulk_enable(ARRAY_SIZE(gc08a3_supply_name), + gc08a3->supplies); + if (ret < 0) { + dev_err(gc08a3->dev, "failed to enable regulators: %d\n", ret); + return ret; + } + + ret = clk_prepare_enable(gc08a3->xclk); + if (ret < 0) { + regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name), + gc08a3->supplies); + dev_err(gc08a3->dev, "clk prepare enable failed\n"); + return ret; + } + + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); + + gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); + + return 0; +} + +static int gc08a3_power_off(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct gc08a3 *gc08a3 = to_gc08a3(sd); + + clk_disable_unprepare(gc08a3->xclk); + + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); + + gpiod_set_value_cansleep(gc08a3->reset_gpio, 0); + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); + + regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name), + gc08a3->supplies); + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); + + return 0; +} + +static int gc08a3_enum_mbus_code(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_mbus_code_enum *code) +{ + if (code->index > 0) + return -EINVAL; + + code->code = GC08A3_MBUS_CODE; + + return 0; +} + +static int gc08a3_enum_frame_size(struct v4l2_subdev *subdev, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_frame_size_enum *fse) +{ + if (fse->code != GC08A3_MBUS_CODE) + return -EINVAL; + + if (fse->index >= ARRAY_SIZE(gc08a3_modes)) + return -EINVAL; + + fse->min_width = gc08a3_modes[fse->index].width; + fse->max_width = gc08a3_modes[fse->index].width; + fse->min_height = gc08a3_modes[fse->index].height; + fse->max_height = gc08a3_modes[fse->index].height; + + return 0; +} + +static int gc08a3_update_cur_mode_controls(struct gc08a3 *gc08a3, + const struct gc08a3_mode *mode) +{ + s64 exposure_max, h_blank; + int ret = 0; + + ret = __v4l2_ctrl_modify_range(gc08a3->vblank, + mode->vts_min - mode->height, + GC08A3_VTS_MAX - mode->height, 1, + mode->vts_def - mode->height); + if (ret) + dev_err(gc08a3->dev, "VB ctrl range update failed\n"); + + h_blank = mode->hts - mode->width; + ret = __v4l2_ctrl_modify_range(gc08a3->hblank, h_blank, h_blank, 1, + h_blank); + if (ret) + dev_err(gc08a3->dev, "HB ctrl range update failed\n"); + + exposure_max = mode->vts_def - GC08A3_EXP_MARGIN; + ret = __v4l2_ctrl_modify_range(gc08a3->exposure, GC08A3_EXP_MIN, + exposure_max, GC08A3_EXP_STEP, + exposure_max); + if (ret) + dev_err(gc08a3->dev, "exposure ctrl range update failed\n"); + + return ret; +} + +static void gc08a3_update_pad_format(struct gc08a3 *gc08a3, + const struct gc08a3_mode *mode, + struct v4l2_mbus_framefmt *fmt) +{ + fmt->width = mode->width; + fmt->height = mode->height; + fmt->code = GC08A3_MBUS_CODE; + fmt->field = V4L2_FIELD_NONE; + fmt->colorspace = V4L2_COLORSPACE_SRGB; + fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace); + fmt->quantization = + V4L2_MAP_QUANTIZATION_DEFAULT(true, + fmt->colorspace, + fmt->ycbcr_enc); + fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace); +} + +static int gc08a3_set_format(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, + struct v4l2_subdev_format *fmt) +{ + struct gc08a3 *gc08a3 = to_gc08a3(sd); + struct v4l2_mbus_framefmt *mbus_fmt; + struct v4l2_rect *crop; + const struct gc08a3_mode *mode; + + mode = v4l2_find_nearest_size(gc08a3_modes, ARRAY_SIZE(gc08a3_modes), + width, height, fmt->format.width, + fmt->format.height); + + /*update crop info to subdev state*/ + crop = v4l2_subdev_state_get_crop(state, 0); + crop->width = mode->width; + crop->height = mode->height; + + /*update fmt info to subdev state*/ + gc08a3_update_pad_format(gc08a3, mode, &fmt->format); + mbus_fmt = v4l2_subdev_state_get_format(state, 0); + *mbus_fmt = fmt->format; + + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) + return 0; + + gc08a3->cur_mode = mode; + gc08a3_update_cur_mode_controls(gc08a3, mode); + + return 0; +} + +static int gc08a3_get_selection(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, + struct v4l2_subdev_selection *sel) +{ + struct gc08a3 *gc08a3 = to_gc08a3(sd); + + switch (sel->target) { + case V4L2_SEL_TGT_CROP: + sel->r = *v4l2_subdev_state_get_crop(state, 0); + break; + case V4L2_SEL_TGT_CROP_BOUNDS: + sel->r.top = 0; + sel->r.left = 0; + sel->r.width = GC08A3_NATIVE_WIDTH; + sel->r.height = GC08A3_NATIVE_HEIGHT; + break; + case V4L2_SEL_TGT_CROP_DEFAULT: + if (gc08a3->cur_mode->width == GC08A3_NATIVE_WIDTH) { + sel->r.top = 0; + sel->r.left = 0; + sel->r.width = gc08a3_modes[0].width; + sel->r.height = gc08a3_modes[0].height; + } else { + sel->r.top = 0; + sel->r.left = 0; + sel->r.width = gc08a3_modes[1].width; + sel->r.height = gc08a3_modes[1].height; + } + break; + default: + return -EINVAL; + } + + return 0; +} + +static int gc08a3_init_state(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state) +{ + struct v4l2_subdev_format fmt = { + .which = V4L2_SUBDEV_FORMAT_TRY, + .pad = 0, + .format = { + .code = GC08A3_MBUS_CODE, + .width = gc08a3_modes[0].width, + .height = gc08a3_modes[0].height, + }, + }; + + gc08a3_set_format(sd, state, &fmt); + + return 0; +} + +static int gc08a3_set_ctrl_hflip(struct gc08a3 *gc08a3, u32 ctrl_val) +{ + int ret; + u64 val; + + ret = cci_read(gc08a3->regmap, GC08A3_FLIP_REG, &val, NULL); + if (ret) { + dev_err(gc08a3->dev, "read hflip register failed: %d\n", ret); + return ret; + } + + val = ctrl_val ? (val | GC08A3_FLIP_H_MASK) : + (val & ~GC08A3_FLIP_H_MASK); + ret = cci_write(gc08a3->regmap, GC08A3_FLIP_REG, val, NULL); + if (ret < 0) + dev_err(gc08a3->dev, "Error %d\n", ret); + + return ret; +} + +static int gc08a3_set_ctrl_vflip(struct gc08a3 *gc08a3, u32 ctrl_val) +{ + int ret; + u64 val; + + ret = cci_read(gc08a3->regmap, GC08A3_FLIP_REG, &val, NULL); + if (ret) { + dev_err(gc08a3->dev, "read vflip register failed: %d\n", ret); + return ret; + } + + val = ctrl_val ? (val | GC08A3_FLIP_V_MASK) : + (val & ~GC08A3_FLIP_V_MASK); + ret = cci_write(gc08a3->regmap, GC08A3_FLIP_REG, val, NULL); + if (ret < 0) + dev_err(gc08a3->dev, "Error %d\n", ret); + + return ret; +} + +static int gc08a3_test_pattern(struct gc08a3 *gc08a3, u32 pattern_menu) +{ + u32 pattern = 0; + int ret; + + if (pattern_menu) { + switch (pattern_menu) { + case 1: + pattern = 0x00; + break; + case 2: + pattern = 0x10; + break; + case 3: + case 4: + case 5: + case 6: + case 7: + pattern = pattern_menu + 1; + break; + } + + ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN, + GC08A3_TEST_PATTERN_EN, NULL); + if (ret) + return ret; + + ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_IDX, + pattern, NULL); + + } else { + ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN, + 0x00, NULL); + } + + return ret; +} + +static int gc08a3_set_ctrl(struct v4l2_ctrl *ctrl) +{ + struct gc08a3 *gc08a3 = + container_of(ctrl->handler, struct gc08a3, ctrls); + int ret = 0; + s64 exposure_max; + + if (ctrl->id == V4L2_CID_VBLANK) { + /* Update max exposure while meeting expected vblanking */ + exposure_max = gc08a3->cur_mode->height + ctrl->val - + GC08A3_EXP_MARGIN; + __v4l2_ctrl_modify_range(gc08a3->exposure, + gc08a3->exposure->minimum, + exposure_max, gc08a3->exposure->step, + exposure_max); + } + + /* + * Applying V4L2 control value only happens + * when power is on for streaming + */ + if (!pm_runtime_get_if_in_use(gc08a3->dev)) + return 0; + + switch (ctrl->id) { + case V4L2_CID_EXPOSURE: + ret = cci_write(gc08a3->regmap, GC08A3_EXP_REG, + ctrl->val, NULL); + break; + + case V4L2_CID_ANALOGUE_GAIN: + ret = cci_write(gc08a3->regmap, GC08A3_AGAIN_REG, + ctrl->val, NULL); + break; + + case V4L2_CID_VBLANK: + ret = cci_write(gc08a3->regmap, GC08A3_FRAME_LENGTH_REG, + gc08a3->cur_mode->height + ctrl->val, NULL); + break; + + case V4L2_CID_HFLIP: + ret = gc08a3_set_ctrl_hflip(gc08a3, ctrl->val); + break; + + case V4L2_CID_VFLIP: + ret = gc08a3_set_ctrl_vflip(gc08a3, ctrl->val); + break; + + case V4L2_CID_TEST_PATTERN: + ret = gc08a3_test_pattern(gc08a3, ctrl->val); + break; + + default: + break; + } + + pm_runtime_put(gc08a3->dev); + + return ret; +} + +static const struct v4l2_ctrl_ops gc08a3_ctrl_ops = { + .s_ctrl = gc08a3_set_ctrl, +}; + +static int gc08a3_start_streaming(struct gc08a3 *gc08a3) +{ + const struct gc08a3_mode *mode; + const struct gc08a3_reg_list *reg_list; + const struct gc08a3_link_freq_config *link_freq_cfg; + int ret; + + ret = pm_runtime_resume_and_get(gc08a3->dev); + if (ret < 0) + return ret; + + link_freq_cfg = gc08a3->cur_mode->link_freq_configs; + + reg_list = &link_freq_cfg->reg_list; + ret = cci_multi_reg_write(gc08a3->regmap, + reg_list->regs, reg_list->num_of_regs, NULL); + if (ret) { + dev_err(gc08a3->dev, "could not sent common table %d\n", ret); + goto err_rpm_put; + } + + mode = gc08a3->cur_mode; + reg_list = &mode->reg_list; + ret = cci_multi_reg_write(gc08a3->regmap, + reg_list->regs, reg_list->num_of_regs, NULL); + if (ret < 0) { + dev_err(gc08a3->dev, "could not sent mode table %d\n", ret); + goto err_rpm_put; + } + + ret = __v4l2_ctrl_handler_setup(&gc08a3->ctrls); + if (ret < 0) { + dev_err(gc08a3->dev, "could not sync v4l2 controls\n"); + goto err_rpm_put; + } + + ret = cci_write(gc08a3->regmap, GC08A3_STRAEMING_REG, 1, NULL); + if (ret < 0) { + dev_err(gc08a3->dev, "write STRAEMING_REG failed: %d\n", ret); + goto err_rpm_put; + } + + return 0; + +err_rpm_put: + pm_runtime_put(gc08a3->dev); + return ret; +} + +static int gc08a3_stop_streaming(struct gc08a3 *gc08a3) +{ + int ret; + + ret = cci_write(gc08a3->regmap, GC08A3_STRAEMING_REG, 0, NULL); + if (ret < 0) + dev_err(gc08a3->dev, "could not sent stop streaming %d\n", ret); + + pm_runtime_put(gc08a3->dev); + return ret; +} + +static int gc08a3_s_stream(struct v4l2_subdev *subdev, int enable) +{ + struct gc08a3 *gc08a3 = to_gc08a3(subdev); + struct v4l2_subdev_state *state; + int ret; + + state = v4l2_subdev_lock_and_get_active_state(subdev); + + if (enable) + ret = gc08a3_start_streaming(gc08a3); + else + ret = gc08a3_stop_streaming(gc08a3); + + gc08a3->streaming = enable; + v4l2_subdev_unlock_state(state); + + return ret; +} + +static int +gc08a3_enum_frame_interval(struct v4l2_subdev *subdev, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_frame_interval_enum *fie) +{ + unsigned int index = fie->index; + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(gc08a3_modes); i++) { + if (fie->width != gc08a3_modes[i].width || + fie->height != gc08a3_modes[i].height) + continue; + + if (index-- == 0) { + fie->interval.numerator = 1; + fie->interval.denominator = + gc08a3_modes[i].max_framerate / 10; + return 0; + } + } + + return -EINVAL; +} + +static const struct v4l2_subdev_video_ops gc08a3_video_ops = { + .s_stream = gc08a3_s_stream, +}; + +static const struct v4l2_subdev_pad_ops gc08a3_subdev_pad_ops = { + .enum_mbus_code = gc08a3_enum_mbus_code, + .enum_frame_size = gc08a3_enum_frame_size, + .enum_frame_interval = gc08a3_enum_frame_interval, + .get_fmt = v4l2_subdev_get_fmt, + .set_fmt = gc08a3_set_format, + .get_selection = gc08a3_get_selection, +}; + +static const struct v4l2_subdev_core_ops gc08a3_core_ops = { + .subscribe_event = v4l2_ctrl_subdev_subscribe_event, + .unsubscribe_event = v4l2_event_subdev_unsubscribe, +}; + +static const struct v4l2_subdev_ops gc08a3_subdev_ops = { + .core = &gc08a3_core_ops, + .video = &gc08a3_video_ops, + .pad = &gc08a3_subdev_pad_ops, +}; + +static const struct v4l2_subdev_internal_ops gc08a3_internal_ops = { + .init_state = gc08a3_init_state, +}; + +static int gc08a3_get_regulators(struct device *dev, struct gc08a3 *gc08a3) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(gc08a3_supply_name); i++) + gc08a3->supplies[i].supply = gc08a3_supply_name[i]; + + return devm_regulator_bulk_get(dev, ARRAY_SIZE(gc08a3_supply_name), + gc08a3->supplies); +} + +static int gc08a3_parse_fwnode(struct device *dev, struct gc08a3 *gc08a3) +{ + struct fwnode_handle *endpoint; + struct v4l2_fwnode_endpoint bus_cfg = { + .bus_type = V4L2_MBUS_CSI2_DPHY, + }; + int ret; + + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL); + if (!endpoint) { + dev_err(dev, "endpoint node not found\n"); + return -EINVAL; + } + + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg); + if (ret) { + dev_err(dev, "parsing endpoint node failed\n"); + goto done; + } + + ret = v4l2_link_freq_to_bitmap(dev, bus_cfg.link_frequencies, + bus_cfg.nr_of_link_frequencies, + link_freq_menu_items, + ARRAY_SIZE(link_freq_menu_items), + &gc08a3->link_freq_bitmap); + if (ret) + goto done; + +done: + v4l2_fwnode_endpoint_free(&bus_cfg); + fwnode_handle_put(endpoint); + return ret; +} + +static int gc08a3_init_controls(struct gc08a3 *gc08a3) +{ + struct i2c_client *client = v4l2_get_subdevdata(&gc08a3->sd); + const struct gc08a3_mode *mode = &gc08a3_modes[0]; + const struct v4l2_ctrl_ops *ops = &gc08a3_ctrl_ops; + struct v4l2_fwnode_device_properties props; + struct v4l2_ctrl_handler *ctrl_hdlr; + s64 exposure_max, h_blank; + int ret; + + ctrl_hdlr = &gc08a3->ctrls; + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10); + if (ret) + return ret; + + gc08a3->link_freq = + v4l2_ctrl_new_int_menu(ctrl_hdlr, + &gc08a3_ctrl_ops, + V4L2_CID_LINK_FREQ, + ARRAY_SIZE(link_freq_menu_items) - 1, + 0, link_freq_menu_items); + if (gc08a3->link_freq) + gc08a3->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + gc08a3->pixel_rate = + v4l2_ctrl_new_std(ctrl_hdlr, + &gc08a3_ctrl_ops, + V4L2_CID_PIXEL_RATE, 0, + to_pixel_rate(GC08A3_LINK_FREQ_336MHZ_INDEX), + 1, + to_pixel_rate(GC08A3_LINK_FREQ_336MHZ_INDEX)); + + gc08a3->vblank = + v4l2_ctrl_new_std(ctrl_hdlr, + &gc08a3_ctrl_ops, V4L2_CID_VBLANK, + mode->vts_min - mode->height, + GC08A3_VTS_MAX - mode->height, 1, + mode->vts_def - mode->height); + + h_blank = mode->hts - mode->width; + gc08a3->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, + V4L2_CID_HBLANK, h_blank, h_blank, 1, + h_blank); + if (gc08a3->hblank) + gc08a3->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, + V4L2_CID_ANALOGUE_GAIN, GC08A3_AGAIN_MIN, + GC08A3_AGAIN_MAX, GC08A3_AGAIN_STEP, + GC08A3_AGAIN_MIN); + + exposure_max = mode->vts_def - GC08A3_EXP_MARGIN; + gc08a3->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, + V4L2_CID_EXPOSURE, GC08A3_EXP_MIN, + exposure_max, GC08A3_EXP_STEP, + exposure_max); + + v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &gc08a3_ctrl_ops, + V4L2_CID_TEST_PATTERN, + ARRAY_SIZE(gc08a3_test_pattern_menu) - 1, + 0, 0, gc08a3_test_pattern_menu); + + v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, V4L2_CID_HFLIP, 0, + 1, 1, 0); + + v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops, V4L2_CID_VFLIP, 0, + 1, 1, 0); + + /* register properties to fwnode (e.g. rotation, orientation) */ + ret = v4l2_fwnode_device_parse(&client->dev, &props); + if (ret) + goto error_ctrls; + + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, ops, &props); + if (ret) + goto error_ctrls; + + if (ctrl_hdlr->error) { + ret = ctrl_hdlr->error; + goto error_ctrls; + } + + gc08a3->sd.ctrl_handler = ctrl_hdlr; + + return 0; + +error_ctrls: + v4l2_ctrl_handler_free(ctrl_hdlr); + + return ret; +} + +static int gc08a3_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct gc08a3 *gc08a3; + int ret; + + gc08a3 = devm_kzalloc(dev, sizeof(*gc08a3), GFP_KERNEL); + if (!gc08a3) + return -ENOMEM; + + gc08a3->dev = dev; + + ret = gc08a3_parse_fwnode(dev, gc08a3); + if (ret) + return ret; + + gc08a3->regmap = devm_cci_regmap_init_i2c(client, 16); + if (IS_ERR(gc08a3->regmap)) + return dev_err_probe(dev, PTR_ERR(gc08a3->regmap), + "failed to init CCI\n"); + + gc08a3->xclk = devm_clk_get(dev, NULL); + if (IS_ERR(gc08a3->xclk)) + return dev_err_probe(dev, PTR_ERR(gc08a3->xclk), + "failed to get xclk\n"); + + ret = clk_set_rate(gc08a3->xclk, GC08A3_DEFAULT_CLK_FREQ); + if (ret) + return dev_err_probe(dev, ret, + "failed to set xclk frequency\n"); + + ret = gc08a3_get_regulators(dev, gc08a3); + if (ret < 0) + return dev_err_probe(dev, ret, + "failed to get regulators\n"); + + gc08a3->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); + if (IS_ERR(gc08a3->reset_gpio)) + return dev_err_probe(dev, PTR_ERR(gc08a3->reset_gpio), + "failed to get gpio\n"); + + v4l2_i2c_subdev_init(&gc08a3->sd, client, &gc08a3_subdev_ops); + gc08a3->sd.internal_ops = &gc08a3_internal_ops; + gc08a3->cur_mode = &gc08a3_modes[0]; + + ret = gc08a3_power_on(gc08a3->dev); + if (ret) + return dev_err_probe(dev, ret, + "failed to sensor power on\n"); + + ret = gc08a3_identify_module(gc08a3); + if (ret) { + dev_err(&client->dev, "failed to find sensor: %d\n", ret); + goto err_power_off; + } + + ret = gc08a3_init_controls(gc08a3); + if (ret) { + dev_err(&client->dev, "failed to init controls: %d", ret); + goto err_power_off; + } + + gc08a3->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | + V4L2_SUBDEV_FL_HAS_EVENTS; + gc08a3->pad.flags = MEDIA_PAD_FL_SOURCE; + gc08a3->sd.dev = &client->dev; + gc08a3->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; + + ret = media_entity_pads_init(&gc08a3->sd.entity, 1, &gc08a3->pad); + if (ret < 0) { + dev_err(dev, "could not register media entity\n"); + goto err_v4l2_ctrl_handler_free; + } + + gc08a3->sd.state_lock = gc08a3->ctrls.lock; + ret = v4l2_subdev_init_finalize(&gc08a3->sd); + if (ret < 0) { + dev_err(dev, "v4l2 subdev init error: %d\n", ret); + goto err_media_entity_cleanup; + } + + pm_runtime_set_active(gc08a3->dev); + pm_runtime_enable(gc08a3->dev); + pm_runtime_idle(gc08a3->dev); + + ret = v4l2_async_register_subdev_sensor(&gc08a3->sd); + if (ret < 0) { + dev_err(dev, "could not register v4l2 device\n"); + goto err_rpm; + } + + return 0; + +err_rpm: + pm_runtime_disable(gc08a3->dev); + v4l2_subdev_cleanup(&gc08a3->sd); + +err_media_entity_cleanup: + media_entity_cleanup(&gc08a3->sd.entity); + +err_v4l2_ctrl_handler_free: + v4l2_ctrl_handler_free(gc08a3->sd.ctrl_handler); + +err_power_off: + gc08a3_power_off(gc08a3->dev); + + return ret; +} + +static void gc08a3_remove(struct i2c_client *client) +{ + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct gc08a3 *gc08a3 = to_gc08a3(sd); + + v4l2_async_unregister_subdev(&gc08a3->sd); + v4l2_subdev_cleanup(sd); + media_entity_cleanup(&gc08a3->sd.entity); + v4l2_ctrl_handler_free(&gc08a3->ctrls); + + pm_runtime_disable(&client->dev); + if (!pm_runtime_status_suspended(&client->dev)) + gc08a3_power_off(gc08a3->dev); + pm_runtime_set_suspended(&client->dev); +} + +static const struct of_device_id gc08a3_of_match[] = { + { .compatible = "galaxycore,gc08a3" }, + {} +}; +MODULE_DEVICE_TABLE(of, gc08a3_of_match); + +static const struct dev_pm_ops gc08a3_pm_ops = { + SET_RUNTIME_PM_OPS(gc08a3_power_off, gc08a3_power_on, NULL) +}; + +static struct i2c_driver gc08a3_i2c_driver = { + .driver = { + .of_match_table = gc08a3_of_match, + .pm = &gc08a3_pm_ops, + .name = "gc08a3", + }, + .probe = gc08a3_probe, + .remove = gc08a3_remove, +}; + +module_i2c_driver(gc08a3_i2c_driver); + +MODULE_DESCRIPTION("GalaxyCore gc08a3 Camera driver"); +MODULE_AUTHOR("Zhi Mao <zhi.mao@mediatek.com>"); +MODULE_LICENSE("GPL");
Add a V4L2 sub-device driver for Galaxycore GC08A3 image sensor. Signed-off-by: Zhi Mao <zhi.mao@mediatek.com> --- drivers/media/i2c/Kconfig | 10 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/gc08a3.c | 1448 ++++++++++++++++++++++++++++++++++++ 3 files changed, 1459 insertions(+) create mode 100644 drivers/media/i2c/gc08a3.c