| 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: <9d73a955-e242-42c6-804b-a11d8e8d24ed@kernel.org>
Date: Wed, 10 Sep 2025 17:32:18 +0200
From: Hans de Goede <hansg@...nel.org>
To: Kate Hsuan <hpa@...hat.com>, Mauro Carvalho Chehab <mchehab@...nel.org>,
Hans de Goede <hdegoede@...hat.com>,
Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7] media: Add t4ka3 camera sensor driver
p.s.
On 8-Nov-24 10:18 AM, Kate Hsuan wrote:
...
> diff --git a/drivers/media/i2c/t4ka3.c b/drivers/media/i2c/t4ka3.c
> new file mode 100644
> index 000000000000..ec0fcccf93b7
> --- /dev/null
> +++ b/drivers/media/i2c/t4ka3.c
> +static int t4ka3_update_exposure_range(struct t4ka3_data *sensor)
> +{
> + int exp_max = sensor->mode.fmt.height + sensor->ctrls.vblank->val -
> + T4KA3_COARSE_INTEGRATION_TIME_MARGIN;
The sensor->mode.fmt.height on the line above will no longer work
Instead you will need something like this:
struct v4l2_subdev_state *active_state =
v4l2_subdev_get_locked_active_state(&sensor->sd);
struct v4l2_mbus_framefmt *active_fmt =
v4l2_subdev_state_get_format(active_state, 0);
int exp_max = active_fmt->height + sensor->ctrls.vblank->val -
T4KA3_COARSE_INTEGRATION_TIME_MARGIN;
> +
> + return __v4l2_ctrl_modify_range(sensor->ctrls.exposure, 0, exp_max,
> + 1, exp_max);
> +}
...
> +static void t4ka3_calc_mode(struct t4ka3_data *sensor)
> +{
> + int width = sensor->mode.fmt.width;
> + int height = sensor->mode.fmt.height;
And a similar change here, ...
Except that this one is also going to need active_crop.
> + int binning;
> +
> + if (width <= (sensor->mode.crop.width / 2) &&
> + height <= (sensor->mode.crop.height / 2))
> + binning = 2;
> + else
> + binning = 1;
> +
> + width *= binning;
> + height *= binning;
> +
> + sensor->mode.binning = binning;
> + sensor->mode.win_x = (sensor->mode.crop.left +
> + (sensor->mode.crop.width - width) / 2) & ~1;
> + sensor->mode.win_y = (sensor->mode.crop.top +
> + (sensor->mode.crop.height - height) / 2) & ~1;
> + /*
> + * t4ka's window is done after binning, but must still be a multiple of 2 ?
> + * Round up to avoid top 2 black lines in 1640x1230 (quarter res) case.
> + */
> + sensor->mode.win_x = DIV_ROUND_UP(sensor->mode.win_x, binning);
> + sensor->mode.win_y = DIV_ROUND_UP(sensor->mode.win_y, binning);
> +}
> +
> +static void t4ka3_get_vblank_limits(struct t4ka3_data *sensor, int *min, int *max, int *def)
> +{
and here.
> + *min = T4KA3_MIN_VBLANK + (sensor->mode.binning - 1) * sensor->mode.fmt.height;
> + *max = T4KA3_MAX_VBLANK - sensor->mode.fmt.height;
> + *def = T4KA3_LINES_PER_FRAME_30FPS - sensor->mode.fmt.height;
> +}
...
> +/* Horizontal flip the image. */
> +static int t4ka3_t_hflip(struct v4l2_subdev *sd, int value)
> +{
> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> + int ret;
> + u64 val;
> +
> + if (sensor->streaming)
> + return -EBUSY;
> +
> + val = value ? T4KA3_HFLIP_BIT : 0;
> +
> + ret = cci_update_bits(sensor->regmap, T4KA3_REG_IMG_ORIENTATION,
> + T4KA3_HFLIP_BIT, val, NULL);
> + if (ret)
> + return ret;
> +
> + t4ka3_set_bayer_order(sensor, &sensor->mode.fmt);
and here you will also need to get the active_fmt and
pass that to t4ka3_set_bayer_order()
> + return 0;
> +}
> +
> +/* Vertically flip the image */
> +static int t4ka3_t_vflip(struct v4l2_subdev *sd, int value)
> +{
> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> + int ret;
> + u64 val;
> +
> + if (sensor->streaming)
> + return -EBUSY;
> +
> + val = value ? T4KA3_VFLIP_BIT : 0;
> +
> + ret = cci_update_bits(sensor->regmap, T4KA3_REG_IMG_ORIENTATION,
> + T4KA3_VFLIP_BIT, val, NULL);
> + if (ret)
> + return ret;
> +
> + t4ka3_set_bayer_order(sensor, &sensor->mode.fmt);
Idem.
> + return 0;
> +}
Note t4ka3_t_vflip() + t4ka3_t_hflip() should be merged
into 1: t4ka3_set_vflip() helper which takes an "u8 flip_bit"
as extra arg. These 2 are identical other then using
T4KA3_HFLIP_BIT vs T4KA3_VFLIP_BIT so just make that an arch
and merge the 2.
> +
> +static int t4ka3_test_pattern(struct t4ka3_data *sensor, s32 value)
> +{
> + return cci_write(sensor->regmap, T4KA3_REG_TEST_PATTERN_MODE, value, NULL);
> +}
> +
> +static int t4ka3_detect(struct t4ka3_data *sensor, u16 *id)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
> + struct i2c_adapter *adapter = client->adapter;
> + u64 high, low;
> + int ret = 0;
> +
> + /* i2c check */
> + if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
> + return -ENODEV;
> +
> + /* check sensor chip ID */
> + cci_read(sensor->regmap, T4KA3_REG_PRODUCT_ID_HIGH, &high, &ret);
> + cci_read(sensor->regmap, T4KA3_REG_PRODUCT_ID_LOW, &low, &ret);
> + if (ret)
> + return ret;
> +
> + *id = (((u8)high) << 8) | (u8)low;
> + if (*id != T4KA3_PRODUCT_ID) {
> + dev_err(sensor->dev, "main sensor t4ka3 ID error\n");
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +static int t4ka3_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct t4ka3_data *sensor = ctrl_to_t4ka3(ctrl);
> + int ret;
> +
> + /* Update exposure range on vblank changes */
> + if (ctrl->id == V4L2_CID_VBLANK) {
> + ret = t4ka3_update_exposure_range(sensor);
> + if (ret)
> + return ret;
> + }
> +
> + /* Only apply changes to the controls if the device is powered up */
> + if (!pm_runtime_get_if_in_use(sensor->sd.dev)) {
> + t4ka3_set_bayer_order(sensor, &sensor->mode.fmt);
Again pass active format here.
It seems that a t4ka3_get_active_fmt(struct t4ka3_data *sensor)
helper would be good, instead of writing this out everywhere.
> + return 0;
> + }
> +
> + switch (ctrl->id) {
> + case V4L2_CID_TEST_PATTERN:
> + ret = t4ka3_test_pattern(sensor, ctrl->val);
> + break;
> + case V4L2_CID_VFLIP:
> + ret = t4ka3_t_vflip(&sensor->sd, ctrl->val);
> + break;
> + case V4L2_CID_HFLIP:
> + ret = t4ka3_t_hflip(&sensor->sd, ctrl->val);
> + break;
> + case V4L2_CID_VBLANK:
> + ret = cci_write(sensor->regmap, T4KA3_REG_FRAME_LENGTH_LINES,
> + sensor->mode.fmt.height + ctrl->val, NULL);
idem.
> + break;
> + case V4L2_CID_EXPOSURE:
> + ret = cci_write(sensor->regmap, T4KA3_REG_COARSE_INTEGRATION_TIME,
> + ctrl->val, NULL);
> + break;
> + case V4L2_CID_ANALOGUE_GAIN:
> + ret = cci_write(sensor->regmap, T4KA3_REG_GLOBAL_GAIN,
> + ctrl->val, NULL);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + pm_runtime_put(sensor->sd.dev);
> + return ret;
> +}
> +
> +static int t4ka3_set_mode(struct t4ka3_data *sensor)
> +{
> + int ret = 0;
And another place where we will need active_fmt.
> +
> + cci_write(sensor->regmap, T4KA3_REG_HORZ_OUTPUT_SIZE, sensor->mode.fmt.width, &ret);
> + /* Write mode-height - 2 otherwise things don't work, hw-bug ? */
> + cci_write(sensor->regmap, T4KA3_REG_VERT_OUTPUT_SIZE, sensor->mode.fmt.height - 2, &ret);
> + /* Note overwritten by __v4l2_ctrl_handler_setup() based on vblank ctrl */
> + cci_write(sensor->regmap, T4KA3_REG_FRAME_LENGTH_LINES, T4KA3_LINES_PER_FRAME_30FPS, &ret);
> + cci_write(sensor->regmap, T4KA3_REG_PIXELS_PER_LINE, T4KA3_PIXELS_PER_LINE, &ret);
> + /* Always use the full sensor, using window to crop */
> + cci_write(sensor->regmap, T4KA3_REG_HORZ_START, 0, &ret);
> + cci_write(sensor->regmap, T4KA3_REG_VERT_START, 0, &ret);
> + cci_write(sensor->regmap, T4KA3_REG_HORZ_END, T4KA3_NATIVE_WIDTH - 1, &ret);
> + cci_write(sensor->regmap, T4KA3_REG_VERT_END, T4KA3_NATIVE_HEIGHT - 1, &ret);
> + /* Set window */
> + cci_write(sensor->regmap, T4KA3_REG_WIN_START_X, sensor->mode.win_x, &ret);
> + cci_write(sensor->regmap, T4KA3_REG_WIN_START_Y, sensor->mode.win_y, &ret);
> + cci_write(sensor->regmap, T4KA3_REG_WIN_WIDTH, sensor->mode.fmt.width, &ret);
> + cci_write(sensor->regmap, T4KA3_REG_WIN_HEIGHT, sensor->mode.fmt.height, &ret);
> + /* Write 1 to unknown register 0x0900 */
> + cci_write(sensor->regmap, T4KA3_REG_0900, 1, &ret);
> + cci_write(sensor->regmap, T4KA3_REG_BINNING, T4KA3_BINNING_VAL(sensor->mode.binning), &ret);
> +
> + return ret;
> +}
> +
...
> +static int t4ka3_init_controls(struct t4ka3_data *sensor)
> +{
> + const struct v4l2_ctrl_ops *ops = &t4ka3_ctrl_ops;
> + struct t4ka3_ctrls *ctrls = &sensor->ctrls;
> + struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> + struct v4l2_fwnode_device_properties props;
> + int ret, min, max, def;
> + static const char * const test_pattern_menu[] = {
> + "Disabled",
> + "Solid White",
> + "Color Bars",
> + "Gradient",
> + "Random Data",
> + };
> +
> + v4l2_ctrl_handler_init(hdl, 11);
> +
> + hdl->lock = &sensor->lock;
> +
> + ctrls->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> + ctrls->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> +
> + ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops,
> + V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(test_pattern_menu) - 1,
> + 0, 0, test_pattern_menu);
> + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
> + 0, 0, sensor->link_freq);
> + ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE,
> + 0, T4KA3_PIXEL_RATE,
> + 1, T4KA3_PIXEL_RATE);
> +
> + t4ka3_get_vblank_limits(sensor, &min, &max, &def);
> + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, min, max, 1, def);
> +
> + def = T4KA3_PIXELS_PER_LINE - sensor->mode.fmt.width;
> + ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
> + def, def, 1, def);
Here you can just use T4KA3_ACTIVE_WIDTH instead.
> +
> + max = T4KA3_LINES_PER_FRAME_30FPS - T4KA3_COARSE_INTEGRATION_TIME_MARGIN;
> + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> + 0, max, 1, max);
> +
> + ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> + T4KA3_MIN_GLOBAL_GAIN_SUPPORTED,
> + T4KA3_MAX_GLOBAL_GAIN_SUPPORTED,
> + 1, T4KA3_MIN_GLOBAL_GAIN_SUPPORTED);
> +
> + ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> + if (ret)
> + return ret;
> +
> + v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> +
> + if (hdl->error)
> + return hdl->error;
> +
> + ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> + ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> + ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> + ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + sensor->sd.ctrl_handler = hdl;
> + return 0;
> +}
...
> +static int t4ka3_probe(struct i2c_client *client)
> +{
> + struct t4ka3_data *sensor;
> + int ret;
> +
> + /* allocate sensor device & init sub device */
> + sensor = devm_kzalloc(&client->dev, sizeof(*sensor), GFP_KERNEL);
> + if (!sensor)
> + return -ENOMEM;
> +
> + sensor->dev = &client->dev;
> +
> + ret = t4ka3_check_hwcfg(sensor);
> + if (ret)
> + return ret;
> +
> + mutex_init(&sensor->lock);
> +
> + sensor->link_freq[0] = T4KA3_LINK_FREQ;
These 2 lines:
> + sensor->mode.crop = t4ka3_default_crop;
> + t4ka3_fill_format(sensor, &sensor->mode.fmt, T4KA3_ACTIVE_WIDTH, T4KA3_ACTIVE_HEIGHT);
can be dropped.
> + t4ka3_calc_mode(sensor);
> +
> + v4l2_i2c_subdev_init(&sensor->sd, client, &t4ka3_ops);
> + sensor->sd.internal_ops = &t4ka3_internal_ops;
> +
> + sensor->powerdown_gpio = devm_gpiod_get(&client->dev, "powerdown",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(sensor->powerdown_gpio))
> + return dev_err_probe(&client->dev, PTR_ERR(sensor->powerdown_gpio),
> + "getting powerdown GPIO\n");
> +
> + sensor->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(sensor->reset_gpio))
> + return dev_err_probe(&client->dev, PTR_ERR(sensor->reset_gpio),
> + "getting reset GPIO\n");
> +
> + sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> + if (IS_ERR(sensor->regmap))
> + return PTR_ERR(sensor->regmap);
> +
> + ret = t4ka3_pm_resume(sensor->dev);
> + if (ret)
> + return ret;
> +
> + pm_runtime_set_active(&client->dev);
> + pm_runtime_get_noresume(&client->dev);
> + pm_runtime_enable(&client->dev);
> +
> + sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> + sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> + ret = t4ka3_init_controls(sensor);
> + if (ret)
> + goto err_controls;
> +
> + ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> + if (ret)
> + goto err_controls;
> +
> + ret = v4l2_async_register_subdev_sensor(&sensor->sd);
> + if (ret)
> + goto err_media_entity;
> +
> + pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> + pm_runtime_use_autosuspend(&client->dev);
> + pm_runtime_put_autosuspend(&client->dev);
> +
> + return 0;
> +
> +err_media_entity:
> + media_entity_cleanup(&sensor->sd.entity);
> +err_controls:
> + v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> + pm_runtime_disable(&client->dev);
> + pm_runtime_put_noidle(&client->dev);
> + return ret;
> +}
Regards,
Hans
Powered by blists - more mailing lists