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

["Zhi Mao (毛智)" <zhi.mao@mediatek.com>]

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
>