* [PATCH v11] media: Add t4ka3 camera sensor driver
@ 2026-03-16 8:57 Kate Hsuan
2026-03-16 22:00 ` Sakari Ailus
0 siblings, 1 reply; 12+ messages in thread
From: Kate Hsuan @ 2026-03-16 8:57 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Hans de Goede, Sakari Ailus
Cc: linux-media, linux-kernel, Kate Hsuan, Hans de Goede
Add the t4ka3 driver from:
https://github.com/kitakar5525/surface3-atomisp-cameras.git
With many cleanups / changes (almost a full rewrite) to make it suitable
for upstream:
* Remove the VCM and VCM-OTP support, the mainline kernel models VCMs and
calibration data eeproms as separate v4l2-subdev-s.
* Remove the integration-factor t4ka3_get_intg_factor() support and IOCTL,
this provided info to userspace through an atomisp private IOCTL.
* Turn atomisp specific exposure/gain IOCTL into standard v4l2 controls.
* Use normal ACPI power-management in combination with runtime-pm support
instead of atomisp specific GMIN power-management code.
* Turn into a standard V4L2 sensor driver using
v4l2_async_register_subdev_sensor().
* Add vblank, hblank, and link-freq controls; drop get_frame_interval().
* Use CCI register helpers.
* Calculate values for modes instead of using fixed register-value lists,
allowing arbritrary modes.
* Add get_selection() and set_selection() support
* Add a CSI2 bus configuration check
This been tested on a Xiaomi Mipad2 tablet which has a T4KA3 sensor with
DW9761 VCM as back sensor.
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Co-developed-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
Changes in v11:
1. Rebase on the latest next branch.
Changes in v10:
1. Fix the format settings.
2. Fix the hblank range calculation.
3. In t4ka3_enable_stream(), powerdown when pm_runtime_get_sync() fails.
4. Fix the clean up call sequence when removing the driver.
5. Fix the error handling in t4ka3_probe().
Changes in v9:
1. Apply Hans' fix patch to fix the lock issue and squash it into this
patch.
https://lore.kernel.org/linux-media/33dd5660-efb6-47e0-9672-f3ae65751185@kernel.org/
Changes in v8:
1. Drop the local mutex lock and v4l2-core manages all the locking.
2. __t4ka3_get_pad_format() and __t4ka3_get_pad_crop() are replaced with
v4l2_subdev_state_get_format() and v4l2_subdev_state_get_crop().
3. The deprecated s_stream was replaced with enable_streams() and
disable_streams().
4. Drop unused functions.
5. t4ka3_get_active_format() helper is used to get the active format.
6. v4l2_link_freq_to_bitmap() is used to check and get the supported
link frequency.
Changes in v7:
1. Add pixel_rate control.
Changes in v6:
1. t4ka3_s_config() was removed.
2. The unused macros were removed.
3. The runtime pm initial flow was improved.
4. In remove(), if the device is not in the "suspend" state, the device
will be manually turned off.
Changes in v5:
1. Improved Kconfig help description.
Changes in v4:
1. Another CI issue fixes.
Changes in v3:
1. Fix the issues reported by the CI system.
Changes in v2:
1. The regmap information was obtained before configuring runtime PM so
probe() can return without disabling runtime PM.
2. In t4ka3_s_stream(), return -EBUSY when the streaming is enabled.
---
drivers/media/i2c/Kconfig | 12 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/t4ka3.c | 1085 ++++++++++++++++++++++++++++++++++++
3 files changed, 1098 insertions(+)
create mode 100644 drivers/media/i2c/t4ka3.c
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 20482be35f26..6344defdbb51 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -790,6 +790,18 @@ config VIDEO_S5KJN1
To compile this driver as a module, choose M here: the
module will be called s5kjn1.
+config VIDEO_T4KA3
+ tristate "Toshiba T4KA3 sensor support"
+ depends on ACPI || COMPILE_TEST
+ depends on GPIOLIB
+ select V4L2_CCI_I2C
+ help
+ This is a Video4Linux2 sensor driver for the Toshiba T4KA3 8 MP
+ camera sensor.
+
+ To compile this driver as a module, choose M here: the
+ module will be called t4ka3.
+
config VIDEO_VD55G1
tristate "ST VD55G1 sensor support"
select V4L2_CCI_I2C
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index a3a6396df3c4..64c0c7964998 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -139,6 +139,7 @@ obj-$(CONFIG_VIDEO_SAA717X) += saa717x.o
obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
+obj-$(CONFIG_VIDEO_T4KA3) += t4ka3.o
obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
obj-$(CONFIG_VIDEO_TC358746) += tc358746.o
obj-$(CONFIG_VIDEO_TDA1997X) += tda1997x.o
diff --git a/drivers/media/i2c/t4ka3.c b/drivers/media/i2c/t4ka3.c
new file mode 100644
index 000000000000..d9af5e51f7a8
--- /dev/null
+++ b/drivers/media/i2c/t4ka3.c
@@ -0,0 +1,1085 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Support for T4KA3 8M camera sensor.
+ *
+ * Copyright (C) 2015 Intel Corporation. All Rights Reserved.
+ * Copyright (C) 2016 XiaoMi, Inc.
+ * Copyright (C) 2024 Hans de Goede <hansg@kernel.org>
+ */
+
+#include <linux/acpi.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+#include <media/media-entity.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-cci.h>
+#include <media/v4l2-common.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+#define T4KA3_NATIVE_WIDTH 3280
+#define T4KA3_NATIVE_HEIGHT 2464
+#define T4KA3_NATIVE_START_LEFT 0
+#define T4KA3_NATIVE_START_TOP 0
+#define T4KA3_ACTIVE_WIDTH 3280
+#define T4KA3_ACTIVE_HEIGHT 2460
+#define T4KA3_ACTIVE_START_LEFT 0
+#define T4KA3_ACTIVE_START_TOP 2
+#define T4KA3_MIN_CROP_WIDTH 2
+#define T4KA3_MIN_CROP_HEIGHT 2
+
+#define T4KA3_PIXELS_PER_LINE 3440
+#define T4KA3_LINES_PER_FRAME_30FPS 2492
+#define T4KA3_FPS 30
+#define T4KA3_PIXEL_RATE \
+ (T4KA3_PIXELS_PER_LINE * T4KA3_LINES_PER_FRAME_30FPS * T4KA3_FPS)
+
+/*
+ * TODO this really should be derived from the 19.2 MHz xvclk combined
+ * with the PLL settings. But without a datasheet this is the closest
+ * approximation possible.
+ *
+ * link-freq = pixel_rate * bpp / (lanes * 2)
+ * (lanes * 2) because CSI lanes use double-data-rate (DDR) signalling.
+ * bpp = 10 and lanes = 4
+ */
+#define T4KA3_LINK_FREQ ((u64)T4KA3_PIXEL_RATE * 10 / 8)
+
+/* For enum_frame_size() full-size + binned-/quarter-size */
+#define T4KA3_FRAME_SIZES 2
+
+#define T4KA3_REG_PRODUCT_ID_HIGH CCI_REG8(0x0000)
+#define T4KA3_REG_PRODUCT_ID_LOW CCI_REG8(0x0001)
+#define T4KA3_PRODUCT_ID 0x1490
+
+#define T4KA3_REG_STREAM CCI_REG8(0x0100)
+#define T4KA3_REG_IMG_ORIENTATION CCI_REG8(0x0101)
+#define T4KA3_HFLIP_BIT BIT(0)
+#define T4KA3_VFLIP_BIT BIT(1)
+#define T4KA3_REG_PARAM_HOLD CCI_REG8(0x0104)
+#define T4KA3_REG_COARSE_INTEGRATION_TIME CCI_REG16(0x0202)
+#define T4KA3_COARSE_INTEGRATION_TIME_MARGIN 6
+#define T4KA3_REG_DIGGAIN_GREEN_R CCI_REG16(0x020e)
+#define T4KA3_REG_DIGGAIN_RED CCI_REG16(0x0210)
+#define T4KA3_REG_DIGGAIN_BLUE CCI_REG16(0x0212)
+#define T4KA3_REG_DIGGAIN_GREEN_B CCI_REG16(0x0214)
+#define T4KA3_REG_GLOBAL_GAIN CCI_REG16(0x0234)
+#define T4KA3_MIN_GLOBAL_GAIN_SUPPORTED 0x0080
+#define T4KA3_MAX_GLOBAL_GAIN_SUPPORTED 0x07ff
+#define T4KA3_REG_FRAME_LENGTH_LINES CCI_REG16(0x0340) /* aka VTS */
+/* FIXME: need a datasheet to verify the min + max vblank values */
+#define T4KA3_MIN_VBLANK 4
+#define T4KA3_MAX_VBLANK 0xffff
+#define T4KA3_REG_PIXELS_PER_LINE CCI_REG16(0x0342) /* aka HTS */
+/* These 2 being horz/vert start is a guess (no datasheet), always 0 */
+#define T4KA3_REG_HORZ_START CCI_REG16(0x0344)
+#define T4KA3_REG_VERT_START CCI_REG16(0x0346)
+/* Always 3279 (T4KA3_NATIVE_WIDTH - 1, window is used to crop */
+#define T4KA3_REG_HORZ_END CCI_REG16(0x0348)
+/* Always 2463 (T4KA3_NATIVE_HEIGHT - 1, window is used to crop */
+#define T4KA3_REG_VERT_END CCI_REG16(0x034a)
+/* Output size (after cropping/window) */
+#define T4KA3_REG_HORZ_OUTPUT_SIZE CCI_REG16(0x034c)
+#define T4KA3_REG_VERT_OUTPUT_SIZE CCI_REG16(0x034e)
+/* Window/crop start + size *after* binning */
+#define T4KA3_REG_WIN_START_X CCI_REG16(0x0408)
+#define T4KA3_REG_WIN_START_Y CCI_REG16(0x040a)
+#define T4KA3_REG_WIN_WIDTH CCI_REG16(0x040c)
+#define T4KA3_REG_WIN_HEIGHT CCI_REG16(0x040e)
+#define T4KA3_REG_TEST_PATTERN_MODE CCI_REG8(0x0601)
+/* Unknown register at address 0x0900 */
+#define T4KA3_REG_0900 CCI_REG8(0x0900)
+#define T4KA3_REG_BINNING CCI_REG8(0x0901)
+#define T4KA3_BINNING_VAL(_b) \
+ ({ typeof(_b) (b) = (_b); \
+ ((b) << 4) | (b); })
+
+struct t4ka3_ctrls {
+ struct v4l2_ctrl_handler handler;
+ struct v4l2_ctrl *hflip;
+ struct v4l2_ctrl *vflip;
+ struct v4l2_ctrl *vblank;
+ struct v4l2_ctrl *hblank;
+ struct v4l2_ctrl *exposure;
+ struct v4l2_ctrl *gain;
+ struct v4l2_ctrl *test_pattern;
+ struct v4l2_ctrl *link_freq;
+ struct v4l2_ctrl *pixel_rate;
+};
+
+struct t4ka3_mode {
+ int binning;
+ u16 win_x;
+ u16 win_y;
+};
+
+struct t4ka3_data {
+ struct v4l2_subdev sd;
+ struct media_pad pad;
+ struct mutex lock; /* serialize sensor's ioctl */
+ struct t4ka3_ctrls ctrls;
+ struct t4ka3_mode mode;
+ struct device *dev;
+ struct regmap *regmap;
+ struct gpio_desc *powerdown_gpio;
+ struct gpio_desc *reset_gpio;
+ s64 link_freq[1];
+ int streaming;
+
+ /* MIPI lane info */
+ u32 link_freq_index;
+ u8 mipi_lanes;
+};
+
+/* init settings */
+static const struct cci_reg_sequence t4ka3_init_config[] = {
+ {CCI_REG8(0x4136), 0x13},
+ {CCI_REG8(0x4137), 0x33},
+ {CCI_REG8(0x3094), 0x01},
+ {CCI_REG8(0x0233), 0x01},
+ {CCI_REG8(0x4B06), 0x01},
+ {CCI_REG8(0x4B07), 0x01},
+ {CCI_REG8(0x3028), 0x01},
+ {CCI_REG8(0x3032), 0x14},
+ {CCI_REG8(0x305C), 0x0C},
+ {CCI_REG8(0x306D), 0x0A},
+ {CCI_REG8(0x3071), 0xFA},
+ {CCI_REG8(0x307E), 0x0A},
+ {CCI_REG8(0x307F), 0xFC},
+ {CCI_REG8(0x3091), 0x04},
+ {CCI_REG8(0x3092), 0x60},
+ {CCI_REG8(0x3096), 0xC0},
+ {CCI_REG8(0x3100), 0x07},
+ {CCI_REG8(0x3101), 0x4C},
+ {CCI_REG8(0x3118), 0xCC},
+ {CCI_REG8(0x3139), 0x06},
+ {CCI_REG8(0x313A), 0x06},
+ {CCI_REG8(0x313B), 0x04},
+ {CCI_REG8(0x3143), 0x02},
+ {CCI_REG8(0x314F), 0x0E},
+ {CCI_REG8(0x3169), 0x99},
+ {CCI_REG8(0x316A), 0x99},
+ {CCI_REG8(0x3171), 0x05},
+ {CCI_REG8(0x31A1), 0xA7},
+ {CCI_REG8(0x31A2), 0x9C},
+ {CCI_REG8(0x31A3), 0x8F},
+ {CCI_REG8(0x31A4), 0x75},
+ {CCI_REG8(0x31A5), 0xEE},
+ {CCI_REG8(0x31A6), 0xEA},
+ {CCI_REG8(0x31A7), 0xE4},
+ {CCI_REG8(0x31A8), 0xE4},
+ {CCI_REG8(0x31DF), 0x05},
+ {CCI_REG8(0x31EC), 0x1B},
+ {CCI_REG8(0x31ED), 0x1B},
+ {CCI_REG8(0x31EE), 0x1B},
+ {CCI_REG8(0x31F0), 0x1B},
+ {CCI_REG8(0x31F1), 0x1B},
+ {CCI_REG8(0x31F2), 0x1B},
+ {CCI_REG8(0x3204), 0x3F},
+ {CCI_REG8(0x3205), 0x03},
+ {CCI_REG8(0x3210), 0x01},
+ {CCI_REG8(0x3216), 0x68},
+ {CCI_REG8(0x3217), 0x58},
+ {CCI_REG8(0x3218), 0x58},
+ {CCI_REG8(0x321A), 0x68},
+ {CCI_REG8(0x321B), 0x60},
+ {CCI_REG8(0x3238), 0x03},
+ {CCI_REG8(0x3239), 0x03},
+ {CCI_REG8(0x323A), 0x05},
+ {CCI_REG8(0x323B), 0x06},
+ {CCI_REG8(0x3243), 0x03},
+ {CCI_REG8(0x3244), 0x08},
+ {CCI_REG8(0x3245), 0x01},
+ {CCI_REG8(0x3307), 0x19},
+ {CCI_REG8(0x3308), 0x19},
+ {CCI_REG8(0x3320), 0x01},
+ {CCI_REG8(0x3326), 0x15},
+ {CCI_REG8(0x3327), 0x0D},
+ {CCI_REG8(0x3328), 0x01},
+ {CCI_REG8(0x3380), 0x01},
+ {CCI_REG8(0x339E), 0x07},
+ {CCI_REG8(0x3424), 0x00},
+ {CCI_REG8(0x343C), 0x01},
+ {CCI_REG8(0x3398), 0x04},
+ {CCI_REG8(0x343A), 0x10},
+ {CCI_REG8(0x339A), 0x22},
+ {CCI_REG8(0x33B4), 0x00},
+ {CCI_REG8(0x3393), 0x01},
+ {CCI_REG8(0x33B3), 0x6E},
+ {CCI_REG8(0x3433), 0x06},
+ {CCI_REG8(0x3433), 0x00},
+ {CCI_REG8(0x33B3), 0x00},
+ {CCI_REG8(0x3393), 0x03},
+ {CCI_REG8(0x33B4), 0x03},
+ {CCI_REG8(0x343A), 0x00},
+ {CCI_REG8(0x339A), 0x00},
+ {CCI_REG8(0x3398), 0x00}
+};
+
+static const struct cci_reg_sequence t4ka3_pre_mode_set_regs[] = {
+ {CCI_REG8(0x0112), 0x0A},
+ {CCI_REG8(0x0113), 0x0A},
+ {CCI_REG8(0x0114), 0x03},
+ {CCI_REG8(0x4136), 0x13},
+ {CCI_REG8(0x4137), 0x33},
+ {CCI_REG8(0x0820), 0x0A},
+ {CCI_REG8(0x0821), 0x0D},
+ {CCI_REG8(0x0822), 0x00},
+ {CCI_REG8(0x0823), 0x00},
+ {CCI_REG8(0x0301), 0x0A},
+ {CCI_REG8(0x0303), 0x01},
+ {CCI_REG8(0x0305), 0x04},
+ {CCI_REG8(0x0306), 0x02},
+ {CCI_REG8(0x0307), 0x18},
+ {CCI_REG8(0x030B), 0x01},
+};
+
+static const struct cci_reg_sequence t4ka3_post_mode_set_regs[] = {
+ {CCI_REG8(0x0902), 0x00},
+ {CCI_REG8(0x4220), 0x00},
+ {CCI_REG8(0x4222), 0x01},
+ {CCI_REG8(0x3380), 0x01},
+ {CCI_REG8(0x3090), 0x88},
+ {CCI_REG8(0x3394), 0x20},
+ {CCI_REG8(0x3090), 0x08},
+ {CCI_REG8(0x3394), 0x10}
+};
+
+static const s64 link_freq_menu_items[] = {
+ T4KA3_LINK_FREQ,
+};
+
+static inline struct t4ka3_data *to_t4ka3_sensor(struct v4l2_subdev *sd)
+{
+ return container_of(sd, struct t4ka3_data, sd);
+}
+
+static inline struct t4ka3_data *ctrl_to_t4ka3(struct v4l2_ctrl *ctrl)
+{
+ return container_of(ctrl->handler, struct t4ka3_data, ctrls.handler);
+}
+
+/* T4KA3 default GRBG */
+static const int t4ka3_hv_flip_bayer_order[] = {
+ MEDIA_BUS_FMT_SGRBG10_1X10,
+ MEDIA_BUS_FMT_SBGGR10_1X10,
+ MEDIA_BUS_FMT_SRGGB10_1X10,
+ MEDIA_BUS_FMT_SGBRG10_1X10,
+};
+
+static const struct v4l2_rect t4ka3_default_crop = {
+ .left = T4KA3_ACTIVE_START_LEFT,
+ .top = T4KA3_ACTIVE_START_TOP,
+ .width = T4KA3_ACTIVE_WIDTH,
+ .height = T4KA3_ACTIVE_HEIGHT,
+};
+
+static int t4ka3_detect(struct t4ka3_data *sensor, u16 *id);
+
+static void t4ka3_set_bayer_order(struct t4ka3_data *sensor,
+ struct v4l2_mbus_framefmt *fmt)
+{
+ int hv_flip = 0;
+
+ if (sensor->ctrls.vflip && sensor->ctrls.vflip->val)
+ hv_flip += 1;
+
+ if (sensor->ctrls.hflip && sensor->ctrls.hflip->val)
+ hv_flip += 2;
+
+ fmt->code = t4ka3_hv_flip_bayer_order[hv_flip];
+}
+
+static struct v4l2_mbus_framefmt *t4ka3_get_active_format(struct t4ka3_data *sensor)
+{
+ struct v4l2_subdev_state *active_state =
+ v4l2_subdev_get_locked_active_state(&sensor->sd);
+
+ return v4l2_subdev_state_get_format(active_state, 0);
+}
+
+static struct v4l2_rect *t4ka3_get_active_crop(struct t4ka3_data *sensor)
+{
+ struct v4l2_subdev_state *active_state =
+ v4l2_subdev_get_locked_active_state(&sensor->sd);
+
+ return v4l2_subdev_state_get_crop(active_state, 0);
+}
+
+static int t4ka3_update_exposure_range(struct t4ka3_data *sensor)
+{
+ struct v4l2_mbus_framefmt *fmt;
+
+ fmt = t4ka3_get_active_format(sensor);
+
+ int exp_max = 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_fill_format(struct t4ka3_data *sensor,
+ struct v4l2_mbus_framefmt *fmt,
+ unsigned int width, unsigned int height)
+{
+ memset(fmt, 0, sizeof(*fmt));
+ fmt->width = width;
+ fmt->height = height;
+ fmt->field = V4L2_FIELD_NONE;
+ fmt->colorspace = V4L2_COLORSPACE_RAW;
+ t4ka3_set_bayer_order(sensor, fmt);
+}
+
+static void t4ka3_calc_mode(struct t4ka3_data *sensor)
+{
+ struct v4l2_mbus_framefmt *fmt;
+ struct v4l2_rect *crop;
+ int width;
+ int height;
+ int binning;
+
+ fmt = t4ka3_get_active_format(sensor);
+ crop = t4ka3_get_active_crop(sensor);
+
+ width = fmt->width;
+ height = fmt->height;
+
+ if (width <= (crop->width / 2) && height <= (crop->height / 2))
+ binning = 2;
+ else
+ binning = 1;
+
+ width *= binning;
+ height *= binning;
+
+ sensor->mode.binning = binning;
+ sensor->mode.win_x = (crop->left + (crop->width - width) / 2) & ~1;
+ sensor->mode.win_y = (crop->top + (crop->height - height) / 2) & ~1;
+ /*
+ * t4ka3'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)
+{
+ struct v4l2_mbus_framefmt *fmt;
+
+ fmt = t4ka3_get_active_format(sensor);
+
+ *min = T4KA3_MIN_VBLANK + (sensor->mode.binning - 1) * fmt->height;
+ *max = T4KA3_MAX_VBLANK - fmt->height;
+ *def = T4KA3_LINES_PER_FRAME_30FPS - fmt->height;
+}
+
+static int t4ka3_set_pad_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_format *format)
+{
+ struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
+ struct v4l2_mbus_framefmt *try_fmt;
+ struct v4l2_mbus_framefmt *fmt;
+ const struct v4l2_rect *crop;
+ unsigned int width, height;
+ int min, max, def, ret = 0;
+
+ crop = t4ka3_get_active_crop(sensor);
+ fmt = t4ka3_get_active_format(sensor);
+
+ /* Limit set_fmt max size to crop width / height */
+ width = clamp_val(ALIGN(format->format.width, 2),
+ T4KA3_MIN_CROP_WIDTH, crop->width);
+ height = clamp_val(ALIGN(format->format.height, 2),
+ T4KA3_MIN_CROP_HEIGHT, crop->height);
+ t4ka3_fill_format(sensor, &format->format, width, height);
+
+ if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+ try_fmt = v4l2_subdev_state_get_format(sd_state, 0);
+ *try_fmt = format->format;
+ return 0;
+ }
+
+ if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE && sensor->streaming)
+ return -EBUSY;
+
+ *v4l2_subdev_state_get_format(sd_state, 0) = format->format;
+
+ if (format->which == V4L2_SUBDEV_FORMAT_TRY)
+ return 0;
+
+ t4ka3_calc_mode(sensor);
+
+ /* vblank range is height dependent adjust and reset to default */
+ t4ka3_get_vblank_limits(sensor, &min, &max, &def);
+ ret = __v4l2_ctrl_modify_range(sensor->ctrls.vblank, min, max, 1, def);
+ if (ret)
+ return ret;
+
+ ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, def);
+ if (ret)
+ return ret;
+
+ def = T4KA3_PIXELS_PER_LINE - fmt->width;
+ ret = __v4l2_ctrl_modify_range(sensor->ctrls.hblank, def, def, 1, def);
+ if (ret)
+ return ret;
+
+ ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.hblank, def);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+/* Horizontal or vertically flip the image */
+static int t4ka3_t_vflip(struct v4l2_subdev *sd, int value, u8 flip_bit)
+{
+ struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
+ struct v4l2_mbus_framefmt *fmt;
+ int ret;
+ u64 val;
+
+ if (sensor->streaming)
+ return -EBUSY;
+
+ val = value ? flip_bit : 0;
+
+ ret = cci_update_bits(sensor->regmap, T4KA3_REG_IMG_ORIENTATION,
+ flip_bit, val, NULL);
+ if (ret)
+ return ret;
+
+ fmt = t4ka3_get_active_format(sensor);
+ t4ka3_set_bayer_order(sensor, fmt);
+ return 0;
+}
+
+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);
+ struct v4l2_mbus_framefmt *fmt;
+ int ret;
+
+ /* Update exposure range on vblank changes */
+ if (ctrl->id == V4L2_CID_VBLANK) {
+ ret = t4ka3_update_exposure_range(sensor);
+ if (ret)
+ return ret;
+ }
+
+ fmt = t4ka3_get_active_format(sensor);
+
+ /* 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, fmt);
+ 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, T4KA3_VFLIP_BIT);
+ break;
+ case V4L2_CID_HFLIP:
+ ret = t4ka3_t_vflip(&sensor->sd, ctrl->val, T4KA3_HFLIP_BIT);
+ break;
+ case V4L2_CID_VBLANK:
+ ret = cci_write(sensor->regmap, T4KA3_REG_FRAME_LENGTH_LINES,
+ fmt->height + ctrl->val, NULL);
+ 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)
+{
+ struct v4l2_mbus_framefmt *fmt;
+ int ret = 0;
+
+ fmt = t4ka3_get_active_format(sensor);
+
+ cci_write(sensor->regmap, T4KA3_REG_HORZ_OUTPUT_SIZE, fmt->width, &ret);
+ /* Write mode-height - 2 otherwise things don't work, hw-bug ? */
+ cci_write(sensor->regmap, T4KA3_REG_VERT_OUTPUT_SIZE, 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, fmt->width, &ret);
+ cci_write(sensor->regmap, T4KA3_REG_WIN_HEIGHT, 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_enable_stream(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
+ u32 pad, u64 streams_mask)
+{
+ struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
+ int ret;
+
+ ret = pm_runtime_get_sync(sensor->sd.dev);
+ if (ret < 0) {
+ dev_err(sensor->dev, "power-up err.\n");
+ goto error_powerdown;
+ }
+
+ cci_multi_reg_write(sensor->regmap, t4ka3_init_config,
+ ARRAY_SIZE(t4ka3_init_config), &ret);
+ /* enable group hold */
+ cci_write(sensor->regmap, T4KA3_REG_PARAM_HOLD, 1, &ret);
+ cci_multi_reg_write(sensor->regmap, t4ka3_pre_mode_set_regs,
+ ARRAY_SIZE(t4ka3_pre_mode_set_regs), &ret);
+ if (ret)
+ goto error_powerdown;
+
+ ret = t4ka3_set_mode(sensor);
+ if (ret)
+ goto error_powerdown;
+
+ ret = cci_multi_reg_write(sensor->regmap, t4ka3_post_mode_set_regs,
+ ARRAY_SIZE(t4ka3_post_mode_set_regs), NULL);
+ if (ret)
+ goto error_powerdown;
+
+ /* Restore value of all ctrls */
+ ret = __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
+ if (ret)
+ goto error_powerdown;
+
+ /* disable group hold */
+ cci_write(sensor->regmap, T4KA3_REG_PARAM_HOLD, 0, &ret);
+ cci_write(sensor->regmap, T4KA3_REG_STREAM, 1, &ret);
+ if (ret)
+ goto error_powerdown;
+
+ sensor->streaming = 1;
+
+ return ret;
+
+error_powerdown:
+ pm_runtime_put(sensor->sd.dev);
+ return ret;
+}
+
+static int t4ka3_disable_stream(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
+ u32 pad, u64 streams_mask)
+{
+ struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
+ int ret;
+
+ ret = cci_write(sensor->regmap, T4KA3_REG_STREAM, 0, NULL);
+ pm_runtime_put(sensor->sd.dev);
+ sensor->streaming = 0;
+ return ret;
+}
+
+static int t4ka3_get_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_selection *sel)
+{
+ switch (sel->target) {
+ case V4L2_SEL_TGT_CROP:
+ sel->r = *v4l2_subdev_state_get_crop(state, sel->pad);
+ break;
+ case V4L2_SEL_TGT_NATIVE_SIZE:
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ sel->r.top = 0;
+ sel->r.left = 0;
+ sel->r.width = T4KA3_NATIVE_WIDTH;
+ sel->r.height = T4KA3_NATIVE_HEIGHT;
+ break;
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ sel->r = t4ka3_default_crop;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int t4ka3_set_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_selection *sel)
+{
+ struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
+ struct v4l2_mbus_framefmt *format;
+ struct v4l2_rect *crop;
+ struct v4l2_rect rect;
+
+ if (sel->target != V4L2_SEL_TGT_CROP)
+ return -EINVAL;
+
+ /*
+ * Clamp the boundaries of the crop rectangle to the size of the sensor
+ * pixel array. Align to multiples of 2 to ensure Bayer pattern isn't
+ * disrupted.
+ */
+ rect.left = clamp_val(ALIGN(sel->r.left, 2),
+ T4KA3_NATIVE_START_LEFT, T4KA3_NATIVE_WIDTH);
+ rect.top = clamp_val(ALIGN(sel->r.top, 2),
+ T4KA3_NATIVE_START_TOP, T4KA3_NATIVE_HEIGHT);
+ rect.width = clamp_val(ALIGN(sel->r.width, 2), T4KA3_MIN_CROP_WIDTH,
+ T4KA3_NATIVE_WIDTH - rect.left);
+ rect.height = clamp_val(ALIGN(sel->r.height, 2), T4KA3_MIN_CROP_HEIGHT,
+ T4KA3_NATIVE_HEIGHT - rect.top);
+
+ crop = v4l2_subdev_state_get_crop(state, sel->pad);
+
+ if (rect.width != crop->width || rect.height != crop->height) {
+ /*
+ * Reset the output image size if the crop rectangle size has
+ * been modified.
+ */
+ format = v4l2_subdev_state_get_format(state, sel->pad);
+ format->width = rect.width;
+ format->height = rect.height;
+ if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+ t4ka3_calc_mode(sensor);
+ }
+
+ sel->r = *crop = rect;
+
+ return 0;
+}
+
+static int
+t4ka3_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ if (code->index)
+ return -EINVAL;
+
+ code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
+ return 0;
+}
+
+static int t4ka3_enum_frame_size(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_frame_size_enum *fse)
+{
+ struct v4l2_rect *crop;
+
+ if (fse->index >= T4KA3_FRAME_SIZES)
+ return -EINVAL;
+
+ crop = v4l2_subdev_state_get_crop(sd_state, fse->pad);
+
+ fse->min_width = crop->width / (fse->index + 1);
+ fse->min_height = crop->height / (fse->index + 1);
+ fse->max_width = fse->min_width;
+ fse->max_height = fse->min_height;
+
+ return 0;
+}
+
+static int t4ka3_check_hwcfg(struct t4ka3_data *sensor)
+{
+ struct fwnode_handle *fwnode = dev_fwnode(sensor->dev);
+ struct v4l2_fwnode_endpoint bus_cfg = {
+ .bus_type = V4L2_MBUS_CSI2_DPHY,
+ };
+ struct fwnode_handle *endpoint;
+ unsigned long link_freq_bitmap;
+ int ret;
+
+ /*
+ * Sometimes the fwnode graph is initialized by the bridge driver.
+ * Bridge drivers doing this may also add GPIO mappings, wait for this.
+ */
+ endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
+ if (!endpoint)
+ return dev_err_probe(sensor->dev, -EPROBE_DEFER,
+ "waiting for fwnode graph endpoint\n");
+
+ ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
+ fwnode_handle_put(endpoint);
+ if (ret)
+ return ret;
+
+ ret = v4l2_link_freq_to_bitmap(sensor->dev, bus_cfg.link_frequencies,
+ bus_cfg.nr_of_link_frequencies,
+ link_freq_menu_items,
+ ARRAY_SIZE(link_freq_menu_items),
+ &link_freq_bitmap);
+
+ if (ret == -ENOENT) {
+ dev_err_probe(sensor->dev, -ENOENT,
+ "No match found between driver-supported link frequencies.\n");
+ goto out_free_bus_cfg;
+ }
+
+ if (ret == -ENODATA) {
+ dev_err_probe(sensor->dev, -ENODATA,
+ "No link frequency was specified in the firmware.\n");
+ goto out_free_bus_cfg;
+ }
+
+ sensor->link_freq_index = ffs(link_freq_bitmap) - 1;
+
+ /* 4 MIPI lanes */
+ if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
+ ret = dev_err_probe(sensor->dev, -EINVAL,
+ "number of CSI2 data lanes %u is not supported\n",
+ bus_cfg.bus.mipi_csi2.num_data_lanes);
+ goto out_free_bus_cfg;
+ }
+
+ sensor->mipi_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
+
+out_free_bus_cfg:
+ v4l2_fwnode_endpoint_free(&bus_cfg);
+
+ return ret;
+}
+
+static int t4ka3_init_state(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state)
+{
+ struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
+
+ *v4l2_subdev_state_get_crop(sd_state, 0) = t4ka3_default_crop;
+
+ t4ka3_fill_format(sensor, v4l2_subdev_state_get_format(sd_state, 0),
+ T4KA3_ACTIVE_WIDTH, T4KA3_ACTIVE_HEIGHT);
+ return 0;
+}
+
+static const struct v4l2_ctrl_ops t4ka3_ctrl_ops = {
+ .s_ctrl = t4ka3_s_ctrl,
+};
+
+static const struct v4l2_subdev_video_ops t4ka3_video_ops = {
+ .s_stream = v4l2_subdev_s_stream_helper,
+};
+
+static const struct v4l2_subdev_pad_ops t4ka3_pad_ops = {
+ .enum_mbus_code = t4ka3_enum_mbus_code,
+ .enum_frame_size = t4ka3_enum_frame_size,
+ .get_fmt = v4l2_subdev_get_fmt,
+ .set_fmt = t4ka3_set_pad_format,
+ .get_selection = t4ka3_get_selection,
+ .set_selection = t4ka3_set_selection,
+ .enable_streams = t4ka3_enable_stream,
+ .disable_streams = t4ka3_disable_stream,
+};
+
+static const struct v4l2_subdev_ops t4ka3_ops = {
+ .video = &t4ka3_video_ops,
+ .pad = &t4ka3_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops t4ka3_internal_ops = {
+ .init_state = t4ka3_init_state,
+};
+
+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);
+
+ v4l2_subdev_lock_state(sensor->sd.active_state);
+ t4ka3_calc_mode(sensor);
+ t4ka3_get_vblank_limits(sensor, &min, &max, &def);
+ v4l2_subdev_unlock_state(sensor->sd.active_state);
+
+ ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, min, max, 1, def);
+
+ def = T4KA3_PIXELS_PER_LINE - T4KA3_ACTIVE_WIDTH;
+ ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
+ def, def, 1, def);
+
+ 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_pm_suspend(struct device *dev)
+{
+ struct t4ka3_data *sensor = dev_get_drvdata(dev);
+
+ gpiod_set_value_cansleep(sensor->powerdown_gpio, 1);
+ gpiod_set_value_cansleep(sensor->reset_gpio, 1);
+
+ return 0;
+}
+
+static int t4ka3_pm_resume(struct device *dev)
+{
+ struct t4ka3_data *sensor = dev_get_drvdata(dev);
+ u16 sensor_id;
+ int ret;
+
+ usleep_range(5000, 6000);
+
+ gpiod_set_value_cansleep(sensor->powerdown_gpio, 0);
+ gpiod_set_value_cansleep(sensor->reset_gpio, 0);
+
+ /* waiting for the sensor after powering up */
+ msleep(20);
+
+ ret = t4ka3_detect(sensor, &sensor_id);
+ if (ret) {
+ dev_err(sensor->dev, "sensor detect failed\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(t4ka3_pm_ops, t4ka3_pm_suspend, t4ka3_pm_resume, NULL);
+
+static void t4ka3_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
+
+ v4l2_async_unregister_subdev(&sensor->sd);
+ v4l2_ctrl_handler_free(&sensor->ctrls.handler);
+ v4l2_subdev_cleanup(sd);
+ media_entity_cleanup(&sensor->sd.entity);
+
+ /*
+ * Disable runtime PM. In case runtime PM is disabled in the kernel,
+ * make sure to turn power off manually.
+ */
+ pm_runtime_disable(&client->dev);
+ if (!pm_runtime_status_suspended(&client->dev))
+ t4ka3_pm_suspend(&client->dev);
+ pm_runtime_set_suspended(&client->dev);
+}
+
+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;
+
+ 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 = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
+ if (ret)
+ goto err_pm_disable;
+
+ sensor->sd.state_lock = sensor->ctrls.handler.lock;
+ ret = v4l2_subdev_init_finalize(&sensor->sd);
+ if (ret < 0) {
+ dev_err(&client->dev, "failed to init subdev: %d", ret);
+ goto err_media_entity;
+ }
+
+ ret = t4ka3_init_controls(sensor);
+ if (ret)
+ goto err_controls;
+
+ ret = v4l2_async_register_subdev_sensor(&sensor->sd);
+ if (ret)
+ goto err_controls;
+
+ pm_runtime_set_autosuspend_delay(&client->dev, 1000);
+ pm_runtime_use_autosuspend(&client->dev);
+ pm_runtime_put_autosuspend(&client->dev);
+
+ return 0;
+
+err_controls:
+ v4l2_ctrl_handler_free(&sensor->ctrls.handler);
+ v4l2_subdev_cleanup(&sensor->sd);
+
+err_media_entity:
+ media_entity_cleanup(&sensor->sd.entity);
+
+err_pm_disable:
+ pm_runtime_disable(&client->dev);
+ pm_runtime_put_noidle(&client->dev);
+ t4ka3_pm_suspend(&client->dev);
+
+ return ret;
+}
+
+static struct acpi_device_id t4ka3_acpi_match[] = {
+ { "XMCC0003" },
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, t4ka3_acpi_match);
+
+static struct i2c_driver t4ka3_driver = {
+ .driver = {
+ .name = "t4ka3",
+ .acpi_match_table = ACPI_PTR(t4ka3_acpi_match),
+ .pm = pm_sleep_ptr(&t4ka3_pm_ops),
+ },
+ .probe = t4ka3_probe,
+ .remove = t4ka3_remove,
+};
+module_i2c_driver(t4ka3_driver)
+
+MODULE_DESCRIPTION("A low-level driver for T4KA3 sensor");
+MODULE_AUTHOR("HARVEY LV <harvey.lv@intel.com>");
+MODULE_AUTHOR("Kate Hsuan <hpa@redhat.com>");
+MODULE_LICENSE("GPL");
--
2.53.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v11] media: Add t4ka3 camera sensor driver
2026-03-16 8:57 [PATCH v11] media: Add t4ka3 camera sensor driver Kate Hsuan
@ 2026-03-16 22:00 ` Sakari Ailus
2026-03-17 12:04 ` Hans de Goede
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Sakari Ailus @ 2026-03-16 22:00 UTC (permalink / raw)
To: Kate Hsuan
Cc: Mauro Carvalho Chehab, Hans de Goede, linux-media, linux-kernel,
Hans de Goede
Hi Kate,
Thanks for the patch.
Where have you seen this sensor being used, if I may ask?
On Mon, Mar 16, 2026 at 04:57:04PM +0800, Kate Hsuan wrote:
> Add the t4ka3 driver from:
> https://github.com/kitakar5525/surface3-atomisp-cameras.git
>
> With many cleanups / changes (almost a full rewrite) to make it suitable
> for upstream:
>
> * Remove the VCM and VCM-OTP support, the mainline kernel models VCMs and
> calibration data eeproms as separate v4l2-subdev-s.
>
> * Remove the integration-factor t4ka3_get_intg_factor() support and IOCTL,
> this provided info to userspace through an atomisp private IOCTL.
>
> * Turn atomisp specific exposure/gain IOCTL into standard v4l2 controls.
>
> * Use normal ACPI power-management in combination with runtime-pm support
> instead of atomisp specific GMIN power-management code.
>
> * Turn into a standard V4L2 sensor driver using
> v4l2_async_register_subdev_sensor().
>
> * Add vblank, hblank, and link-freq controls; drop get_frame_interval().
>
> * Use CCI register helpers.
>
> * Calculate values for modes instead of using fixed register-value lists,
> allowing arbritrary modes.
>
> * Add get_selection() and set_selection() support
>
> * Add a CSI2 bus configuration check
>
> This been tested on a Xiaomi Mipad2 tablet which has a T4KA3 sensor with
> DW9761 VCM as back sensor.
>
> Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> Co-developed-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
> Changes in v11:
> 1. Rebase on the latest next branch.
>
> Changes in v10:
> 1. Fix the format settings.
> 2. Fix the hblank range calculation.
> 3. In t4ka3_enable_stream(), powerdown when pm_runtime_get_sync() fails.
> 4. Fix the clean up call sequence when removing the driver.
> 5. Fix the error handling in t4ka3_probe().
>
> Changes in v9:
> 1. Apply Hans' fix patch to fix the lock issue and squash it into this
> patch.
> https://lore.kernel.org/linux-media/33dd5660-efb6-47e0-9672-f3ae65751185@kernel.org/
>
> Changes in v8:
> 1. Drop the local mutex lock and v4l2-core manages all the locking.
> 2. __t4ka3_get_pad_format() and __t4ka3_get_pad_crop() are replaced with
> v4l2_subdev_state_get_format() and v4l2_subdev_state_get_crop().
> 3. The deprecated s_stream was replaced with enable_streams() and
> disable_streams().
> 4. Drop unused functions.
> 5. t4ka3_get_active_format() helper is used to get the active format.
> 6. v4l2_link_freq_to_bitmap() is used to check and get the supported
> link frequency.
>
> Changes in v7:
> 1. Add pixel_rate control.
>
> Changes in v6:
> 1. t4ka3_s_config() was removed.
> 2. The unused macros were removed.
> 3. The runtime pm initial flow was improved.
> 4. In remove(), if the device is not in the "suspend" state, the device
> will be manually turned off.
>
> Changes in v5:
> 1. Improved Kconfig help description.
>
> Changes in v4:
> 1. Another CI issue fixes.
>
> Changes in v3:
> 1. Fix the issues reported by the CI system.
>
> Changes in v2:
> 1. The regmap information was obtained before configuring runtime PM so
> probe() can return without disabling runtime PM.
> 2. In t4ka3_s_stream(), return -EBUSY when the streaming is enabled.
> ---
> drivers/media/i2c/Kconfig | 12 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/t4ka3.c | 1085 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 1098 insertions(+)
> create mode 100644 drivers/media/i2c/t4ka3.c
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 20482be35f26..6344defdbb51 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -790,6 +790,18 @@ config VIDEO_S5KJN1
> To compile this driver as a module, choose M here: the
> module will be called s5kjn1.
>
> +config VIDEO_T4KA3
> + tristate "Toshiba T4KA3 sensor support"
> + depends on ACPI || COMPILE_TEST
> + depends on GPIOLIB
> + select V4L2_CCI_I2C
> + help
> + This is a Video4Linux2 sensor driver for the Toshiba T4KA3 8 MP
> + camera sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called t4ka3.
> +
> config VIDEO_VD55G1
> tristate "ST VD55G1 sensor support"
> select V4L2_CCI_I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index a3a6396df3c4..64c0c7964998 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -139,6 +139,7 @@ obj-$(CONFIG_VIDEO_SAA717X) += saa717x.o
> obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
> obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
> obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
> +obj-$(CONFIG_VIDEO_T4KA3) += t4ka3.o
> obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
> obj-$(CONFIG_VIDEO_TC358746) += tc358746.o
> obj-$(CONFIG_VIDEO_TDA1997X) += tda1997x.o
> diff --git a/drivers/media/i2c/t4ka3.c b/drivers/media/i2c/t4ka3.c
> new file mode 100644
> index 000000000000..d9af5e51f7a8
> --- /dev/null
> +++ b/drivers/media/i2c/t4ka3.c
> @@ -0,0 +1,1085 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Support for T4KA3 8M camera sensor.
> + *
> + * Copyright (C) 2015 Intel Corporation. All Rights Reserved.
> + * Copyright (C) 2016 XiaoMi, Inc.
> + * Copyright (C) 2024 Hans de Goede <hansg@kernel.org>
Any 2026 copyrights?
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/dev_printk.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +#include <media/media-entity.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-cci.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define T4KA3_NATIVE_WIDTH 3280
> +#define T4KA3_NATIVE_HEIGHT 2464
> +#define T4KA3_NATIVE_START_LEFT 0
> +#define T4KA3_NATIVE_START_TOP 0
> +#define T4KA3_ACTIVE_WIDTH 3280
> +#define T4KA3_ACTIVE_HEIGHT 2460
> +#define T4KA3_ACTIVE_START_LEFT 0
> +#define T4KA3_ACTIVE_START_TOP 2
> +#define T4KA3_MIN_CROP_WIDTH 2
> +#define T4KA3_MIN_CROP_HEIGHT 2
> +
> +#define T4KA3_PIXELS_PER_LINE 3440
> +#define T4KA3_LINES_PER_FRAME_30FPS 2492
> +#define T4KA3_FPS 30
> +#define T4KA3_PIXEL_RATE \
> + (T4KA3_PIXELS_PER_LINE * T4KA3_LINES_PER_FRAME_30FPS * T4KA3_FPS)
> +
> +/*
> + * TODO this really should be derived from the 19.2 MHz xvclk combined
> + * with the PLL settings. But without a datasheet this is the closest
> + * approximation possible.
> + *
> + * link-freq = pixel_rate * bpp / (lanes * 2)
> + * (lanes * 2) because CSI lanes use double-data-rate (DDR) signalling.
> + * bpp = 10 and lanes = 4
> + */
> +#define T4KA3_LINK_FREQ ((u64)T4KA3_PIXEL_RATE * 10 / 8)
> +
> +/* For enum_frame_size() full-size + binned-/quarter-size */
> +#define T4KA3_FRAME_SIZES 2
> +
> +#define T4KA3_REG_PRODUCT_ID_HIGH CCI_REG8(0x0000)
> +#define T4KA3_REG_PRODUCT_ID_LOW CCI_REG8(0x0001)
> +#define T4KA3_PRODUCT_ID 0x1490
> +
> +#define T4KA3_REG_STREAM CCI_REG8(0x0100)
> +#define T4KA3_REG_IMG_ORIENTATION CCI_REG8(0x0101)
> +#define T4KA3_HFLIP_BIT BIT(0)
> +#define T4KA3_VFLIP_BIT BIT(1)
> +#define T4KA3_REG_PARAM_HOLD CCI_REG8(0x0104)
> +#define T4KA3_REG_COARSE_INTEGRATION_TIME CCI_REG16(0x0202)
> +#define T4KA3_COARSE_INTEGRATION_TIME_MARGIN 6
> +#define T4KA3_REG_DIGGAIN_GREEN_R CCI_REG16(0x020e)
> +#define T4KA3_REG_DIGGAIN_RED CCI_REG16(0x0210)
> +#define T4KA3_REG_DIGGAIN_BLUE CCI_REG16(0x0212)
> +#define T4KA3_REG_DIGGAIN_GREEN_B CCI_REG16(0x0214)
> +#define T4KA3_REG_GLOBAL_GAIN CCI_REG16(0x0234)
> +#define T4KA3_MIN_GLOBAL_GAIN_SUPPORTED 0x0080
> +#define T4KA3_MAX_GLOBAL_GAIN_SUPPORTED 0x07ff
> +#define T4KA3_REG_FRAME_LENGTH_LINES CCI_REG16(0x0340) /* aka VTS */
> +/* FIXME: need a datasheet to verify the min + max vblank values */
> +#define T4KA3_MIN_VBLANK 4
> +#define T4KA3_MAX_VBLANK 0xffff
> +#define T4KA3_REG_PIXELS_PER_LINE CCI_REG16(0x0342) /* aka HTS */
> +/* These 2 being horz/vert start is a guess (no datasheet), always 0 */
> +#define T4KA3_REG_HORZ_START CCI_REG16(0x0344)
> +#define T4KA3_REG_VERT_START CCI_REG16(0x0346)
> +/* Always 3279 (T4KA3_NATIVE_WIDTH - 1, window is used to crop */
> +#define T4KA3_REG_HORZ_END CCI_REG16(0x0348)
> +/* Always 2463 (T4KA3_NATIVE_HEIGHT - 1, window is used to crop */
> +#define T4KA3_REG_VERT_END CCI_REG16(0x034a)
> +/* Output size (after cropping/window) */
> +#define T4KA3_REG_HORZ_OUTPUT_SIZE CCI_REG16(0x034c)
> +#define T4KA3_REG_VERT_OUTPUT_SIZE CCI_REG16(0x034e)
> +/* Window/crop start + size *after* binning */
> +#define T4KA3_REG_WIN_START_X CCI_REG16(0x0408)
> +#define T4KA3_REG_WIN_START_Y CCI_REG16(0x040a)
> +#define T4KA3_REG_WIN_WIDTH CCI_REG16(0x040c)
> +#define T4KA3_REG_WIN_HEIGHT CCI_REG16(0x040e)
> +#define T4KA3_REG_TEST_PATTERN_MODE CCI_REG8(0x0601)
> +/* Unknown register at address 0x0900 */
> +#define T4KA3_REG_0900 CCI_REG8(0x0900)
> +#define T4KA3_REG_BINNING CCI_REG8(0x0901)
> +#define T4KA3_BINNING_VAL(_b) \
> + ({ typeof(_b) (b) = (_b); \
> + ((b) << 4) | (b); })
I'd either use an inline function or a regular macro here; in the latter
case I wouldn't mind about the checkpatch.pl warning related to argument
double use.
> +
> +struct t4ka3_ctrls {
> + struct v4l2_ctrl_handler handler;
> + struct v4l2_ctrl *hflip;
> + struct v4l2_ctrl *vflip;
> + struct v4l2_ctrl *vblank;
> + struct v4l2_ctrl *hblank;
> + struct v4l2_ctrl *exposure;
> + struct v4l2_ctrl *gain;
> + struct v4l2_ctrl *test_pattern;
> + struct v4l2_ctrl *link_freq;
> + struct v4l2_ctrl *pixel_rate;
Do you need all these in the struct? E.g. gain appears to be unused.
> +};
> +
> +struct t4ka3_mode {
> + int binning;
> + u16 win_x;
> + u16 win_y;
The rest of the fields have just a space between the type and the field
name. I'd do the same here.
> +};
> +
> +struct t4ka3_data {
> + struct v4l2_subdev sd;
> + struct media_pad pad;
> + struct mutex lock; /* serialize sensor's ioctl */
> + struct t4ka3_ctrls ctrls;
> + struct t4ka3_mode mode;
> + struct device *dev;
> + struct regmap *regmap;
> + struct gpio_desc *powerdown_gpio;
> + struct gpio_desc *reset_gpio;
> + s64 link_freq[1];
> + int streaming;
> +
> + /* MIPI lane info */
> + u32 link_freq_index;
> + u8 mipi_lanes;
> +};
> +
> +/* init settings */
> +static const struct cci_reg_sequence t4ka3_init_config[] = {
> + {CCI_REG8(0x4136), 0x13},
{ Spaces inside braces, please. },
> + {CCI_REG8(0x4137), 0x33},
> + {CCI_REG8(0x3094), 0x01},
> + {CCI_REG8(0x0233), 0x01},
> + {CCI_REG8(0x4B06), 0x01},
> + {CCI_REG8(0x4B07), 0x01},
> + {CCI_REG8(0x3028), 0x01},
> + {CCI_REG8(0x3032), 0x14},
> + {CCI_REG8(0x305C), 0x0C},
> + {CCI_REG8(0x306D), 0x0A},
> + {CCI_REG8(0x3071), 0xFA},
> + {CCI_REG8(0x307E), 0x0A},
> + {CCI_REG8(0x307F), 0xFC},
> + {CCI_REG8(0x3091), 0x04},
> + {CCI_REG8(0x3092), 0x60},
> + {CCI_REG8(0x3096), 0xC0},
> + {CCI_REG8(0x3100), 0x07},
> + {CCI_REG8(0x3101), 0x4C},
> + {CCI_REG8(0x3118), 0xCC},
> + {CCI_REG8(0x3139), 0x06},
> + {CCI_REG8(0x313A), 0x06},
> + {CCI_REG8(0x313B), 0x04},
> + {CCI_REG8(0x3143), 0x02},
> + {CCI_REG8(0x314F), 0x0E},
> + {CCI_REG8(0x3169), 0x99},
> + {CCI_REG8(0x316A), 0x99},
> + {CCI_REG8(0x3171), 0x05},
> + {CCI_REG8(0x31A1), 0xA7},
> + {CCI_REG8(0x31A2), 0x9C},
> + {CCI_REG8(0x31A3), 0x8F},
> + {CCI_REG8(0x31A4), 0x75},
> + {CCI_REG8(0x31A5), 0xEE},
> + {CCI_REG8(0x31A6), 0xEA},
> + {CCI_REG8(0x31A7), 0xE4},
> + {CCI_REG8(0x31A8), 0xE4},
> + {CCI_REG8(0x31DF), 0x05},
> + {CCI_REG8(0x31EC), 0x1B},
> + {CCI_REG8(0x31ED), 0x1B},
> + {CCI_REG8(0x31EE), 0x1B},
> + {CCI_REG8(0x31F0), 0x1B},
> + {CCI_REG8(0x31F1), 0x1B},
> + {CCI_REG8(0x31F2), 0x1B},
> + {CCI_REG8(0x3204), 0x3F},
> + {CCI_REG8(0x3205), 0x03},
> + {CCI_REG8(0x3210), 0x01},
> + {CCI_REG8(0x3216), 0x68},
> + {CCI_REG8(0x3217), 0x58},
> + {CCI_REG8(0x3218), 0x58},
> + {CCI_REG8(0x321A), 0x68},
> + {CCI_REG8(0x321B), 0x60},
> + {CCI_REG8(0x3238), 0x03},
> + {CCI_REG8(0x3239), 0x03},
> + {CCI_REG8(0x323A), 0x05},
> + {CCI_REG8(0x323B), 0x06},
> + {CCI_REG8(0x3243), 0x03},
> + {CCI_REG8(0x3244), 0x08},
> + {CCI_REG8(0x3245), 0x01},
> + {CCI_REG8(0x3307), 0x19},
> + {CCI_REG8(0x3308), 0x19},
> + {CCI_REG8(0x3320), 0x01},
> + {CCI_REG8(0x3326), 0x15},
> + {CCI_REG8(0x3327), 0x0D},
> + {CCI_REG8(0x3328), 0x01},
> + {CCI_REG8(0x3380), 0x01},
> + {CCI_REG8(0x339E), 0x07},
> + {CCI_REG8(0x3424), 0x00},
> + {CCI_REG8(0x343C), 0x01},
> + {CCI_REG8(0x3398), 0x04},
> + {CCI_REG8(0x343A), 0x10},
> + {CCI_REG8(0x339A), 0x22},
> + {CCI_REG8(0x33B4), 0x00},
> + {CCI_REG8(0x3393), 0x01},
> + {CCI_REG8(0x33B3), 0x6E},
> + {CCI_REG8(0x3433), 0x06},
> + {CCI_REG8(0x3433), 0x00},
> + {CCI_REG8(0x33B3), 0x00},
> + {CCI_REG8(0x3393), 0x03},
> + {CCI_REG8(0x33B4), 0x03},
> + {CCI_REG8(0x343A), 0x00},
> + {CCI_REG8(0x339A), 0x00},
> + {CCI_REG8(0x3398), 0x00}
> +};
> +
> +static const struct cci_reg_sequence t4ka3_pre_mode_set_regs[] = {
> + {CCI_REG8(0x0112), 0x0A},
> + {CCI_REG8(0x0113), 0x0A},
> + {CCI_REG8(0x0114), 0x03},
> + {CCI_REG8(0x4136), 0x13},
> + {CCI_REG8(0x4137), 0x33},
> + {CCI_REG8(0x0820), 0x0A},
> + {CCI_REG8(0x0821), 0x0D},
> + {CCI_REG8(0x0822), 0x00},
> + {CCI_REG8(0x0823), 0x00},
> + {CCI_REG8(0x0301), 0x0A},
> + {CCI_REG8(0x0303), 0x01},
> + {CCI_REG8(0x0305), 0x04},
> + {CCI_REG8(0x0306), 0x02},
> + {CCI_REG8(0x0307), 0x18},
> + {CCI_REG8(0x030B), 0x01},
> +};
> +
> +static const struct cci_reg_sequence t4ka3_post_mode_set_regs[] = {
> + {CCI_REG8(0x0902), 0x00},
> + {CCI_REG8(0x4220), 0x00},
> + {CCI_REG8(0x4222), 0x01},
> + {CCI_REG8(0x3380), 0x01},
> + {CCI_REG8(0x3090), 0x88},
> + {CCI_REG8(0x3394), 0x20},
> + {CCI_REG8(0x3090), 0x08},
> + {CCI_REG8(0x3394), 0x10}
> +};
> +
> +static const s64 link_freq_menu_items[] = {
> + T4KA3_LINK_FREQ,
> +};
> +
> +static inline struct t4ka3_data *to_t4ka3_sensor(struct v4l2_subdev *sd)
> +{
> + return container_of(sd, struct t4ka3_data, sd);
> +}
> +
> +static inline struct t4ka3_data *ctrl_to_t4ka3(struct v4l2_ctrl *ctrl)
> +{
> + return container_of(ctrl->handler, struct t4ka3_data, ctrls.handler);
> +}
I'd use macros and container_of_const().
> +
> +/* T4KA3 default GRBG */
> +static const int t4ka3_hv_flip_bayer_order[] = {
> + MEDIA_BUS_FMT_SGRBG10_1X10,
> + MEDIA_BUS_FMT_SBGGR10_1X10,
> + MEDIA_BUS_FMT_SRGGB10_1X10,
> + MEDIA_BUS_FMT_SGBRG10_1X10,
> +};
> +
> +static const struct v4l2_rect t4ka3_default_crop = {
> + .left = T4KA3_ACTIVE_START_LEFT,
> + .top = T4KA3_ACTIVE_START_TOP,
> + .width = T4KA3_ACTIVE_WIDTH,
> + .height = T4KA3_ACTIVE_HEIGHT,
> +};
> +
> +static int t4ka3_detect(struct t4ka3_data *sensor, u16 *id);
Not needed.
> +
> +static void t4ka3_set_bayer_order(struct t4ka3_data *sensor,
> + struct v4l2_mbus_framefmt *fmt)
> +{
> + int hv_flip = 0;
unsigned int?
> +
> + if (sensor->ctrls.vflip && sensor->ctrls.vflip->val)
> + hv_flip += 1;
> +
> + if (sensor->ctrls.hflip && sensor->ctrls.hflip->val)
> + hv_flip += 2;
> +
> + fmt->code = t4ka3_hv_flip_bayer_order[hv_flip];
> +}
> +
> +static struct v4l2_mbus_framefmt *t4ka3_get_active_format(struct t4ka3_data *sensor)
> +{
> + struct v4l2_subdev_state *active_state =
> + v4l2_subdev_get_locked_active_state(&sensor->sd);
> +
> + return v4l2_subdev_state_get_format(active_state, 0);
> +}
> +
> +static struct v4l2_rect *t4ka3_get_active_crop(struct t4ka3_data *sensor)
> +{
> + struct v4l2_subdev_state *active_state =
> + v4l2_subdev_get_locked_active_state(&sensor->sd);
> +
> + return v4l2_subdev_state_get_crop(active_state, 0);
Please avoid adding such helpers.
> +}
> +
> +static int t4ka3_update_exposure_range(struct t4ka3_data *sensor)
> +{
> + struct v4l2_mbus_framefmt *fmt;
> +
> + fmt = t4ka3_get_active_format(sensor);
Can be assigned in declaration.
> +
> + int exp_max = 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_fill_format(struct t4ka3_data *sensor,
> + struct v4l2_mbus_framefmt *fmt,
> + unsigned int width, unsigned int height)
> +{
> + memset(fmt, 0, sizeof(*fmt));
> + fmt->width = width;
> + fmt->height = height;
> + fmt->field = V4L2_FIELD_NONE;
> + fmt->colorspace = V4L2_COLORSPACE_RAW;
> + t4ka3_set_bayer_order(sensor, fmt);
> +}
> +
> +static void t4ka3_calc_mode(struct t4ka3_data *sensor)
> +{
> + struct v4l2_mbus_framefmt *fmt;
> + struct v4l2_rect *crop;
> + int width;
> + int height;
> + int binning;
> +
> + fmt = t4ka3_get_active_format(sensor);
> + crop = t4ka3_get_active_crop(sensor);
Ditto.
> +
> + width = fmt->width;
> + height = fmt->height;
> +
> + if (width <= (crop->width / 2) && height <= (crop->height / 2))
> + binning = 2;
> + else
> + binning = 1;
> +
> + width *= binning;
> + height *= binning;
> +
> + sensor->mode.binning = binning;
> + sensor->mode.win_x = (crop->left + (crop->width - width) / 2) & ~1;
> + sensor->mode.win_y = (crop->top + (crop->height - height) / 2) & ~1;
> + /*
> + * t4ka3'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)
> +{
> + struct v4l2_mbus_framefmt *fmt;
> +
> + fmt = t4ka3_get_active_format(sensor);
Ditto.
> +
> + *min = T4KA3_MIN_VBLANK + (sensor->mode.binning - 1) * fmt->height;
> + *max = T4KA3_MAX_VBLANK - fmt->height;
> + *def = T4KA3_LINES_PER_FRAME_30FPS - fmt->height;
> +}
> +
> +static int t4ka3_set_pad_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_format *format)
> +{
> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> + struct v4l2_mbus_framefmt *try_fmt;
> + struct v4l2_mbus_framefmt *fmt;
> + const struct v4l2_rect *crop;
> + unsigned int width, height;
> + int min, max, def, ret = 0;
> +
> + crop = t4ka3_get_active_crop(sensor);
> + fmt = t4ka3_get_active_format(sensor);
> +
> + /* Limit set_fmt max size to crop width / height */
> + width = clamp_val(ALIGN(format->format.width, 2),
> + T4KA3_MIN_CROP_WIDTH, crop->width);
> + height = clamp_val(ALIGN(format->format.height, 2),
> + T4KA3_MIN_CROP_HEIGHT, crop->height);
> + t4ka3_fill_format(sensor, &format->format, width, height);
> +
> + if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> + try_fmt = v4l2_subdev_state_get_format(sd_state, 0);
> + *try_fmt = format->format;
> + return 0;
> + }
> +
> + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE && sensor->streaming)
> + return -EBUSY;
> +
> + *v4l2_subdev_state_get_format(sd_state, 0) = format->format;
> +
> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> + return 0;
> +
> + t4ka3_calc_mode(sensor);
> +
> + /* vblank range is height dependent adjust and reset to default */
> + t4ka3_get_vblank_limits(sensor, &min, &max, &def);
> + ret = __v4l2_ctrl_modify_range(sensor->ctrls.vblank, min, max, 1, def);
> + if (ret)
> + return ret;
> +
> + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, def);
> + if (ret)
> + return ret;
> +
> + def = T4KA3_PIXELS_PER_LINE - fmt->width;
> + ret = __v4l2_ctrl_modify_range(sensor->ctrls.hblank, def, def, 1, def);
> + if (ret)
> + return ret;
> +
> + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.hblank, def);
> + if (ret)
> + return ret;
return __v4l2_ctrl_s_ctrl(...);
> +
> + return 0;
> +}
> +
> +/* Horizontal or vertically flip the image */
> +static int t4ka3_t_vflip(struct v4l2_subdev *sd, int value, u8 flip_bit)
> +{
> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> + struct v4l2_mbus_framefmt *fmt;
> + int ret;
> + u64 val;
> +
> + if (sensor->streaming)
> + return -EBUSY;
> +
> + val = value ? flip_bit : 0;
> +
> + ret = cci_update_bits(sensor->regmap, T4KA3_REG_IMG_ORIENTATION,
> + flip_bit, val, NULL);
> + if (ret)
> + return ret;
> +
> + fmt = t4ka3_get_active_format(sensor);
> + t4ka3_set_bayer_order(sensor, fmt);
A newline would be nice here.
> + return 0;
> +}
> +
> +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);
> + struct v4l2_mbus_framefmt *fmt;
> + int ret;
> +
> + /* Update exposure range on vblank changes */
> + if (ctrl->id == V4L2_CID_VBLANK) {
> + ret = t4ka3_update_exposure_range(sensor);
> + if (ret)
> + return ret;
> + }
> +
> + fmt = t4ka3_get_active_format(sensor);
You could assign this in declaration.
> +
> + /* 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, fmt);
Does this call belong here?
> + 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, T4KA3_VFLIP_BIT);
> + break;
> + case V4L2_CID_HFLIP:
> + ret = t4ka3_t_vflip(&sensor->sd, ctrl->val, T4KA3_HFLIP_BIT);
> + break;
> + case V4L2_CID_VBLANK:
> + ret = cci_write(sensor->regmap, T4KA3_REG_FRAME_LENGTH_LINES,
> + fmt->height + ctrl->val, NULL);
> + 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);
Newline here?
> + return ret;
> +}
> +
> +static int t4ka3_set_mode(struct t4ka3_data *sensor)
> +{
> + struct v4l2_mbus_framefmt *fmt;
> + int ret = 0;
> +
> + fmt = t4ka3_get_active_format(sensor);
> +
> + cci_write(sensor->regmap, T4KA3_REG_HORZ_OUTPUT_SIZE, fmt->width, &ret);
> + /* Write mode-height - 2 otherwise things don't work, hw-bug ? */
> + cci_write(sensor->regmap, T4KA3_REG_VERT_OUTPUT_SIZE, 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, fmt->width, &ret);
> + cci_write(sensor->regmap, T4KA3_REG_WIN_HEIGHT, 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_enable_stream(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
> + u32 pad, u64 streams_mask)
> +{
> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> + int ret;
> +
> + ret = pm_runtime_get_sync(sensor->sd.dev);
> + if (ret < 0) {
> + dev_err(sensor->dev, "power-up err.\n");
> + goto error_powerdown;
> + }
> +
> + cci_multi_reg_write(sensor->regmap, t4ka3_init_config,
> + ARRAY_SIZE(t4ka3_init_config), &ret);
> + /* enable group hold */
> + cci_write(sensor->regmap, T4KA3_REG_PARAM_HOLD, 1, &ret);
> + cci_multi_reg_write(sensor->regmap, t4ka3_pre_mode_set_regs,
> + ARRAY_SIZE(t4ka3_pre_mode_set_regs), &ret);
> + if (ret)
> + goto error_powerdown;
> +
> + ret = t4ka3_set_mode(sensor);
> + if (ret)
> + goto error_powerdown;
> +
> + ret = cci_multi_reg_write(sensor->regmap, t4ka3_post_mode_set_regs,
> + ARRAY_SIZE(t4ka3_post_mode_set_regs), NULL);
> + if (ret)
> + goto error_powerdown;
> +
> + /* Restore value of all ctrls */
> + ret = __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
> + if (ret)
> + goto error_powerdown;
> +
> + /* disable group hold */
> + cci_write(sensor->regmap, T4KA3_REG_PARAM_HOLD, 0, &ret);
> + cci_write(sensor->regmap, T4KA3_REG_STREAM, 1, &ret);
> + if (ret)
> + goto error_powerdown;
> +
> + sensor->streaming = 1;
> +
> + return ret;
> +
> +error_powerdown:
> + pm_runtime_put(sensor->sd.dev);
And here?
> + return ret;
> +}
> +
> +static int t4ka3_disable_stream(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
> + u32 pad, u64 streams_mask)
> +{
> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> + int ret;
> +
> + ret = cci_write(sensor->regmap, T4KA3_REG_STREAM, 0, NULL);
> + pm_runtime_put(sensor->sd.dev);
> + sensor->streaming = 0;
> + return ret;
Return 0 here but complain about it.
> +}
> +
> +static int t4ka3_get_selection(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_selection *sel)
> +{
> + switch (sel->target) {
> + case V4L2_SEL_TGT_CROP:
> + sel->r = *v4l2_subdev_state_get_crop(state, sel->pad);
> + break;
> + case V4L2_SEL_TGT_NATIVE_SIZE:
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + sel->r.top = 0;
> + sel->r.left = 0;
> + sel->r.width = T4KA3_NATIVE_WIDTH;
> + sel->r.height = T4KA3_NATIVE_HEIGHT;
> + break;
> + case V4L2_SEL_TGT_CROP_DEFAULT:
> + sel->r = t4ka3_default_crop;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int t4ka3_set_selection(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_selection *sel)
> +{
> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> + struct v4l2_mbus_framefmt *format;
> + struct v4l2_rect *crop;
> + struct v4l2_rect rect;
> +
> + if (sel->target != V4L2_SEL_TGT_CROP)
> + return -EINVAL;
> +
> + /*
> + * Clamp the boundaries of the crop rectangle to the size of the sensor
> + * pixel array. Align to multiples of 2 to ensure Bayer pattern isn't
> + * disrupted.
> + */
> + rect.left = clamp_val(ALIGN(sel->r.left, 2),
> + T4KA3_NATIVE_START_LEFT, T4KA3_NATIVE_WIDTH);
> + rect.top = clamp_val(ALIGN(sel->r.top, 2),
> + T4KA3_NATIVE_START_TOP, T4KA3_NATIVE_HEIGHT);
> + rect.width = clamp_val(ALIGN(sel->r.width, 2), T4KA3_MIN_CROP_WIDTH,
> + T4KA3_NATIVE_WIDTH - rect.left);
> + rect.height = clamp_val(ALIGN(sel->r.height, 2), T4KA3_MIN_CROP_HEIGHT,
> + T4KA3_NATIVE_HEIGHT - rect.top);
> +
> + crop = v4l2_subdev_state_get_crop(state, sel->pad);
> +
> + if (rect.width != crop->width || rect.height != crop->height) {
> + /*
> + * Reset the output image size if the crop rectangle size has
> + * been modified.
> + */
> + format = v4l2_subdev_state_get_format(state, sel->pad);
> + format->width = rect.width;
> + format->height = rect.height;
> + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> + t4ka3_calc_mode(sensor);
> + }
> +
> + sel->r = *crop = rect;
> +
> + return 0;
> +}
> +
> +static int
> +t4ka3_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + if (code->index)
> + return -EINVAL;
> +
> + code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> + return 0;
> +}
> +
> +static int t4ka3_enum_frame_size(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + struct v4l2_rect *crop;
> +
> + if (fse->index >= T4KA3_FRAME_SIZES)
> + return -EINVAL;
> +
> + crop = v4l2_subdev_state_get_crop(sd_state, fse->pad);
> +
> + fse->min_width = crop->width / (fse->index + 1);
> + fse->min_height = crop->height / (fse->index + 1);
> + fse->max_width = fse->min_width;
> + fse->max_height = fse->min_height;
> +
> + return 0;
> +}
> +
> +static int t4ka3_check_hwcfg(struct t4ka3_data *sensor)
> +{
> + struct fwnode_handle *fwnode = dev_fwnode(sensor->dev);
> + struct v4l2_fwnode_endpoint bus_cfg = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY,
> + };
> + struct fwnode_handle *endpoint;
> + unsigned long link_freq_bitmap;
> + int ret;
> +
> + /*
> + * Sometimes the fwnode graph is initialized by the bridge driver.
> + * Bridge drivers doing this may also add GPIO mappings, wait for this.
> + */
No need for such a comment.
> + endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> + if (!endpoint)
> + return dev_err_probe(sensor->dev, -EPROBE_DEFER,
> + "waiting for fwnode graph endpoint\n");
This
<URL:https://git.linuxtv.org/sailus/media_tree.git/commit/?h=cleanup&id=8181d18d45d593d8499cbf0e83de08c6d913516c>
will be merged soon.
> +
> + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
> + fwnode_handle_put(endpoint);
> + if (ret)
> + return ret;
> +
> + ret = v4l2_link_freq_to_bitmap(sensor->dev, bus_cfg.link_frequencies,
> + bus_cfg.nr_of_link_frequencies,
> + link_freq_menu_items,
> + ARRAY_SIZE(link_freq_menu_items),
> + &link_freq_bitmap);
> +
> + if (ret == -ENOENT) {
> + dev_err_probe(sensor->dev, -ENOENT,
> + "No match found between driver-supported link frequencies.\n");
> + goto out_free_bus_cfg;
> + }
> +
> + if (ret == -ENODATA) {
> + dev_err_probe(sensor->dev, -ENODATA,
> + "No link frequency was specified in the firmware.\n");
> + goto out_free_bus_cfg;
> + }
No need for printing these error messages -- v4l2_link_freq_to_bitmap()
already does.
> +
> + sensor->link_freq_index = ffs(link_freq_bitmap) - 1;
> +
> + /* 4 MIPI lanes */
> + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> + ret = dev_err_probe(sensor->dev, -EINVAL,
> + "number of CSI2 data lanes %u is not supported\n",
> + bus_cfg.bus.mipi_csi2.num_data_lanes);
> + goto out_free_bus_cfg;
> + }
> +
> + sensor->mipi_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
> +
> +out_free_bus_cfg:
> + v4l2_fwnode_endpoint_free(&bus_cfg);
> +
> + return ret;
> +}
> +
> +static int t4ka3_init_state(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state)
> +{
> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> +
> + *v4l2_subdev_state_get_crop(sd_state, 0) = t4ka3_default_crop;
> +
> + t4ka3_fill_format(sensor, v4l2_subdev_state_get_format(sd_state, 0),
> + T4KA3_ACTIVE_WIDTH, T4KA3_ACTIVE_HEIGHT);
> + return 0;
> +}
> +
> +static const struct v4l2_ctrl_ops t4ka3_ctrl_ops = {
> + .s_ctrl = t4ka3_s_ctrl,
> +};
> +
> +static const struct v4l2_subdev_video_ops t4ka3_video_ops = {
> + .s_stream = v4l2_subdev_s_stream_helper,
> +};
> +
> +static const struct v4l2_subdev_pad_ops t4ka3_pad_ops = {
> + .enum_mbus_code = t4ka3_enum_mbus_code,
> + .enum_frame_size = t4ka3_enum_frame_size,
> + .get_fmt = v4l2_subdev_get_fmt,
> + .set_fmt = t4ka3_set_pad_format,
> + .get_selection = t4ka3_get_selection,
> + .set_selection = t4ka3_set_selection,
> + .enable_streams = t4ka3_enable_stream,
> + .disable_streams = t4ka3_disable_stream,
> +};
> +
> +static const struct v4l2_subdev_ops t4ka3_ops = {
> + .video = &t4ka3_video_ops,
> + .pad = &t4ka3_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops t4ka3_internal_ops = {
> + .init_state = t4ka3_init_state,
> +};
> +
> +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);
> +
> + v4l2_subdev_lock_state(sensor->sd.active_state);
> + t4ka3_calc_mode(sensor);
> + t4ka3_get_vblank_limits(sensor, &min, &max, &def);
> + v4l2_subdev_unlock_state(sensor->sd.active_state);
> +
> + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, min, max, 1, def);
> +
> + def = T4KA3_PIXELS_PER_LINE - T4KA3_ACTIVE_WIDTH;
> + ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
> + def, def, 1, def);
> +
> + 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;
A newline would be nice here.
> + return 0;
> +}
> +
> +static int t4ka3_pm_suspend(struct device *dev)
> +{
> + struct t4ka3_data *sensor = dev_get_drvdata(dev);
> +
> + gpiod_set_value_cansleep(sensor->powerdown_gpio, 1);
> + gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> +
> + return 0;
> +}
> +
> +static int t4ka3_pm_resume(struct device *dev)
> +{
> + struct t4ka3_data *sensor = dev_get_drvdata(dev);
> + u16 sensor_id;
> + int ret;
> +
> + usleep_range(5000, 6000);
> +
> + gpiod_set_value_cansleep(sensor->powerdown_gpio, 0);
> + gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> +
> + /* waiting for the sensor after powering up */
> + msleep(20);
fsleep() maybe?
> +
> + ret = t4ka3_detect(sensor, &sensor_id);
> + if (ret) {
> + dev_err(sensor->dev, "sensor detect failed\n");
> + return ret;
What about gpio values in this case?
> + }
> +
> + return 0;
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(t4ka3_pm_ops, t4ka3_pm_suspend, t4ka3_pm_resume, NULL);
You could run
$ ./scripts/checkpatch.pl --strict --max-line-length=80
on the patch.
> +
> +static void t4ka3_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> +
> + v4l2_async_unregister_subdev(&sensor->sd);
> + v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> + v4l2_subdev_cleanup(sd);
> + media_entity_cleanup(&sensor->sd.entity);
> +
> + /*
> + * Disable runtime PM. In case runtime PM is disabled in the kernel,
> + * make sure to turn power off manually.
> + */
> + pm_runtime_disable(&client->dev);
> + if (!pm_runtime_status_suspended(&client->dev))
> + t4ka3_pm_suspend(&client->dev);
> + pm_runtime_set_suspended(&client->dev);
> +}
> +
> +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;
The driver supports a single link frequency. Could the array holding the
requencies be static const?
> +
> + 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);
You can omit get_noresume() here...
> + 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 = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> + if (ret)
> + goto err_pm_disable;
> +
> + sensor->sd.state_lock = sensor->ctrls.handler.lock;
> + ret = v4l2_subdev_init_finalize(&sensor->sd);
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to init subdev: %d", ret);
> + goto err_media_entity;
> + }
> +
> + ret = t4ka3_init_controls(sensor);
> + if (ret)
> + goto err_controls;
> +
> + ret = v4l2_async_register_subdev_sensor(&sensor->sd);
> + if (ret)
> + goto err_controls;
> +
> + pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> + pm_runtime_use_autosuspend(&client->dev);
> + pm_runtime_put_autosuspend(&client->dev);
as well as the two autosuspend functions above by switching to
pm_runtime_idle() here.
> +
> + return 0;
> +
> +err_controls:
> + v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> + v4l2_subdev_cleanup(&sensor->sd);
> +
> +err_media_entity:
> + media_entity_cleanup(&sensor->sd.entity);
> +
> +err_pm_disable:
> + pm_runtime_disable(&client->dev);
> + pm_runtime_put_noidle(&client->dev);
> + t4ka3_pm_suspend(&client->dev);
> +
> + return ret;
> +}
> +
> +static struct acpi_device_id t4ka3_acpi_match[] = {
const?
> + { "XMCC0003" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, t4ka3_acpi_match);
> +
> +static struct i2c_driver t4ka3_driver = {
> + .driver = {
> + .name = "t4ka3",
> + .acpi_match_table = ACPI_PTR(t4ka3_acpi_match),
> + .pm = pm_sleep_ptr(&t4ka3_pm_ops),
> + },
> + .probe = t4ka3_probe,
> + .remove = t4ka3_remove,
> +};
> +module_i2c_driver(t4ka3_driver)
> +
> +MODULE_DESCRIPTION("A low-level driver for T4KA3 sensor");
> +MODULE_AUTHOR("HARVEY LV <harvey.lv@intel.com>");
> +MODULE_AUTHOR("Kate Hsuan <hpa@redhat.com>");
> +MODULE_LICENSE("GPL");
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v11] media: Add t4ka3 camera sensor driver
2026-03-16 22:00 ` Sakari Ailus
@ 2026-03-17 12:04 ` Hans de Goede
2026-03-17 12:24 ` Hans de Goede
2026-03-17 13:24 ` Kate Hsuan
2 siblings, 0 replies; 12+ messages in thread
From: Hans de Goede @ 2026-03-17 12:04 UTC (permalink / raw)
To: Sakari Ailus, Kate Hsuan
Cc: Mauro Carvalho Chehab, linux-media, linux-kernel, Hans de Goede
Hi Sakari,
On 16-Mar-26 23:00, Sakari Ailus wrote:
> Hi Kate,
>
> Thanks for the patch.
>
> Where have you seen this sensor being used, if I may ask?
Both Kate and I have tested this driver on a Xiaomi Mi Pad 2
where it connected to the atomisp part of a Cherry Trail SoC.
Regards,
Hans
>
> On Mon, Mar 16, 2026 at 04:57:04PM +0800, Kate Hsuan wrote:
>> Add the t4ka3 driver from:
>> https://github.com/kitakar5525/surface3-atomisp-cameras.git
>>
>> With many cleanups / changes (almost a full rewrite) to make it suitable
>> for upstream:
>>
>> * Remove the VCM and VCM-OTP support, the mainline kernel models VCMs and
>> calibration data eeproms as separate v4l2-subdev-s.
>>
>> * Remove the integration-factor t4ka3_get_intg_factor() support and IOCTL,
>> this provided info to userspace through an atomisp private IOCTL.
>>
>> * Turn atomisp specific exposure/gain IOCTL into standard v4l2 controls.
>>
>> * Use normal ACPI power-management in combination with runtime-pm support
>> instead of atomisp specific GMIN power-management code.
>>
>> * Turn into a standard V4L2 sensor driver using
>> v4l2_async_register_subdev_sensor().
>>
>> * Add vblank, hblank, and link-freq controls; drop get_frame_interval().
>>
>> * Use CCI register helpers.
>>
>> * Calculate values for modes instead of using fixed register-value lists,
>> allowing arbritrary modes.
>>
>> * Add get_selection() and set_selection() support
>>
>> * Add a CSI2 bus configuration check
>>
>> This been tested on a Xiaomi Mipad2 tablet which has a T4KA3 sensor with
>> DW9761 VCM as back sensor.
>>
>> Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
>> Co-developed-by: Hans de Goede <hansg@kernel.org>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> Signed-off-by: Kate Hsuan <hpa@redhat.com>
>> ---
>> Changes in v11:
>> 1. Rebase on the latest next branch.
>>
>> Changes in v10:
>> 1. Fix the format settings.
>> 2. Fix the hblank range calculation.
>> 3. In t4ka3_enable_stream(), powerdown when pm_runtime_get_sync() fails.
>> 4. Fix the clean up call sequence when removing the driver.
>> 5. Fix the error handling in t4ka3_probe().
>>
>> Changes in v9:
>> 1. Apply Hans' fix patch to fix the lock issue and squash it into this
>> patch.
>> https://lore.kernel.org/linux-media/33dd5660-efb6-47e0-9672-f3ae65751185@kernel.org/
>>
>> Changes in v8:
>> 1. Drop the local mutex lock and v4l2-core manages all the locking.
>> 2. __t4ka3_get_pad_format() and __t4ka3_get_pad_crop() are replaced with
>> v4l2_subdev_state_get_format() and v4l2_subdev_state_get_crop().
>> 3. The deprecated s_stream was replaced with enable_streams() and
>> disable_streams().
>> 4. Drop unused functions.
>> 5. t4ka3_get_active_format() helper is used to get the active format.
>> 6. v4l2_link_freq_to_bitmap() is used to check and get the supported
>> link frequency.
>>
>> Changes in v7:
>> 1. Add pixel_rate control.
>>
>> Changes in v6:
>> 1. t4ka3_s_config() was removed.
>> 2. The unused macros were removed.
>> 3. The runtime pm initial flow was improved.
>> 4. In remove(), if the device is not in the "suspend" state, the device
>> will be manually turned off.
>>
>> Changes in v5:
>> 1. Improved Kconfig help description.
>>
>> Changes in v4:
>> 1. Another CI issue fixes.
>>
>> Changes in v3:
>> 1. Fix the issues reported by the CI system.
>>
>> Changes in v2:
>> 1. The regmap information was obtained before configuring runtime PM so
>> probe() can return without disabling runtime PM.
>> 2. In t4ka3_s_stream(), return -EBUSY when the streaming is enabled.
>> ---
>> drivers/media/i2c/Kconfig | 12 +
>> drivers/media/i2c/Makefile | 1 +
>> drivers/media/i2c/t4ka3.c | 1085 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 1098 insertions(+)
>> create mode 100644 drivers/media/i2c/t4ka3.c
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 20482be35f26..6344defdbb51 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -790,6 +790,18 @@ config VIDEO_S5KJN1
>> To compile this driver as a module, choose M here: the
>> module will be called s5kjn1.
>>
>> +config VIDEO_T4KA3
>> + tristate "Toshiba T4KA3 sensor support"
>> + depends on ACPI || COMPILE_TEST
>> + depends on GPIOLIB
>> + select V4L2_CCI_I2C
>> + help
>> + This is a Video4Linux2 sensor driver for the Toshiba T4KA3 8 MP
>> + camera sensor.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called t4ka3.
>> +
>> config VIDEO_VD55G1
>> tristate "ST VD55G1 sensor support"
>> select V4L2_CCI_I2C
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index a3a6396df3c4..64c0c7964998 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -139,6 +139,7 @@ obj-$(CONFIG_VIDEO_SAA717X) += saa717x.o
>> obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
>> obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
>> obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
>> +obj-$(CONFIG_VIDEO_T4KA3) += t4ka3.o
>> obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
>> obj-$(CONFIG_VIDEO_TC358746) += tc358746.o
>> obj-$(CONFIG_VIDEO_TDA1997X) += tda1997x.o
>> diff --git a/drivers/media/i2c/t4ka3.c b/drivers/media/i2c/t4ka3.c
>> new file mode 100644
>> index 000000000000..d9af5e51f7a8
>> --- /dev/null
>> +++ b/drivers/media/i2c/t4ka3.c
>> @@ -0,0 +1,1085 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Support for T4KA3 8M camera sensor.
>> + *
>> + * Copyright (C) 2015 Intel Corporation. All Rights Reserved.
>> + * Copyright (C) 2016 XiaoMi, Inc.
>> + * Copyright (C) 2024 Hans de Goede <hansg@kernel.org>
>
> Any 2026 copyrights?
>
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/bits.h>
>> +#include <linux/delay.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/mutex.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +#include <linux/types.h>
>> +
>> +#include <media/media-entity.h>
>> +#include <media/v4l2-async.h>
>> +#include <media/v4l2-cci.h>
>> +#include <media/v4l2-common.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-fwnode.h>
>> +#include <media/v4l2-subdev.h>
>> +
>> +#define T4KA3_NATIVE_WIDTH 3280
>> +#define T4KA3_NATIVE_HEIGHT 2464
>> +#define T4KA3_NATIVE_START_LEFT 0
>> +#define T4KA3_NATIVE_START_TOP 0
>> +#define T4KA3_ACTIVE_WIDTH 3280
>> +#define T4KA3_ACTIVE_HEIGHT 2460
>> +#define T4KA3_ACTIVE_START_LEFT 0
>> +#define T4KA3_ACTIVE_START_TOP 2
>> +#define T4KA3_MIN_CROP_WIDTH 2
>> +#define T4KA3_MIN_CROP_HEIGHT 2
>> +
>> +#define T4KA3_PIXELS_PER_LINE 3440
>> +#define T4KA3_LINES_PER_FRAME_30FPS 2492
>> +#define T4KA3_FPS 30
>> +#define T4KA3_PIXEL_RATE \
>> + (T4KA3_PIXELS_PER_LINE * T4KA3_LINES_PER_FRAME_30FPS * T4KA3_FPS)
>> +
>> +/*
>> + * TODO this really should be derived from the 19.2 MHz xvclk combined
>> + * with the PLL settings. But without a datasheet this is the closest
>> + * approximation possible.
>> + *
>> + * link-freq = pixel_rate * bpp / (lanes * 2)
>> + * (lanes * 2) because CSI lanes use double-data-rate (DDR) signalling.
>> + * bpp = 10 and lanes = 4
>> + */
>> +#define T4KA3_LINK_FREQ ((u64)T4KA3_PIXEL_RATE * 10 / 8)
>> +
>> +/* For enum_frame_size() full-size + binned-/quarter-size */
>> +#define T4KA3_FRAME_SIZES 2
>> +
>> +#define T4KA3_REG_PRODUCT_ID_HIGH CCI_REG8(0x0000)
>> +#define T4KA3_REG_PRODUCT_ID_LOW CCI_REG8(0x0001)
>> +#define T4KA3_PRODUCT_ID 0x1490
>> +
>> +#define T4KA3_REG_STREAM CCI_REG8(0x0100)
>> +#define T4KA3_REG_IMG_ORIENTATION CCI_REG8(0x0101)
>> +#define T4KA3_HFLIP_BIT BIT(0)
>> +#define T4KA3_VFLIP_BIT BIT(1)
>> +#define T4KA3_REG_PARAM_HOLD CCI_REG8(0x0104)
>> +#define T4KA3_REG_COARSE_INTEGRATION_TIME CCI_REG16(0x0202)
>> +#define T4KA3_COARSE_INTEGRATION_TIME_MARGIN 6
>> +#define T4KA3_REG_DIGGAIN_GREEN_R CCI_REG16(0x020e)
>> +#define T4KA3_REG_DIGGAIN_RED CCI_REG16(0x0210)
>> +#define T4KA3_REG_DIGGAIN_BLUE CCI_REG16(0x0212)
>> +#define T4KA3_REG_DIGGAIN_GREEN_B CCI_REG16(0x0214)
>> +#define T4KA3_REG_GLOBAL_GAIN CCI_REG16(0x0234)
>> +#define T4KA3_MIN_GLOBAL_GAIN_SUPPORTED 0x0080
>> +#define T4KA3_MAX_GLOBAL_GAIN_SUPPORTED 0x07ff
>> +#define T4KA3_REG_FRAME_LENGTH_LINES CCI_REG16(0x0340) /* aka VTS */
>> +/* FIXME: need a datasheet to verify the min + max vblank values */
>> +#define T4KA3_MIN_VBLANK 4
>> +#define T4KA3_MAX_VBLANK 0xffff
>> +#define T4KA3_REG_PIXELS_PER_LINE CCI_REG16(0x0342) /* aka HTS */
>> +/* These 2 being horz/vert start is a guess (no datasheet), always 0 */
>> +#define T4KA3_REG_HORZ_START CCI_REG16(0x0344)
>> +#define T4KA3_REG_VERT_START CCI_REG16(0x0346)
>> +/* Always 3279 (T4KA3_NATIVE_WIDTH - 1, window is used to crop */
>> +#define T4KA3_REG_HORZ_END CCI_REG16(0x0348)
>> +/* Always 2463 (T4KA3_NATIVE_HEIGHT - 1, window is used to crop */
>> +#define T4KA3_REG_VERT_END CCI_REG16(0x034a)
>> +/* Output size (after cropping/window) */
>> +#define T4KA3_REG_HORZ_OUTPUT_SIZE CCI_REG16(0x034c)
>> +#define T4KA3_REG_VERT_OUTPUT_SIZE CCI_REG16(0x034e)
>> +/* Window/crop start + size *after* binning */
>> +#define T4KA3_REG_WIN_START_X CCI_REG16(0x0408)
>> +#define T4KA3_REG_WIN_START_Y CCI_REG16(0x040a)
>> +#define T4KA3_REG_WIN_WIDTH CCI_REG16(0x040c)
>> +#define T4KA3_REG_WIN_HEIGHT CCI_REG16(0x040e)
>> +#define T4KA3_REG_TEST_PATTERN_MODE CCI_REG8(0x0601)
>> +/* Unknown register at address 0x0900 */
>> +#define T4KA3_REG_0900 CCI_REG8(0x0900)
>> +#define T4KA3_REG_BINNING CCI_REG8(0x0901)
>> +#define T4KA3_BINNING_VAL(_b) \
>> + ({ typeof(_b) (b) = (_b); \
>> + ((b) << 4) | (b); })
>
> I'd either use an inline function or a regular macro here; in the latter
> case I wouldn't mind about the checkpatch.pl warning related to argument
> double use.
>
>> +
>> +struct t4ka3_ctrls {
>> + struct v4l2_ctrl_handler handler;
>> + struct v4l2_ctrl *hflip;
>> + struct v4l2_ctrl *vflip;
>> + struct v4l2_ctrl *vblank;
>> + struct v4l2_ctrl *hblank;
>> + struct v4l2_ctrl *exposure;
>> + struct v4l2_ctrl *gain;
>> + struct v4l2_ctrl *test_pattern;
>> + struct v4l2_ctrl *link_freq;
>> + struct v4l2_ctrl *pixel_rate;
>
> Do you need all these in the struct? E.g. gain appears to be unused.
>
>> +};
>> +
>> +struct t4ka3_mode {
>> + int binning;
>> + u16 win_x;
>> + u16 win_y;
>
> The rest of the fields have just a space between the type and the field
> name. I'd do the same here.
>
>> +};
>> +
>> +struct t4ka3_data {
>> + struct v4l2_subdev sd;
>> + struct media_pad pad;
>> + struct mutex lock; /* serialize sensor's ioctl */
>> + struct t4ka3_ctrls ctrls;
>> + struct t4ka3_mode mode;
>> + struct device *dev;
>> + struct regmap *regmap;
>> + struct gpio_desc *powerdown_gpio;
>> + struct gpio_desc *reset_gpio;
>> + s64 link_freq[1];
>> + int streaming;
>> +
>> + /* MIPI lane info */
>> + u32 link_freq_index;
>> + u8 mipi_lanes;
>> +};
>> +
>> +/* init settings */
>> +static const struct cci_reg_sequence t4ka3_init_config[] = {
>> + {CCI_REG8(0x4136), 0x13},
>
> { Spaces inside braces, please. },
>
>> + {CCI_REG8(0x4137), 0x33},
>> + {CCI_REG8(0x3094), 0x01},
>> + {CCI_REG8(0x0233), 0x01},
>> + {CCI_REG8(0x4B06), 0x01},
>> + {CCI_REG8(0x4B07), 0x01},
>> + {CCI_REG8(0x3028), 0x01},
>> + {CCI_REG8(0x3032), 0x14},
>> + {CCI_REG8(0x305C), 0x0C},
>> + {CCI_REG8(0x306D), 0x0A},
>> + {CCI_REG8(0x3071), 0xFA},
>> + {CCI_REG8(0x307E), 0x0A},
>> + {CCI_REG8(0x307F), 0xFC},
>> + {CCI_REG8(0x3091), 0x04},
>> + {CCI_REG8(0x3092), 0x60},
>> + {CCI_REG8(0x3096), 0xC0},
>> + {CCI_REG8(0x3100), 0x07},
>> + {CCI_REG8(0x3101), 0x4C},
>> + {CCI_REG8(0x3118), 0xCC},
>> + {CCI_REG8(0x3139), 0x06},
>> + {CCI_REG8(0x313A), 0x06},
>> + {CCI_REG8(0x313B), 0x04},
>> + {CCI_REG8(0x3143), 0x02},
>> + {CCI_REG8(0x314F), 0x0E},
>> + {CCI_REG8(0x3169), 0x99},
>> + {CCI_REG8(0x316A), 0x99},
>> + {CCI_REG8(0x3171), 0x05},
>> + {CCI_REG8(0x31A1), 0xA7},
>> + {CCI_REG8(0x31A2), 0x9C},
>> + {CCI_REG8(0x31A3), 0x8F},
>> + {CCI_REG8(0x31A4), 0x75},
>> + {CCI_REG8(0x31A5), 0xEE},
>> + {CCI_REG8(0x31A6), 0xEA},
>> + {CCI_REG8(0x31A7), 0xE4},
>> + {CCI_REG8(0x31A8), 0xE4},
>> + {CCI_REG8(0x31DF), 0x05},
>> + {CCI_REG8(0x31EC), 0x1B},
>> + {CCI_REG8(0x31ED), 0x1B},
>> + {CCI_REG8(0x31EE), 0x1B},
>> + {CCI_REG8(0x31F0), 0x1B},
>> + {CCI_REG8(0x31F1), 0x1B},
>> + {CCI_REG8(0x31F2), 0x1B},
>> + {CCI_REG8(0x3204), 0x3F},
>> + {CCI_REG8(0x3205), 0x03},
>> + {CCI_REG8(0x3210), 0x01},
>> + {CCI_REG8(0x3216), 0x68},
>> + {CCI_REG8(0x3217), 0x58},
>> + {CCI_REG8(0x3218), 0x58},
>> + {CCI_REG8(0x321A), 0x68},
>> + {CCI_REG8(0x321B), 0x60},
>> + {CCI_REG8(0x3238), 0x03},
>> + {CCI_REG8(0x3239), 0x03},
>> + {CCI_REG8(0x323A), 0x05},
>> + {CCI_REG8(0x323B), 0x06},
>> + {CCI_REG8(0x3243), 0x03},
>> + {CCI_REG8(0x3244), 0x08},
>> + {CCI_REG8(0x3245), 0x01},
>> + {CCI_REG8(0x3307), 0x19},
>> + {CCI_REG8(0x3308), 0x19},
>> + {CCI_REG8(0x3320), 0x01},
>> + {CCI_REG8(0x3326), 0x15},
>> + {CCI_REG8(0x3327), 0x0D},
>> + {CCI_REG8(0x3328), 0x01},
>> + {CCI_REG8(0x3380), 0x01},
>> + {CCI_REG8(0x339E), 0x07},
>> + {CCI_REG8(0x3424), 0x00},
>> + {CCI_REG8(0x343C), 0x01},
>> + {CCI_REG8(0x3398), 0x04},
>> + {CCI_REG8(0x343A), 0x10},
>> + {CCI_REG8(0x339A), 0x22},
>> + {CCI_REG8(0x33B4), 0x00},
>> + {CCI_REG8(0x3393), 0x01},
>> + {CCI_REG8(0x33B3), 0x6E},
>> + {CCI_REG8(0x3433), 0x06},
>> + {CCI_REG8(0x3433), 0x00},
>> + {CCI_REG8(0x33B3), 0x00},
>> + {CCI_REG8(0x3393), 0x03},
>> + {CCI_REG8(0x33B4), 0x03},
>> + {CCI_REG8(0x343A), 0x00},
>> + {CCI_REG8(0x339A), 0x00},
>> + {CCI_REG8(0x3398), 0x00}
>> +};
>> +
>> +static const struct cci_reg_sequence t4ka3_pre_mode_set_regs[] = {
>> + {CCI_REG8(0x0112), 0x0A},
>> + {CCI_REG8(0x0113), 0x0A},
>> + {CCI_REG8(0x0114), 0x03},
>> + {CCI_REG8(0x4136), 0x13},
>> + {CCI_REG8(0x4137), 0x33},
>> + {CCI_REG8(0x0820), 0x0A},
>> + {CCI_REG8(0x0821), 0x0D},
>> + {CCI_REG8(0x0822), 0x00},
>> + {CCI_REG8(0x0823), 0x00},
>> + {CCI_REG8(0x0301), 0x0A},
>> + {CCI_REG8(0x0303), 0x01},
>> + {CCI_REG8(0x0305), 0x04},
>> + {CCI_REG8(0x0306), 0x02},
>> + {CCI_REG8(0x0307), 0x18},
>> + {CCI_REG8(0x030B), 0x01},
>> +};
>> +
>> +static const struct cci_reg_sequence t4ka3_post_mode_set_regs[] = {
>> + {CCI_REG8(0x0902), 0x00},
>> + {CCI_REG8(0x4220), 0x00},
>> + {CCI_REG8(0x4222), 0x01},
>> + {CCI_REG8(0x3380), 0x01},
>> + {CCI_REG8(0x3090), 0x88},
>> + {CCI_REG8(0x3394), 0x20},
>> + {CCI_REG8(0x3090), 0x08},
>> + {CCI_REG8(0x3394), 0x10}
>> +};
>> +
>> +static const s64 link_freq_menu_items[] = {
>> + T4KA3_LINK_FREQ,
>> +};
>> +
>> +static inline struct t4ka3_data *to_t4ka3_sensor(struct v4l2_subdev *sd)
>> +{
>> + return container_of(sd, struct t4ka3_data, sd);
>> +}
>> +
>> +static inline struct t4ka3_data *ctrl_to_t4ka3(struct v4l2_ctrl *ctrl)
>> +{
>> + return container_of(ctrl->handler, struct t4ka3_data, ctrls.handler);
>> +}
>
> I'd use macros and container_of_const().
>
>> +
>> +/* T4KA3 default GRBG */
>> +static const int t4ka3_hv_flip_bayer_order[] = {
>> + MEDIA_BUS_FMT_SGRBG10_1X10,
>> + MEDIA_BUS_FMT_SBGGR10_1X10,
>> + MEDIA_BUS_FMT_SRGGB10_1X10,
>> + MEDIA_BUS_FMT_SGBRG10_1X10,
>> +};
>> +
>> +static const struct v4l2_rect t4ka3_default_crop = {
>> + .left = T4KA3_ACTIVE_START_LEFT,
>> + .top = T4KA3_ACTIVE_START_TOP,
>> + .width = T4KA3_ACTIVE_WIDTH,
>> + .height = T4KA3_ACTIVE_HEIGHT,
>> +};
>> +
>> +static int t4ka3_detect(struct t4ka3_data *sensor, u16 *id);
>
> Not needed.
>
>> +
>> +static void t4ka3_set_bayer_order(struct t4ka3_data *sensor,
>> + struct v4l2_mbus_framefmt *fmt)
>> +{
>> + int hv_flip = 0;
>
> unsigned int?
>
>> +
>> + if (sensor->ctrls.vflip && sensor->ctrls.vflip->val)
>> + hv_flip += 1;
>> +
>> + if (sensor->ctrls.hflip && sensor->ctrls.hflip->val)
>> + hv_flip += 2;
>> +
>> + fmt->code = t4ka3_hv_flip_bayer_order[hv_flip];
>> +}
>> +
>> +static struct v4l2_mbus_framefmt *t4ka3_get_active_format(struct t4ka3_data *sensor)
>> +{
>> + struct v4l2_subdev_state *active_state =
>> + v4l2_subdev_get_locked_active_state(&sensor->sd);
>> +
>> + return v4l2_subdev_state_get_format(active_state, 0);
>> +}
>> +
>> +static struct v4l2_rect *t4ka3_get_active_crop(struct t4ka3_data *sensor)
>> +{
>> + struct v4l2_subdev_state *active_state =
>> + v4l2_subdev_get_locked_active_state(&sensor->sd);
>> +
>> + return v4l2_subdev_state_get_crop(active_state, 0);
>
> Please avoid adding such helpers.
>
>> +}
>> +
>> +static int t4ka3_update_exposure_range(struct t4ka3_data *sensor)
>> +{
>> + struct v4l2_mbus_framefmt *fmt;
>> +
>> + fmt = t4ka3_get_active_format(sensor);
>
> Can be assigned in declaration.
>
>> +
>> + int exp_max = 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_fill_format(struct t4ka3_data *sensor,
>> + struct v4l2_mbus_framefmt *fmt,
>> + unsigned int width, unsigned int height)
>> +{
>> + memset(fmt, 0, sizeof(*fmt));
>> + fmt->width = width;
>> + fmt->height = height;
>> + fmt->field = V4L2_FIELD_NONE;
>> + fmt->colorspace = V4L2_COLORSPACE_RAW;
>> + t4ka3_set_bayer_order(sensor, fmt);
>> +}
>> +
>> +static void t4ka3_calc_mode(struct t4ka3_data *sensor)
>> +{
>> + struct v4l2_mbus_framefmt *fmt;
>> + struct v4l2_rect *crop;
>> + int width;
>> + int height;
>> + int binning;
>> +
>> + fmt = t4ka3_get_active_format(sensor);
>> + crop = t4ka3_get_active_crop(sensor);
>
> Ditto.
>
>> +
>> + width = fmt->width;
>> + height = fmt->height;
>> +
>> + if (width <= (crop->width / 2) && height <= (crop->height / 2))
>> + binning = 2;
>> + else
>> + binning = 1;
>> +
>> + width *= binning;
>> + height *= binning;
>> +
>> + sensor->mode.binning = binning;
>> + sensor->mode.win_x = (crop->left + (crop->width - width) / 2) & ~1;
>> + sensor->mode.win_y = (crop->top + (crop->height - height) / 2) & ~1;
>> + /*
>> + * t4ka3'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)
>> +{
>> + struct v4l2_mbus_framefmt *fmt;
>> +
>> + fmt = t4ka3_get_active_format(sensor);
>
> Ditto.
>
>> +
>> + *min = T4KA3_MIN_VBLANK + (sensor->mode.binning - 1) * fmt->height;
>> + *max = T4KA3_MAX_VBLANK - fmt->height;
>> + *def = T4KA3_LINES_PER_FRAME_30FPS - fmt->height;
>> +}
>> +
>> +static int t4ka3_set_pad_format(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_format *format)
>> +{
>> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
>> + struct v4l2_mbus_framefmt *try_fmt;
>> + struct v4l2_mbus_framefmt *fmt;
>> + const struct v4l2_rect *crop;
>> + unsigned int width, height;
>> + int min, max, def, ret = 0;
>> +
>> + crop = t4ka3_get_active_crop(sensor);
>> + fmt = t4ka3_get_active_format(sensor);
>> +
>> + /* Limit set_fmt max size to crop width / height */
>> + width = clamp_val(ALIGN(format->format.width, 2),
>> + T4KA3_MIN_CROP_WIDTH, crop->width);
>> + height = clamp_val(ALIGN(format->format.height, 2),
>> + T4KA3_MIN_CROP_HEIGHT, crop->height);
>> + t4ka3_fill_format(sensor, &format->format, width, height);
>> +
>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>> + try_fmt = v4l2_subdev_state_get_format(sd_state, 0);
>> + *try_fmt = format->format;
>> + return 0;
>> + }
>> +
>> + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE && sensor->streaming)
>> + return -EBUSY;
>> +
>> + *v4l2_subdev_state_get_format(sd_state, 0) = format->format;
>> +
>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>> + return 0;
>> +
>> + t4ka3_calc_mode(sensor);
>> +
>> + /* vblank range is height dependent adjust and reset to default */
>> + t4ka3_get_vblank_limits(sensor, &min, &max, &def);
>> + ret = __v4l2_ctrl_modify_range(sensor->ctrls.vblank, min, max, 1, def);
>> + if (ret)
>> + return ret;
>> +
>> + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, def);
>> + if (ret)
>> + return ret;
>> +
>> + def = T4KA3_PIXELS_PER_LINE - fmt->width;
>> + ret = __v4l2_ctrl_modify_range(sensor->ctrls.hblank, def, def, 1, def);
>> + if (ret)
>> + return ret;
>> +
>> + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.hblank, def);
>> + if (ret)
>> + return ret;
>
> return __v4l2_ctrl_s_ctrl(...);
>
>> +
>> + return 0;
>> +}
>> +
>> +/* Horizontal or vertically flip the image */
>> +static int t4ka3_t_vflip(struct v4l2_subdev *sd, int value, u8 flip_bit)
>> +{
>> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
>> + struct v4l2_mbus_framefmt *fmt;
>> + int ret;
>> + u64 val;
>> +
>> + if (sensor->streaming)
>> + return -EBUSY;
>> +
>> + val = value ? flip_bit : 0;
>> +
>> + ret = cci_update_bits(sensor->regmap, T4KA3_REG_IMG_ORIENTATION,
>> + flip_bit, val, NULL);
>> + if (ret)
>> + return ret;
>> +
>> + fmt = t4ka3_get_active_format(sensor);
>> + t4ka3_set_bayer_order(sensor, fmt);
>
> A newline would be nice here.
>
>> + return 0;
>> +}
>> +
>> +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);
>> + struct v4l2_mbus_framefmt *fmt;
>> + int ret;
>> +
>> + /* Update exposure range on vblank changes */
>> + if (ctrl->id == V4L2_CID_VBLANK) {
>> + ret = t4ka3_update_exposure_range(sensor);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + fmt = t4ka3_get_active_format(sensor);
>
> You could assign this in declaration.
>
>> +
>> + /* 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, fmt);
>
> Does this call belong here?
>
>> + 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, T4KA3_VFLIP_BIT);
>> + break;
>> + case V4L2_CID_HFLIP:
>> + ret = t4ka3_t_vflip(&sensor->sd, ctrl->val, T4KA3_HFLIP_BIT);
>> + break;
>> + case V4L2_CID_VBLANK:
>> + ret = cci_write(sensor->regmap, T4KA3_REG_FRAME_LENGTH_LINES,
>> + fmt->height + ctrl->val, NULL);
>> + 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);
>
> Newline here?
>
>> + return ret;
>> +}
>> +
>> +static int t4ka3_set_mode(struct t4ka3_data *sensor)
>> +{
>> + struct v4l2_mbus_framefmt *fmt;
>> + int ret = 0;
>> +
>> + fmt = t4ka3_get_active_format(sensor);
>> +
>> + cci_write(sensor->regmap, T4KA3_REG_HORZ_OUTPUT_SIZE, fmt->width, &ret);
>> + /* Write mode-height - 2 otherwise things don't work, hw-bug ? */
>> + cci_write(sensor->regmap, T4KA3_REG_VERT_OUTPUT_SIZE, 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, fmt->width, &ret);
>> + cci_write(sensor->regmap, T4KA3_REG_WIN_HEIGHT, 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_enable_stream(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
>> + u32 pad, u64 streams_mask)
>> +{
>> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
>> + int ret;
>> +
>> + ret = pm_runtime_get_sync(sensor->sd.dev);
>> + if (ret < 0) {
>> + dev_err(sensor->dev, "power-up err.\n");
>> + goto error_powerdown;
>> + }
>> +
>> + cci_multi_reg_write(sensor->regmap, t4ka3_init_config,
>> + ARRAY_SIZE(t4ka3_init_config), &ret);
>> + /* enable group hold */
>> + cci_write(sensor->regmap, T4KA3_REG_PARAM_HOLD, 1, &ret);
>> + cci_multi_reg_write(sensor->regmap, t4ka3_pre_mode_set_regs,
>> + ARRAY_SIZE(t4ka3_pre_mode_set_regs), &ret);
>> + if (ret)
>> + goto error_powerdown;
>> +
>> + ret = t4ka3_set_mode(sensor);
>> + if (ret)
>> + goto error_powerdown;
>> +
>> + ret = cci_multi_reg_write(sensor->regmap, t4ka3_post_mode_set_regs,
>> + ARRAY_SIZE(t4ka3_post_mode_set_regs), NULL);
>> + if (ret)
>> + goto error_powerdown;
>> +
>> + /* Restore value of all ctrls */
>> + ret = __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
>> + if (ret)
>> + goto error_powerdown;
>> +
>> + /* disable group hold */
>> + cci_write(sensor->regmap, T4KA3_REG_PARAM_HOLD, 0, &ret);
>> + cci_write(sensor->regmap, T4KA3_REG_STREAM, 1, &ret);
>> + if (ret)
>> + goto error_powerdown;
>> +
>> + sensor->streaming = 1;
>> +
>> + return ret;
>> +
>> +error_powerdown:
>> + pm_runtime_put(sensor->sd.dev);
>
> And here?
>
>> + return ret;
>> +}
>> +
>> +static int t4ka3_disable_stream(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
>> + u32 pad, u64 streams_mask)
>> +{
>> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
>> + int ret;
>> +
>> + ret = cci_write(sensor->regmap, T4KA3_REG_STREAM, 0, NULL);
>> + pm_runtime_put(sensor->sd.dev);
>> + sensor->streaming = 0;
>> + return ret;
>
> Return 0 here but complain about it.
>
>> +}
>> +
>> +static int t4ka3_get_selection(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_selection *sel)
>> +{
>> + switch (sel->target) {
>> + case V4L2_SEL_TGT_CROP:
>> + sel->r = *v4l2_subdev_state_get_crop(state, sel->pad);
>> + break;
>> + case V4L2_SEL_TGT_NATIVE_SIZE:
>> + case V4L2_SEL_TGT_CROP_BOUNDS:
>> + sel->r.top = 0;
>> + sel->r.left = 0;
>> + sel->r.width = T4KA3_NATIVE_WIDTH;
>> + sel->r.height = T4KA3_NATIVE_HEIGHT;
>> + break;
>> + case V4L2_SEL_TGT_CROP_DEFAULT:
>> + sel->r = t4ka3_default_crop;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int t4ka3_set_selection(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_selection *sel)
>> +{
>> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
>> + struct v4l2_mbus_framefmt *format;
>> + struct v4l2_rect *crop;
>> + struct v4l2_rect rect;
>> +
>> + if (sel->target != V4L2_SEL_TGT_CROP)
>> + return -EINVAL;
>> +
>> + /*
>> + * Clamp the boundaries of the crop rectangle to the size of the sensor
>> + * pixel array. Align to multiples of 2 to ensure Bayer pattern isn't
>> + * disrupted.
>> + */
>> + rect.left = clamp_val(ALIGN(sel->r.left, 2),
>> + T4KA3_NATIVE_START_LEFT, T4KA3_NATIVE_WIDTH);
>> + rect.top = clamp_val(ALIGN(sel->r.top, 2),
>> + T4KA3_NATIVE_START_TOP, T4KA3_NATIVE_HEIGHT);
>> + rect.width = clamp_val(ALIGN(sel->r.width, 2), T4KA3_MIN_CROP_WIDTH,
>> + T4KA3_NATIVE_WIDTH - rect.left);
>> + rect.height = clamp_val(ALIGN(sel->r.height, 2), T4KA3_MIN_CROP_HEIGHT,
>> + T4KA3_NATIVE_HEIGHT - rect.top);
>> +
>> + crop = v4l2_subdev_state_get_crop(state, sel->pad);
>> +
>> + if (rect.width != crop->width || rect.height != crop->height) {
>> + /*
>> + * Reset the output image size if the crop rectangle size has
>> + * been modified.
>> + */
>> + format = v4l2_subdev_state_get_format(state, sel->pad);
>> + format->width = rect.width;
>> + format->height = rect.height;
>> + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>> + t4ka3_calc_mode(sensor);
>> + }
>> +
>> + sel->r = *crop = rect;
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +t4ka3_enum_mbus_code(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> + if (code->index)
>> + return -EINVAL;
>> +
>> + code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
>> + return 0;
>> +}
>> +
>> +static int t4ka3_enum_frame_size(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_frame_size_enum *fse)
>> +{
>> + struct v4l2_rect *crop;
>> +
>> + if (fse->index >= T4KA3_FRAME_SIZES)
>> + return -EINVAL;
>> +
>> + crop = v4l2_subdev_state_get_crop(sd_state, fse->pad);
>> +
>> + fse->min_width = crop->width / (fse->index + 1);
>> + fse->min_height = crop->height / (fse->index + 1);
>> + fse->max_width = fse->min_width;
>> + fse->max_height = fse->min_height;
>> +
>> + return 0;
>> +}
>> +
>> +static int t4ka3_check_hwcfg(struct t4ka3_data *sensor)
>> +{
>> + struct fwnode_handle *fwnode = dev_fwnode(sensor->dev);
>> + struct v4l2_fwnode_endpoint bus_cfg = {
>> + .bus_type = V4L2_MBUS_CSI2_DPHY,
>> + };
>> + struct fwnode_handle *endpoint;
>> + unsigned long link_freq_bitmap;
>> + int ret;
>> +
>> + /*
>> + * Sometimes the fwnode graph is initialized by the bridge driver.
>> + * Bridge drivers doing this may also add GPIO mappings, wait for this.
>> + */
>
> No need for such a comment.
>
>> + endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
>> + if (!endpoint)
>> + return dev_err_probe(sensor->dev, -EPROBE_DEFER,
>> + "waiting for fwnode graph endpoint\n");
>
> This
> <URL:https://git.linuxtv.org/sailus/media_tree.git/commit/?h=cleanup&id=8181d18d45d593d8499cbf0e83de08c6d913516c>
> will be merged soon.
>
>> +
>> + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
>> + fwnode_handle_put(endpoint);
>> + if (ret)
>> + return ret;
>> +
>> + ret = v4l2_link_freq_to_bitmap(sensor->dev, bus_cfg.link_frequencies,
>> + bus_cfg.nr_of_link_frequencies,
>> + link_freq_menu_items,
>> + ARRAY_SIZE(link_freq_menu_items),
>> + &link_freq_bitmap);
>> +
>> + if (ret == -ENOENT) {
>> + dev_err_probe(sensor->dev, -ENOENT,
>> + "No match found between driver-supported link frequencies.\n");
>> + goto out_free_bus_cfg;
>> + }
>> +
>> + if (ret == -ENODATA) {
>> + dev_err_probe(sensor->dev, -ENODATA,
>> + "No link frequency was specified in the firmware.\n");
>> + goto out_free_bus_cfg;
>> + }
>
> No need for printing these error messages -- v4l2_link_freq_to_bitmap()
> already does.
>
>> +
>> + sensor->link_freq_index = ffs(link_freq_bitmap) - 1;
>> +
>> + /* 4 MIPI lanes */
>> + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
>> + ret = dev_err_probe(sensor->dev, -EINVAL,
>> + "number of CSI2 data lanes %u is not supported\n",
>> + bus_cfg.bus.mipi_csi2.num_data_lanes);
>> + goto out_free_bus_cfg;
>> + }
>> +
>> + sensor->mipi_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
>> +
>> +out_free_bus_cfg:
>> + v4l2_fwnode_endpoint_free(&bus_cfg);
>> +
>> + return ret;
>> +}
>> +
>> +static int t4ka3_init_state(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state)
>> +{
>> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
>> +
>> + *v4l2_subdev_state_get_crop(sd_state, 0) = t4ka3_default_crop;
>> +
>> + t4ka3_fill_format(sensor, v4l2_subdev_state_get_format(sd_state, 0),
>> + T4KA3_ACTIVE_WIDTH, T4KA3_ACTIVE_HEIGHT);
>> + return 0;
>> +}
>> +
>> +static const struct v4l2_ctrl_ops t4ka3_ctrl_ops = {
>> + .s_ctrl = t4ka3_s_ctrl,
>> +};
>> +
>> +static const struct v4l2_subdev_video_ops t4ka3_video_ops = {
>> + .s_stream = v4l2_subdev_s_stream_helper,
>> +};
>> +
>> +static const struct v4l2_subdev_pad_ops t4ka3_pad_ops = {
>> + .enum_mbus_code = t4ka3_enum_mbus_code,
>> + .enum_frame_size = t4ka3_enum_frame_size,
>> + .get_fmt = v4l2_subdev_get_fmt,
>> + .set_fmt = t4ka3_set_pad_format,
>> + .get_selection = t4ka3_get_selection,
>> + .set_selection = t4ka3_set_selection,
>> + .enable_streams = t4ka3_enable_stream,
>> + .disable_streams = t4ka3_disable_stream,
>> +};
>> +
>> +static const struct v4l2_subdev_ops t4ka3_ops = {
>> + .video = &t4ka3_video_ops,
>> + .pad = &t4ka3_pad_ops,
>> +};
>> +
>> +static const struct v4l2_subdev_internal_ops t4ka3_internal_ops = {
>> + .init_state = t4ka3_init_state,
>> +};
>> +
>> +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);
>> +
>> + v4l2_subdev_lock_state(sensor->sd.active_state);
>> + t4ka3_calc_mode(sensor);
>> + t4ka3_get_vblank_limits(sensor, &min, &max, &def);
>> + v4l2_subdev_unlock_state(sensor->sd.active_state);
>> +
>> + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, min, max, 1, def);
>> +
>> + def = T4KA3_PIXELS_PER_LINE - T4KA3_ACTIVE_WIDTH;
>> + ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
>> + def, def, 1, def);
>> +
>> + 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;
>
> A newline would be nice here.
>
>> + return 0;
>> +}
>> +
>> +static int t4ka3_pm_suspend(struct device *dev)
>> +{
>> + struct t4ka3_data *sensor = dev_get_drvdata(dev);
>> +
>> + gpiod_set_value_cansleep(sensor->powerdown_gpio, 1);
>> + gpiod_set_value_cansleep(sensor->reset_gpio, 1);
>> +
>> + return 0;
>> +}
>> +
>> +static int t4ka3_pm_resume(struct device *dev)
>> +{
>> + struct t4ka3_data *sensor = dev_get_drvdata(dev);
>> + u16 sensor_id;
>> + int ret;
>> +
>> + usleep_range(5000, 6000);
>> +
>> + gpiod_set_value_cansleep(sensor->powerdown_gpio, 0);
>> + gpiod_set_value_cansleep(sensor->reset_gpio, 0);
>> +
>> + /* waiting for the sensor after powering up */
>> + msleep(20);
>
> fsleep() maybe?
>
>> +
>> + ret = t4ka3_detect(sensor, &sensor_id);
>> + if (ret) {
>> + dev_err(sensor->dev, "sensor detect failed\n");
>> + return ret;
>
> What about gpio values in this case?
>
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static DEFINE_RUNTIME_DEV_PM_OPS(t4ka3_pm_ops, t4ka3_pm_suspend, t4ka3_pm_resume, NULL);
>
> You could run
>
> $ ./scripts/checkpatch.pl --strict --max-line-length=80
>
> on the patch.
>
>> +
>> +static void t4ka3_remove(struct i2c_client *client)
>> +{
>> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
>> +
>> + v4l2_async_unregister_subdev(&sensor->sd);
>> + v4l2_ctrl_handler_free(&sensor->ctrls.handler);
>> + v4l2_subdev_cleanup(sd);
>> + media_entity_cleanup(&sensor->sd.entity);
>> +
>> + /*
>> + * Disable runtime PM. In case runtime PM is disabled in the kernel,
>> + * make sure to turn power off manually.
>> + */
>> + pm_runtime_disable(&client->dev);
>> + if (!pm_runtime_status_suspended(&client->dev))
>> + t4ka3_pm_suspend(&client->dev);
>> + pm_runtime_set_suspended(&client->dev);
>> +}
>> +
>> +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;
>
> The driver supports a single link frequency. Could the array holding the
> requencies be static const?
>
>> +
>> + 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);
>
> You can omit get_noresume() here...
>
>> + 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 = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
>> + if (ret)
>> + goto err_pm_disable;
>> +
>> + sensor->sd.state_lock = sensor->ctrls.handler.lock;
>> + ret = v4l2_subdev_init_finalize(&sensor->sd);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "failed to init subdev: %d", ret);
>> + goto err_media_entity;
>> + }
>> +
>> + ret = t4ka3_init_controls(sensor);
>> + if (ret)
>> + goto err_controls;
>> +
>> + ret = v4l2_async_register_subdev_sensor(&sensor->sd);
>> + if (ret)
>> + goto err_controls;
>> +
>> + pm_runtime_set_autosuspend_delay(&client->dev, 1000);
>> + pm_runtime_use_autosuspend(&client->dev);
>> + pm_runtime_put_autosuspend(&client->dev);
>
> as well as the two autosuspend functions above by switching to
> pm_runtime_idle() here.
>
>> +
>> + return 0;
>> +
>> +err_controls:
>> + v4l2_ctrl_handler_free(&sensor->ctrls.handler);
>> + v4l2_subdev_cleanup(&sensor->sd);
>> +
>> +err_media_entity:
>> + media_entity_cleanup(&sensor->sd.entity);
>> +
>> +err_pm_disable:
>> + pm_runtime_disable(&client->dev);
>> + pm_runtime_put_noidle(&client->dev);
>> + t4ka3_pm_suspend(&client->dev);
>> +
>> + return ret;
>> +}
>> +
>> +static struct acpi_device_id t4ka3_acpi_match[] = {
>
> const?
>
>> + { "XMCC0003" },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(acpi, t4ka3_acpi_match);
>> +
>> +static struct i2c_driver t4ka3_driver = {
>> + .driver = {
>> + .name = "t4ka3",
>> + .acpi_match_table = ACPI_PTR(t4ka3_acpi_match),
>> + .pm = pm_sleep_ptr(&t4ka3_pm_ops),
>> + },
>> + .probe = t4ka3_probe,
>> + .remove = t4ka3_remove,
>> +};
>> +module_i2c_driver(t4ka3_driver)
>> +
>> +MODULE_DESCRIPTION("A low-level driver for T4KA3 sensor");
>> +MODULE_AUTHOR("HARVEY LV <harvey.lv@intel.com>");
>> +MODULE_AUTHOR("Kate Hsuan <hpa@redhat.com>");
>> +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v11] media: Add t4ka3 camera sensor driver
2026-03-16 22:00 ` Sakari Ailus
2026-03-17 12:04 ` Hans de Goede
@ 2026-03-17 12:24 ` Hans de Goede
2026-03-17 13:26 ` Kate Hsuan
2026-03-17 17:58 ` Sakari Ailus
2026-03-17 13:24 ` Kate Hsuan
2 siblings, 2 replies; 12+ messages in thread
From: Hans de Goede @ 2026-03-17 12:24 UTC (permalink / raw)
To: Sakari Ailus, Kate Hsuan
Cc: Mauro Carvalho Chehab, linux-media, linux-kernel, Hans de Goede
Hi Sakari,
On 16-Mar-26 23:00, Sakari Ailus wrote:
<snip>
>> diff --git a/drivers/media/i2c/t4ka3.c b/drivers/media/i2c/t4ka3.c
>> new file mode 100644
>> index 000000000000..d9af5e51f7a8
>> --- /dev/null
>> +++ b/drivers/media/i2c/t4ka3.c
<snip>
>> +static struct v4l2_mbus_framefmt *t4ka3_get_active_format(struct t4ka3_data *sensor)
>> +{
>> + struct v4l2_subdev_state *active_state =
>> + v4l2_subdev_get_locked_active_state(&sensor->sd);
>> +
>> + return v4l2_subdev_state_get_format(active_state, 0);
>> +}
>> +
>> +static struct v4l2_rect *t4ka3_get_active_crop(struct t4ka3_data *sensor)
>> +{
>> + struct v4l2_subdev_state *active_state =
>> + v4l2_subdev_get_locked_active_state(&sensor->sd);
>> +
>> + return v4l2_subdev_state_get_crop(active_state, 0);
>
> Please avoid adding such helpers.
The problem is that we need to know the active-fmt/-crop in some places
without access to it. E.g. when the vblank ctrl gets set this influences
the range of the exposure control, so we need active_fmt.height to
calculate the values to pass to v4l2_ctrl_modify_range() and we need
this from a v4l2_ctrl_ops.s_ctrl callback which does not get passed
in the (active) fmt.
Since the ctrl lock is used as the main sensor-driver lock too,
we can always safely call v4l2_subdev_get_locked_active_state()
in these cases, since we are always holding the lock.
The alternative would be to store a copy of the active fmt/crop
inside struct t4ka3_data, but I thought that the whole direction
for sensor drivers was to stop having (and needing to update) their
own shadow copy of the active_state and instead direct use
the active_state ?
<snip>
>> +static int t4ka3_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct t4ka3_data *sensor = ctrl_to_t4ka3(ctrl);
>> + struct v4l2_mbus_framefmt *fmt;
>> + int ret;
>> +
>> + /* Update exposure range on vblank changes */
>> + if (ctrl->id == V4L2_CID_VBLANK) {
>> + ret = t4ka3_update_exposure_range(sensor);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + fmt = t4ka3_get_active_format(sensor);
>
> You could assign this in declaration.
>
>> +
>> + /* 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, fmt);
>
> Does this call belong here?
Yes, if the hflip/vflip controls change then fmt->code needs to be
updated to the now changed bayer-order. t4ka3_set_bayer_order()
uses the cached ctrl->val values so it is cheap enough to
always do this instead of checking if the changed ctrl is
vflip or hflip.
In case the sensor is actually streaming and we don't hit this path,
the t4ka3_t_vflip()helper will return -EBUSY since changing
the active fmt while streaming is not a good idea.
Looking at this again, I do think that: t4ka3_t_vflip() should
be renamed to t4ka3_update_hvflip() because the current name
is weird.
Regards,
Hans
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v11] media: Add t4ka3 camera sensor driver
2026-03-16 22:00 ` Sakari Ailus
2026-03-17 12:04 ` Hans de Goede
2026-03-17 12:24 ` Hans de Goede
@ 2026-03-17 13:24 ` Kate Hsuan
2026-03-17 17:53 ` Sakari Ailus
2 siblings, 1 reply; 12+ messages in thread
From: Kate Hsuan @ 2026-03-17 13:24 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mauro Carvalho Chehab, Hans de Goede, linux-media, linux-kernel,
Hans de Goede
Hi Sakari,
Thank you for reviewing it.
And thank you, Hans
On Tue, Mar 17, 2026 at 6:00 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Kate,
>
> Thanks for the patch.
>
> Where have you seen this sensor being used, if I may ask?
It can be found on the Xiaomi Pad2 that is based on an Intel Cherry
Trail platform.
>
> On Mon, Mar 16, 2026 at 04:57:04PM +0800, Kate Hsuan wrote:
> > Add the t4ka3 driver from:
> > https://github.com/kitakar5525/surface3-atomisp-cameras.git
> >
> > With many cleanups / changes (almost a full rewrite) to make it suitable
> > for upstream:
> >
> > * Remove the VCM and VCM-OTP support, the mainline kernel models VCMs and
> > calibration data eeproms as separate v4l2-subdev-s.
> >
> > * Remove the integration-factor t4ka3_get_intg_factor() support and IOCTL,
> > this provided info to userspace through an atomisp private IOCTL.
> >
> > * Turn atomisp specific exposure/gain IOCTL into standard v4l2 controls.
> >
> > * Use normal ACPI power-management in combination with runtime-pm support
> > instead of atomisp specific GMIN power-management code.
> >
> > * Turn into a standard V4L2 sensor driver using
> > v4l2_async_register_subdev_sensor().
> >
> > * Add vblank, hblank, and link-freq controls; drop get_frame_interval().
> >
> > * Use CCI register helpers.
> >
> > * Calculate values for modes instead of using fixed register-value lists,
> > allowing arbritrary modes.
> >
> > * Add get_selection() and set_selection() support
> >
> > * Add a CSI2 bus configuration check
> >
> > This been tested on a Xiaomi Mipad2 tablet which has a T4KA3 sensor with
> > DW9761 VCM as back sensor.
> >
> > Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> > Co-developed-by: Hans de Goede <hansg@kernel.org>
> > Signed-off-by: Hans de Goede <hansg@kernel.org>
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
> > ---
> > Changes in v11:
> > 1. Rebase on the latest next branch.
> >
> > Changes in v10:
> > 1. Fix the format settings.
> > 2. Fix the hblank range calculation.
> > 3. In t4ka3_enable_stream(), powerdown when pm_runtime_get_sync() fails.
> > 4. Fix the clean up call sequence when removing the driver.
> > 5. Fix the error handling in t4ka3_probe().
> >
> > Changes in v9:
> > 1. Apply Hans' fix patch to fix the lock issue and squash it into this
> > patch.
> > https://lore.kernel.org/linux-media/33dd5660-efb6-47e0-9672-f3ae65751185@kernel.org/
> >
> > Changes in v8:
> > 1. Drop the local mutex lock and v4l2-core manages all the locking.
> > 2. __t4ka3_get_pad_format() and __t4ka3_get_pad_crop() are replaced with
> > v4l2_subdev_state_get_format() and v4l2_subdev_state_get_crop().
> > 3. The deprecated s_stream was replaced with enable_streams() and
> > disable_streams().
> > 4. Drop unused functions.
> > 5. t4ka3_get_active_format() helper is used to get the active format.
> > 6. v4l2_link_freq_to_bitmap() is used to check and get the supported
> > link frequency.
> >
> > Changes in v7:
> > 1. Add pixel_rate control.
> >
> > Changes in v6:
> > 1. t4ka3_s_config() was removed.
> > 2. The unused macros were removed.
> > 3. The runtime pm initial flow was improved.
> > 4. In remove(), if the device is not in the "suspend" state, the device
> > will be manually turned off.
> >
> > Changes in v5:
> > 1. Improved Kconfig help description.
> >
> > Changes in v4:
> > 1. Another CI issue fixes.
> >
> > Changes in v3:
> > 1. Fix the issues reported by the CI system.
> >
> > Changes in v2:
> > 1. The regmap information was obtained before configuring runtime PM so
> > probe() can return without disabling runtime PM.
> > 2. In t4ka3_s_stream(), return -EBUSY when the streaming is enabled.
> > ---
> > drivers/media/i2c/Kconfig | 12 +
> > drivers/media/i2c/Makefile | 1 +
> > drivers/media/i2c/t4ka3.c | 1085 ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 1098 insertions(+)
> > create mode 100644 drivers/media/i2c/t4ka3.c
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 20482be35f26..6344defdbb51 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -790,6 +790,18 @@ config VIDEO_S5KJN1
> > To compile this driver as a module, choose M here: the
> > module will be called s5kjn1.
> >
> > +config VIDEO_T4KA3
> > + tristate "Toshiba T4KA3 sensor support"
> > + depends on ACPI || COMPILE_TEST
> > + depends on GPIOLIB
> > + select V4L2_CCI_I2C
> > + help
> > + This is a Video4Linux2 sensor driver for the Toshiba T4KA3 8 MP
> > + camera sensor.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called t4ka3.
> > +
> > config VIDEO_VD55G1
> > tristate "ST VD55G1 sensor support"
> > select V4L2_CCI_I2C
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index a3a6396df3c4..64c0c7964998 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -139,6 +139,7 @@ obj-$(CONFIG_VIDEO_SAA717X) += saa717x.o
> > obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
> > obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
> > obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
> > +obj-$(CONFIG_VIDEO_T4KA3) += t4ka3.o
> > obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
> > obj-$(CONFIG_VIDEO_TC358746) += tc358746.o
> > obj-$(CONFIG_VIDEO_TDA1997X) += tda1997x.o
> > diff --git a/drivers/media/i2c/t4ka3.c b/drivers/media/i2c/t4ka3.c
> > new file mode 100644
> > index 000000000000..d9af5e51f7a8
> > --- /dev/null
> > +++ b/drivers/media/i2c/t4ka3.c
> > @@ -0,0 +1,1085 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Support for T4KA3 8M camera sensor.
> > + *
> > + * Copyright (C) 2015 Intel Corporation. All Rights Reserved.
> > + * Copyright (C) 2016 XiaoMi, Inc.
> > + * Copyright (C) 2024 Hans de Goede <hansg@kernel.org>
>
> Any 2026 copyrights?
>
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/bits.h>
> > +#include <linux/delay.h>
> > +#include <linux/dev_printk.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/mutex.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/types.h>
> > +
> > +#include <media/media-entity.h>
> > +#include <media/v4l2-async.h>
> > +#include <media/v4l2-cci.h>
> > +#include <media/v4l2-common.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-fwnode.h>
> > +#include <media/v4l2-subdev.h>
> > +
> > +#define T4KA3_NATIVE_WIDTH 3280
> > +#define T4KA3_NATIVE_HEIGHT 2464
> > +#define T4KA3_NATIVE_START_LEFT 0
> > +#define T4KA3_NATIVE_START_TOP 0
> > +#define T4KA3_ACTIVE_WIDTH 3280
> > +#define T4KA3_ACTIVE_HEIGHT 2460
> > +#define T4KA3_ACTIVE_START_LEFT 0
> > +#define T4KA3_ACTIVE_START_TOP 2
> > +#define T4KA3_MIN_CROP_WIDTH 2
> > +#define T4KA3_MIN_CROP_HEIGHT 2
> > +
> > +#define T4KA3_PIXELS_PER_LINE 3440
> > +#define T4KA3_LINES_PER_FRAME_30FPS 2492
> > +#define T4KA3_FPS 30
> > +#define T4KA3_PIXEL_RATE \
> > + (T4KA3_PIXELS_PER_LINE * T4KA3_LINES_PER_FRAME_30FPS * T4KA3_FPS)
> > +
> > +/*
> > + * TODO this really should be derived from the 19.2 MHz xvclk combined
> > + * with the PLL settings. But without a datasheet this is the closest
> > + * approximation possible.
> > + *
> > + * link-freq = pixel_rate * bpp / (lanes * 2)
> > + * (lanes * 2) because CSI lanes use double-data-rate (DDR) signalling.
> > + * bpp = 10 and lanes = 4
> > + */
> > +#define T4KA3_LINK_FREQ ((u64)T4KA3_PIXEL_RATE * 10 / 8)
> > +
> > +/* For enum_frame_size() full-size + binned-/quarter-size */
> > +#define T4KA3_FRAME_SIZES 2
> > +
> > +#define T4KA3_REG_PRODUCT_ID_HIGH CCI_REG8(0x0000)
> > +#define T4KA3_REG_PRODUCT_ID_LOW CCI_REG8(0x0001)
> > +#define T4KA3_PRODUCT_ID 0x1490
> > +
> > +#define T4KA3_REG_STREAM CCI_REG8(0x0100)
> > +#define T4KA3_REG_IMG_ORIENTATION CCI_REG8(0x0101)
> > +#define T4KA3_HFLIP_BIT BIT(0)
> > +#define T4KA3_VFLIP_BIT BIT(1)
> > +#define T4KA3_REG_PARAM_HOLD CCI_REG8(0x0104)
> > +#define T4KA3_REG_COARSE_INTEGRATION_TIME CCI_REG16(0x0202)
> > +#define T4KA3_COARSE_INTEGRATION_TIME_MARGIN 6
> > +#define T4KA3_REG_DIGGAIN_GREEN_R CCI_REG16(0x020e)
> > +#define T4KA3_REG_DIGGAIN_RED CCI_REG16(0x0210)
> > +#define T4KA3_REG_DIGGAIN_BLUE CCI_REG16(0x0212)
> > +#define T4KA3_REG_DIGGAIN_GREEN_B CCI_REG16(0x0214)
> > +#define T4KA3_REG_GLOBAL_GAIN CCI_REG16(0x0234)
> > +#define T4KA3_MIN_GLOBAL_GAIN_SUPPORTED 0x0080
> > +#define T4KA3_MAX_GLOBAL_GAIN_SUPPORTED 0x07ff
> > +#define T4KA3_REG_FRAME_LENGTH_LINES CCI_REG16(0x0340) /* aka VTS */
> > +/* FIXME: need a datasheet to verify the min + max vblank values */
> > +#define T4KA3_MIN_VBLANK 4
> > +#define T4KA3_MAX_VBLANK 0xffff
> > +#define T4KA3_REG_PIXELS_PER_LINE CCI_REG16(0x0342) /* aka HTS */
> > +/* These 2 being horz/vert start is a guess (no datasheet), always 0 */
> > +#define T4KA3_REG_HORZ_START CCI_REG16(0x0344)
> > +#define T4KA3_REG_VERT_START CCI_REG16(0x0346)
> > +/* Always 3279 (T4KA3_NATIVE_WIDTH - 1, window is used to crop */
> > +#define T4KA3_REG_HORZ_END CCI_REG16(0x0348)
> > +/* Always 2463 (T4KA3_NATIVE_HEIGHT - 1, window is used to crop */
> > +#define T4KA3_REG_VERT_END CCI_REG16(0x034a)
> > +/* Output size (after cropping/window) */
> > +#define T4KA3_REG_HORZ_OUTPUT_SIZE CCI_REG16(0x034c)
> > +#define T4KA3_REG_VERT_OUTPUT_SIZE CCI_REG16(0x034e)
> > +/* Window/crop start + size *after* binning */
> > +#define T4KA3_REG_WIN_START_X CCI_REG16(0x0408)
> > +#define T4KA3_REG_WIN_START_Y CCI_REG16(0x040a)
> > +#define T4KA3_REG_WIN_WIDTH CCI_REG16(0x040c)
> > +#define T4KA3_REG_WIN_HEIGHT CCI_REG16(0x040e)
> > +#define T4KA3_REG_TEST_PATTERN_MODE CCI_REG8(0x0601)
> > +/* Unknown register at address 0x0900 */
> > +#define T4KA3_REG_0900 CCI_REG8(0x0900)
> > +#define T4KA3_REG_BINNING CCI_REG8(0x0901)
> > +#define T4KA3_BINNING_VAL(_b) \
> > + ({ typeof(_b) (b) = (_b); \
> > + ((b) << 4) | (b); })
>
> I'd either use an inline function or a regular macro here; in the latter
> case I wouldn't mind about the checkpatch.pl warning related to argument
> double use.
>
> > +
> > +struct t4ka3_ctrls {
> > + struct v4l2_ctrl_handler handler;
> > + struct v4l2_ctrl *hflip;
> > + struct v4l2_ctrl *vflip;
> > + struct v4l2_ctrl *vblank;
> > + struct v4l2_ctrl *hblank;
> > + struct v4l2_ctrl *exposure;
> > + struct v4l2_ctrl *gain;
> > + struct v4l2_ctrl *test_pattern;
> > + struct v4l2_ctrl *link_freq;
> > + struct v4l2_ctrl *pixel_rate;
>
> Do you need all these in the struct? E.g. gain appears to be unused.
I'll review all the items in it and drop the unused variables.
>
> > +};
> > +
> > +struct t4ka3_mode {
> > + int binning;
> > + u16 win_x;
> > + u16 win_y;
>
> The rest of the fields have just a space between the type and the field
> name. I'd do the same here.
I'll fix it.
>
> > +};
> > +
> > +struct t4ka3_data {
> > + struct v4l2_subdev sd;
> > + struct media_pad pad;
> > + struct mutex lock; /* serialize sensor's ioctl */
> > + struct t4ka3_ctrls ctrls;
> > + struct t4ka3_mode mode;
> > + struct device *dev;
> > + struct regmap *regmap;
> > + struct gpio_desc *powerdown_gpio;
> > + struct gpio_desc *reset_gpio;
> > + s64 link_freq[1];
> > + int streaming;
> > +
> > + /* MIPI lane info */
> > + u32 link_freq_index;
> > + u8 mipi_lanes;
> > +};
> > +
> > +/* init settings */
> > +static const struct cci_reg_sequence t4ka3_init_config[] = {
> > + {CCI_REG8(0x4136), 0x13},
>
> { Spaces inside braces, please. },
>
> > + {CCI_REG8(0x4137), 0x33},
> > + {CCI_REG8(0x3094), 0x01},
> > + {CCI_REG8(0x0233), 0x01},
> > + {CCI_REG8(0x4B06), 0x01},
> > + {CCI_REG8(0x4B07), 0x01},
> > + {CCI_REG8(0x3028), 0x01},
> > + {CCI_REG8(0x3032), 0x14},
> > + {CCI_REG8(0x305C), 0x0C},
> > + {CCI_REG8(0x306D), 0x0A},
> > + {CCI_REG8(0x3071), 0xFA},
> > + {CCI_REG8(0x307E), 0x0A},
> > + {CCI_REG8(0x307F), 0xFC},
> > + {CCI_REG8(0x3091), 0x04},
> > + {CCI_REG8(0x3092), 0x60},
> > + {CCI_REG8(0x3096), 0xC0},
> > + {CCI_REG8(0x3100), 0x07},
> > + {CCI_REG8(0x3101), 0x4C},
> > + {CCI_REG8(0x3118), 0xCC},
> > + {CCI_REG8(0x3139), 0x06},
> > + {CCI_REG8(0x313A), 0x06},
> > + {CCI_REG8(0x313B), 0x04},
> > + {CCI_REG8(0x3143), 0x02},
> > + {CCI_REG8(0x314F), 0x0E},
> > + {CCI_REG8(0x3169), 0x99},
> > + {CCI_REG8(0x316A), 0x99},
> > + {CCI_REG8(0x3171), 0x05},
> > + {CCI_REG8(0x31A1), 0xA7},
> > + {CCI_REG8(0x31A2), 0x9C},
> > + {CCI_REG8(0x31A3), 0x8F},
> > + {CCI_REG8(0x31A4), 0x75},
> > + {CCI_REG8(0x31A5), 0xEE},
> > + {CCI_REG8(0x31A6), 0xEA},
> > + {CCI_REG8(0x31A7), 0xE4},
> > + {CCI_REG8(0x31A8), 0xE4},
> > + {CCI_REG8(0x31DF), 0x05},
> > + {CCI_REG8(0x31EC), 0x1B},
> > + {CCI_REG8(0x31ED), 0x1B},
> > + {CCI_REG8(0x31EE), 0x1B},
> > + {CCI_REG8(0x31F0), 0x1B},
> > + {CCI_REG8(0x31F1), 0x1B},
> > + {CCI_REG8(0x31F2), 0x1B},
> > + {CCI_REG8(0x3204), 0x3F},
> > + {CCI_REG8(0x3205), 0x03},
> > + {CCI_REG8(0x3210), 0x01},
> > + {CCI_REG8(0x3216), 0x68},
> > + {CCI_REG8(0x3217), 0x58},
> > + {CCI_REG8(0x3218), 0x58},
> > + {CCI_REG8(0x321A), 0x68},
> > + {CCI_REG8(0x321B), 0x60},
> > + {CCI_REG8(0x3238), 0x03},
> > + {CCI_REG8(0x3239), 0x03},
> > + {CCI_REG8(0x323A), 0x05},
> > + {CCI_REG8(0x323B), 0x06},
> > + {CCI_REG8(0x3243), 0x03},
> > + {CCI_REG8(0x3244), 0x08},
> > + {CCI_REG8(0x3245), 0x01},
> > + {CCI_REG8(0x3307), 0x19},
> > + {CCI_REG8(0x3308), 0x19},
> > + {CCI_REG8(0x3320), 0x01},
> > + {CCI_REG8(0x3326), 0x15},
> > + {CCI_REG8(0x3327), 0x0D},
> > + {CCI_REG8(0x3328), 0x01},
> > + {CCI_REG8(0x3380), 0x01},
> > + {CCI_REG8(0x339E), 0x07},
> > + {CCI_REG8(0x3424), 0x00},
> > + {CCI_REG8(0x343C), 0x01},
> > + {CCI_REG8(0x3398), 0x04},
> > + {CCI_REG8(0x343A), 0x10},
> > + {CCI_REG8(0x339A), 0x22},
> > + {CCI_REG8(0x33B4), 0x00},
> > + {CCI_REG8(0x3393), 0x01},
> > + {CCI_REG8(0x33B3), 0x6E},
> > + {CCI_REG8(0x3433), 0x06},
> > + {CCI_REG8(0x3433), 0x00},
> > + {CCI_REG8(0x33B3), 0x00},
> > + {CCI_REG8(0x3393), 0x03},
> > + {CCI_REG8(0x33B4), 0x03},
> > + {CCI_REG8(0x343A), 0x00},
> > + {CCI_REG8(0x339A), 0x00},
> > + {CCI_REG8(0x3398), 0x00}
> > +};
> > +
> > +static const struct cci_reg_sequence t4ka3_pre_mode_set_regs[] = {
> > + {CCI_REG8(0x0112), 0x0A},
> > + {CCI_REG8(0x0113), 0x0A},
> > + {CCI_REG8(0x0114), 0x03},
> > + {CCI_REG8(0x4136), 0x13},
> > + {CCI_REG8(0x4137), 0x33},
> > + {CCI_REG8(0x0820), 0x0A},
> > + {CCI_REG8(0x0821), 0x0D},
> > + {CCI_REG8(0x0822), 0x00},
> > + {CCI_REG8(0x0823), 0x00},
> > + {CCI_REG8(0x0301), 0x0A},
> > + {CCI_REG8(0x0303), 0x01},
> > + {CCI_REG8(0x0305), 0x04},
> > + {CCI_REG8(0x0306), 0x02},
> > + {CCI_REG8(0x0307), 0x18},
> > + {CCI_REG8(0x030B), 0x01},
> > +};
> > +
> > +static const struct cci_reg_sequence t4ka3_post_mode_set_regs[] = {
> > + {CCI_REG8(0x0902), 0x00},
> > + {CCI_REG8(0x4220), 0x00},
> > + {CCI_REG8(0x4222), 0x01},
> > + {CCI_REG8(0x3380), 0x01},
> > + {CCI_REG8(0x3090), 0x88},
> > + {CCI_REG8(0x3394), 0x20},
> > + {CCI_REG8(0x3090), 0x08},
> > + {CCI_REG8(0x3394), 0x10}
> > +};
> > +
> > +static const s64 link_freq_menu_items[] = {
> > + T4KA3_LINK_FREQ,
> > +};
> > +
> > +static inline struct t4ka3_data *to_t4ka3_sensor(struct v4l2_subdev *sd)
> > +{
> > + return container_of(sd, struct t4ka3_data, sd);
> > +}
> > +
> > +static inline struct t4ka3_data *ctrl_to_t4ka3(struct v4l2_ctrl *ctrl)
> > +{
> > + return container_of(ctrl->handler, struct t4ka3_data, ctrls.handler);
> > +}
>
> I'd use macros and container_of_const().
>
> > +
> > +/* T4KA3 default GRBG */
> > +static const int t4ka3_hv_flip_bayer_order[] = {
> > + MEDIA_BUS_FMT_SGRBG10_1X10,
> > + MEDIA_BUS_FMT_SBGGR10_1X10,
> > + MEDIA_BUS_FMT_SRGGB10_1X10,
> > + MEDIA_BUS_FMT_SGBRG10_1X10,
> > +};
> > +
> > +static const struct v4l2_rect t4ka3_default_crop = {
> > + .left = T4KA3_ACTIVE_START_LEFT,
> > + .top = T4KA3_ACTIVE_START_TOP,
> > + .width = T4KA3_ACTIVE_WIDTH,
> > + .height = T4KA3_ACTIVE_HEIGHT,
> > +};
> > +
> > +static int t4ka3_detect(struct t4ka3_data *sensor, u16 *id);
>
> Not needed.
Okay.
>
> > +
> > +static void t4ka3_set_bayer_order(struct t4ka3_data *sensor,
> > + struct v4l2_mbus_framefmt *fmt)
> > +{
> > + int hv_flip = 0;
>
> unsigned int?
unsigned int :)
>
> > +
> > + if (sensor->ctrls.vflip && sensor->ctrls.vflip->val)
> > + hv_flip += 1;
> > +
> > + if (sensor->ctrls.hflip && sensor->ctrls.hflip->val)
> > + hv_flip += 2;
> > +
> > + fmt->code = t4ka3_hv_flip_bayer_order[hv_flip];
> > +}
> > +
> > +static struct v4l2_mbus_framefmt *t4ka3_get_active_format(struct t4ka3_data *sensor)
> > +{
> > + struct v4l2_subdev_state *active_state =
> > + v4l2_subdev_get_locked_active_state(&sensor->sd);
> > +
> > + return v4l2_subdev_state_get_format(active_state, 0);
> > +}
> > +
> > +static struct v4l2_rect *t4ka3_get_active_crop(struct t4ka3_data *sensor)
> > +{
> > + struct v4l2_subdev_state *active_state =
> > + v4l2_subdev_get_locked_active_state(&sensor->sd);
> > +
> > + return v4l2_subdev_state_get_crop(active_state, 0);
>
> Please avoid adding such helpers.
As Hans mentioned, we can put active format and crop in the t4ka3_data
or keep the helpers.
>
> > +}
> > +
> > +static int t4ka3_update_exposure_range(struct t4ka3_data *sensor)
> > +{
> > + struct v4l2_mbus_framefmt *fmt;
> > +
> > + fmt = t4ka3_get_active_format(sensor);
>
> Can be assigned in declaration.
Okay
>
> > +
> > + int exp_max = 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_fill_format(struct t4ka3_data *sensor,
> > + struct v4l2_mbus_framefmt *fmt,
> > + unsigned int width, unsigned int height)
> > +{
> > + memset(fmt, 0, sizeof(*fmt));
> > + fmt->width = width;
> > + fmt->height = height;
> > + fmt->field = V4L2_FIELD_NONE;
> > + fmt->colorspace = V4L2_COLORSPACE_RAW;
> > + t4ka3_set_bayer_order(sensor, fmt);
> > +}
> > +
> > +static void t4ka3_calc_mode(struct t4ka3_data *sensor)
> > +{
> > + struct v4l2_mbus_framefmt *fmt;
> > + struct v4l2_rect *crop;
> > + int width;
> > + int height;
> > + int binning;
> > +
> > + fmt = t4ka3_get_active_format(sensor);
> > + crop = t4ka3_get_active_crop(sensor);
>
> Ditto.
>
> > +
> > + width = fmt->width;
> > + height = fmt->height;
> > +
> > + if (width <= (crop->width / 2) && height <= (crop->height / 2))
> > + binning = 2;
> > + else
> > + binning = 1;
> > +
> > + width *= binning;
> > + height *= binning;
> > +
> > + sensor->mode.binning = binning;
> > + sensor->mode.win_x = (crop->left + (crop->width - width) / 2) & ~1;
> > + sensor->mode.win_y = (crop->top + (crop->height - height) / 2) & ~1;
> > + /*
> > + * t4ka3'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)
> > +{
> > + struct v4l2_mbus_framefmt *fmt;
> > +
> > + fmt = t4ka3_get_active_format(sensor);
>
> Ditto.
>
> > +
> > + *min = T4KA3_MIN_VBLANK + (sensor->mode.binning - 1) * fmt->height;
> > + *max = T4KA3_MAX_VBLANK - fmt->height;
> > + *def = T4KA3_LINES_PER_FRAME_30FPS - fmt->height;
> > +}
> > +
> > +static int t4ka3_set_pad_format(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_format *format)
> > +{
> > + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> > + struct v4l2_mbus_framefmt *try_fmt;
> > + struct v4l2_mbus_framefmt *fmt;
> > + const struct v4l2_rect *crop;
> > + unsigned int width, height;
> > + int min, max, def, ret = 0;
> > +
> > + crop = t4ka3_get_active_crop(sensor);
> > + fmt = t4ka3_get_active_format(sensor);
> > +
> > + /* Limit set_fmt max size to crop width / height */
> > + width = clamp_val(ALIGN(format->format.width, 2),
> > + T4KA3_MIN_CROP_WIDTH, crop->width);
> > + height = clamp_val(ALIGN(format->format.height, 2),
> > + T4KA3_MIN_CROP_HEIGHT, crop->height);
> > + t4ka3_fill_format(sensor, &format->format, width, height);
> > +
> > + if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> > + try_fmt = v4l2_subdev_state_get_format(sd_state, 0);
> > + *try_fmt = format->format;
> > + return 0;
> > + }
> > +
> > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE && sensor->streaming)
> > + return -EBUSY;
> > +
> > + *v4l2_subdev_state_get_format(sd_state, 0) = format->format;
> > +
> > + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> > + return 0;
> > +
> > + t4ka3_calc_mode(sensor);
> > +
> > + /* vblank range is height dependent adjust and reset to default */
> > + t4ka3_get_vblank_limits(sensor, &min, &max, &def);
> > + ret = __v4l2_ctrl_modify_range(sensor->ctrls.vblank, min, max, 1, def);
> > + if (ret)
> > + return ret;
> > +
> > + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, def);
> > + if (ret)
> > + return ret;
> > +
> > + def = T4KA3_PIXELS_PER_LINE - fmt->width;
> > + ret = __v4l2_ctrl_modify_range(sensor->ctrls.hblank, def, def, 1, def);
> > + if (ret)
> > + return ret;
> > +
> > + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.hblank, def);
> > + if (ret)
> > + return ret;
>
> return __v4l2_ctrl_s_ctrl(...);
Okay
>
> > +
> > + return 0;
> > +}
> > +
> > +/* Horizontal or vertically flip the image */
> > +static int t4ka3_t_vflip(struct v4l2_subdev *sd, int value, u8 flip_bit)
> > +{
> > + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> > + struct v4l2_mbus_framefmt *fmt;
> > + int ret;
> > + u64 val;
> > +
> > + if (sensor->streaming)
> > + return -EBUSY;
> > +
> > + val = value ? flip_bit : 0;
> > +
> > + ret = cci_update_bits(sensor->regmap, T4KA3_REG_IMG_ORIENTATION,
> > + flip_bit, val, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + fmt = t4ka3_get_active_format(sensor);
> > + t4ka3_set_bayer_order(sensor, fmt);
>
> A newline would be nice here.
Okay
>
> > + return 0;
> > +}
> > +
> > +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);
> > + struct v4l2_mbus_framefmt *fmt;
> > + int ret;
> > +
> > + /* Update exposure range on vblank changes */
> > + if (ctrl->id == V4L2_CID_VBLANK) {
> > + ret = t4ka3_update_exposure_range(sensor);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + fmt = t4ka3_get_active_format(sensor);
>
> You could assign this in declaration.
Okay
>
> > +
> > + /* 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, fmt);
>
> Does this call belong here?
I think it can be. It is a simple update of t4ka3_hv_flip_bayer_order.
>
> > + 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, T4KA3_VFLIP_BIT);
> > + break;
> > + case V4L2_CID_HFLIP:
> > + ret = t4ka3_t_vflip(&sensor->sd, ctrl->val, T4KA3_HFLIP_BIT);
> > + break;
> > + case V4L2_CID_VBLANK:
> > + ret = cci_write(sensor->regmap, T4KA3_REG_FRAME_LENGTH_LINES,
> > + fmt->height + ctrl->val, NULL);
> > + 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);
>
> Newline here?
Okay
>
> > + return ret;
> > +}
> > +
> > +static int t4ka3_set_mode(struct t4ka3_data *sensor)
> > +{
> > + struct v4l2_mbus_framefmt *fmt;
> > + int ret = 0;
> > +
> > + fmt = t4ka3_get_active_format(sensor);
> > +
> > + cci_write(sensor->regmap, T4KA3_REG_HORZ_OUTPUT_SIZE, fmt->width, &ret);
> > + /* Write mode-height - 2 otherwise things don't work, hw-bug ? */
> > + cci_write(sensor->regmap, T4KA3_REG_VERT_OUTPUT_SIZE, 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, fmt->width, &ret);
> > + cci_write(sensor->regmap, T4KA3_REG_WIN_HEIGHT, 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_enable_stream(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
> > + u32 pad, u64 streams_mask)
> > +{
> > + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> > + int ret;
> > +
> > + ret = pm_runtime_get_sync(sensor->sd.dev);
> > + if (ret < 0) {
> > + dev_err(sensor->dev, "power-up err.\n");
> > + goto error_powerdown;
> > + }
> > +
> > + cci_multi_reg_write(sensor->regmap, t4ka3_init_config,
> > + ARRAY_SIZE(t4ka3_init_config), &ret);
> > + /* enable group hold */
> > + cci_write(sensor->regmap, T4KA3_REG_PARAM_HOLD, 1, &ret);
> > + cci_multi_reg_write(sensor->regmap, t4ka3_pre_mode_set_regs,
> > + ARRAY_SIZE(t4ka3_pre_mode_set_regs), &ret);
> > + if (ret)
> > + goto error_powerdown;
> > +
> > + ret = t4ka3_set_mode(sensor);
> > + if (ret)
> > + goto error_powerdown;
> > +
> > + ret = cci_multi_reg_write(sensor->regmap, t4ka3_post_mode_set_regs,
> > + ARRAY_SIZE(t4ka3_post_mode_set_regs), NULL);
> > + if (ret)
> > + goto error_powerdown;
> > +
> > + /* Restore value of all ctrls */
> > + ret = __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
> > + if (ret)
> > + goto error_powerdown;
> > +
> > + /* disable group hold */
> > + cci_write(sensor->regmap, T4KA3_REG_PARAM_HOLD, 0, &ret);
> > + cci_write(sensor->regmap, T4KA3_REG_STREAM, 1, &ret);
> > + if (ret)
> > + goto error_powerdown;
> > +
> > + sensor->streaming = 1;
> > +
> > + return ret;
> > +
> > +error_powerdown:
> > + pm_runtime_put(sensor->sd.dev);
>
> And here?
Okay
>
> > + return ret;
> > +}
> > +
> > +static int t4ka3_disable_stream(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
> > + u32 pad, u64 streams_mask)
> > +{
> > + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> > + int ret;
> > +
> > + ret = cci_write(sensor->regmap, T4KA3_REG_STREAM, 0, NULL);
> > + pm_runtime_put(sensor->sd.dev);
> > + sensor->streaming = 0;
> > + return ret;
>
> Return 0 here but complain about it.
Do you mean return 0 here and print a message when ret != 0?
>
> > +}
> > +
> > +static int t4ka3_get_selection(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *state,
> > + struct v4l2_subdev_selection *sel)
> > +{
> > + switch (sel->target) {
> > + case V4L2_SEL_TGT_CROP:
> > + sel->r = *v4l2_subdev_state_get_crop(state, sel->pad);
> > + break;
> > + case V4L2_SEL_TGT_NATIVE_SIZE:
> > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > + sel->r.top = 0;
> > + sel->r.left = 0;
> > + sel->r.width = T4KA3_NATIVE_WIDTH;
> > + sel->r.height = T4KA3_NATIVE_HEIGHT;
> > + break;
> > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > + sel->r = t4ka3_default_crop;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int t4ka3_set_selection(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *state,
> > + struct v4l2_subdev_selection *sel)
> > +{
> > + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> > + struct v4l2_mbus_framefmt *format;
> > + struct v4l2_rect *crop;
> > + struct v4l2_rect rect;
> > +
> > + if (sel->target != V4L2_SEL_TGT_CROP)
> > + return -EINVAL;
> > +
> > + /*
> > + * Clamp the boundaries of the crop rectangle to the size of the sensor
> > + * pixel array. Align to multiples of 2 to ensure Bayer pattern isn't
> > + * disrupted.
> > + */
> > + rect.left = clamp_val(ALIGN(sel->r.left, 2),
> > + T4KA3_NATIVE_START_LEFT, T4KA3_NATIVE_WIDTH);
> > + rect.top = clamp_val(ALIGN(sel->r.top, 2),
> > + T4KA3_NATIVE_START_TOP, T4KA3_NATIVE_HEIGHT);
> > + rect.width = clamp_val(ALIGN(sel->r.width, 2), T4KA3_MIN_CROP_WIDTH,
> > + T4KA3_NATIVE_WIDTH - rect.left);
> > + rect.height = clamp_val(ALIGN(sel->r.height, 2), T4KA3_MIN_CROP_HEIGHT,
> > + T4KA3_NATIVE_HEIGHT - rect.top);
> > +
> > + crop = v4l2_subdev_state_get_crop(state, sel->pad);
> > +
> > + if (rect.width != crop->width || rect.height != crop->height) {
> > + /*
> > + * Reset the output image size if the crop rectangle size has
> > + * been modified.
> > + */
> > + format = v4l2_subdev_state_get_format(state, sel->pad);
> > + format->width = rect.width;
> > + format->height = rect.height;
> > + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > + t4ka3_calc_mode(sensor);
> > + }
> > +
> > + sel->r = *crop = rect;
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +t4ka3_enum_mbus_code(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > + if (code->index)
> > + return -EINVAL;
> > +
> > + code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> > + return 0;
> > +}
> > +
> > +static int t4ka3_enum_frame_size(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > + struct v4l2_rect *crop;
> > +
> > + if (fse->index >= T4KA3_FRAME_SIZES)
> > + return -EINVAL;
> > +
> > + crop = v4l2_subdev_state_get_crop(sd_state, fse->pad);
> > +
> > + fse->min_width = crop->width / (fse->index + 1);
> > + fse->min_height = crop->height / (fse->index + 1);
> > + fse->max_width = fse->min_width;
> > + fse->max_height = fse->min_height;
> > +
> > + return 0;
> > +}
> > +
> > +static int t4ka3_check_hwcfg(struct t4ka3_data *sensor)
> > +{
> > + struct fwnode_handle *fwnode = dev_fwnode(sensor->dev);
> > + struct v4l2_fwnode_endpoint bus_cfg = {
> > + .bus_type = V4L2_MBUS_CSI2_DPHY,
> > + };
> > + struct fwnode_handle *endpoint;
> > + unsigned long link_freq_bitmap;
> > + int ret;
> > +
> > + /*
> > + * Sometimes the fwnode graph is initialized by the bridge driver.
> > + * Bridge drivers doing this may also add GPIO mappings, wait for this.
> > + */
>
> No need for such a comment.
I'll drop it.
>
> > + endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > + if (!endpoint)
> > + return dev_err_probe(sensor->dev, -EPROBE_DEFER,
> > + "waiting for fwnode graph endpoint\n");
>
> This
> <URL:https://git.linuxtv.org/sailus/media_tree.git/commit/?h=cleanup&id=8181d18d45d593d8499cbf0e83de08c6d913516c>
> will be merged soon.
Does it mean "return -EPROBE_DEFER;" is enough?
>
> > +
> > + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
> > + fwnode_handle_put(endpoint);
> > + if (ret)
> > + return ret;
> > +
> > + ret = v4l2_link_freq_to_bitmap(sensor->dev, bus_cfg.link_frequencies,
> > + bus_cfg.nr_of_link_frequencies,
> > + link_freq_menu_items,
> > + ARRAY_SIZE(link_freq_menu_items),
> > + &link_freq_bitmap);
> > +
> > + if (ret == -ENOENT) {
> > + dev_err_probe(sensor->dev, -ENOENT,
> > + "No match found between driver-supported link frequencies.\n");
> > + goto out_free_bus_cfg;
> > + }
> > +
> > + if (ret == -ENODATA) {
> > + dev_err_probe(sensor->dev, -ENODATA,
> > + "No link frequency was specified in the firmware.\n");
> > + goto out_free_bus_cfg;
> > + }
>
> No need for printing these error messages -- v4l2_link_freq_to_bitmap()
> already does.
I'll drop the above two if sections.
>
> > +
> > + sensor->link_freq_index = ffs(link_freq_bitmap) - 1;
> > +
> > + /* 4 MIPI lanes */
> > + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> > + ret = dev_err_probe(sensor->dev, -EINVAL,
> > + "number of CSI2 data lanes %u is not supported\n",
> > + bus_cfg.bus.mipi_csi2.num_data_lanes);
> > + goto out_free_bus_cfg;
> > + }
> > +
> > + sensor->mipi_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
> > +
> > +out_free_bus_cfg:
> > + v4l2_fwnode_endpoint_free(&bus_cfg);
> > +
> > + return ret;
> > +}
> > +
> > +static int t4ka3_init_state(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *sd_state)
> > +{
> > + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> > +
> > + *v4l2_subdev_state_get_crop(sd_state, 0) = t4ka3_default_crop;
> > +
> > + t4ka3_fill_format(sensor, v4l2_subdev_state_get_format(sd_state, 0),
> > + T4KA3_ACTIVE_WIDTH, T4KA3_ACTIVE_HEIGHT);
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops t4ka3_ctrl_ops = {
> > + .s_ctrl = t4ka3_s_ctrl,
> > +};
> > +
> > +static const struct v4l2_subdev_video_ops t4ka3_video_ops = {
> > + .s_stream = v4l2_subdev_s_stream_helper,
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops t4ka3_pad_ops = {
> > + .enum_mbus_code = t4ka3_enum_mbus_code,
> > + .enum_frame_size = t4ka3_enum_frame_size,
> > + .get_fmt = v4l2_subdev_get_fmt,
> > + .set_fmt = t4ka3_set_pad_format,
> > + .get_selection = t4ka3_get_selection,
> > + .set_selection = t4ka3_set_selection,
> > + .enable_streams = t4ka3_enable_stream,
> > + .disable_streams = t4ka3_disable_stream,
> > +};
> > +
> > +static const struct v4l2_subdev_ops t4ka3_ops = {
> > + .video = &t4ka3_video_ops,
> > + .pad = &t4ka3_pad_ops,
> > +};
> > +
> > +static const struct v4l2_subdev_internal_ops t4ka3_internal_ops = {
> > + .init_state = t4ka3_init_state,
> > +};
> > +
> > +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);
> > +
> > + v4l2_subdev_lock_state(sensor->sd.active_state);
> > + t4ka3_calc_mode(sensor);
> > + t4ka3_get_vblank_limits(sensor, &min, &max, &def);
> > + v4l2_subdev_unlock_state(sensor->sd.active_state);
> > +
> > + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, min, max, 1, def);
> > +
> > + def = T4KA3_PIXELS_PER_LINE - T4KA3_ACTIVE_WIDTH;
> > + ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
> > + def, def, 1, def);
> > +
> > + 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;
>
> A newline would be nice here.
okay
>
> > + return 0;
> > +}
> > +
> > +static int t4ka3_pm_suspend(struct device *dev)
> > +{
> > + struct t4ka3_data *sensor = dev_get_drvdata(dev);
> > +
> > + gpiod_set_value_cansleep(sensor->powerdown_gpio, 1);
> > + gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> > +
> > + return 0;
> > +}
> > +
> > +static int t4ka3_pm_resume(struct device *dev)
> > +{
> > + struct t4ka3_data *sensor = dev_get_drvdata(dev);
> > + u16 sensor_id;
> > + int ret;
> > +
> > + usleep_range(5000, 6000);
> > +
> > + gpiod_set_value_cansleep(sensor->powerdown_gpio, 0);
> > + gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> > +
> > + /* waiting for the sensor after powering up */
> > + msleep(20);
>
> fsleep() maybe?
I can change it.
>
> > +
> > + ret = t4ka3_detect(sensor, &sensor_id);
> > + if (ret) {
> > + dev_err(sensor->dev, "sensor detect failed\n");
> > + return ret;
>
> What about gpio values in this case?
both powerdown_gpio and reset_gpio are 0 when resuming and 1 when suspended.
t4ka3_detect() reads the sensor name through i2c. If it finds the
product ID then return 0;
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static DEFINE_RUNTIME_DEV_PM_OPS(t4ka3_pm_ops, t4ka3_pm_suspend, t4ka3_pm_resume, NULL);
>
> You could run
>
> $ ./scripts/checkpatch.pl --strict --max-line-length=80
I'll try it. :)
>
> on the patch.
>
> > +
> > +static void t4ka3_remove(struct i2c_client *client)
> > +{
> > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> > +
> > + v4l2_async_unregister_subdev(&sensor->sd);
> > + v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> > + v4l2_subdev_cleanup(sd);
> > + media_entity_cleanup(&sensor->sd.entity);
> > +
> > + /*
> > + * Disable runtime PM. In case runtime PM is disabled in the kernel,
> > + * make sure to turn power off manually.
> > + */
> > + pm_runtime_disable(&client->dev);
> > + if (!pm_runtime_status_suspended(&client->dev))
> > + t4ka3_pm_suspend(&client->dev);
> > + pm_runtime_set_suspended(&client->dev);
> > +}
> > +
> > +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;
>
> The driver supports a single link frequency. Could the array holding the
> requencies be static const?
i'll try it. It is used by v4l2_ctrl_new_int_menu()
>
> > +
> > + 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);
>
> You can omit get_noresume() here...
Okay
>
> > + 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 = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> > + if (ret)
> > + goto err_pm_disable;
> > +
> > + sensor->sd.state_lock = sensor->ctrls.handler.lock;
> > + ret = v4l2_subdev_init_finalize(&sensor->sd);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "failed to init subdev: %d", ret);
> > + goto err_media_entity;
> > + }
> > +
> > + ret = t4ka3_init_controls(sensor);
> > + if (ret)
> > + goto err_controls;
> > +
> > + ret = v4l2_async_register_subdev_sensor(&sensor->sd);
> > + if (ret)
> > + goto err_controls;
> > +
> > + pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> > + pm_runtime_use_autosuspend(&client->dev);
> > + pm_runtime_put_autosuspend(&client->dev);
>
> as well as the two autosuspend functions above by switching to
> pm_runtime_idle() here.
okay
>
> > +
> > + return 0;
> > +
> > +err_controls:
> > + v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> > + v4l2_subdev_cleanup(&sensor->sd);
> > +
> > +err_media_entity:
> > + media_entity_cleanup(&sensor->sd.entity);
> > +
> > +err_pm_disable:
> > + pm_runtime_disable(&client->dev);
> > + pm_runtime_put_noidle(&client->dev);
> > + t4ka3_pm_suspend(&client->dev);
> > +
> > + return ret;
> > +}
> > +
> > +static struct acpi_device_id t4ka3_acpi_match[] = {
>
> const?
const
>
> > + { "XMCC0003" },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(acpi, t4ka3_acpi_match);
> > +
> > +static struct i2c_driver t4ka3_driver = {
> > + .driver = {
> > + .name = "t4ka3",
> > + .acpi_match_table = ACPI_PTR(t4ka3_acpi_match),
> > + .pm = pm_sleep_ptr(&t4ka3_pm_ops),
> > + },
> > + .probe = t4ka3_probe,
> > + .remove = t4ka3_remove,
> > +};
> > +module_i2c_driver(t4ka3_driver)
> > +
> > +MODULE_DESCRIPTION("A low-level driver for T4KA3 sensor");
> > +MODULE_AUTHOR("HARVEY LV <harvey.lv@intel.com>");
> > +MODULE_AUTHOR("Kate Hsuan <hpa@redhat.com>");
> > +MODULE_LICENSE("GPL");
>
> --
> Kind regards,
>
> Sakari Ailus
>
I'll propose the v12 patch to include all the changes based on the comments.
--
BR,
Kate
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v11] media: Add t4ka3 camera sensor driver
2026-03-17 12:24 ` Hans de Goede
@ 2026-03-17 13:26 ` Kate Hsuan
2026-03-17 17:58 ` Sakari Ailus
1 sibling, 0 replies; 12+ messages in thread
From: Kate Hsuan @ 2026-03-17 13:26 UTC (permalink / raw)
To: Hans de Goede
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-kernel,
Hans de Goede
Hi Hans,
Thank you for providing the answer
On Tue, Mar 17, 2026 at 8:25 PM Hans de Goede
<johannes.goede@oss.qualcomm.com> wrote:
>
> Hi Sakari,
>
> On 16-Mar-26 23:00, Sakari Ailus wrote:
>
> <snip>
>
> >> diff --git a/drivers/media/i2c/t4ka3.c b/drivers/media/i2c/t4ka3.c
> >> new file mode 100644
> >> index 000000000000..d9af5e51f7a8
> >> --- /dev/null
> >> +++ b/drivers/media/i2c/t4ka3.c
>
> <snip>
>
> >> +static struct v4l2_mbus_framefmt *t4ka3_get_active_format(struct t4ka3_data *sensor)
> >> +{
> >> + struct v4l2_subdev_state *active_state =
> >> + v4l2_subdev_get_locked_active_state(&sensor->sd);
> >> +
> >> + return v4l2_subdev_state_get_format(active_state, 0);
> >> +}
> >> +
> >> +static struct v4l2_rect *t4ka3_get_active_crop(struct t4ka3_data *sensor)
> >> +{
> >> + struct v4l2_subdev_state *active_state =
> >> + v4l2_subdev_get_locked_active_state(&sensor->sd);
> >> +
> >> + return v4l2_subdev_state_get_crop(active_state, 0);
> >
> > Please avoid adding such helpers.
>
> The problem is that we need to know the active-fmt/-crop in some places
> without access to it. E.g. when the vblank ctrl gets set this influences
> the range of the exposure control, so we need active_fmt.height to
> calculate the values to pass to v4l2_ctrl_modify_range() and we need
> this from a v4l2_ctrl_ops.s_ctrl callback which does not get passed
> in the (active) fmt.
>
> Since the ctrl lock is used as the main sensor-driver lock too,
> we can always safely call v4l2_subdev_get_locked_active_state()
> in these cases, since we are always holding the lock.
>
> The alternative would be to store a copy of the active fmt/crop
> inside struct t4ka3_data, but I thought that the whole direction
> for sensor drivers was to stop having (and needing to update) their
> own shadow copy of the active_state and instead direct use
> the active_state ?
>
> <snip>
>
> >> +static int t4ka3_s_ctrl(struct v4l2_ctrl *ctrl)
> >> +{
> >> + struct t4ka3_data *sensor = ctrl_to_t4ka3(ctrl);
> >> + struct v4l2_mbus_framefmt *fmt;
> >> + int ret;
> >> +
> >> + /* Update exposure range on vblank changes */
> >> + if (ctrl->id == V4L2_CID_VBLANK) {
> >> + ret = t4ka3_update_exposure_range(sensor);
> >> + if (ret)
> >> + return ret;
> >> + }
> >> +
> >> + fmt = t4ka3_get_active_format(sensor);
> >
> > You could assign this in declaration.
> >
> >> +
> >> + /* 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, fmt);
> >
> > Does this call belong here?
>
> Yes, if the hflip/vflip controls change then fmt->code needs to be
> updated to the now changed bayer-order. t4ka3_set_bayer_order()
> uses the cached ctrl->val values so it is cheap enough to
> always do this instead of checking if the changed ctrl is
> vflip or hflip.
>
> In case the sensor is actually streaming and we don't hit this path,
> the t4ka3_t_vflip()helper will return -EBUSY since changing
> the active fmt while streaming is not a good idea.
>
> Looking at this again, I do think that: t4ka3_t_vflip() should
> be renamed to t4ka3_update_hvflip() because the current name
> is weird.
I'll change the name of it.
>
> Regards,
>
> Hans
>
>
--
BR,
Kate
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v11] media: Add t4ka3 camera sensor driver
2026-03-17 13:24 ` Kate Hsuan
@ 2026-03-17 17:53 ` Sakari Ailus
2026-03-17 20:17 ` Hans de Goede
2026-03-18 7:28 ` Kate Hsuan
0 siblings, 2 replies; 12+ messages in thread
From: Sakari Ailus @ 2026-03-17 17:53 UTC (permalink / raw)
To: Kate Hsuan
Cc: Mauro Carvalho Chehab, Hans de Goede, linux-media, linux-kernel,
Hans de Goede
Hi Kate,
On Tue, Mar 17, 2026 at 09:24:20PM +0800, Kate Hsuan wrote:
> Hi Sakari,
>
> Thank you for reviewing it.
>
> And thank you, Hans
>
> On Tue, Mar 17, 2026 at 6:00 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Kate,
> >
> > Thanks for the patch.
> >
> > Where have you seen this sensor being used, if I may ask?
>
> It can be found on the Xiaomi Pad2 that is based on an Intel Cherry
> Trail platform.
Ack, thanks for the info!
> > > +static struct v4l2_mbus_framefmt *t4ka3_get_active_format(struct t4ka3_data *sensor)
> > > +{
> > > + struct v4l2_subdev_state *active_state =
> > > + v4l2_subdev_get_locked_active_state(&sensor->sd);
> > > +
> > > + return v4l2_subdev_state_get_format(active_state, 0);
> > > +}
> > > +
> > > +static struct v4l2_rect *t4ka3_get_active_crop(struct t4ka3_data *sensor)
> > > +{
> > > + struct v4l2_subdev_state *active_state =
> > > + v4l2_subdev_get_locked_active_state(&sensor->sd);
> > > +
> > > + return v4l2_subdev_state_get_crop(active_state, 0);
> >
> > Please avoid adding such helpers.
> As Hans mentioned, we can put active format and crop in the t4ka3_data
> or keep the helpers.
What prevents you doing:
struct v4l2_subdev_state *active_state =
v4l2_subdev_get_locked_active_state(&sensor->sd);
struct v4l2_rect *r = v4l2_subdev_state_get_crop(active_state, 0);
in the code? In a lot of the cases you could simply pass the state to the
function using it as the caller already has it.
It also seems the driver operates in active state whereas any state should
be valid. In general, file handle specific try state should resemble the
active state as much as possible with the exception that it won't be
applied to hardware.
In the long run I'd also expect the control values to move to state.
...
> > > +static int t4ka3_s_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > + struct t4ka3_data *sensor = ctrl_to_t4ka3(ctrl);
> > > + struct v4l2_mbus_framefmt *fmt;
> > > + int ret;
> > > +
> > > + /* Update exposure range on vblank changes */
> > > + if (ctrl->id == V4L2_CID_VBLANK) {
> > > + ret = t4ka3_update_exposure_range(sensor);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + fmt = t4ka3_get_active_format(sensor);
> >
> > You could assign this in declaration.
> Okay
> >
> > > +
> > > + /* 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, fmt);
> >
> > Does this call belong here?
> I think it can be. It is a simple update of t4ka3_hv_flip_bayer_order.
Why are you doing it here? It basically assigns the Bayer order to the
active format based on the flipping controls. Why not to do that when
changing the flipping controls?
> >
> > > + 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, T4KA3_VFLIP_BIT);
> > > + break;
> > > + case V4L2_CID_HFLIP:
> > > + ret = t4ka3_t_vflip(&sensor->sd, ctrl->val, T4KA3_HFLIP_BIT);
> > > + break;
> > > + case V4L2_CID_VBLANK:
> > > + ret = cci_write(sensor->regmap, T4KA3_REG_FRAME_LENGTH_LINES,
> > > + fmt->height + ctrl->val, NULL);
> > > + 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);
> >
> > Newline here?
> Okay
> >
> > > + return ret;
> > > +}
...
> > > +static int t4ka3_disable_stream(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
> > > + u32 pad, u64 streams_mask)
> > > +{
> > > + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> > > + int ret;
> > > +
> > > + ret = cci_write(sensor->regmap, T4KA3_REG_STREAM, 0, NULL);
> > > + pm_runtime_put(sensor->sd.dev);
> > > + sensor->streaming = 0;
> > > + return ret;
> >
> > Return 0 here but complain about it.
> Do you mean return 0 here and print a message when ret != 0?
Yes, please.
...
> > > +static int t4ka3_check_hwcfg(struct t4ka3_data *sensor)
> > > +{
> > > + struct fwnode_handle *fwnode = dev_fwnode(sensor->dev);
> > > + struct v4l2_fwnode_endpoint bus_cfg = {
> > > + .bus_type = V4L2_MBUS_CSI2_DPHY,
> > > + };
> > > + struct fwnode_handle *endpoint;
> > > + unsigned long link_freq_bitmap;
> > > + int ret;
> > > +
> > > + /*
> > > + * Sometimes the fwnode graph is initialized by the bridge driver.
> > > + * Bridge drivers doing this may also add GPIO mappings, wait for this.
> > > + */
> >
> > No need for such a comment.
> I'll drop it.
>
> >
> > > + endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > > + if (!endpoint)
> > > + return dev_err_probe(sensor->dev, -EPROBE_DEFER,
> > > + "waiting for fwnode graph endpoint\n");
> >
> > This
> > <URL:https://git.linuxtv.org/sailus/media_tree.git/commit/?h=cleanup&id=8181d18d45d593d8499cbf0e83de08c6d913516c>
> > will be merged soon.
> Does it mean "return -EPROBE_DEFER;" is enough?
You can omit checking for errors here.
> >
> > > +
> > > + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
> > > + fwnode_handle_put(endpoint);
> > > + if (ret)
> > > + return ret;
...
> > > +static int t4ka3_pm_resume(struct device *dev)
> > > +{
> > > + struct t4ka3_data *sensor = dev_get_drvdata(dev);
> > > + u16 sensor_id;
> > > + int ret;
> > > +
> > > + usleep_range(5000, 6000);
> > > +
> > > + gpiod_set_value_cansleep(sensor->powerdown_gpio, 0);
> > > + gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> > > +
> > > + /* waiting for the sensor after powering up */
> > > + msleep(20);
> >
> > fsleep() maybe?
> I can change it.
> >
> > > +
> > > + ret = t4ka3_detect(sensor, &sensor_id);
> > > + if (ret) {
> > > + dev_err(sensor->dev, "sensor detect failed\n");
> > > + return ret;
> >
> > What about gpio values in this case?
> both powerdown_gpio and reset_gpio are 0 when resuming and 1 when suspended.
> t4ka3_detect() reads the sensor name through i2c. If it finds the
> product ID then return 0;
What if t4ka3_detect() returns an error? What happens then?
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v11] media: Add t4ka3 camera sensor driver
2026-03-17 12:24 ` Hans de Goede
2026-03-17 13:26 ` Kate Hsuan
@ 2026-03-17 17:58 ` Sakari Ailus
2026-03-17 19:56 ` Hans de Goede
1 sibling, 1 reply; 12+ messages in thread
From: Sakari Ailus @ 2026-03-17 17:58 UTC (permalink / raw)
To: Hans de Goede
Cc: Kate Hsuan, Mauro Carvalho Chehab, linux-media, linux-kernel,
Hans de Goede
Hi Hans,
On Tue, Mar 17, 2026 at 01:24:57PM +0100, Hans de Goede wrote:
> Hi Sakari,
>
> On 16-Mar-26 23:00, Sakari Ailus wrote:
>
> <snip>
>
> >> diff --git a/drivers/media/i2c/t4ka3.c b/drivers/media/i2c/t4ka3.c
> >> new file mode 100644
> >> index 000000000000..d9af5e51f7a8
> >> --- /dev/null
> >> +++ b/drivers/media/i2c/t4ka3.c
>
> <snip>
>
> >> +static struct v4l2_mbus_framefmt *t4ka3_get_active_format(struct t4ka3_data *sensor)
> >> +{
> >> + struct v4l2_subdev_state *active_state =
> >> + v4l2_subdev_get_locked_active_state(&sensor->sd);
> >> +
> >> + return v4l2_subdev_state_get_format(active_state, 0);
> >> +}
> >> +
> >> +static struct v4l2_rect *t4ka3_get_active_crop(struct t4ka3_data *sensor)
> >> +{
> >> + struct v4l2_subdev_state *active_state =
> >> + v4l2_subdev_get_locked_active_state(&sensor->sd);
> >> +
> >> + return v4l2_subdev_state_get_crop(active_state, 0);
> >
> > Please avoid adding such helpers.
>
> The problem is that we need to know the active-fmt/-crop in some places
> without access to it. E.g. when the vblank ctrl gets set this influences
> the range of the exposure control, so we need active_fmt.height to
> calculate the values to pass to v4l2_ctrl_modify_range() and we need
> this from a v4l2_ctrl_ops.s_ctrl callback which does not get passed
> in the (active) fmt.
>
> Since the ctrl lock is used as the main sensor-driver lock too,
> we can always safely call v4l2_subdev_get_locked_active_state()
> in these cases, since we are always holding the lock.
I replied to Kate's e-mail already, but indeed you should get the active
state if you really need the active state. In a lot of cases it's "any"
state which is already available to the caller of these functions.
>
> The alternative would be to store a copy of the active fmt/crop
> inside struct t4ka3_data, but I thought that the whole direction
> for sensor drivers was to stop having (and needing to update) their
> own shadow copy of the active_state and instead direct use
> the active_state ?
Correct.
>
> <snip>
>
> >> +static int t4ka3_s_ctrl(struct v4l2_ctrl *ctrl)
> >> +{
> >> + struct t4ka3_data *sensor = ctrl_to_t4ka3(ctrl);
> >> + struct v4l2_mbus_framefmt *fmt;
> >> + int ret;
> >> +
> >> + /* Update exposure range on vblank changes */
> >> + if (ctrl->id == V4L2_CID_VBLANK) {
> >> + ret = t4ka3_update_exposure_range(sensor);
> >> + if (ret)
> >> + return ret;
> >> + }
> >> +
> >> + fmt = t4ka3_get_active_format(sensor);
> >
> > You could assign this in declaration.
> >
> >> +
> >> + /* 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, fmt);
> >
> > Does this call belong here?
>
> Yes, if the hflip/vflip controls change then fmt->code needs to be
> updated to the now changed bayer-order. t4ka3_set_bayer_order()
> uses the cached ctrl->val values so it is cheap enough to
> always do this instead of checking if the changed ctrl is
> vflip or hflip.
>
> In case the sensor is actually streaming and we don't hit this path,
> the t4ka3_t_vflip()helper will return -EBUSY since changing
> the active fmt while streaming is not a good idea.
>
> Looking at this again, I do think that: t4ka3_t_vflip() should
> be renamed to t4ka3_update_hvflip() because the current name
> is weird.
This should also apply to non-active state (albeit implementation requires
implementing get_fmt locally). This will get "fixed" with the metadata
series eventually with generic raw formats.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v11] media: Add t4ka3 camera sensor driver
2026-03-17 17:58 ` Sakari Ailus
@ 2026-03-17 19:56 ` Hans de Goede
2026-03-17 22:35 ` Sakari Ailus
0 siblings, 1 reply; 12+ messages in thread
From: Hans de Goede @ 2026-03-17 19:56 UTC (permalink / raw)
To: Sakari Ailus
Cc: Kate Hsuan, Mauro Carvalho Chehab, linux-media, linux-kernel,
Hans de Goede
Hi,
On 17-Mar-26 18:58, Sakari Ailus wrote:
...
>>>> +
>>>> + /* 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, fmt);
>>>
>>> Does this call belong here?
>>
>> Yes, if the hflip/vflip controls change then fmt->code needs to be
>> updated to the now changed bayer-order. t4ka3_set_bayer_order()
>> uses the cached ctrl->val values so it is cheap enough to
>> always do this instead of checking if the changed ctrl is
>> vflip or hflip.
>>
>> In case the sensor is actually streaming and we don't hit this path,
>> the t4ka3_t_vflip()helper will return -EBUSY since changing
>> the active fmt while streaming is not a good idea.
>
> This should also apply to non-active state (albeit implementation requires
> implementing get_fmt locally). This will get "fixed" with the metadata
> series eventually with generic raw formats.
Ok, so what you are saying is that given the special need to
set fmt->code based on the flip ctrl values, this driver should
not use v4l2_subdev_get_fmt directly, but instead it should
provide its own subdev_get_fmt() which wraps v4l2_subdev_get_fmt()
overriding fmt->code based on the flip settings before returning it ?
Do I have that right ?
Regards,
Hans
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v11] media: Add t4ka3 camera sensor driver
2026-03-17 17:53 ` Sakari Ailus
@ 2026-03-17 20:17 ` Hans de Goede
2026-03-18 7:28 ` Kate Hsuan
1 sibling, 0 replies; 12+ messages in thread
From: Hans de Goede @ 2026-03-17 20:17 UTC (permalink / raw)
To: Sakari Ailus, Kate Hsuan
Cc: Mauro Carvalho Chehab, linux-media, linux-kernel, Hans de Goede
Hi Sakari,
On 17-Mar-26 18:53, Sakari Ailus wrote:
...
>>>> +static struct v4l2_mbus_framefmt *t4ka3_get_active_format(struct t4ka3_data *sensor)
>>>> +{
>>>> + struct v4l2_subdev_state *active_state =
>>>> + v4l2_subdev_get_locked_active_state(&sensor->sd);
>>>> +
>>>> + return v4l2_subdev_state_get_format(active_state, 0);
>>>> +}
>>>> +
>>>> +static struct v4l2_rect *t4ka3_get_active_crop(struct t4ka3_data *sensor)
>>>> +{
>>>> + struct v4l2_subdev_state *active_state =
>>>> + v4l2_subdev_get_locked_active_state(&sensor->sd);
>>>> +
>>>> + return v4l2_subdev_state_get_crop(active_state, 0);
>>>
>>> Please avoid adding such helpers.
>> As Hans mentioned, we can put active format and crop in the t4ka3_data
>> or keep the helpers.
>
> What prevents you doing:
>
> struct v4l2_subdev_state *active_state =
> v4l2_subdev_get_locked_active_state(&sensor->sd);
> struct v4l2_rect *r = v4l2_subdev_state_get_crop(active_state, 0);
>
> in the code? In a lot of the cases you could simply pass the state to the
> function using it as the caller already has it.
Ok, I've done a quick audit of the code and it indeed seems that
in some cases, especially in t4ka3_set_pad_format() these helpers
are used unnecessary (and even buggy in case of the crop in
t4ka3_set_pad_format()).
So I agree it is probably better to avoid these and in functions
where we have a sd_state pass the result of:
v4l2_subdev_state_get_format(sd_state, [0|sel->pad])
and:
v4l2_subdev_state_get_crop(sd_state, [0|sel->pad])
to helpers which currently rely on t4ka3_get_active_[format|crop]()
such as t4ka3_calc_mode().
E.g. all callers of t4ka3_calc_mode() already get sd_state passed
in; and both callers also already both should get crop and fmt
from the sd_state (t4ka3_set_pad_format() wrongly uses the
functions to get the active fmt/crop for this).
So we can simply pass the already retrieved fmt + crop
into t4ka3_calc_mode().
As for other calles of t4ka3_get_active_[format|crop]() if
they really do not have sd_state access then lets just
write out the code as suggested by Sakari.
Kate, let me know if you need any help with this.
Regards,
Hans
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v11] media: Add t4ka3 camera sensor driver
2026-03-17 19:56 ` Hans de Goede
@ 2026-03-17 22:35 ` Sakari Ailus
0 siblings, 0 replies; 12+ messages in thread
From: Sakari Ailus @ 2026-03-17 22:35 UTC (permalink / raw)
To: Hans de Goede
Cc: Kate Hsuan, Mauro Carvalho Chehab, linux-media, linux-kernel,
Hans de Goede
Hi Hans,
On Tue, Mar 17, 2026 at 08:56:47PM +0100, Hans de Goede wrote:
> Hi,
>
> On 17-Mar-26 18:58, Sakari Ailus wrote:
>
> ...
>
> >>>> +
> >>>> + /* 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, fmt);
> >>>
> >>> Does this call belong here?
> >>
> >> Yes, if the hflip/vflip controls change then fmt->code needs to be
> >> updated to the now changed bayer-order. t4ka3_set_bayer_order()
> >> uses the cached ctrl->val values so it is cheap enough to
> >> always do this instead of checking if the changed ctrl is
> >> vflip or hflip.
> >>
> >> In case the sensor is actually streaming and we don't hit this path,
> >> the t4ka3_t_vflip()helper will return -EBUSY since changing
> >> the active fmt while streaming is not a good idea.
> >
> > This should also apply to non-active state (albeit implementation requires
> > implementing get_fmt locally). This will get "fixed" with the metadata
> > series eventually with generic raw formats.
>
> Ok, so what you are saying is that given the special need to
> set fmt->code based on the flip ctrl values, this driver should
> not use v4l2_subdev_get_fmt directly, but instead it should
> provide its own subdev_get_fmt() which wraps v4l2_subdev_get_fmt()
> overriding fmt->code based on the flip settings before returning it ?
>
> Do I have that right ?
I'd like to say "no", but yes, that's right. :-)
--
Sakari Ailus
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v11] media: Add t4ka3 camera sensor driver
2026-03-17 17:53 ` Sakari Ailus
2026-03-17 20:17 ` Hans de Goede
@ 2026-03-18 7:28 ` Kate Hsuan
1 sibling, 0 replies; 12+ messages in thread
From: Kate Hsuan @ 2026-03-18 7:28 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mauro Carvalho Chehab, Hans de Goede, linux-media, linux-kernel,
Hans de Goede
Hi Sakari and Hans,
On Wed, Mar 18, 2026 at 1:54 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Kate,
>
> On Tue, Mar 17, 2026 at 09:24:20PM +0800, Kate Hsuan wrote:
> > Hi Sakari,
> >
> > Thank you for reviewing it.
> >
> > And thank you, Hans
> >
> > On Tue, Mar 17, 2026 at 6:00 AM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Kate,
> > >
> > > Thanks for the patch.
> > >
> > > Where have you seen this sensor being used, if I may ask?
> >
> > It can be found on the Xiaomi Pad2 that is based on an Intel Cherry
> > Trail platform.
>
> Ack, thanks for the info!
>
> > > > +static struct v4l2_mbus_framefmt *t4ka3_get_active_format(struct t4ka3_data *sensor)
> > > > +{
> > > > + struct v4l2_subdev_state *active_state =
> > > > + v4l2_subdev_get_locked_active_state(&sensor->sd);
> > > > +
> > > > + return v4l2_subdev_state_get_format(active_state, 0);
> > > > +}
> > > > +
> > > > +static struct v4l2_rect *t4ka3_get_active_crop(struct t4ka3_data *sensor)
> > > > +{
> > > > + struct v4l2_subdev_state *active_state =
> > > > + v4l2_subdev_get_locked_active_state(&sensor->sd);
> > > > +
> > > > + return v4l2_subdev_state_get_crop(active_state, 0);
> > >
> > > Please avoid adding such helpers.
> > As Hans mentioned, we can put active format and crop in the t4ka3_data
> > or keep the helpers.
>
> What prevents you doing:
>
> struct v4l2_subdev_state *active_state =
> v4l2_subdev_get_locked_active_state(&sensor->sd);
> struct v4l2_rect *r = v4l2_subdev_state_get_crop(active_state, 0);
>
> in the code? In a lot of the cases you could simply pass the state to the
> function using it as the caller already has it.
>
> It also seems the driver operates in active state whereas any state should
> be valid. In general, file handle specific try state should resemble the
> active state as much as possible with the exception that it won't be
> applied to hardware.
>
> In the long run I'd also expect the control values to move to state.
>
If the caller has the state, the state will be used and passed to the
function without calling the code mentioned above, such as
(v4l2_subdev_get_locked_active_state()...).
v4l2_subdev_state_get_format() and v4l2_subdev_state_get_crop will be
used in this case.
If the caller doesn't have it, the code mentioned above will be called
to get fmt and crop.
> ...
>
> > > > +static int t4ka3_s_ctrl(struct v4l2_ctrl *ctrl)
> > > > +{
> > > > + struct t4ka3_data *sensor = ctrl_to_t4ka3(ctrl);
> > > > + struct v4l2_mbus_framefmt *fmt;
> > > > + int ret;
> > > > +
> > > > + /* Update exposure range on vblank changes */
> > > > + if (ctrl->id == V4L2_CID_VBLANK) {
> > > > + ret = t4ka3_update_exposure_range(sensor);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > > > +
> > > > + fmt = t4ka3_get_active_format(sensor);
> > >
> > > You could assign this in declaration.
> > Okay
> > >
> > > > +
> > > > + /* 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, fmt);
> > >
> > > Does this call belong here?
> > I think it can be. It is a simple update of t4ka3_hv_flip_bayer_order.
>
> Why are you doing it here? It basically assigns the Bayer order to the
> active format based on the flipping controls. Why not to do that when
> changing the flipping controls?
What I understand is
Move the t4ka3_set_bayer_order() into t4ka3_t_vflip() and change the
value based on the change of vflip and hflip value.
in t4ka3_t_vflip(), t4ka3_set_bayer_order is called in the function so
setting up bayer order here can be dropped.
A helper is needed and will invoke v4l2_subdev_get_fmt() to find the format.
>
> > >
> > > > + 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, T4KA3_VFLIP_BIT);
> > > > + break;
> > > > + case V4L2_CID_HFLIP:
> > > > + ret = t4ka3_t_vflip(&sensor->sd, ctrl->val, T4KA3_HFLIP_BIT);
> > > > + break;
> > > > + case V4L2_CID_VBLANK:
> > > > + ret = cci_write(sensor->regmap, T4KA3_REG_FRAME_LENGTH_LINES,
> > > > + fmt->height + ctrl->val, NULL);
> > > > + 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);
> > >
> > > Newline here?
> > Okay
> > >
> > > > + return ret;
> > > > +}
>
> ...
>
> > > > +static int t4ka3_disable_stream(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
> > > > + u32 pad, u64 streams_mask)
> > > > +{
> > > > + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> > > > + int ret;
> > > > +
> > > > + ret = cci_write(sensor->regmap, T4KA3_REG_STREAM, 0, NULL);
> > > > + pm_runtime_put(sensor->sd.dev);
> > > > + sensor->streaming = 0;
> > > > + return ret;
> > >
> > > Return 0 here but complain about it.
> > Do you mean return 0 here and print a message when ret != 0?
>
> Yes, please.
>
> ...
>
> > > > +static int t4ka3_check_hwcfg(struct t4ka3_data *sensor)
> > > > +{
> > > > + struct fwnode_handle *fwnode = dev_fwnode(sensor->dev);
> > > > + struct v4l2_fwnode_endpoint bus_cfg = {
> > > > + .bus_type = V4L2_MBUS_CSI2_DPHY,
> > > > + };
> > > > + struct fwnode_handle *endpoint;
> > > > + unsigned long link_freq_bitmap;
> > > > + int ret;
> > > > +
> > > > + /*
> > > > + * Sometimes the fwnode graph is initialized by the bridge driver.
> > > > + * Bridge drivers doing this may also add GPIO mappings, wait for this.
> > > > + */
> > >
> > > No need for such a comment.
> > I'll drop it.
> >
> > >
> > > > + endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > > > + if (!endpoint)
> > > > + return dev_err_probe(sensor->dev, -EPROBE_DEFER,
> > > > + "waiting for fwnode graph endpoint\n");
> > >
> > > This
> > > <URL:https://git.linuxtv.org/sailus/media_tree.git/commit/?h=cleanup&id=8181d18d45d593d8499cbf0e83de08c6d913516c>
> > > will be merged soon.
> > Does it mean "return -EPROBE_DEFER;" is enough?
>
> You can omit checking for errors here.
OKay
>
> > >
> > > > +
> > > > + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
> > > > + fwnode_handle_put(endpoint);
> > > > + if (ret)
> > > > + return ret;
>
> ...
>
> > > > +static int t4ka3_pm_resume(struct device *dev)
> > > > +{
> > > > + struct t4ka3_data *sensor = dev_get_drvdata(dev);
> > > > + u16 sensor_id;
> > > > + int ret;
> > > > +
> > > > + usleep_range(5000, 6000);
> > > > +
> > > > + gpiod_set_value_cansleep(sensor->powerdown_gpio, 0);
> > > > + gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> > > > +
> > > > + /* waiting for the sensor after powering up */
> > > > + msleep(20);
> > >
> > > fsleep() maybe?
> > I can change it.
> > >
> > > > +
> > > > + ret = t4ka3_detect(sensor, &sensor_id);
> > > > + if (ret) {
> > > > + dev_err(sensor->dev, "sensor detect failed\n");
> > > > + return ret;
> > >
> > > What about gpio values in this case?
> > both powerdown_gpio and reset_gpio are 0 when resuming and 1 when suspended.
> > t4ka3_detect() reads the sensor name through i2c. If it finds the
> > product ID then return 0;
>
> What if t4ka3_detect() returns an error? What happens then?
That means the sensor replies with an unexpected value and returns
-ENODEV to notify that "no such device" and then power it off.
I need I have to power it off when t4ka3_detect() returns an error. I'll fix it.
Or I can move it into probe().
>
> --
> Kind regards,
>
> Sakari Ailus
>
--
BR,
Kate
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2026-03-18 7:29 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-16 8:57 [PATCH v11] media: Add t4ka3 camera sensor driver Kate Hsuan
2026-03-16 22:00 ` Sakari Ailus
2026-03-17 12:04 ` Hans de Goede
2026-03-17 12:24 ` Hans de Goede
2026-03-17 13:26 ` Kate Hsuan
2026-03-17 17:58 ` Sakari Ailus
2026-03-17 19:56 ` Hans de Goede
2026-03-17 22:35 ` Sakari Ailus
2026-03-17 13:24 ` Kate Hsuan
2026-03-17 17:53 ` Sakari Ailus
2026-03-17 20:17 ` Hans de Goede
2026-03-18 7:28 ` Kate Hsuan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox