Re: [PATCH v4 2/2] media: i2c: Add GC08A3 image sensor driver

From: Zhi Mao (毛智)
Date: Mon Feb 19 2024 - 21:21:47 EST

Next message: Stephen Rothwell: "linux-next: manual merge of the kvm-arm tree with the arm64 tree"
Previous message: Waiman Long: "Re: [PATCH v3] clocksource: Scale the max retry number of watchdog read according to CPU numbers"
In reply to: sakari.ailus@xxxxxxxxxxxxxxx: "Re: [PATCH v4 2/2] media: i2c: Add GC08A3 image sensor driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@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..
>
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
>

Next message: Stephen Rothwell: "linux-next: manual merge of the kvm-arm tree with the arm64 tree"
Previous message: Waiman Long: "Re: [PATCH v3] clocksource: Scale the max retry number of watchdog read according to CPU numbers"
In reply to: sakari.ailus@xxxxxxxxxxxxxxx: "Re: [PATCH v4 2/2] media: i2c: Add GC08A3 image sensor driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]