| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <21370bd8-502b-4b4e-8c9b-6d13c60685d5@collabora.com>
Date: Wed, 7 Feb 2024 15:42:31 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Zhi Mao <zhi.mao@...iatek.com>, mchehab@...nel.org, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, sakari.ailus@...ux.intel.com
Cc: laurent.pinchart@...asonboard.com, shengnan.wang@...iatek.com,
yaya.chang@...iatek.com, 10572168@...com,
Project_Global_Chrome_Upstream_Group@...iatek.com, yunkec@...omium.org,
conor+dt@...nel.org, matthias.bgg@...il.com, jacopo.mondi@...asonboard.com,
hverkuil-cisco@...all.nl, heiko@...ech.de, jernej.skrabec@...il.com,
macromorgan@...mail.com, linus.walleij@...aro.org, hdegoede@...hat.com,
tomi.valkeinen@...asonboard.com, gerald.loacker@...fvision.net,
andy.shevchenko@...il.com, bingbu.cao@...el.com,
dan.scally@...asonboard.com, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v4 2/2] media: i2c: Add GC08A3 image sensor driver
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@...iatek.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@...iatek.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
Powered by blists - more mailing lists