Re: [PATCH v4 2/2] media: i2c: Add GC08A3 image sensor driver
From: AngeloGioacchino Del Regno
Date: Wed Feb 07 2024 - 09:42:47 EST
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@xxxxxxxxxxxx>
---
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@xxxxxxxxxxxx>
+ */
+
.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