* Re: [PATCH v3 2/2] media: i2c: Add Samsung S5K3T2 image sensor driver
2026-09-24 5:46 ` [PATCH v3 2/2] media: i2c: Add Samsung S5K3T2 image sensor driver Armandas Kvietkus via B4 Relay
@ 2026-09-24 5:58 ` sashiko-bot
2026-10-01 7:19 ` Sakari Ailus
2026-10-01 18:41 ` Elgin Perumbilly
2 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-09-24 5:58 UTC (permalink / raw)
To: Armandas Kvietkus; +Cc: robh, devicetree, media-ci, conor+dt
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: i2c: s5k3t2: PM usage counter underflow due to incorrect pm_runtime_get_if_active() check
- [High] media: i2c: s5k3t2: Array filter ignores index leading to NULL pointer dereference
--
commit 3fc6dedbde02c5967afbd9449d7a6ccc791af98e
Author: Armandas Kvietkus <armandas.kvietkus@proton.me>
Subject: media: i2c: Add Samsung S5K3T2 image sensor driver
This commit adds a driver for the Samsung S5K3T2 20 Mpixel CMOS image sensor.
It supports three modes and manages power using runtime PM with autosuspend.
> diff --git a/drivers/media/i2c/s5k3t2.c b/drivers/media/i2c/s5k3t2.c
> new file mode 100644
> index 0000000000000..5958312177753
> --- /dev/null
> +++ b/drivers/media/i2c/s5k3t2.c
[ ... ]
> @@ -0,0 +1,1360 @@
[ ... ]
> +static int s5k3t2_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> + if (!pm_runtime_get_if_active(s5k3t2->dev))
> + return 0;
[Severity: High]
Does this code handle negative error codes correctly?
If pm_runtime_get_if_active() returns a negative error code (such as
-EINVAL when runtime PM is disabled, which can happen during driver unbind
or system suspend phases), !(-EINVAL) evaluates to 0.
This bypasses the early return and allows execution to proceed to access
the hardware when the device is not active.
> +
> + switch (ctrl->id) {
> + case V4L2_CID_ANALOGUE_GAIN:
[ ... ]
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + pm_runtime_put(s5k3t2->dev);
[Severity: High]
Can this cause a PM usage counter underflow?
If pm_runtime_get_if_active() returned a negative error code earlier,
the usage counter was not incremented. Calling pm_runtime_put() here
would unconditionally decrement the counter, leading to an underflow.
> +
> + return ret;
> +}
[ ... ]
> +static int s5k3t2_init_controls(struct s5k3t2 *s5k3t2)
> +{
> + struct v4l2_ctrl_handler *ctrl_hdlr = &s5k3t2->ctrl_handler;
> + const struct s5k3t2_mode *mode = s5k3t2->mode;
> + s64 hblank, vblank, exposure_max;
> + struct v4l2_fwnode_device_properties props;
> + int ret;
> +
> + v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> +
> + s5k3t2->link_freq =
> + v4l2_ctrl_new_int_menu(ctrl_hdlr, &s5k3t2_ctrl_ops,
> + V4L2_CID_LINK_FREQ,
> + ARRAY_SIZE(s5k3t2_link_freq_menu) - 1,
> + mode->link_freq_index,
> + s5k3t2_link_freq_menu);
> + if (s5k3t2->link_freq)
> + s5k3t2->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + s5k3t2->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
> + V4L2_CID_PIXEL_RATE,
> + S5K3T2_PIXEL_RATE,
> + S5K3T2_PIXEL_RATE, 1,
> + S5K3T2_PIXEL_RATE);
> +
> + hblank = mode->hts - mode->width;
[Severity: High]
Can mode be NULL here?
If v4l2_find_nearest_size_conditional() returns NULL during probe,
mode is NULL and this assignment will trigger a NULL pointer dereference.
This occurs because s5k3t2_filter_by_link_freq() always evaluates the
first element of the modes array, leading to a failure to match any mode
if the board doesn't use the link frequency of the first element.
> + s5k3t2->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
> + V4L2_CID_HBLANK, hblank,
> + hblank, 1, hblank);
[ ... ]
> +static bool s5k3t2_filter_by_link_freq(const void *array, size_t index,
> + const void *context)
> +{
> + const struct s5k3t2_mode *mode = array;
> + const struct s5k3t2 *s5k3t2 = context;
> +
> + return s5k3t2->link_freq_bitmap & BIT(mode->link_freq_index);
[Severity: High]
Does this filter ignore the index parameter?
By assigning array to mode directly, it only ever checks the first element
of the array instead of indexing into it.
If the first element's link frequency is missing from the device tree
configuration, this function will always return false for every index,
causing v4l2_find_nearest_size_conditional() to return NULL.
> +}
[ ... ]
> +static int s5k3t2_probe(struct i2c_client *client)
> +{
[ ... ]
> + s5k3t2->mode =
> + v4l2_find_nearest_size_conditional(s5k3t2_supported_modes,
> + ARRAY_SIZE(s5k3t2_supported_modes),
> + width, height,
> + S5K3T2_DEFAULT_WIDTH,
> + S5K3T2_DEFAULT_HEIGHT,
> + s5k3t2_filter_by_link_freq,
> + s5k3t2);
> + ret = s5k3t2_init_controls(s5k3t2);
[Severity: High]
If v4l2_find_nearest_size_conditional() returns NULL, s5k3t2->mode will be
set to NULL, and s5k3t2_init_controls() will dereference it.
Is it possible to add a check for NULL here?
> + if (ret) {
> + dev_err_probe(s5k3t2->dev, ret, "failed to init controls\n");
> + goto power_off;
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-upstream-s5k3t2-v3-0-a5c58dfcec29@proton.me?part=2
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3 2/2] media: i2c: Add Samsung S5K3T2 image sensor driver
2026-09-24 5:46 ` [PATCH v3 2/2] media: i2c: Add Samsung S5K3T2 image sensor driver Armandas Kvietkus via B4 Relay
2026-09-24 5:58 ` sashiko-bot
@ 2026-10-01 7:19 ` Sakari Ailus
2026-10-01 18:36 ` armandas.kvietkus
2026-10-01 18:41 ` Elgin Perumbilly
2 siblings, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2026-10-01 7:19 UTC (permalink / raw)
To: armandas.kvietkus
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-media, devicetree, linux-kernel
Hi Armandas,
Thank you for the patch. Please see my comments below.
On Thu, Sep 24, 2026 at 07:46:52AM +0200, Armandas Kvietkus via B4 Relay wrote:
> From: Armandas Kvietkus <armandas.kvietkus@proton.me>
>
> Add a driver for the Samsung S5K3T2 20 Mpixel CMOS image sensor.
>
> The driver supports three modes. All of them are RAW10 on four D-PHY
> lanes.
>
> 5184x3880 at 15 fps, link frequency 448 MHz
> 2592x1940 at 60 fps, link frequency 448 MHz
> 1280x720 at 120 fps, link frequency 300 MHz
>
> A mode is hidden if the board does not list the link frequency it needs.
Please drop, nothing specific to the sensor or the driver.
Is Documentation/process/coding-assistants.rst relevant for this
contribution?
>
> The pixel clock does not change between modes, so the pixel rate control
> is constant. The frame rate follows from the line length and the frame
> length.
>
> The controls are exposure, analogue gain, vertical blanking, horizontal
> flip, vertical flip and test pattern. Rotation and orientation come from
> the device tree. Power is handled by runtime PM with autosuspend.
>
> The register tables come from the vendor camera firmware. They were
> checked on hardware.
>
> Tested on a Xiaomi POCO F3 with the Qualcomm CAMSS driver.
>
> Signed-off-by: Armandas Kvietkus <armandas.kvietkus@proton.me>
> ---
> MAINTAINERS | 1 +
> drivers/media/i2c/Kconfig | 10 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/s5k3t2.c | 1360 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 1372 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1924dbd20..e625cf3a2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -24386,6 +24386,7 @@ L: linux-media@vger.kernel.org
> S: Maintained
> T: git git://linuxtv.org/media_tree.git
> F: Documentation/devicetree/bindings/media/i2c/samsung,s5k3t2.yaml
> +F: drivers/media/i2c/s5k3t2.c
>
> SAMSUNG S5K5BAF CAMERA DRIVER
> M: Sylwester Nawrocki <s.nawrocki@samsung.com>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 5c52007f9..999aeecd2 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -804,6 +804,16 @@ config VIDEO_S5K3M5
> To compile this driver as a module, choose M here: the
> module will be called s5k3m5.
>
> +config VIDEO_S5K3T2
> + tristate "Samsung S5K3T2 sensor support"
> + select V4L2_CCI_I2C
> + help
> + This is a V4L2 sensor driver for Samsung S5K3T2 20MP raw
> + camera sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called s5k3t2.
> +
> config VIDEO_S5K5BAF
> tristate "Samsung S5K5BAF sensor support"
> help
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index d04bd5724..9a25d8200 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -130,6 +130,7 @@ obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o
> obj-$(CONFIG_VIDEO_RJ54N1) += rj54n1cb0c.o
> obj-$(CONFIG_VIDEO_S5C73M3) += s5c73m3/
> obj-$(CONFIG_VIDEO_S5K3M5) += s5k3m5.o
> +obj-$(CONFIG_VIDEO_S5K3T2) += s5k3t2.o
> obj-$(CONFIG_VIDEO_S5K5BAF) += s5k5baf.o
> obj-$(CONFIG_VIDEO_S5K6A3) += s5k6a3.o
> obj-$(CONFIG_VIDEO_S5KJN1) += s5kjn1.o
> diff --git a/drivers/media/i2c/s5k3t2.c b/drivers/media/i2c/s5k3t2.c
> new file mode 100644
> index 000000000..595831217
> --- /dev/null
> +++ b/drivers/media/i2c/s5k3t2.c
> @@ -0,0 +1,1360 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2026 Armandas Kvietkus <armandas.kvietkus@proton.me>
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/units.h>
> +#include <media/v4l2-cci.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +
> +#define S5K3T2_LINK_FREQ_448MHZ (448 * HZ_PER_MHZ)
> +#define S5K3T2_LINK_FREQ_300MHZ (300 * HZ_PER_MHZ)
> +#define S5K3T2_MCLK_FREQ 19200000
> +#define S5K3T2_PIXEL_RATE 676571429
> +#define S5K3T2_DATA_LANES 4
> +
> +#define S5K3T2_REG_CHIP_ID CCI_REG16(0x0000)
> +#define S5K3T2_CHIP_ID 0x3142
> +
> +#define S5K3T2_REG_CTRL_MODE CCI_REG16(0x0100)
> +#define S5K3T2_MODE_STREAMING BIT(8)
> +#define S5K3T2_VFLIP BIT(1)
> +#define S5K3T2_HFLIP BIT(0)
> +
> +#define S5K3T2_REG_EXPOSURE CCI_REG16(0x0202)
> +#define S5K3T2_EXPOSURE_MIN 8
> +#define S5K3T2_EXPOSURE_STEP 1
> +#define S5K3T2_EXPOSURE_MARGIN 4
> +
> +#define S5K3T2_REG_AGAIN CCI_REG16(0x0204)
> +#define S5K3T2_AGAIN_MIN 1
> +#define S5K3T2_AGAIN_MAX 16
> +#define S5K3T2_AGAIN_STEP 1
> +#define S5K3T2_AGAIN_DEFAULT 1
> +#define S5K3T2_AGAIN_SHIFT 5
> +
> +#define S5K3T2_REG_VTS CCI_REG16(0x0340)
> +#define S5K3T2_VTS_MAX 0xffff
> +
> +#define S5K3T2_REG_TEST_PATTERN CCI_REG16(0x0600)
> +
> +#define S5K3T2_NATIVE_WIDTH 5192
> +#define S5K3T2_NATIVE_HEIGHT 3888
> +
> +#define S5K3T2_DEFAULT_WIDTH 2592
> +#define S5K3T2_DEFAULT_HEIGHT 1940
> +
> +#define to_s5k3t2(_sd) container_of(_sd, struct s5k3t2, sd)
> +
> +static const s64 s5k3t2_link_freq_menu[] = {
> + S5K3T2_LINK_FREQ_448MHZ,
> + S5K3T2_LINK_FREQ_300MHZ,
> +};
> +
> +static const u32 s5k3t2_mbus_formats[] = {
> + MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10,
> + MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10,
> +};
> +
> +struct s5k3t2_reg_list {
> + const struct cci_reg_sequence *regs;
> + unsigned int num_regs;
> +};
> +
> +struct s5k3t2_mode {
> + u32 width;
> + u32 height;
> + u32 hts;
> + u32 vts;
> + u32 exposure;
> + unsigned int link_freq_index;
> + struct v4l2_rect crop;
> + const struct s5k3t2_reg_list reg_list;
> +};
> +
> +static const char * const s5k3t2_test_pattern_menu[] = {
> + "Disabled",
> + "Solid colour",
> + "Colour bars",
> + "Fade to grey colour bars",
> + "PN9",
> +};
> +
> +static const char * const s5k3t2_supply_names[] = {
> + "vdda",
> + "vddd",
> + "vddio",
> +};
> +
> +#define S5K3T2_NUM_SUPPLIES ARRAY_SIZE(s5k3t2_supply_names)
Please use plain ARRAY_SIZE() where you need it instead of a macro.
> +
> +struct s5k3t2 {
> + struct device *dev;
> + struct regmap *regmap;
> + struct clk *mclk;
> + struct gpio_desc *reset_gpio;
> + struct regulator_bulk_data supplies[S5K3T2_NUM_SUPPLIES];
> +
> + struct v4l2_subdev sd;
> + struct media_pad pad;
> +
> + struct v4l2_ctrl_handler ctrl_handler;
> + struct v4l2_ctrl *link_freq;
> + struct v4l2_ctrl *pixel_rate;
> + struct v4l2_ctrl *hblank;
> + struct v4l2_ctrl *vblank;
> + struct v4l2_ctrl *exposure;
> + struct v4l2_ctrl *vflip;
> + struct v4l2_ctrl *hflip;
> +
> + const struct s5k3t2_mode *mode;
> + unsigned long link_freq_bitmap;
> +};
> +
> +static const struct cci_reg_sequence s5k3t2_init_setting[] = {
> + { CCI_REG16(0x6214), 0xff7d },
> + { CCI_REG16(0x6218), 0x0000 },
> + { CCI_REG16(0x0a02), 0x003f },
> + { CCI_REG16(0x6028), 0x2000 },
> + { CCI_REG16(0x602a), 0x3aec },
> + { CCI_REG16(0x6f12), 0x0000 },
> + { CCI_REG16(0x6f12), 0x0000 },
> + { CCI_REG16(0x6f12), 0x0549 },
> + { CCI_REG16(0x6f12), 0x0448 },
> + { CCI_REG16(0x6f12), 0x054a },
> + { CCI_REG16(0x6f12), 0xc1f8 },
> + { CCI_REG16(0x6f12), 0x2c05 },
> + { CCI_REG16(0x6f12), 0x101a },
> + { CCI_REG16(0x6f12), 0xa1f8 },
> + { CCI_REG16(0x6f12), 0x3005 },
> + { CCI_REG16(0x6f12), 0x00f0 },
> + { CCI_REG16(0x6f12), 0x7bb8 },
> + { CCI_REG16(0x6f12), 0x2000 },
> + { CCI_REG16(0x6f12), 0x3ca0 },
> + { CCI_REG16(0x6f12), 0x2000 },
> + { CCI_REG16(0x6f12), 0x2670 },
> + { CCI_REG16(0x6f12), 0x2000 },
> + { CCI_REG16(0x6f12), 0x9c00 },
> + { CCI_REG16(0x6f12), 0x70b5 },
> + { CCI_REG16(0x6f12), 0x0646 },
> + { CCI_REG16(0x6f12), 0x4348 },
> + { CCI_REG16(0x6f12), 0x0022 },
> + { CCI_REG16(0x6f12), 0x0168 },
> + { CCI_REG16(0x6f12), 0x0c0c },
> + { CCI_REG16(0x6f12), 0x8db2 },
> + { CCI_REG16(0x6f12), 0x2946 },
> + { CCI_REG16(0x6f12), 0x2046 },
> + { CCI_REG16(0x6f12), 0x00f0 },
> + { CCI_REG16(0x6f12), 0x9bf8 },
> + { CCI_REG16(0x6f12), 0x3046 },
> + { CCI_REG16(0x6f12), 0x00f0 },
> + { CCI_REG16(0x6f12), 0x9df8 },
> + { CCI_REG16(0x6f12), 0x3d48 },
> + { CCI_REG16(0x6f12), 0x3e4a },
> + { CCI_REG16(0x6f12), 0x0830 },
> + { CCI_REG16(0x6f12), 0x0188 },
> + { CCI_REG16(0x6f12), 0x1180 },
> + { CCI_REG16(0x6f12), 0x911c },
> + { CCI_REG16(0x6f12), 0x4088 },
> + { CCI_REG16(0x6f12), 0x0880 },
> + { CCI_REG16(0x6f12), 0x2946 },
> + { CCI_REG16(0x6f12), 0x2046 },
> + { CCI_REG16(0x6f12), 0xbde8 },
> + { CCI_REG16(0x6f12), 0x7040 },
> + { CCI_REG16(0x6f12), 0x0122 },
> + { CCI_REG16(0x6f12), 0x00f0 },
> + { CCI_REG16(0x6f12), 0x89b8 },
> + { CCI_REG16(0x6f12), 0x70b5 },
> + { CCI_REG16(0x6f12), 0x0646 },
> + { CCI_REG16(0x6f12), 0x3548 },
> + { CCI_REG16(0x6f12), 0x0022 },
> + { CCI_REG16(0x6f12), 0x4068 },
> + { CCI_REG16(0x6f12), 0x84b2 },
> + { CCI_REG16(0x6f12), 0x050c },
> + { CCI_REG16(0x6f12), 0x2146 },
> + { CCI_REG16(0x6f12), 0x2846 },
> + { CCI_REG16(0x6f12), 0x00f0 },
> + { CCI_REG16(0x6f12), 0x7ef8 },
> + { CCI_REG16(0x6f12), 0x3046 },
> + { CCI_REG16(0x6f12), 0x00f0 },
> + { CCI_REG16(0x6f12), 0x85f8 },
> + { CCI_REG16(0x6f12), 0x3149 },
> + { CCI_REG16(0x6f12), 0x314b },
> + { CCI_REG16(0x6f12), 0x0120 },
> + { CCI_REG16(0x6f12), 0x0988 },
> + { CCI_REG16(0x6f12), 0x40ea },
> + { CCI_REG16(0x6f12), 0x0110 },
> + { CCI_REG16(0x6f12), 0x5881 },
> + { CCI_REG16(0x6f12), 0x2f48 },
> + { CCI_REG16(0x6f12), 0x0078 },
> + { CCI_REG16(0x6f12), 0x68b1 },
> + { CCI_REG16(0x6f12), 0x0022 },
> + { CCI_REG16(0x6f12), 0x2e48 },
> + { CCI_REG16(0x6f12), 0x2f49 },
> + { CCI_REG16(0x6f12), 0x80f8 },
> + { CCI_REG16(0x6f12), 0xc428 },
> + { CCI_REG16(0x6f12), 0x0a80 },
> + { CCI_REG16(0x6f12), 0x2e49 },
> + { CCI_REG16(0x6f12), 0x0e78 },
> + { CCI_REG16(0x6f12), 0x36b1 },
> + { CCI_REG16(0x6f12), 0x90f8 },
> + { CCI_REG16(0x6f12), 0xe803 },
> + { CCI_REG16(0x6f12), 0x18b1 },
> + { CCI_REG16(0x6f12), 0x0120 },
> + { CCI_REG16(0x6f12), 0x02e0 },
> + { CCI_REG16(0x6f12), 0x0122 },
> + { CCI_REG16(0x6f12), 0xf0e7 },
> + { CCI_REG16(0x6f12), 0x0020 },
> + { CCI_REG16(0x6f12), 0x4978 },
> + { CCI_REG16(0x6f12), 0x01b1 },
> + { CCI_REG16(0x6f12), 0x42b1 },
> + { CCI_REG16(0x6f12), 0x0021 },
> + { CCI_REG16(0x6f12), 0x40ea },
> + { CCI_REG16(0x6f12), 0x0110 },
> + { CCI_REG16(0x6f12), 0x5880 },
> + { CCI_REG16(0x6f12), 0x00f0 },
> + { CCI_REG16(0x6f12), 0x66f8 },
> + { CCI_REG16(0x6f12), 0x0128 },
> + { CCI_REG16(0x6f12), 0x02d0 },
> + { CCI_REG16(0x6f12), 0x1be0 },
> + { CCI_REG16(0x6f12), 0x0121 },
> + { CCI_REG16(0x6f12), 0xf5e7 },
> + { CCI_REG16(0x6f12), 0x2248 },
> + { CCI_REG16(0x6f12), 0x234a },
> + { CCI_REG16(0x6f12), 0x234e },
> + { CCI_REG16(0x6f12), 0xb0f8 },
> + { CCI_REG16(0x6f12), 0x7211 },
> + { CCI_REG16(0x6f12), 0x92f8 },
> + { CCI_REG16(0x6f12), 0x9420 },
> + { CCI_REG16(0x6f12), 0x90f8 },
> + { CCI_REG16(0x6f12), 0x7401 },
> + { CCI_REG16(0x6f12), 0xd140 },
> + { CCI_REG16(0x6f12), 0xd040 },
> + { CCI_REG16(0x6f12), 0x4318 },
> + { CCI_REG16(0x6f12), 0x96f8 },
> + { CCI_REG16(0x6f12), 0x7f63 },
> + { CCI_REG16(0x6f12), 0x581e },
> + { CCI_REG16(0x6f12), 0x022e },
> + { CCI_REG16(0x6f12), 0x03d9 },
> + { CCI_REG16(0x6f12), 0x0220 },
> + { CCI_REG16(0x6f12), 0x9040 },
> + { CCI_REG16(0x6f12), 0x181a },
> + { CCI_REG16(0x6f12), 0x401c },
> + { CCI_REG16(0x6f12), 0x164a },
> + { CCI_REG16(0x6f12), 0x703a },
> + { CCI_REG16(0x6f12), 0x1180 },
> + { CCI_REG16(0x6f12), 0x911c },
> + { CCI_REG16(0x6f12), 0x0880 },
> + { CCI_REG16(0x6f12), 0x2146 },
> + { CCI_REG16(0x6f12), 0x2846 },
> + { CCI_REG16(0x6f12), 0xbde8 },
> + { CCI_REG16(0x6f12), 0x7040 },
> + { CCI_REG16(0x6f12), 0x0122 },
> + { CCI_REG16(0x6f12), 0x00f0 },
> + { CCI_REG16(0x6f12), 0x31b8 },
> + { CCI_REG16(0x6f12), 0x10b5 },
> + { CCI_REG16(0x6f12), 0x0022 },
> + { CCI_REG16(0x6f12), 0xaff2 },
> + { CCI_REG16(0x6f12), 0xb501 },
> + { CCI_REG16(0x6f12), 0x1348 },
> + { CCI_REG16(0x6f12), 0x00f0 },
> + { CCI_REG16(0x6f12), 0x3ef8 },
> + { CCI_REG16(0x6f12), 0x064c },
> + { CCI_REG16(0x6f12), 0x0022 },
> + { CCI_REG16(0x6f12), 0xaff2 },
> + { CCI_REG16(0x6f12), 0xff01 },
> + { CCI_REG16(0x6f12), 0x6060 },
> + { CCI_REG16(0x6f12), 0x1048 },
> + { CCI_REG16(0x6f12), 0x00f0 },
> + { CCI_REG16(0x6f12), 0x36f8 },
> + { CCI_REG16(0x6f12), 0x0f49 },
> + { CCI_REG16(0x6f12), 0x2060 },
> + { CCI_REG16(0x6f12), 0x7a20 },
> + { CCI_REG16(0x6f12), 0x0968 },
> + { CCI_REG16(0x6f12), 0x4883 },
> + { CCI_REG16(0x6f12), 0x10bd },
> + { CCI_REG16(0x6f12), 0x2000 },
> + { CCI_REG16(0x6f12), 0x3c90 },
> + { CCI_REG16(0x6f12), 0x4000 },
> + { CCI_REG16(0x6f12), 0x950c },
> + { CCI_REG16(0x6f12), 0x2000 },
> + { CCI_REG16(0x6f12), 0x16f0 },
> + { CCI_REG16(0x6f12), 0x4000 },
> + { CCI_REG16(0x6f12), 0xd000 },
> + { CCI_REG16(0x6f12), 0x2000 },
> + { CCI_REG16(0x6f12), 0x19a0 },
> + { CCI_REG16(0x6f12), 0x2000 },
> + { CCI_REG16(0x6f12), 0x30c0 },
> + { CCI_REG16(0x6f12), 0x4000 },
> + { CCI_REG16(0x6f12), 0x9800 },
> + { CCI_REG16(0x6f12), 0x2000 },
> + { CCI_REG16(0x6f12), 0x1dd0 },
> + { CCI_REG16(0x6f12), 0x2000 },
> + { CCI_REG16(0x6f12), 0x17c0 },
> + { CCI_REG16(0x6f12), 0x2000 },
> + { CCI_REG16(0x6f12), 0x2210 },
> + { CCI_REG16(0x6f12), 0x2000 },
> + { CCI_REG16(0x6f12), 0x2670 },
> + { CCI_REG16(0x6f12), 0x0000 },
> + { CCI_REG16(0x6f12), 0xf45f },
> + { CCI_REG16(0x6f12), 0x0000 },
> + { CCI_REG16(0x6f12), 0xd957 },
> + { CCI_REG16(0x6f12), 0x2000 },
> + { CCI_REG16(0x6f12), 0x08c0 },
> + { CCI_REG16(0x6f12), 0x49f6 },
> + { CCI_REG16(0x6f12), 0x213c },
> + { CCI_REG16(0x6f12), 0xc0f2 },
> + { CCI_REG16(0x6f12), 0x000c },
> + { CCI_REG16(0x6f12), 0x6047 },
> + { CCI_REG16(0x6f12), 0x4df6 },
> + { CCI_REG16(0x6f12), 0x571c },
> + { CCI_REG16(0x6f12), 0xc0f2 },
> + { CCI_REG16(0x6f12), 0x000c },
> + { CCI_REG16(0x6f12), 0x6047 },
> + { CCI_REG16(0x6f12), 0x4ff2 },
> + { CCI_REG16(0x6f12), 0x5f4c },
> + { CCI_REG16(0x6f12), 0xc0f2 },
> + { CCI_REG16(0x6f12), 0x000c },
> + { CCI_REG16(0x6f12), 0x6047 },
> + { CCI_REG16(0x6f12), 0x44f2 },
> + { CCI_REG16(0x6f12), 0x9b0c },
> + { CCI_REG16(0x6f12), 0xc0f2 },
> + { CCI_REG16(0x6f12), 0x000c },
> + { CCI_REG16(0x6f12), 0x6047 },
> + { CCI_REG16(0x6f12), 0x4bf2 },
> + { CCI_REG16(0x6f12), 0xed0c },
> + { CCI_REG16(0x6f12), 0xc0f2 },
> + { CCI_REG16(0x6f12), 0x000c },
> + { CCI_REG16(0x6f12), 0x6047 },
> + { CCI_REG16(0x6f12), 0x0000 },
> + { CCI_REG16(0x6028), 0x2000 },
> + { CCI_REG16(0x602a), 0x1014 },
> + { CCI_REG16(0x6f12), 0x0100 },
> + { CCI_REG16(0x6f12), 0x7000 },
> + { CCI_REG16(0x602a), 0x108a },
> + { CCI_REG16(0x6f12), 0x0006 },
> + { CCI_REG16(0x602a), 0x1092 },
> + { CCI_REG16(0x6f12), 0x0005 },
> + { CCI_REG16(0x602a), 0x1096 },
> + { CCI_REG16(0x6f12), 0x0002 },
> + { CCI_REG16(0x6f12), 0x001a },
> + { CCI_REG16(0x602a), 0x109c },
> + { CCI_REG16(0x6f12), 0x0014 },
> + { CCI_REG16(0x602a), 0x10a2 },
> + { CCI_REG16(0x6f12), 0x0022 },
> + { CCI_REG16(0x602a), 0x10ae },
> + { CCI_REG16(0x6f12), 0x0007 },
> + { CCI_REG16(0x602a), 0x10c2 },
> + { CCI_REG16(0x6f12), 0x001e },
> + { CCI_REG16(0x602a), 0x10f4 },
> + { CCI_REG16(0x6f12), 0x0003 },
> + { CCI_REG16(0x6f12), 0x0003 },
> + { CCI_REG16(0x602a), 0x110a },
> + { CCI_REG16(0x6f12), 0x0000 },
> + { CCI_REG16(0x602a), 0x113e },
> + { CCI_REG16(0x6f12), 0x0001 },
> + { CCI_REG16(0x6f12), 0x014e },
> + { CCI_REG16(0x602a), 0x13ea },
> + { CCI_REG16(0x6f12), 0x160f },
> + { CCI_REG16(0x6f12), 0x0d00 },
> + { CCI_REG16(0x602a), 0x13fa },
> + { CCI_REG16(0x6f12), 0x009d },
> + { CCI_REG16(0x6f12), 0x0107 },
> + { CCI_REG16(0x602a), 0x14e2 },
> + { CCI_REG16(0x6f12), 0x04c2 },
> + { CCI_REG16(0x6f12), 0x02ae },
> + { CCI_REG16(0x6f12), 0x0020 },
> + { CCI_REG16(0x6028), 0x4000 },
> + { CCI_REG16(0xf44a), 0x0010 },
> + { CCI_REG16(0xf46a), 0xb6a0 },
> + { CCI_REG16(0x6028), 0x2000 },
> + { CCI_REG16(0x602a), 0x0f5e },
> + { CCI_REG16(0x6f12), 0x0200 },
> + { CCI_REG16(0x602a), 0x0f90 },
> + { CCI_REG16(0x6f12), 0x0000 },
> + { CCI_REG16(0x602a), 0x17c0 },
> + { CCI_REG16(0x6f12), 0x0010 },
> + { CCI_REG16(0x6f12), 0x0201 },
> + { CCI_REG16(0x602a), 0x1da2 },
> + { CCI_REG16(0x6f12), 0x0001 },
> + { CCI_REG16(0x6f12), 0x0203 },
> + { CCI_REG16(0x6f12), 0x0405 },
> + { CCI_REG16(0x6f12), 0x0607 },
> + { CCI_REG16(0x6f12), 0x0809 },
> + { CCI_REG16(0x6f12), 0x0a0b },
> + { CCI_REG16(0x6f12), 0x0c0d },
> + { CCI_REG16(0x6f12), 0x0e0f },
> + { CCI_REG16(0x6028), 0x4000 },
> + { CCI_REG16(0x020c), 0x0001 },
> + { CCI_REG16(0x0bc6), 0x0000 },
> + { CCI_REG16(0x0d00), 0x0000 },
> + { CCI_REG16(0xb13c), 0x0800 },
> +};
> +
> +static const struct cci_reg_sequence s5k3t2_5184x3880_mode[] = {
> + { CCI_REG16(0x6028), 0x4000 },
> + { CCI_REG16(0x0136), 0x1300 },
> + { CCI_REG16(0x013e), 0x00c8 },
> + { CCI_REG16(0x0304), 0x0003 },
> + { CCI_REG16(0x0306), 0x00b9 },
> + { CCI_REG16(0x6028), 0x2000 },
> + { CCI_REG16(0x602a), 0x0e0e },
> + { CCI_REG16(0x6f12), 0x0103 },
> + { CCI_REG16(0x6f12), 0x0000 },
> + { CCI_REG16(0x602a), 0x1078 },
> + { CCI_REG16(0x6f12), 0x04a6 },
> + { CCI_REG16(0x6028), 0x4000 },
> + { CCI_REG16(0x030c), 0x0000 },
> + { CCI_REG16(0x0302), 0x0001 },
> + { CCI_REG16(0x0300), 0x0007 },
> + { CCI_REG16(0x030e), 0x0003 },
> + { CCI_REG16(0x0310), 0x008c },
> + { CCI_REG16(0x0312), 0x0001 },
> + { CCI_REG16(0x0308), 0x0008 },
> + { CCI_REG16(0x030a), 0x0001 },
> + { CCI_REG16(0x0344), 0x0008 },
> + { CCI_REG16(0x0346), 0x0008 },
> + { CCI_REG16(0x0348), 0x1447 },
> + { CCI_REG16(0x034a), 0x0f2f },
> + { CCI_REG16(0x034c), 0x1440 },
> + { CCI_REG16(0x034e), 0x0f28 },
> + { CCI_REG16(0x0350), 0x0000 },
> + { CCI_REG16(0x0352), 0x0000 },
> + { CCI_REG16(0x0900), 0x0111 },
> + { CCI_REG16(0x0404), 0x1000 },
> + { CCI_REG16(0x0380), 0x0001 },
> + { CCI_REG16(0x0382), 0x0001 },
> + { CCI_REG16(0x0384), 0x0001 },
> + { CCI_REG16(0x0386), 0x0001 },
> + { CCI_REG16(0x0342), 0x2a80 },
> + { CCI_REG16(0x0340), 0x1000 },
> + { CCI_REG16(0x6028), 0x4000 },
> + { CCI_REG16(0x010c), 0x0000 },
> + { CCI_REG16(0x0114), 0x0300 },
> + { CCI_REG16(0x0116), 0x3000 },
> + { CCI_REG16(0x011a), 0x0001 },
> + { CCI_REG16(0x0118), 0x0002 },
> + { CCI_REG16(0x6028), 0x2000 },
> + { CCI_REG16(0x602a), 0x0e92 },
> + { CCI_REG16(0x6f12), 0xffff },
> + { CCI_REG16(0x602a), 0x165a },
> + { CCI_REG16(0x6f12), 0x0003 },
> + { CCI_REG16(0x6028), 0x4000 },
> + { CCI_REG16(0x0b04), 0x0001 },
> + { CCI_REG16(0x0b06), 0x0101 },
> + { CCI_REG16(0x0fea), 0x04a0 },
> + { CCI_REG16(0x6028), 0x2000 },
> + { CCI_REG16(0x602a), 0x3c98 },
> + { CCI_REG16(0x6f12), 0x0a58 },
> + { CCI_REG16(0x6f12), 0x1477 },
> + { CCI_REG16(0x602a), 0x1da0 },
> + { CCI_REG16(0x6f12), 0x0010 },
> + { CCI_REG16(0x602a), 0x10ac },
> + { CCI_REG16(0x6f12), 0x000a },
> + { CCI_REG16(0x602a), 0x1110 },
> + { CCI_REG16(0x6f12), 0x001d },
> + { CCI_REG16(0x6f12), 0x003f },
> + { CCI_REG16(0x602a), 0x13e8 },
> + { CCI_REG16(0x6f12), 0x0804 },
> + { CCI_REG16(0x602a), 0x13f8 },
> + { CCI_REG16(0x6f12), 0x38c8 },
> +};
> +
> +static const struct cci_reg_sequence s5k3t2_2592x1940_mode[] = {
> + { CCI_REG16(0x6028), 0x4000 },
> + { CCI_REG16(0x0136), 0x1300 },
> + { CCI_REG16(0x013e), 0x00c8 },
> + { CCI_REG16(0x0304), 0x0003 },
> + { CCI_REG16(0x0306), 0x00b9 },
> + { CCI_REG16(0x6028), 0x2000 },
> + { CCI_REG16(0x602a), 0x0e0e },
> + { CCI_REG16(0x6f12), 0x0103 },
> + { CCI_REG16(0x6f12), 0x0000 },
> + { CCI_REG16(0x602a), 0x1078 },
> + { CCI_REG16(0x6f12), 0x04a6 },
> + { CCI_REG16(0x6028), 0x4000 },
> + { CCI_REG16(0x030c), 0x0000 },
> + { CCI_REG16(0x0302), 0x0001 },
> + { CCI_REG16(0x0300), 0x0007 },
> + { CCI_REG16(0x030e), 0x0003 },
> + { CCI_REG16(0x0310), 0x008c },
> + { CCI_REG16(0x0312), 0x0001 },
> + { CCI_REG16(0x0308), 0x0008 },
> + { CCI_REG16(0x030a), 0x0001 },
> + { CCI_REG16(0x0344), 0x0008 },
> + { CCI_REG16(0x0346), 0x0008 },
> + { CCI_REG16(0x0348), 0x1447 },
> + { CCI_REG16(0x034a), 0x0f2f },
> + { CCI_REG16(0x034c), 0x0a20 },
> + { CCI_REG16(0x034e), 0x0794 },
> + { CCI_REG16(0x0350), 0x0000 },
> + { CCI_REG16(0x0352), 0x0000 },
> + { CCI_REG16(0x0900), 0x0122 },
> + { CCI_REG16(0x0404), 0x1000 },
> + { CCI_REG16(0x0380), 0x0002 },
> + { CCI_REG16(0x0382), 0x0002 },
> + { CCI_REG16(0x0384), 0x0002 },
> + { CCI_REG16(0x0386), 0x0002 },
> + { CCI_REG16(0x0342), 0x1610 },
> + { CCI_REG16(0x0340), 0x07cc },
> + { CCI_REG16(0x6028), 0x4000 },
> + { CCI_REG16(0x010c), 0x0000 },
> + { CCI_REG16(0x0114), 0x0300 },
> + { CCI_REG16(0x0116), 0x3000 },
> + { CCI_REG16(0x011a), 0x0001 },
> + { CCI_REG16(0x0118), 0x0002 },
> + { CCI_REG16(0x6028), 0x2000 },
> + { CCI_REG16(0x602a), 0x0e92 },
> + { CCI_REG16(0x6f12), 0xffff },
> + { CCI_REG16(0x602a), 0x165a },
> + { CCI_REG16(0x6f12), 0x0003 },
> + { CCI_REG16(0x6028), 0x4000 },
> + { CCI_REG16(0x0b04), 0x0001 },
> + { CCI_REG16(0x0b06), 0x0101 },
> + { CCI_REG16(0x0fea), 0x04a0 },
> + { CCI_REG16(0x6028), 0x2000 },
> + { CCI_REG16(0x602a), 0x3c98 },
> + { CCI_REG16(0x6f12), 0x052c },
> + { CCI_REG16(0x6f12), 0x0a3b },
> + { CCI_REG16(0x602a), 0x1da0 },
> + { CCI_REG16(0x6f12), 0x0010 },
> + { CCI_REG16(0x602a), 0x10ac },
> + { CCI_REG16(0x6f12), 0x0014 },
> + { CCI_REG16(0x602a), 0x1110 },
> + { CCI_REG16(0x6f12), 0x001d },
> + { CCI_REG16(0x6f12), 0x004d },
> + { CCI_REG16(0x602a), 0x13e8 },
> + { CCI_REG16(0x6f12), 0x080f },
> + { CCI_REG16(0x602a), 0x13f8 },
> + { CCI_REG16(0x6f12), 0x38c8 },
> +};
> +
> +static const struct cci_reg_sequence s5k3t2_1280x720_mode[] = {
> + { CCI_REG16(0x6028), 0x4000 },
> + { CCI_REG16(0x0136), 0x1300 },
> + { CCI_REG16(0x013e), 0x00c8 },
> + { CCI_REG16(0x0304), 0x0003 },
> + { CCI_REG16(0x0306), 0x00b9 },
> + { CCI_REG16(0x6028), 0x2000 },
> + { CCI_REG16(0x602a), 0x0e0e },
> + { CCI_REG16(0x6f12), 0x0104 },
> + { CCI_REG16(0x6f12), 0x0000 },
> + { CCI_REG16(0x602a), 0x1078 },
> + { CCI_REG16(0x6f12), 0x04a6 },
> + { CCI_REG16(0x6028), 0x4000 },
> + { CCI_REG16(0x030c), 0x0000 },
> + { CCI_REG16(0x0302), 0x0001 },
> + { CCI_REG16(0x0300), 0x0007 },
> + { CCI_REG16(0x030e), 0x0002 },
> + { CCI_REG16(0x0310), 0x007d },
> + { CCI_REG16(0x0312), 0x0002 },
> + { CCI_REG16(0x0308), 0x0008 },
> + { CCI_REG16(0x030a), 0x0001 },
> + { CCI_REG16(0x0344), 0x0028 },
> + { CCI_REG16(0x0346), 0x01fc },
> + { CCI_REG16(0x0348), 0x1427 },
> + { CCI_REG16(0x034a), 0x0d3b },
> + { CCI_REG16(0x034c), 0x0500 },
> + { CCI_REG16(0x034e), 0x02d0 },
> + { CCI_REG16(0x0350), 0x0000 },
> + { CCI_REG16(0x0352), 0x0000 },
> + { CCI_REG16(0x0900), 0x0124 },
> + { CCI_REG16(0x0404), 0x2000 },
> + { CCI_REG16(0x0380), 0x0002 },
> + { CCI_REG16(0x0382), 0x0002 },
> + { CCI_REG16(0x0384), 0x0002 },
> + { CCI_REG16(0x0386), 0x0006 },
> + { CCI_REG16(0x0342), 0x1620 },
> + { CCI_REG16(0x0340), 0x03e0 },
> + { CCI_REG16(0x6028), 0x4000 },
> + { CCI_REG16(0x010c), 0x0000 },
> + { CCI_REG16(0x0114), 0x0300 },
> + { CCI_REG16(0x0116), 0x3000 },
> + { CCI_REG16(0x011a), 0x0001 },
> + { CCI_REG16(0x0118), 0x0002 },
> + { CCI_REG16(0x6028), 0x2000 },
> + { CCI_REG16(0x602a), 0x0e92 },
> + { CCI_REG16(0x6f12), 0xffff },
> + { CCI_REG16(0x602a), 0x165a },
> + { CCI_REG16(0x6f12), 0x0020 },
> + { CCI_REG16(0x6028), 0x4000 },
> + { CCI_REG16(0x0b04), 0x0001 },
> + { CCI_REG16(0x0b06), 0x0101 },
> + { CCI_REG16(0x0fea), 0x04a0 },
> + { CCI_REG16(0x6028), 0x2000 },
> + { CCI_REG16(0x602a), 0x3c98 },
> + { CCI_REG16(0x6f12), 0x0296 },
> + { CCI_REG16(0x6f12), 0x0515 },
> + { CCI_REG16(0x602a), 0x1da0 },
> + { CCI_REG16(0x6f12), 0x0110 },
> + { CCI_REG16(0x602a), 0x10ac },
> + { CCI_REG16(0x6f12), 0x0014 },
> + { CCI_REG16(0x602a), 0x1110 },
> + { CCI_REG16(0x6f12), 0x001d },
> + { CCI_REG16(0x6f12), 0x004d },
> + { CCI_REG16(0x602a), 0x13e8 },
> + { CCI_REG16(0x6f12), 0x080f },
> + { CCI_REG16(0x602a), 0x13f8 },
> + { CCI_REG16(0x6f12), 0x38c8 },
> +};
> +
> +static const struct s5k3t2_mode s5k3t2_supported_modes[] = {
> + {
> + .width = 5184,
> + .height = 3880,
> + .hts = 10880,
> + .vts = 4096,
> + .exposure = 1000,
> + .link_freq_index = 0,
> + .crop = {
> + .left = 8,
> + .top = 8,
> + .width = 5184,
> + .height = 3880,
> + },
> + .reg_list = {
> + .regs = s5k3t2_5184x3880_mode,
> + .num_regs = ARRAY_SIZE(s5k3t2_5184x3880_mode),
> + },
> + },
> + {
> + .width = 2592,
> + .height = 1940,
> + .hts = 5648,
> + .vts = 1996,
> + .exposure = 1000,
> + .link_freq_index = 0,
> + .crop = {
> + .left = 8,
> + .top = 8,
> + .width = 5184,
> + .height = 3880,
> + },
> + .reg_list = {
> + .regs = s5k3t2_2592x1940_mode,
> + .num_regs = ARRAY_SIZE(s5k3t2_2592x1940_mode),
> + },
> + },
> + {
> + .width = 1280,
> + .height = 720,
> + .hts = 5664,
> + .vts = 992,
> + .exposure = 500,
> + .link_freq_index = 1,
> + .crop = {
> + .left = 40,
> + .top = 508,
> + .width = 5120,
> + .height = 2880,
> + },
> + .reg_list = {
> + .regs = s5k3t2_1280x720_mode,
> + .num_regs = ARRAY_SIZE(s5k3t2_1280x720_mode),
> + },
> + },
> +};
> +
> +static int s5k3t2_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct s5k3t2 *s5k3t2 = container_of(ctrl->handler, struct s5k3t2,
> + ctrl_handler);
> + const struct s5k3t2_mode *mode = s5k3t2->mode;
> + s64 exposure_max;
> + int ret;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_HFLIP:
> + case V4L2_CID_VFLIP:
> + case V4L2_CID_LINK_FREQ:
> + case V4L2_CID_PIXEL_RATE:
> + case V4L2_CID_HBLANK:
For controls that aren't handled here, you can set the handler NULL. See my
other comments on HFLIP and VFLIP, too.
But HBLANK should be handled below, shouldn't it? The fewer of the
configuration that could be dynamic is set through register lists, the
better.
> + return 0;
> + case V4L2_CID_VBLANK:
> + exposure_max = mode->height + ctrl->val - S5K3T2_EXPOSURE_MARGIN;
> + __v4l2_ctrl_modify_range(s5k3t2->exposure,
> + s5k3t2->exposure->minimum,
> + exposure_max,
> + s5k3t2->exposure->step,
> + s5k3t2->exposure->default_value);
> + break;
> + }
> +
> + if (!pm_runtime_get_if_active(s5k3t2->dev))
> + return 0;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_ANALOGUE_GAIN:
> + ret = cci_write(s5k3t2->regmap, S5K3T2_REG_AGAIN,
> + ctrl->val << S5K3T2_AGAIN_SHIFT, NULL);
> + break;
> + case V4L2_CID_EXPOSURE:
> + ret = cci_write(s5k3t2->regmap, S5K3T2_REG_EXPOSURE,
> + ctrl->val, NULL);
> + break;
> + case V4L2_CID_VBLANK:
> + ret = cci_write(s5k3t2->regmap, S5K3T2_REG_VTS,
> + ctrl->val + mode->height, NULL);
> + break;
> + case V4L2_CID_TEST_PATTERN:
> + ret = cci_write(s5k3t2->regmap, S5K3T2_REG_TEST_PATTERN,
> + ctrl->val, NULL);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + pm_runtime_put(s5k3t2->dev);
> +
> + return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops s5k3t2_ctrl_ops = {
> + .s_ctrl = s5k3t2_set_ctrl,
> +};
> +
> +static int s5k3t2_init_controls(struct s5k3t2 *s5k3t2)
> +{
> + struct v4l2_ctrl_handler *ctrl_hdlr = &s5k3t2->ctrl_handler;
> + const struct s5k3t2_mode *mode = s5k3t2->mode;
> + s64 hblank, vblank, exposure_max;
> + struct v4l2_fwnode_device_properties props;
> + int ret;
> +
> + v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> +
> + s5k3t2->link_freq =
> + v4l2_ctrl_new_int_menu(ctrl_hdlr, &s5k3t2_ctrl_ops,
> + V4L2_CID_LINK_FREQ,
> + ARRAY_SIZE(s5k3t2_link_freq_menu) - 1,
> + mode->link_freq_index,
> + s5k3t2_link_freq_menu);
> + if (s5k3t2->link_freq)
> + s5k3t2->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
Hmm. The link frequency is supposed to be set by the user, not by the
driver. How do you change mode in that case?
> +
> + s5k3t2->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
> + V4L2_CID_PIXEL_RATE,
> + S5K3T2_PIXEL_RATE,
> + S5K3T2_PIXEL_RATE, 1,
> + S5K3T2_PIXEL_RATE);
> +
> + hblank = mode->hts - mode->width;
> + s5k3t2->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
> + V4L2_CID_HBLANK, hblank,
> + hblank, 1, hblank);
> + if (s5k3t2->hblank)
> + s5k3t2->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
Set this flag after checking the handler's error condition and you can
remove the check. Same for the other two below.
> +
> + vblank = mode->vts - mode->height;
> + s5k3t2->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
> + V4L2_CID_VBLANK, vblank,
> + S5K3T2_VTS_MAX - mode->height, 1,
> + vblank);
> +
> + v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> + S5K3T2_AGAIN_MIN, S5K3T2_AGAIN_MAX,
> + S5K3T2_AGAIN_STEP, S5K3T2_AGAIN_DEFAULT);
> +
> + exposure_max = mode->vts - S5K3T2_EXPOSURE_MARGIN;
> + s5k3t2->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
> + V4L2_CID_EXPOSURE,
> + S5K3T2_EXPOSURE_MIN,
> + exposure_max,
> + S5K3T2_EXPOSURE_STEP,
> + mode->exposure);
> +
> + v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &s5k3t2_ctrl_ops,
> + V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(s5k3t2_test_pattern_menu) - 1,
> + 0, 0, s5k3t2_test_pattern_menu);
> +
> + s5k3t2->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
> + V4L2_CID_HFLIP, 0, 1, 1, 0);
> + if (s5k3t2->hflip)
> + s5k3t2->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> + s5k3t2->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
> + V4L2_CID_VFLIP, 0, 1, 1, 0);
> + if (s5k3t2->vflip)
> + s5k3t2->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> + ret = v4l2_fwnode_device_parse(s5k3t2->dev, &props);
> + if (ret)
> + goto error_free_hdlr;
Move this before initialising the control handler and error handling is
simplified.
> +
> + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &s5k3t2_ctrl_ops,
> + &props);
> + if (ret)
> + goto error_free_hdlr;
This check is redundant.
> +
> + if (ctrl_hdlr->error) {
> + ret = ctrl_hdlr->error;
You can omit this assignment and return handler's error code.
> + goto error_free_hdlr;
You can free the handler and return here as you have a single location for
error handling.
> + }
> +
> + s5k3t2->sd.ctrl_handler = ctrl_hdlr;
> +
> + return 0;
> +
> +error_free_hdlr:
> + v4l2_ctrl_handler_free(ctrl_hdlr);
> +
> + return ret;
> +}
> +
> +static int s5k3t2_enable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> +{
> + struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
> + const struct s5k3t2_reg_list *reg_list = &s5k3t2->mode->reg_list;
> + int ret;
> +
> + ret = pm_runtime_resume_and_get(s5k3t2->dev);
> + if (ret)
> + return ret;
> +
> + cci_write(s5k3t2->regmap, CCI_REG16(0x6028), 0x4000, &ret);
> + cci_write(s5k3t2->regmap, CCI_REG16(0x0000), 0x0005, &ret);
> + cci_write(s5k3t2->regmap, CCI_REG16(0x0000), S5K3T2_CHIP_ID, &ret);
> + cci_write(s5k3t2->regmap, CCI_REG16(0x6010), 0x0001, &ret);
> + if (ret)
> + goto error;
> +
> + usleep_range(5 * USEC_PER_MSEC, 6 * USEC_PER_MSEC);
> +
> + cci_multi_reg_write(s5k3t2->regmap, s5k3t2_init_setting,
> + ARRAY_SIZE(s5k3t2_init_setting), &ret);
> + cci_multi_reg_write(s5k3t2->regmap, reg_list->regs,
> + reg_list->num_regs, &ret);
> + if (ret)
> + goto error;
> +
> + ret = __v4l2_ctrl_handler_setup(s5k3t2->sd.ctrl_handler);
> +
> + cci_write(s5k3t2->regmap, S5K3T2_REG_CTRL_MODE,
> + S5K3T2_MODE_STREAMING |
> + (s5k3t2->vflip->val ? S5K3T2_VFLIP : 0) |
> + (s5k3t2->hflip->val ? S5K3T2_HFLIP : 0), &ret);
Can you set these using the control handler's set callback? They should be
clustered, too.
> + if (ret)
> + goto error;
> +
> + __v4l2_ctrl_grab(s5k3t2->vflip, true);
> + __v4l2_ctrl_grab(s5k3t2->hflip, true);
> +
> + return 0;
> +
> +error:
> + dev_err(s5k3t2->dev, "failed to start streaming: %d\n", ret);
> + pm_runtime_put_autosuspend(s5k3t2->dev);
> +
> + return ret;
> +}
> +
> +static int s5k3t2_disable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> +{
> + struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
> + int ret;
> +
> + ret = cci_write(s5k3t2->regmap, S5K3T2_REG_CTRL_MODE, 0, NULL);
> + if (ret)
> + dev_err(s5k3t2->dev, "failed to stop streaming: %d\n", ret);
> +
> + __v4l2_ctrl_grab(s5k3t2->vflip, false);
> + __v4l2_ctrl_grab(s5k3t2->hflip, false);
> +
> + pm_runtime_put_autosuspend(s5k3t2->dev);
> +
> + return ret;
> +}
> +
> +static u32 s5k3t2_get_format_code(struct s5k3t2 *s5k3t2)
> +{
> + unsigned int i;
> +
> + i = (s5k3t2->vflip->val ? 2 : 0) | (s5k3t2->hflip->val ? 1 : 0);
> +
> + return s5k3t2_mbus_formats[i];
> +}
> +
> +static void s5k3t2_update_pad_format(struct s5k3t2 *s5k3t2,
How about calling this s5k3t2_format_from_mode()?
> + const struct s5k3t2_mode *mode,
> + struct v4l2_mbus_framefmt *fmt)
> +{
> + fmt->code = s5k3t2_get_format_code(s5k3t2);
> + fmt->width = mode->width;
> + fmt->height = mode->height;
> + fmt->field = V4L2_FIELD_NONE;
> + fmt->colorspace = V4L2_COLORSPACE_RAW;
> + fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> + fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> +}
> +
> +static bool s5k3t2_filter_by_link_freq(const void *array, size_t index,
> + const void *context)
> +{
> + const struct s5k3t2_mode *mode = array;
> + const struct s5k3t2 *s5k3t2 = context;
> +
> + return s5k3t2->link_freq_bitmap & BIT(mode->link_freq_index);
> +}
> +
> +static int s5k3t2_set_pad_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
> + s64 hblank, vblank, exposure_max;
> + const struct s5k3t2_mode *mode;
> +
> + mode = v4l2_find_nearest_size_conditional(s5k3t2_supported_modes,
> + ARRAY_SIZE(s5k3t2_supported_modes),
> + width, height,
> + fmt->format.width,
> + fmt->format.height,
> + s5k3t2_filter_by_link_freq,
> + s5k3t2);
> +
> + s5k3t2_update_pad_format(s5k3t2, mode, &fmt->format);
> +
> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY || s5k3t2->mode == mode)
> + goto set_format;
> +
> + s5k3t2->mode = mode;
Please avoid having state outside the sub-device state. In other words,
obtain the mode using v4l2_find_nearest_size_conditional() where you need
it.
> +
> + __v4l2_ctrl_s_ctrl(s5k3t2->link_freq, mode->link_freq_index);
> +
> + hblank = mode->hts - mode->width;
> + __v4l2_ctrl_modify_range(s5k3t2->hblank, hblank, hblank, 1, hblank);
> +
> + vblank = mode->vts - mode->height;
> + __v4l2_ctrl_modify_range(s5k3t2->vblank, vblank,
> + S5K3T2_VTS_MAX - mode->height, 1, vblank);
> + __v4l2_ctrl_s_ctrl(s5k3t2->vblank, vblank);
> +
> + exposure_max = mode->vts - S5K3T2_EXPOSURE_MARGIN;
> + __v4l2_ctrl_modify_range(s5k3t2->exposure, S5K3T2_EXPOSURE_MIN,
> + exposure_max, S5K3T2_EXPOSURE_STEP,
> + mode->exposure);
> + __v4l2_ctrl_s_ctrl(s5k3t2->exposure, mode->exposure);
> +
> + if (s5k3t2->sd.ctrl_handler->error)
> + return s5k3t2->sd.ctrl_handler->error;
Setting controls won't affect handler's error status; check for return
codes for each call above.
> +
> +set_format:
> + *v4l2_subdev_state_get_format(state, 0) = fmt->format;
> +
> + return 0;
> +}
> +
> +static int s5k3t2_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
> +
> + if (code->index > 0)
> + return -EINVAL;
> +
> + code->code = s5k3t2_get_format_code(s5k3t2);
> +
> + return 0;
> +}
> +
> +static int s5k3t2_enum_frame_size(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
> + unsigned int i, index = fse->index;
> +
> + if (fse->code != s5k3t2_get_format_code(s5k3t2))
> + return -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(s5k3t2_supported_modes); i++) {
> + const struct s5k3t2_mode *mode = &s5k3t2_supported_modes[i];
> +
> + if (!(s5k3t2->link_freq_bitmap & BIT(mode->link_freq_index)))
> + continue;
> +
> + if (index--)
> + continue;
> +
> + fse->min_width = mode->width;
> + fse->max_width = fse->min_width;
> + fse->min_height = mode->height;
> + fse->max_height = fse->min_height;
> +
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int s5k3t2_get_selection(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_selection *sel)
> +{
> + struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
> + const struct s5k3t2_mode *mode = s5k3t2->mode;
> + const struct v4l2_mbus_framefmt *fmt;
> +
> + switch (sel->target) {
> + case V4L2_SEL_TGT_CROP:
> + if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
> + fmt = v4l2_subdev_state_get_format(sd_state, 0);
> + mode = v4l2_find_nearest_size(s5k3t2_supported_modes,
> + ARRAY_SIZE(s5k3t2_supported_modes),
> + width, height,
> + fmt->width, fmt->height);
> + }
> + sel->r = mode->crop;
Set the crop rectangle in the state instead.
> + return 0;
> + case V4L2_SEL_TGT_NATIVE_SIZE:
> + sel->r.left = 0;
> + sel->r.top = 0;
> + sel->r.width = S5K3T2_NATIVE_WIDTH;
> + sel->r.height = S5K3T2_NATIVE_HEIGHT;
> + return 0;
> + case V4L2_SEL_TGT_CROP_DEFAULT:
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + sel->r.left = 8;
> + sel->r.top = 8;
> + sel->r.width = 5184;
> + sel->r.height = 3880;
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int s5k3t2_init_state(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state)
> +{
> + struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
> + struct v4l2_subdev_format fmt = {
> + .which = V4L2_SUBDEV_FORMAT_TRY,
> + .pad = 0,
> + .format = {
> + .width = s5k3t2->mode->width,
> + .height = s5k3t2->mode->height,
> + },
> + };
> +
> + s5k3t2_set_pad_format(sd, state, &fmt);
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_video_ops s5k3t2_video_ops = {
> + .s_stream = v4l2_subdev_s_stream_helper,
> +};
> +
> +static const struct v4l2_subdev_pad_ops s5k3t2_pad_ops = {
> + .set_fmt = s5k3t2_set_pad_format,
> + .get_fmt = v4l2_subdev_get_fmt,
> + .get_selection = s5k3t2_get_selection,
> + .enum_mbus_code = s5k3t2_enum_mbus_code,
> + .enum_frame_size = s5k3t2_enum_frame_size,
> + .enable_streams = s5k3t2_enable_streams,
> + .disable_streams = s5k3t2_disable_streams,
> +};
> +
> +static const struct v4l2_subdev_ops s5k3t2_subdev_ops = {
> + .video = &s5k3t2_video_ops,
> + .pad = &s5k3t2_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops s5k3t2_internal_ops = {
> + .init_state = s5k3t2_init_state,
> +};
> +
> +static const struct media_entity_operations s5k3t2_subdev_entity_ops = {
> + .link_validate = v4l2_subdev_link_validate,
You won't need link validation as you have no sink pads.
> +};
> +
> +static int s5k3t2_identify_sensor(struct s5k3t2 *s5k3t2)
> +{
> + u64 val;
> + int ret;
> +
> + ret = cci_read(s5k3t2->regmap, S5K3T2_REG_CHIP_ID, &val, NULL);
> + if (ret) {
> + dev_err(s5k3t2->dev, "failed to read chip id: %d\n", ret);
> + return ret;
> + }
> +
> + if (val != S5K3T2_CHIP_ID) {
> + dev_err(s5k3t2->dev, "chip id mismatch: %x!=%llx\n",
> + S5K3T2_CHIP_ID, val);
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +static int s5k3t2_check_hwcfg(struct s5k3t2 *s5k3t2)
> +{
> + struct fwnode_handle *fwnode = dev_fwnode(s5k3t2->dev), *ep;
> + struct v4l2_fwnode_endpoint bus_cfg = {
> + .bus = {
> + .mipi_csi2 = {
> + .num_data_lanes = S5K3T2_DATA_LANES,
> + },
> + },
> + .bus_type = V4L2_MBUS_CSI2_DPHY,
> + };
> + int ret;
> +
> + if (!fwnode)
> + return -ENODEV;
Redundant check, please drop.
> +
> + ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> + if (!ep)
> + return -EINVAL;
Ditto.
> +
> + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> + fwnode_handle_put(ep);
> + if (ret)
> + return ret;
> +
> + if (bus_cfg.bus.mipi_csi2.num_data_lanes != S5K3T2_DATA_LANES) {
> + dev_err(s5k3t2->dev, "invalid number of data lanes: %u\n",
> + bus_cfg.bus.mipi_csi2.num_data_lanes);
> + ret = -EINVAL;
> + goto endpoint_free;
> + }
> +
> + ret = v4l2_link_freq_to_bitmap(s5k3t2->dev, bus_cfg.link_frequencies,
> + bus_cfg.nr_of_link_frequencies,
> + s5k3t2_link_freq_menu,
> + ARRAY_SIZE(s5k3t2_link_freq_menu),
> + &s5k3t2->link_freq_bitmap);
> +
> +endpoint_free:
> + v4l2_fwnode_endpoint_free(&bus_cfg);
> +
> + return ret;
> +}
> +
> +static int s5k3t2_power_on(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
> + int ret;
> +
> + ret = regulator_bulk_enable(S5K3T2_NUM_SUPPLIES, s5k3t2->supplies);
> + if (ret)
> + return ret;
> +
> + ret = clk_prepare_enable(s5k3t2->mclk);
> + if (ret)
> + goto disable_regulators;
Please handle the error here without a goto -- you only have a single
location where you do some unwinding.
> +
> + usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC);
> + gpiod_set_value_cansleep(s5k3t2->reset_gpio, 0);
> + usleep_range(10 * USEC_PER_MSEC, 15 * USEC_PER_MSEC);
> +
> + return 0;
> +
> +disable_regulators:
> + regulator_bulk_disable(S5K3T2_NUM_SUPPLIES, s5k3t2->supplies);
> +
> + return ret;
> +}
> +
> +static int s5k3t2_power_off(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
> +
> + gpiod_set_value_cansleep(s5k3t2->reset_gpio, 1);
> +
> + clk_disable_unprepare(s5k3t2->mclk);
> +
> + regulator_bulk_disable(S5K3T2_NUM_SUPPLIES, s5k3t2->supplies);
> +
> + return 0;
> +}
> +
> +static int s5k3t2_probe(struct i2c_client *client)
> +{
> + struct s5k3t2 *s5k3t2;
> + unsigned long freq;
> + unsigned int i;
> + int ret;
> +
> + s5k3t2 = devm_kzalloc(&client->dev, sizeof(*s5k3t2), GFP_KERNEL);
> + if (!s5k3t2)
> + return -ENOMEM;
> +
> + s5k3t2->dev = &client->dev;
> + v4l2_i2c_subdev_init(&s5k3t2->sd, client, &s5k3t2_subdev_ops);
> +
> + s5k3t2->regmap = devm_cci_regmap_init_i2c(client, 16);
> + if (IS_ERR(s5k3t2->regmap))
> + return dev_err_probe(s5k3t2->dev, PTR_ERR(s5k3t2->regmap),
> + "failed to init CCI\n");
> +
> + s5k3t2->mclk = devm_v4l2_sensor_clk_get(s5k3t2->dev, NULL);
> + if (IS_ERR(s5k3t2->mclk))
> + return dev_err_probe(s5k3t2->dev, PTR_ERR(s5k3t2->mclk),
> + "failed to get MCLK clock\n");
> +
> + freq = clk_get_rate(s5k3t2->mclk);
> + if (freq != S5K3T2_MCLK_FREQ)
> + return dev_err_probe(s5k3t2->dev, -EINVAL,
> + "MCLK clock frequency %lu is not supported\n",
> + freq);
> +
> + ret = s5k3t2_check_hwcfg(s5k3t2);
> + if (ret)
> + return dev_err_probe(s5k3t2->dev, ret,
> + "failed to check HW configuration\n");
> +
> + s5k3t2->reset_gpio = devm_gpiod_get_optional(s5k3t2->dev, "reset",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(s5k3t2->reset_gpio))
> + return dev_err_probe(s5k3t2->dev, PTR_ERR(s5k3t2->reset_gpio),
> + "cannot get reset GPIO\n");
> +
> + for (i = 0; i < S5K3T2_NUM_SUPPLIES; i++)
You can declare i here.
> + s5k3t2->supplies[i].supply = s5k3t2_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(s5k3t2->dev, S5K3T2_NUM_SUPPLIES,
> + s5k3t2->supplies);
> + if (ret)
> + return dev_err_probe(s5k3t2->dev, ret,
> + "failed to get supply regulators\n");
> +
> + ret = s5k3t2_power_on(s5k3t2->dev);
> + if (ret)
> + return ret;
> +
> + ret = s5k3t2_identify_sensor(s5k3t2);
> + if (ret) {
> + dev_err_probe(s5k3t2->dev, ret, "failed to find sensor\n");
> + goto power_off;
> + }
> +
> + s5k3t2->mode =
> + v4l2_find_nearest_size_conditional(s5k3t2_supported_modes,
> + ARRAY_SIZE(s5k3t2_supported_modes),
> + width, height,
> + S5K3T2_DEFAULT_WIDTH,
> + S5K3T2_DEFAULT_HEIGHT,
> + s5k3t2_filter_by_link_freq,
> + s5k3t2);
> + ret = s5k3t2_init_controls(s5k3t2);
> + if (ret) {
> + dev_err_probe(s5k3t2->dev, ret, "failed to init controls\n");
> + goto power_off;
> + }
> +
> + s5k3t2->sd.state_lock = s5k3t2->ctrl_handler.lock;
> + s5k3t2->sd.internal_ops = &s5k3t2_internal_ops;
> + s5k3t2->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + s5k3t2->sd.entity.ops = &s5k3t2_subdev_entity_ops;
> + s5k3t2->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> + s5k3t2->pad.flags = MEDIA_PAD_FL_SOURCE;
> +
> + ret = media_entity_pads_init(&s5k3t2->sd.entity, 1, &s5k3t2->pad);
> + if (ret) {
> + dev_err_probe(s5k3t2->dev, ret,
> + "failed to init media entity pads\n");
> + goto v4l2_ctrl_handler_free;
> + }
> +
> + ret = v4l2_subdev_init_finalize(&s5k3t2->sd);
> + if (ret < 0) {
> + dev_err_probe(s5k3t2->dev, ret,
> + "failed to finalize subdev init\n");
> + goto media_entity_cleanup;
> + }
> +
> + pm_runtime_set_active(s5k3t2->dev);
> + pm_runtime_enable(s5k3t2->dev);
> +
> + ret = v4l2_async_register_subdev_sensor(&s5k3t2->sd);
> + if (ret < 0) {
> + dev_err_probe(s5k3t2->dev, ret,
> + "failed to register V4L2 subdev\n");
> + goto subdev_cleanup;
> + }
> +
> + pm_runtime_set_autosuspend_delay(s5k3t2->dev, 1000);
> + pm_runtime_use_autosuspend(s5k3t2->dev);
> + pm_runtime_idle(s5k3t2->dev);
> +
> + return 0;
> +
> +subdev_cleanup:
It'd be nice to prefix the error handling labels with a common prefix, e.g.
"err_".
> + v4l2_subdev_cleanup(&s5k3t2->sd);
> + pm_runtime_disable(s5k3t2->dev);
> + pm_runtime_set_suspended(s5k3t2->dev);
> +
> +media_entity_cleanup:
> + media_entity_cleanup(&s5k3t2->sd.entity);
> +
> +v4l2_ctrl_handler_free:
> + v4l2_ctrl_handler_free(s5k3t2->sd.ctrl_handler);
> +
> +power_off:
> + s5k3t2_power_off(s5k3t2->dev);
> +
> + return ret;
> +}
> +
> +static void s5k3t2_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
> +
> + v4l2_async_unregister_subdev(sd);
> + v4l2_subdev_cleanup(sd);
> + media_entity_cleanup(&sd->entity);
> + v4l2_ctrl_handler_free(sd->ctrl_handler);
pm_runtime_dont_use_autosuspend(s5k3t2);
> + pm_runtime_disable(s5k3t2->dev);
> +
> + if (!pm_runtime_status_suspended(s5k3t2->dev)) {
> + s5k3t2_power_off(s5k3t2->dev);
> + pm_runtime_set_suspended(s5k3t2->dev);
> + }
> +}
> +
> +static const struct dev_pm_ops s5k3t2_pm_ops = {
> + RUNTIME_PM_OPS(s5k3t2_power_off, s5k3t2_power_on, NULL)
> +};
> +
> +static const struct of_device_id s5k3t2_of_match[] = {
> + { .compatible = "samsung,s5k3t2" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, s5k3t2_of_match);
> +
> +static struct i2c_driver s5k3t2_i2c_driver = {
> + .driver = {
> + .name = "s5k3t2",
> + .pm = pm_ptr(&s5k3t2_pm_ops),
> + .of_match_table = s5k3t2_of_match,
> + },
> + .probe = s5k3t2_probe,
> + .remove = s5k3t2_remove,
> +};
> +
> +module_i2c_driver(s5k3t2_i2c_driver);
> +
> +MODULE_AUTHOR("Armandas Kvietkus <armandas.kvietkus@proton.me>");
> +MODULE_DESCRIPTION("Samsung S5K3T2 image sensor driver");
> +MODULE_LICENSE("GPL");
>
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3 2/2] media: i2c: Add Samsung S5K3T2 image sensor driver
2026-10-01 7:19 ` Sakari Ailus
@ 2026-10-01 18:36 ` armandas.kvietkus
0 siblings, 0 replies; 9+ messages in thread
From: armandas.kvietkus @ 2026-10-01 18:36 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-media, devicetree, linux-kernel
Hi Sakari,
Thanks for the review. It is really good to get feedback from a person
after the bot reviews, so I appreciate you taking the time.
On Thu, Oct 01, 2026 at 10:19:21AM +0300, Sakari Ailus wrote:
> Is Documentation/process/coding-assistants.rst relevant for this
> contribution?
No, it is not relevant. No coding assistants were used for this driver.
> But HBLANK should be handled below, shouldn't it? The fewer of the
> configuration that could be dynamic is set through register lists, the
> better.
Yes. The line length register 0x0342 holds the same value as hts in the
mode table. I'll write width + HBLANK there from s_ctrl and take it
out of the mode tables.
> Hmm. The link frequency is supposed to be set by the user, not by the
> driver. How do you change mode in that case?
The mode decides it. Each mode's register list sets the PLL, so the
link frequency comes with the mode. set_fmt picks the mode and then
sets the control to match, like imx258 does.
The modes are also tied to their frequency by bandwidth. 2592x1940 at
60 fps needs about 3.0 gbit/s, and four lanes at 300 MHz carry 2.4
gbit/s. So of the three modes, only 1280x720 fits at 300 MHz.
If you prefer a writable control, I can do that and only offer the
modes that fit the selected frequency. Which one do you want?
> Can you set these using the control handler's set callback? They should be
> clustered, too.
Yes. I'll write the flips from s_ctrl and cluster them.
> How about calling this s5k3t2_format_from_mode()?
Sure.
I'll fix the rest in v4.
Armandas
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: Add Samsung S5K3T2 image sensor driver
2026-09-24 5:46 ` [PATCH v3 2/2] media: i2c: Add Samsung S5K3T2 image sensor driver Armandas Kvietkus via B4 Relay
2026-09-24 5:58 ` sashiko-bot
2026-10-01 7:19 ` Sakari Ailus
@ 2026-10-01 18:41 ` Elgin Perumbilly
2026-10-03 8:27 ` armandas.kvietkus
2 siblings, 1 reply; 9+ messages in thread
From: Elgin Perumbilly @ 2026-10-01 18:41 UTC (permalink / raw)
To: armandas.kvietkus@proton.me
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, Sakari Ailus
Hi Armandas,
A few comments from my side.
>From: Armandas Kvietkus <armandas.kvietkus@proton.me>
>
>Add a driver for the Samsung S5K3T2 20 Mpixel CMOS image sensor.
>
>The driver supports three modes. All of them are RAW10 on four D-PHY
>lanes.
>
> 5184x3880 at 15 fps, link frequency 448 MHz
> 2592x1940 at 60 fps, link frequency 448 MHz
> 1280x720 at 120 fps, link frequency 300 MHz
>
>A mode is hidden if the board does not list the link frequency it needs.
>
>The pixel clock does not change between modes, so the pixel rate control
>is constant. The frame rate follows from the line length and the frame
>length.
>
>The controls are exposure, analogue gain, vertical blanking, horizontal
>flip, vertical flip and test pattern. Rotation and orientation come from
>the device tree. Power is handled by runtime PM with autosuspend.
>
>The register tables come from the vendor camera firmware. They were
>checked on hardware.
>
>Tested on a Xiaomi POCO F3 with the Qualcomm CAMSS driver.
>
>Signed-off-by: Armandas Kvietkus <armandas.kvietkus@proton.me>
>---
> MAINTAINERS | 1 +
> drivers/media/i2c/Kconfig | 10 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/s5k3t2.c | 1360 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 1372 insertions(+)
>
>diff --git a/MAINTAINERS b/MAINTAINERS
>index 1924dbd20..e625cf3a2 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -24386,6 +24386,7 @@ L: linux-media@vger.kernel.org
> S: Maintained
> T: git git://linuxtv.org/media_tree.git
> F: Documentation/devicetree/bindings/media/i2c/samsung,s5k3t2.yaml
>+F: drivers/media/i2c/s5k3t2.c
>
> SAMSUNG S5K5BAF CAMERA DRIVER
> M: Sylwester Nawrocki <s.nawrocki@samsung.com>
>diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>index 5c52007f9..999aeecd2 100644
>--- a/drivers/media/i2c/Kconfig
>+++ b/drivers/media/i2c/Kconfig
>@@ -804,6 +804,16 @@ config VIDEO_S5K3M5
> To compile this driver as a module, choose M here: the
> module will be called s5k3m5.
>
>+config VIDEO_S5K3T2
>+ tristate "Samsung S5K3T2 sensor support"
>+ select V4L2_CCI_I2C
>+ help
>+ This is a V4L2 sensor driver for Samsung S5K3T2 20MP raw
>+ camera sensor.
>+
>+ To compile this driver as a module, choose M here: the
>+ module will be called s5k3t2.
>+
> config VIDEO_S5K5BAF
> tristate "Samsung S5K5BAF sensor support"
> help
>diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>index d04bd5724..9a25d8200 100644
>--- a/drivers/media/i2c/Makefile
>+++ b/drivers/media/i2c/Makefile
>@@ -130,6 +130,7 @@ obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o
> obj-$(CONFIG_VIDEO_RJ54N1) += rj54n1cb0c.o
> obj-$(CONFIG_VIDEO_S5C73M3) += s5c73m3/
> obj-$(CONFIG_VIDEO_S5K3M5) += s5k3m5.o
>+obj-$(CONFIG_VIDEO_S5K3T2) += s5k3t2.o
> obj-$(CONFIG_VIDEO_S5K5BAF) += s5k5baf.o
> obj-$(CONFIG_VIDEO_S5K6A3) += s5k6a3.o
> obj-$(CONFIG_VIDEO_S5KJN1) += s5kjn1.o
>diff --git a/drivers/media/i2c/s5k3t2.c b/drivers/media/i2c/s5k3t2.c
>new file mode 100644
>index 000000000..595831217
>--- /dev/null
>+++ b/drivers/media/i2c/s5k3t2.c
>@@ -0,0 +1,1360 @@
>+// SPDX-License-Identifier: GPL-2.0
>+// Copyright (c) 2026 Armandas Kvietkus <armandas.kvietkus@proton.me>
>+
>+#include <linux/clk.h>
>+#include <linux/delay.h>
>+#include <linux/gpio/consumer.h>
>+#include <linux/i2c.h>
>+#include <linux/module.h>
>+#include <linux/pm_runtime.h>
>+#include <linux/regulator/consumer.h>
>+#include <linux/units.h>
>+#include <media/v4l2-cci.h>
>+#include <media/v4l2-ctrls.h>
>+#include <media/v4l2-device.h>
>+#include <media/v4l2-fwnode.h>
>+
>+#define S5K3T2_LINK_FREQ_448MHZ (448 * HZ_PER_MHZ)
>+#define S5K3T2_LINK_FREQ_300MHZ (300 * HZ_PER_MHZ)
>+#define S5K3T2_MCLK_FREQ 19200000
>+#define S5K3T2_PIXEL_RATE 676571429
>+#define S5K3T2_DATA_LANES 4
>+
>+#define S5K3T2_REG_CHIP_ID CCI_REG16(0x0000)
>+#define S5K3T2_CHIP_ID 0x3142
>+
>+#define S5K3T2_REG_CTRL_MODE CCI_REG16(0x0100)
>+#define S5K3T2_MODE_STREAMING BIT(8)
>+#define S5K3T2_VFLIP BIT(1)
>+#define S5K3T2_HFLIP BIT(0)
>+
>+#define S5K3T2_REG_EXPOSURE CCI_REG16(0x0202)
>+#define S5K3T2_EXPOSURE_MIN 8
>+#define S5K3T2_EXPOSURE_STEP 1
>+#define S5K3T2_EXPOSURE_MARGIN 4
>+
>+#define S5K3T2_REG_AGAIN CCI_REG16(0x0204)
>+#define S5K3T2_AGAIN_MIN 1
>+#define S5K3T2_AGAIN_MAX 16
>+#define S5K3T2_AGAIN_STEP 1
>+#define S5K3T2_AGAIN_DEFAULT 1
>+#define S5K3T2_AGAIN_SHIFT 5
>+
>+#define S5K3T2_REG_VTS CCI_REG16(0x0340)
>+#define S5K3T2_VTS_MAX 0xffff
>+
>+#define S5K3T2_REG_TEST_PATTERN CCI_REG16(0x0600)
>+
>+#define S5K3T2_NATIVE_WIDTH 5192
>+#define S5K3T2_NATIVE_HEIGHT 3888
>+
>+#define S5K3T2_DEFAULT_WIDTH 2592
>+#define S5K3T2_DEFAULT_HEIGHT 1940
>+
>+#define to_s5k3t2(_sd) container_of(_sd, struct s5k3t2, sd)
Please use container_of_const() here
>+
>+static const s64 s5k3t2_link_freq_menu[] = {
>+ S5K3T2_LINK_FREQ_448MHZ,
>+ S5K3T2_LINK_FREQ_300MHZ,
>+};
>+
>+static const u32 s5k3t2_mbus_formats[] = {
>+ MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10,
>+ MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10,
>+};
>+
>+struct s5k3t2_reg_list {
>+ const struct cci_reg_sequence *regs;
>+ unsigned int num_regs;
>+};
>+
>+struct s5k3t2_mode {
>+ u32 width;
>+ u32 height;
>+ u32 hts;
>+ u32 vts;
>+ u32 exposure;
>+ unsigned int link_freq_index;
>+ struct v4l2_rect crop;
>+ const struct s5k3t2_reg_list reg_list;
>+};
>+
>+static const char * const s5k3t2_test_pattern_menu[] = {
>+ "Disabled",
>+ "Solid colour",
>+ "Colour bars",
>+ "Fade to grey colour bars",
>+ "PN9",
>+};
>+
>+static const char * const s5k3t2_supply_names[] = {
>+ "vdda",
>+ "vddd",
>+ "vddio",
>+};
>+
>+#define S5K3T2_NUM_SUPPLIES ARRAY_SIZE(s5k3t2_supply_names)
>+
>+struct s5k3t2 {
>+ struct device *dev;
>+ struct regmap *regmap;
>+ struct clk *mclk;
>+ struct gpio_desc *reset_gpio;
>+ struct regulator_bulk_data supplies[S5K3T2_NUM_SUPPLIES];
>+
>+ struct v4l2_subdev sd;
>+ struct media_pad pad;
>+
>+ struct v4l2_ctrl_handler ctrl_handler;
>+ struct v4l2_ctrl *link_freq;
>+ struct v4l2_ctrl *pixel_rate;
Remove the unwanted pointers if they are not referenced,
as they unnecessarily consume memory.
>+ struct v4l2_ctrl *hblank;
>+ struct v4l2_ctrl *vblank;
>+ struct v4l2_ctrl *exposure;
>+ struct v4l2_ctrl *vflip;
>+ struct v4l2_ctrl *hflip;
>+
>+ const struct s5k3t2_mode *mode;
>+ unsigned long link_freq_bitmap;
>+};
>+
>+static const struct cci_reg_sequence s5k3t2_init_setting[] = {
>+ { CCI_REG16(0x6214), 0xff7d },
>+ { CCI_REG16(0x6218), 0x0000 },
>+ { CCI_REG16(0x0a02), 0x003f },
>+ { CCI_REG16(0x6028), 0x2000 },
>+ { CCI_REG16(0x602a), 0x3aec },
>+ { CCI_REG16(0x6f12), 0x0000 },
>+ { CCI_REG16(0x6f12), 0x0000 },
>+ { CCI_REG16(0x6f12), 0x0549 },
>+ { CCI_REG16(0x6f12), 0x0448 },
>+ { CCI_REG16(0x6f12), 0x054a },
>+ { CCI_REG16(0x6f12), 0xc1f8 },
>+ { CCI_REG16(0x6f12), 0x2c05 },
>+ { CCI_REG16(0x6f12), 0x101a },
>+ { CCI_REG16(0x6f12), 0xa1f8 },
>+ { CCI_REG16(0x6f12), 0x3005 },
>+ { CCI_REG16(0x6f12), 0x00f0 },
>+ { CCI_REG16(0x6f12), 0x7bb8 },
>+ { CCI_REG16(0x6f12), 0x2000 },
>+ { CCI_REG16(0x6f12), 0x3ca0 },
>+ { CCI_REG16(0x6f12), 0x2000 },
>+ { CCI_REG16(0x6f12), 0x2670 },
>+ { CCI_REG16(0x6f12), 0x2000 },
>+ { CCI_REG16(0x6f12), 0x9c00 },
>+ { CCI_REG16(0x6f12), 0x70b5 },
>+ { CCI_REG16(0x6f12), 0x0646 },
>+ { CCI_REG16(0x6f12), 0x4348 },
>+ { CCI_REG16(0x6f12), 0x0022 },
>+ { CCI_REG16(0x6f12), 0x0168 },
>+ { CCI_REG16(0x6f12), 0x0c0c },
>+ { CCI_REG16(0x6f12), 0x8db2 },
>+ { CCI_REG16(0x6f12), 0x2946 },
>+ { CCI_REG16(0x6f12), 0x2046 },
>+ { CCI_REG16(0x6f12), 0x00f0 },
>+ { CCI_REG16(0x6f12), 0x9bf8 },
>+ { CCI_REG16(0x6f12), 0x3046 },
>+ { CCI_REG16(0x6f12), 0x00f0 },
>+ { CCI_REG16(0x6f12), 0x9df8 },
>+ { CCI_REG16(0x6f12), 0x3d48 },
>+ { CCI_REG16(0x6f12), 0x3e4a },
>+ { CCI_REG16(0x6f12), 0x0830 },
>+ { CCI_REG16(0x6f12), 0x0188 },
>+ { CCI_REG16(0x6f12), 0x1180 },
>+ { CCI_REG16(0x6f12), 0x911c },
>+ { CCI_REG16(0x6f12), 0x4088 },
>+ { CCI_REG16(0x6f12), 0x0880 },
>+ { CCI_REG16(0x6f12), 0x2946 },
>+ { CCI_REG16(0x6f12), 0x2046 },
>+ { CCI_REG16(0x6f12), 0xbde8 },
>+ { CCI_REG16(0x6f12), 0x7040 },
>+ { CCI_REG16(0x6f12), 0x0122 },
>+ { CCI_REG16(0x6f12), 0x00f0 },
>+ { CCI_REG16(0x6f12), 0x89b8 },
>+ { CCI_REG16(0x6f12), 0x70b5 },
>+ { CCI_REG16(0x6f12), 0x0646 },
>+ { CCI_REG16(0x6f12), 0x3548 },
>+ { CCI_REG16(0x6f12), 0x0022 },
>+ { CCI_REG16(0x6f12), 0x4068 },
>+ { CCI_REG16(0x6f12), 0x84b2 },
>+ { CCI_REG16(0x6f12), 0x050c },
>+ { CCI_REG16(0x6f12), 0x2146 },
>+ { CCI_REG16(0x6f12), 0x2846 },
>+ { CCI_REG16(0x6f12), 0x00f0 },
>+ { CCI_REG16(0x6f12), 0x7ef8 },
>+ { CCI_REG16(0x6f12), 0x3046 },
>+ { CCI_REG16(0x6f12), 0x00f0 },
>+ { CCI_REG16(0x6f12), 0x85f8 },
>+ { CCI_REG16(0x6f12), 0x3149 },
>+ { CCI_REG16(0x6f12), 0x314b },
>+ { CCI_REG16(0x6f12), 0x0120 },
>+ { CCI_REG16(0x6f12), 0x0988 },
>+ { CCI_REG16(0x6f12), 0x40ea },
>+ { CCI_REG16(0x6f12), 0x0110 },
>+ { CCI_REG16(0x6f12), 0x5881 },
>+ { CCI_REG16(0x6f12), 0x2f48 },
>+ { CCI_REG16(0x6f12), 0x0078 },
>+ { CCI_REG16(0x6f12), 0x68b1 },
>+ { CCI_REG16(0x6f12), 0x0022 },
>+ { CCI_REG16(0x6f12), 0x2e48 },
>+ { CCI_REG16(0x6f12), 0x2f49 },
>+ { CCI_REG16(0x6f12), 0x80f8 },
>+ { CCI_REG16(0x6f12), 0xc428 },
>+ { CCI_REG16(0x6f12), 0x0a80 },
>+ { CCI_REG16(0x6f12), 0x2e49 },
>+ { CCI_REG16(0x6f12), 0x0e78 },
>+ { CCI_REG16(0x6f12), 0x36b1 },
>+ { CCI_REG16(0x6f12), 0x90f8 },
>+ { CCI_REG16(0x6f12), 0xe803 },
>+ { CCI_REG16(0x6f12), 0x18b1 },
>+ { CCI_REG16(0x6f12), 0x0120 },
>+ { CCI_REG16(0x6f12), 0x02e0 },
>+ { CCI_REG16(0x6f12), 0x0122 },
>+ { CCI_REG16(0x6f12), 0xf0e7 },
>+ { CCI_REG16(0x6f12), 0x0020 },
>+ { CCI_REG16(0x6f12), 0x4978 },
>+ { CCI_REG16(0x6f12), 0x01b1 },
>+ { CCI_REG16(0x6f12), 0x42b1 },
>+ { CCI_REG16(0x6f12), 0x0021 },
>+ { CCI_REG16(0x6f12), 0x40ea },
>+ { CCI_REG16(0x6f12), 0x0110 },
>+ { CCI_REG16(0x6f12), 0x5880 },
>+ { CCI_REG16(0x6f12), 0x00f0 },
>+ { CCI_REG16(0x6f12), 0x66f8 },
>+ { CCI_REG16(0x6f12), 0x0128 },
>+ { CCI_REG16(0x6f12), 0x02d0 },
>+ { CCI_REG16(0x6f12), 0x1be0 },
>+ { CCI_REG16(0x6f12), 0x0121 },
>+ { CCI_REG16(0x6f12), 0xf5e7 },
>+ { CCI_REG16(0x6f12), 0x2248 },
>+ { CCI_REG16(0x6f12), 0x234a },
>+ { CCI_REG16(0x6f12), 0x234e },
>+ { CCI_REG16(0x6f12), 0xb0f8 },
>+ { CCI_REG16(0x6f12), 0x7211 },
>+ { CCI_REG16(0x6f12), 0x92f8 },
>+ { CCI_REG16(0x6f12), 0x9420 },
>+ { CCI_REG16(0x6f12), 0x90f8 },
>+ { CCI_REG16(0x6f12), 0x7401 },
>+ { CCI_REG16(0x6f12), 0xd140 },
>+ { CCI_REG16(0x6f12), 0xd040 },
>+ { CCI_REG16(0x6f12), 0x4318 },
>+ { CCI_REG16(0x6f12), 0x96f8 },
>+ { CCI_REG16(0x6f12), 0x7f63 },
>+ { CCI_REG16(0x6f12), 0x581e },
>+ { CCI_REG16(0x6f12), 0x022e },
>+ { CCI_REG16(0x6f12), 0x03d9 },
>+ { CCI_REG16(0x6f12), 0x0220 },
>+ { CCI_REG16(0x6f12), 0x9040 },
>+ { CCI_REG16(0x6f12), 0x181a },
>+ { CCI_REG16(0x6f12), 0x401c },
>+ { CCI_REG16(0x6f12), 0x164a },
>+ { CCI_REG16(0x6f12), 0x703a },
>+ { CCI_REG16(0x6f12), 0x1180 },
>+ { CCI_REG16(0x6f12), 0x911c },
>+ { CCI_REG16(0x6f12), 0x0880 },
>+ { CCI_REG16(0x6f12), 0x2146 },
>+ { CCI_REG16(0x6f12), 0x2846 },
>+ { CCI_REG16(0x6f12), 0xbde8 },
>+ { CCI_REG16(0x6f12), 0x7040 },
>+ { CCI_REG16(0x6f12), 0x0122 },
>+ { CCI_REG16(0x6f12), 0x00f0 },
>+ { CCI_REG16(0x6f12), 0x31b8 },
>+ { CCI_REG16(0x6f12), 0x10b5 },
>+ { CCI_REG16(0x6f12), 0x0022 },
>+ { CCI_REG16(0x6f12), 0xaff2 },
>+ { CCI_REG16(0x6f12), 0xb501 },
>+ { CCI_REG16(0x6f12), 0x1348 },
>+ { CCI_REG16(0x6f12), 0x00f0 },
>+ { CCI_REG16(0x6f12), 0x3ef8 },
>+ { CCI_REG16(0x6f12), 0x064c },
>+ { CCI_REG16(0x6f12), 0x0022 },
>+ { CCI_REG16(0x6f12), 0xaff2 },
>+ { CCI_REG16(0x6f12), 0xff01 },
>+ { CCI_REG16(0x6f12), 0x6060 },
>+ { CCI_REG16(0x6f12), 0x1048 },
>+ { CCI_REG16(0x6f12), 0x00f0 },
>+ { CCI_REG16(0x6f12), 0x36f8 },
>+ { CCI_REG16(0x6f12), 0x0f49 },
>+ { CCI_REG16(0x6f12), 0x2060 },
>+ { CCI_REG16(0x6f12), 0x7a20 },
>+ { CCI_REG16(0x6f12), 0x0968 },
>+ { CCI_REG16(0x6f12), 0x4883 },
>+ { CCI_REG16(0x6f12), 0x10bd },
>+ { CCI_REG16(0x6f12), 0x2000 },
>+ { CCI_REG16(0x6f12), 0x3c90 },
>+ { CCI_REG16(0x6f12), 0x4000 },
>+ { CCI_REG16(0x6f12), 0x950c },
>+ { CCI_REG16(0x6f12), 0x2000 },
>+ { CCI_REG16(0x6f12), 0x16f0 },
>+ { CCI_REG16(0x6f12), 0x4000 },
>+ { CCI_REG16(0x6f12), 0xd000 },
>+ { CCI_REG16(0x6f12), 0x2000 },
>+ { CCI_REG16(0x6f12), 0x19a0 },
>+ { CCI_REG16(0x6f12), 0x2000 },
>+ { CCI_REG16(0x6f12), 0x30c0 },
>+ { CCI_REG16(0x6f12), 0x4000 },
>+ { CCI_REG16(0x6f12), 0x9800 },
>+ { CCI_REG16(0x6f12), 0x2000 },
>+ { CCI_REG16(0x6f12), 0x1dd0 },
>+ { CCI_REG16(0x6f12), 0x2000 },
>+ { CCI_REG16(0x6f12), 0x17c0 },
>+ { CCI_REG16(0x6f12), 0x2000 },
>+ { CCI_REG16(0x6f12), 0x2210 },
>+ { CCI_REG16(0x6f12), 0x2000 },
>+ { CCI_REG16(0x6f12), 0x2670 },
>+ { CCI_REG16(0x6f12), 0x0000 },
>+ { CCI_REG16(0x6f12), 0xf45f },
>+ { CCI_REG16(0x6f12), 0x0000 },
>+ { CCI_REG16(0x6f12), 0xd957 },
>+ { CCI_REG16(0x6f12), 0x2000 },
>+ { CCI_REG16(0x6f12), 0x08c0 },
>+ { CCI_REG16(0x6f12), 0x49f6 },
>+ { CCI_REG16(0x6f12), 0x213c },
>+ { CCI_REG16(0x6f12), 0xc0f2 },
>+ { CCI_REG16(0x6f12), 0x000c },
>+ { CCI_REG16(0x6f12), 0x6047 },
>+ { CCI_REG16(0x6f12), 0x4df6 },
>+ { CCI_REG16(0x6f12), 0x571c },
>+ { CCI_REG16(0x6f12), 0xc0f2 },
>+ { CCI_REG16(0x6f12), 0x000c },
>+ { CCI_REG16(0x6f12), 0x6047 },
>+ { CCI_REG16(0x6f12), 0x4ff2 },
>+ { CCI_REG16(0x6f12), 0x5f4c },
>+ { CCI_REG16(0x6f12), 0xc0f2 },
>+ { CCI_REG16(0x6f12), 0x000c },
>+ { CCI_REG16(0x6f12), 0x6047 },
>+ { CCI_REG16(0x6f12), 0x44f2 },
>+ { CCI_REG16(0x6f12), 0x9b0c },
>+ { CCI_REG16(0x6f12), 0xc0f2 },
>+ { CCI_REG16(0x6f12), 0x000c },
>+ { CCI_REG16(0x6f12), 0x6047 },
>+ { CCI_REG16(0x6f12), 0x4bf2 },
>+ { CCI_REG16(0x6f12), 0xed0c },
>+ { CCI_REG16(0x6f12), 0xc0f2 },
>+ { CCI_REG16(0x6f12), 0x000c },
>+ { CCI_REG16(0x6f12), 0x6047 },
>+ { CCI_REG16(0x6f12), 0x0000 },
>+ { CCI_REG16(0x6028), 0x2000 },
>+ { CCI_REG16(0x602a), 0x1014 },
>+ { CCI_REG16(0x6f12), 0x0100 },
>+ { CCI_REG16(0x6f12), 0x7000 },
>+ { CCI_REG16(0x602a), 0x108a },
>+ { CCI_REG16(0x6f12), 0x0006 },
>+ { CCI_REG16(0x602a), 0x1092 },
>+ { CCI_REG16(0x6f12), 0x0005 },
>+ { CCI_REG16(0x602a), 0x1096 },
>+ { CCI_REG16(0x6f12), 0x0002 },
>+ { CCI_REG16(0x6f12), 0x001a },
>+ { CCI_REG16(0x602a), 0x109c },
>+ { CCI_REG16(0x6f12), 0x0014 },
>+ { CCI_REG16(0x602a), 0x10a2 },
>+ { CCI_REG16(0x6f12), 0x0022 },
>+ { CCI_REG16(0x602a), 0x10ae },
>+ { CCI_REG16(0x6f12), 0x0007 },
>+ { CCI_REG16(0x602a), 0x10c2 },
>+ { CCI_REG16(0x6f12), 0x001e },
>+ { CCI_REG16(0x602a), 0x10f4 },
>+ { CCI_REG16(0x6f12), 0x0003 },
>+ { CCI_REG16(0x6f12), 0x0003 },
>+ { CCI_REG16(0x602a), 0x110a },
>+ { CCI_REG16(0x6f12), 0x0000 },
>+ { CCI_REG16(0x602a), 0x113e },
>+ { CCI_REG16(0x6f12), 0x0001 },
>+ { CCI_REG16(0x6f12), 0x014e },
>+ { CCI_REG16(0x602a), 0x13ea },
>+ { CCI_REG16(0x6f12), 0x160f },
>+ { CCI_REG16(0x6f12), 0x0d00 },
>+ { CCI_REG16(0x602a), 0x13fa },
>+ { CCI_REG16(0x6f12), 0x009d },
>+ { CCI_REG16(0x6f12), 0x0107 },
>+ { CCI_REG16(0x602a), 0x14e2 },
>+ { CCI_REG16(0x6f12), 0x04c2 },
>+ { CCI_REG16(0x6f12), 0x02ae },
>+ { CCI_REG16(0x6f12), 0x0020 },
>+ { CCI_REG16(0x6028), 0x4000 },
>+ { CCI_REG16(0xf44a), 0x0010 },
>+ { CCI_REG16(0xf46a), 0xb6a0 },
>+ { CCI_REG16(0x6028), 0x2000 },
>+ { CCI_REG16(0x602a), 0x0f5e },
>+ { CCI_REG16(0x6f12), 0x0200 },
>+ { CCI_REG16(0x602a), 0x0f90 },
>+ { CCI_REG16(0x6f12), 0x0000 },
>+ { CCI_REG16(0x602a), 0x17c0 },
>+ { CCI_REG16(0x6f12), 0x0010 },
>+ { CCI_REG16(0x6f12), 0x0201 },
>+ { CCI_REG16(0x602a), 0x1da2 },
>+ { CCI_REG16(0x6f12), 0x0001 },
>+ { CCI_REG16(0x6f12), 0x0203 },
>+ { CCI_REG16(0x6f12), 0x0405 },
>+ { CCI_REG16(0x6f12), 0x0607 },
>+ { CCI_REG16(0x6f12), 0x0809 },
>+ { CCI_REG16(0x6f12), 0x0a0b },
>+ { CCI_REG16(0x6f12), 0x0c0d },
>+ { CCI_REG16(0x6f12), 0x0e0f },
>+ { CCI_REG16(0x6028), 0x4000 },
>+ { CCI_REG16(0x020c), 0x0001 },
>+ { CCI_REG16(0x0bc6), 0x0000 },
>+ { CCI_REG16(0x0d00), 0x0000 },
>+ { CCI_REG16(0xb13c), 0x0800 },
>+};
>+
Curious why 0x6f12 and 0x602a is used repeatedly in sequence here,
unlike in other drivers, Could you explain it.
>+static const struct cci_reg_sequence s5k3t2_5184x3880_mode[] = {
>+ { CCI_REG16(0x6028), 0x4000 },
>+ { CCI_REG16(0x0136), 0x1300 },
>+ { CCI_REG16(0x013e), 0x00c8 },
>+ { CCI_REG16(0x0304), 0x0003 },
>+ { CCI_REG16(0x0306), 0x00b9 },
>+ { CCI_REG16(0x6028), 0x2000 },
>+ { CCI_REG16(0x602a), 0x0e0e },
>+ { CCI_REG16(0x6f12), 0x0103 },
>+ { CCI_REG16(0x6f12), 0x0000 },
>+ { CCI_REG16(0x602a), 0x1078 },
>+ { CCI_REG16(0x6f12), 0x04a6 },
>+ { CCI_REG16(0x6028), 0x4000 },
>+ { CCI_REG16(0x030c), 0x0000 },
>+ { CCI_REG16(0x0302), 0x0001 },
>+ { CCI_REG16(0x0300), 0x0007 },
>+ { CCI_REG16(0x030e), 0x0003 },
>+ { CCI_REG16(0x0310), 0x008c },
>+ { CCI_REG16(0x0312), 0x0001 },
>+ { CCI_REG16(0x0308), 0x0008 },
>+ { CCI_REG16(0x030a), 0x0001 },
>+ { CCI_REG16(0x0344), 0x0008 },
>+ { CCI_REG16(0x0346), 0x0008 },
>+ { CCI_REG16(0x0348), 0x1447 },
>+ { CCI_REG16(0x034a), 0x0f2f },
>+ { CCI_REG16(0x034c), 0x1440 },
>+ { CCI_REG16(0x034e), 0x0f28 },
>+ { CCI_REG16(0x0350), 0x0000 },
>+ { CCI_REG16(0x0352), 0x0000 },
>+ { CCI_REG16(0x0900), 0x0111 },
>+ { CCI_REG16(0x0404), 0x1000 },
>+ { CCI_REG16(0x0380), 0x0001 },
>+ { CCI_REG16(0x0382), 0x0001 },
>+ { CCI_REG16(0x0384), 0x0001 },
>+ { CCI_REG16(0x0386), 0x0001 },
>+ { CCI_REG16(0x0342), 0x2a80 },
>+ { CCI_REG16(0x0340), 0x1000 },
No need to define VTS here as already been set from vertical blank control
>+ { CCI_REG16(0x6028), 0x4000 },
>+ { CCI_REG16(0x010c), 0x0000 },
>+ { CCI_REG16(0x0114), 0x0300 },
>+ { CCI_REG16(0x0116), 0x3000 },
>+ { CCI_REG16(0x011a), 0x0001 },
>+ { CCI_REG16(0x0118), 0x0002 },
>+ { CCI_REG16(0x6028), 0x2000 },
>+ { CCI_REG16(0x602a), 0x0e92 },
>+ { CCI_REG16(0x6f12), 0xffff },
>+ { CCI_REG16(0x602a), 0x165a },
>+ { CCI_REG16(0x6f12), 0x0003 },
>+ { CCI_REG16(0x6028), 0x4000 },
>+ { CCI_REG16(0x0b04), 0x0001 },
>+ { CCI_REG16(0x0b06), 0x0101 },
>+ { CCI_REG16(0x0fea), 0x04a0 },
>+ { CCI_REG16(0x6028), 0x2000 },
>+ { CCI_REG16(0x602a), 0x3c98 },
>+ { CCI_REG16(0x6f12), 0x0a58 },
>+ { CCI_REG16(0x6f12), 0x1477 },
>+ { CCI_REG16(0x602a), 0x1da0 },
>+ { CCI_REG16(0x6f12), 0x0010 },
>+ { CCI_REG16(0x602a), 0x10ac },
>+ { CCI_REG16(0x6f12), 0x000a },
>+ { CCI_REG16(0x602a), 0x1110 },
>+ { CCI_REG16(0x6f12), 0x001d },
>+ { CCI_REG16(0x6f12), 0x003f },
>+ { CCI_REG16(0x602a), 0x13e8 },
>+ { CCI_REG16(0x6f12), 0x0804 },
>+ { CCI_REG16(0x602a), 0x13f8 },
>+ { CCI_REG16(0x6f12), 0x38c8 },
>+};
>+
>+static const struct cci_reg_sequence s5k3t2_2592x1940_mode[] = {
>+ { CCI_REG16(0x6028), 0x4000 },
>+ { CCI_REG16(0x0136), 0x1300 },
>+ { CCI_REG16(0x013e), 0x00c8 },
>+ { CCI_REG16(0x0304), 0x0003 },
>+ { CCI_REG16(0x0306), 0x00b9 },
>+ { CCI_REG16(0x6028), 0x2000 },
>+ { CCI_REG16(0x602a), 0x0e0e },
>+ { CCI_REG16(0x6f12), 0x0103 },
>+ { CCI_REG16(0x6f12), 0x0000 },
>+ { CCI_REG16(0x602a), 0x1078 },
>+ { CCI_REG16(0x6f12), 0x04a6 },
>+ { CCI_REG16(0x6028), 0x4000 },
>+ { CCI_REG16(0x030c), 0x0000 },
>+ { CCI_REG16(0x0302), 0x0001 },
>+ { CCI_REG16(0x0300), 0x0007 },
>+ { CCI_REG16(0x030e), 0x0003 },
>+ { CCI_REG16(0x0310), 0x008c },
>+ { CCI_REG16(0x0312), 0x0001 },
>+ { CCI_REG16(0x0308), 0x0008 },
>+ { CCI_REG16(0x030a), 0x0001 },
>+ { CCI_REG16(0x0344), 0x0008 },
>+ { CCI_REG16(0x0346), 0x0008 },
>+ { CCI_REG16(0x0348), 0x1447 },
>+ { CCI_REG16(0x034a), 0x0f2f },
>+ { CCI_REG16(0x034c), 0x0a20 },
>+ { CCI_REG16(0x034e), 0x0794 },
>+ { CCI_REG16(0x0350), 0x0000 },
>+ { CCI_REG16(0x0352), 0x0000 },
>+ { CCI_REG16(0x0900), 0x0122 },
>+ { CCI_REG16(0x0404), 0x1000 },
>+ { CCI_REG16(0x0380), 0x0002 },
>+ { CCI_REG16(0x0382), 0x0002 },
>+ { CCI_REG16(0x0384), 0x0002 },
>+ { CCI_REG16(0x0386), 0x0002 },
>+ { CCI_REG16(0x0342), 0x1610 },
>+ { CCI_REG16(0x0340), 0x07cc },
No need to define VTS here as already been set from vertical blank control
>+ { CCI_REG16(0x6028), 0x4000 },
>+ { CCI_REG16(0x010c), 0x0000 },
>+ { CCI_REG16(0x0114), 0x0300 },
>+ { CCI_REG16(0x0116), 0x3000 },
>+ { CCI_REG16(0x011a), 0x0001 },
>+ { CCI_REG16(0x0118), 0x0002 },
>+ { CCI_REG16(0x6028), 0x2000 },
>+ { CCI_REG16(0x602a), 0x0e92 },
>+ { CCI_REG16(0x6f12), 0xffff },
>+ { CCI_REG16(0x602a), 0x165a },
>+ { CCI_REG16(0x6f12), 0x0003 },
>+ { CCI_REG16(0x6028), 0x4000 },
>+ { CCI_REG16(0x0b04), 0x0001 },
>+ { CCI_REG16(0x0b06), 0x0101 },
>+ { CCI_REG16(0x0fea), 0x04a0 },
>+ { CCI_REG16(0x6028), 0x2000 },
>+ { CCI_REG16(0x602a), 0x3c98 },
>+ { CCI_REG16(0x6f12), 0x052c },
>+ { CCI_REG16(0x6f12), 0x0a3b },
>+ { CCI_REG16(0x602a), 0x1da0 },
>+ { CCI_REG16(0x6f12), 0x0010 },
>+ { CCI_REG16(0x602a), 0x10ac },
>+ { CCI_REG16(0x6f12), 0x0014 },
>+ { CCI_REG16(0x602a), 0x1110 },
>+ { CCI_REG16(0x6f12), 0x001d },
>+ { CCI_REG16(0x6f12), 0x004d },
>+ { CCI_REG16(0x602a), 0x13e8 },
>+ { CCI_REG16(0x6f12), 0x080f },
>+ { CCI_REG16(0x602a), 0x13f8 },
>+ { CCI_REG16(0x6f12), 0x38c8 },
>+};
>+
>+static const struct cci_reg_sequence s5k3t2_1280x720_mode[] = {
>+ { CCI_REG16(0x6028), 0x4000 },
>+ { CCI_REG16(0x0136), 0x1300 },
>+ { CCI_REG16(0x013e), 0x00c8 },
>+ { CCI_REG16(0x0304), 0x0003 },
>+ { CCI_REG16(0x0306), 0x00b9 },
>+ { CCI_REG16(0x6028), 0x2000 },
>+ { CCI_REG16(0x602a), 0x0e0e },
>+ { CCI_REG16(0x6f12), 0x0104 },
>+ { CCI_REG16(0x6f12), 0x0000 },
>+ { CCI_REG16(0x602a), 0x1078 },
>+ { CCI_REG16(0x6f12), 0x04a6 },
>+ { CCI_REG16(0x6028), 0x4000 },
>+ { CCI_REG16(0x030c), 0x0000 },
>+ { CCI_REG16(0x0302), 0x0001 },
>+ { CCI_REG16(0x0300), 0x0007 },
>+ { CCI_REG16(0x030e), 0x0002 },
>+ { CCI_REG16(0x0310), 0x007d },
>+ { CCI_REG16(0x0312), 0x0002 },
>+ { CCI_REG16(0x0308), 0x0008 },
>+ { CCI_REG16(0x030a), 0x0001 },
>+ { CCI_REG16(0x0344), 0x0028 },
>+ { CCI_REG16(0x0346), 0x01fc },
>+ { CCI_REG16(0x0348), 0x1427 },
>+ { CCI_REG16(0x034a), 0x0d3b },
>+ { CCI_REG16(0x034c), 0x0500 },
>+ { CCI_REG16(0x034e), 0x02d0 },
>+ { CCI_REG16(0x0350), 0x0000 },
>+ { CCI_REG16(0x0352), 0x0000 },
>+ { CCI_REG16(0x0900), 0x0124 },
>+ { CCI_REG16(0x0404), 0x2000 },
>+ { CCI_REG16(0x0380), 0x0002 },
>+ { CCI_REG16(0x0382), 0x0002 },
>+ { CCI_REG16(0x0384), 0x0002 },
>+ { CCI_REG16(0x0386), 0x0006 },
>+ { CCI_REG16(0x0342), 0x1620 },
>+ { CCI_REG16(0x0340), 0x03e0 },
No need to define VTS here as already been set from vertical blank control
>+ { CCI_REG16(0x6028), 0x4000 },
>+ { CCI_REG16(0x010c), 0x0000 },
>+ { CCI_REG16(0x0114), 0x0300 },
>+ { CCI_REG16(0x0116), 0x3000 },
>+ { CCI_REG16(0x011a), 0x0001 },
>+ { CCI_REG16(0x0118), 0x0002 },
>+ { CCI_REG16(0x6028), 0x2000 },
>+ { CCI_REG16(0x602a), 0x0e92 },
>+ { CCI_REG16(0x6f12), 0xffff },
>+ { CCI_REG16(0x602a), 0x165a },
>+ { CCI_REG16(0x6f12), 0x0020 },
>+ { CCI_REG16(0x6028), 0x4000 },
>+ { CCI_REG16(0x0b04), 0x0001 },
>+ { CCI_REG16(0x0b06), 0x0101 },
>+ { CCI_REG16(0x0fea), 0x04a0 },
>+ { CCI_REG16(0x6028), 0x2000 },
>+ { CCI_REG16(0x602a), 0x3c98 },
>+ { CCI_REG16(0x6f12), 0x0296 },
>+ { CCI_REG16(0x6f12), 0x0515 },
>+ { CCI_REG16(0x602a), 0x1da0 },
>+ { CCI_REG16(0x6f12), 0x0110 },
>+ { CCI_REG16(0x602a), 0x10ac },
>+ { CCI_REG16(0x6f12), 0x0014 },
>+ { CCI_REG16(0x602a), 0x1110 },
>+ { CCI_REG16(0x6f12), 0x001d },
>+ { CCI_REG16(0x6f12), 0x004d },
>+ { CCI_REG16(0x602a), 0x13e8 },
>+ { CCI_REG16(0x6f12), 0x080f },
>+ { CCI_REG16(0x602a), 0x13f8 },
>+ { CCI_REG16(0x6f12), 0x38c8 },
>+};
>+
>+static const struct s5k3t2_mode s5k3t2_supported_modes[] = {
>+ {
>+ .width = 5184,
>+ .height = 3880,
>+ .hts = 10880,
>+ .vts = 4096,
>+ .exposure = 1000,
>+ .link_freq_index = 0,
>+ .crop = {
>+ .left = 8,
>+ .top = 8,
>+ .width = 5184,
>+ .height = 3880,
>+ },
>+ .reg_list = {
>+ .regs = s5k3t2_5184x3880_mode,
>+ .num_regs = ARRAY_SIZE(s5k3t2_5184x3880_mode),
>+ },
>+ },
>+ {
>+ .width = 2592,
>+ .height = 1940,
>+ .hts = 5648,
>+ .vts = 1996,
>+ .exposure = 1000,
>+ .link_freq_index = 0,
>+ .crop = {
>+ .left = 8,
>+ .top = 8,
>+ .width = 5184,
>+ .height = 3880,
>+ },
>+ .reg_list = {
>+ .regs = s5k3t2_2592x1940_mode,
>+ .num_regs = ARRAY_SIZE(s5k3t2_2592x1940_mode),
>+ },
>+ },
>+ {
>+ .width = 1280,
>+ .height = 720,
>+ .hts = 5664,
>+ .vts = 992,
>+ .exposure = 500,
>+ .link_freq_index = 1,
>+ .crop = {
>+ .left = 40,
>+ .top = 508,
>+ .width = 5120,
>+ .height = 2880,
>+ },
>+ .reg_list = {
>+ .regs = s5k3t2_1280x720_mode,
>+ .num_regs = ARRAY_SIZE(s5k3t2_1280x720_mode),
>+ },
>+ },
>+};
>+
>+static int s5k3t2_set_ctrl(struct v4l2_ctrl *ctrl)
>+{
>+ struct s5k3t2 *s5k3t2 = container_of(ctrl->handler, struct s5k3t2,
>+ ctrl_handler);
Please use container_of_const() here.
>+ const struct s5k3t2_mode *mode = s5k3t2->mode;
>+ s64 exposure_max;
>+ int ret;
>+
>+ switch (ctrl->id) {
>+ case V4L2_CID_HFLIP:
>+ case V4L2_CID_VFLIP:
>+ case V4L2_CID_LINK_FREQ:
>+ case V4L2_CID_PIXEL_RATE:
>+ case V4L2_CID_HBLANK:
>+ return 0;
>+ case V4L2_CID_VBLANK:
>+ exposure_max = mode->height + ctrl->val - S5K3T2_EXPOSURE_MARGIN;
>+ __v4l2_ctrl_modify_range(s5k3t2->exposure,
>+ s5k3t2->exposure->minimum,
>+ exposure_max,
>+ s5k3t2->exposure->step,
>+ s5k3t2->exposure->default_value);
Please add error checking for the return value of __v4l2_ctrl_modify_range().
>+ break;
>+ }
>+
>+ if (!pm_runtime_get_if_active(s5k3t2->dev))
>+ return 0;
>+
>+ switch (ctrl->id) {
>+ case V4L2_CID_ANALOGUE_GAIN:
>+ ret = cci_write(s5k3t2->regmap, S5K3T2_REG_AGAIN,
>+ ctrl->val << S5K3T2_AGAIN_SHIFT, NULL);
>+ break;
>+ case V4L2_CID_EXPOSURE:
>+ ret = cci_write(s5k3t2->regmap, S5K3T2_REG_EXPOSURE,
>+ ctrl->val, NULL);
>+ break;
>+ case V4L2_CID_VBLANK:
>+ ret = cci_write(s5k3t2->regmap, S5K3T2_REG_VTS,
>+ ctrl->val + mode->height, NULL);
>+ break;
>+ case V4L2_CID_TEST_PATTERN:
>+ ret = cci_write(s5k3t2->regmap, S5K3T2_REG_TEST_PATTERN,
>+ ctrl->val, NULL);
>+ break;
>+ default:
>+ ret = -EINVAL;
>+ break;
>+ }
>+
>+ pm_runtime_put(s5k3t2->dev);
>+
>+ return ret;
>+}
>+
>+static const struct v4l2_ctrl_ops s5k3t2_ctrl_ops = {
>+ .s_ctrl = s5k3t2_set_ctrl,
>+};
>+
>+static int s5k3t2_init_controls(struct s5k3t2 *s5k3t2)
>+{
>+ struct v4l2_ctrl_handler *ctrl_hdlr = &s5k3t2->ctrl_handler;
>+ const struct s5k3t2_mode *mode = s5k3t2->mode;
>+ s64 hblank, vblank, exposure_max;
>+ struct v4l2_fwnode_device_properties props;
>+ int ret;
>+
Please align as reverse christmas tree order
>+ v4l2_ctrl_handler_init(ctrl_hdlr, 11);
>+
>+ s5k3t2->link_freq =
>+ v4l2_ctrl_new_int_menu(ctrl_hdlr, &s5k3t2_ctrl_ops,
>+ V4L2_CID_LINK_FREQ,
>+ ARRAY_SIZE(s5k3t2_link_freq_menu) - 1,
>+ mode->link_freq_index,
>+ s5k3t2_link_freq_menu);
>+ if (s5k3t2->link_freq)
>+ s5k3t2->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>+
>+ s5k3t2->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
>+ V4L2_CID_PIXEL_RATE,
>+ S5K3T2_PIXEL_RATE,
>+ S5K3T2_PIXEL_RATE, 1,
>+ S5K3T2_PIXEL_RATE);
>+
>+ hblank = mode->hts - mode->width;
>+ s5k3t2->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
>+ V4L2_CID_HBLANK, hblank,
>+ hblank, 1, hblank);
>+ if (s5k3t2->hblank)
>+ s5k3t2->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>+
>+ vblank = mode->vts - mode->height;
>+ s5k3t2->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
>+ V4L2_CID_VBLANK, vblank,
>+ S5K3T2_VTS_MAX - mode->height, 1,
>+ vblank);
>+
>+ v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
>+ S5K3T2_AGAIN_MIN, S5K3T2_AGAIN_MAX,
>+ S5K3T2_AGAIN_STEP, S5K3T2_AGAIN_DEFAULT);
>+
>+ exposure_max = mode->vts - S5K3T2_EXPOSURE_MARGIN;
>+ s5k3t2->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
>+ V4L2_CID_EXPOSURE,
>+ S5K3T2_EXPOSURE_MIN,
>+ exposure_max,
>+ S5K3T2_EXPOSURE_STEP,
>+ mode->exposure);
>+
>+ v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &s5k3t2_ctrl_ops,
>+ V4L2_CID_TEST_PATTERN,
>+ ARRAY_SIZE(s5k3t2_test_pattern_menu) - 1,
>+ 0, 0, s5k3t2_test_pattern_menu);
>+
>+ s5k3t2->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
>+ V4L2_CID_HFLIP, 0, 1, 1, 0);
>+ if (s5k3t2->hflip)
>+ s5k3t2->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>+
>+ s5k3t2->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &s5k3t2_ctrl_ops,
>+ V4L2_CID_VFLIP, 0, 1, 1, 0);
>+ if (s5k3t2->vflip)
>+ s5k3t2->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>+
>+ ret = v4l2_fwnode_device_parse(s5k3t2->dev, &props);
>+ if (ret)
>+ goto error_free_hdlr;
>+
>+ ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &s5k3t2_ctrl_ops,
>+ &props);
>+ if (ret)
>+ goto error_free_hdlr;
>+
>+ if (ctrl_hdlr->error) {
>+ ret = ctrl_hdlr->error;
>+ goto error_free_hdlr;
>+ }
>+
>+ s5k3t2->sd.ctrl_handler = ctrl_hdlr;
>+
>+ return 0;
>+
>+error_free_hdlr:
>+ v4l2_ctrl_handler_free(ctrl_hdlr);
>+
>+ return ret;
>+}
>+
>+static int s5k3t2_enable_streams(struct v4l2_subdev *sd,
>+ struct v4l2_subdev_state *state, u32 pad,
>+ u64 streams_mask)
>+{
>+ struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
>+ const struct s5k3t2_reg_list *reg_list = &s5k3t2->mode->reg_list;
>+ int ret;
>+
Please align as reverse christmas tree order
>+ ret = pm_runtime_resume_and_get(s5k3t2->dev);
>+ if (ret)
>+ return ret;
>+
>+ cci_write(s5k3t2->regmap, CCI_REG16(0x6028), 0x4000, &ret);
Could you explain what this register CCI_REG16(0x6028) actually does?
>+ cci_write(s5k3t2->regmap, CCI_REG16(0x0000), 0x0005, &ret);
>+ cci_write(s5k3t2->regmap, CCI_REG16(0x0000), S5K3T2_CHIP_ID, &ret);
It seems unusual to write the values to S5K3T2_REG_CHIP_ID (0x0000),
The chip ID register is generally read-only.
>+ cci_write(s5k3t2->regmap, CCI_REG16(0x6010), 0x0001, &ret);
>+ if (ret)
>+ goto error;
>+
>+ usleep_range(5 * USEC_PER_MSEC, 6 * USEC_PER_MSEC);
>+
>+ cci_multi_reg_write(s5k3t2->regmap, s5k3t2_init_setting,
>+ ARRAY_SIZE(s5k3t2_init_setting), &ret);
>+ cci_multi_reg_write(s5k3t2->regmap, reg_list->regs,
>+ reg_list->num_regs, &ret);
>+ if (ret)
>+ goto error;
>+
>+ ret = __v4l2_ctrl_handler_setup(s5k3t2->sd.ctrl_handler);
>+
>+ cci_write(s5k3t2->regmap, S5K3T2_REG_CTRL_MODE,
>+ S5K3T2_MODE_STREAMING |
>+ (s5k3t2->vflip->val ? S5K3T2_VFLIP : 0) |
>+ (s5k3t2->hflip->val ? S5K3T2_HFLIP : 0), &ret);
>+ if (ret)
>+ goto error;
>+
>+ __v4l2_ctrl_grab(s5k3t2->vflip, true);
>+ __v4l2_ctrl_grab(s5k3t2->hflip, true);
>+
>+ return 0;
>+
>+error:
>+ dev_err(s5k3t2->dev, "failed to start streaming: %d\n", ret);
>+ pm_runtime_put_autosuspend(s5k3t2->dev);
>+
>+ return ret;
>+}
>+
>+static int s5k3t2_disable_streams(struct v4l2_subdev *sd,
>+ struct v4l2_subdev_state *state, u32 pad,
>+ u64 streams_mask)
>+{
>+ struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
>+ int ret;
>+
>+ ret = cci_write(s5k3t2->regmap, S5K3T2_REG_CTRL_MODE, 0, NULL);
>+ if (ret)
>+ dev_err(s5k3t2->dev, "failed to stop streaming: %d\n", ret);
>+
>+ __v4l2_ctrl_grab(s5k3t2->vflip, false);
>+ __v4l2_ctrl_grab(s5k3t2->hflip, false);
>+
>+ pm_runtime_put_autosuspend(s5k3t2->dev);
>+
>+ return ret;
>+}
>+
>+static u32 s5k3t2_get_format_code(struct s5k3t2 *s5k3t2)
>+{
>+ unsigned int i;
>+
>+ i = (s5k3t2->vflip->val ? 2 : 0) | (s5k3t2->hflip->val ? 1 : 0);
>+
>+ return s5k3t2_mbus_formats[i];
>+}
>+
>+static void s5k3t2_update_pad_format(struct s5k3t2 *s5k3t2,
>+ const struct s5k3t2_mode *mode,
>+ struct v4l2_mbus_framefmt *fmt)
>+{
>+ fmt->code = s5k3t2_get_format_code(s5k3t2);
>+ fmt->width = mode->width;
>+ fmt->height = mode->height;
>+ fmt->field = V4L2_FIELD_NONE;
>+ fmt->colorspace = V4L2_COLORSPACE_RAW;
>+ fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>+ fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>+ fmt->xfer_func = V4L2_XFER_FUNC_NONE;
>+}
>+
>+static bool s5k3t2_filter_by_link_freq(const void *array, size_t index,
>+ const void *context)
>+{
>+ const struct s5k3t2_mode *mode = array;
>+ const struct s5k3t2 *s5k3t2 = context;
>+
>+ return s5k3t2->link_freq_bitmap & BIT(mode->link_freq_index);
>+}
>+
>+static int s5k3t2_set_pad_format(struct v4l2_subdev *sd,
>+ struct v4l2_subdev_state *state,
>+ struct v4l2_subdev_format *fmt)
>+{
>+ struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
>+ s64 hblank, vblank, exposure_max;
>+ const struct s5k3t2_mode *mode;
>+
>+ mode = v4l2_find_nearest_size_conditional(s5k3t2_supported_modes,
>+ ARRAY_SIZE(s5k3t2_supported_modes),
>+ width, height,
>+ fmt->format.width,
>+ fmt->format.height,
>+ s5k3t2_filter_by_link_freq,
>+ s5k3t2);
>+
>+ s5k3t2_update_pad_format(s5k3t2, mode, &fmt->format);
>+
>+ if (fmt->which == V4L2_SUBDEV_FORMAT_TRY || s5k3t2->mode == mode)
>+ goto set_format;
It can be handled without goto.
>+
>+ s5k3t2->mode = mode;
>+
>+ __v4l2_ctrl_s_ctrl(s5k3t2->link_freq, mode->link_freq_index);
>+
>+ hblank = mode->hts - mode->width;
>+ __v4l2_ctrl_modify_range(s5k3t2->hblank, hblank, hblank, 1, hblank);
>+
>+ vblank = mode->vts - mode->height;
>+ __v4l2_ctrl_modify_range(s5k3t2->vblank, vblank,
>+ S5K3T2_VTS_MAX - mode->height, 1, vblank);
>+ __v4l2_ctrl_s_ctrl(s5k3t2->vblank, vblank);
>+
>+ exposure_max = mode->vts - S5K3T2_EXPOSURE_MARGIN;
>+ __v4l2_ctrl_modify_range(s5k3t2->exposure, S5K3T2_EXPOSURE_MIN,
>+ exposure_max, S5K3T2_EXPOSURE_STEP,
>+ mode->exposure);
>+ __v4l2_ctrl_s_ctrl(s5k3t2->exposure, mode->exposure);
>+
Could you please add error checking for the all control operations
to handle any failures appropriately.
>+ if (s5k3t2->sd.ctrl_handler->error)
>+ return s5k3t2->sd.ctrl_handler->error;
>+
>+set_format:
>+ *v4l2_subdev_state_get_format(state, 0) = fmt->format;
>+
>+ return 0;
>+}
>+
>+static int s5k3t2_enum_mbus_code(struct v4l2_subdev *sd,
>+ struct v4l2_subdev_state *sd_state,
>+ struct v4l2_subdev_mbus_code_enum *code)
>+{
>+ struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
>+
>+ if (code->index > 0)
>+ return -EINVAL;
>+
>+ code->code = s5k3t2_get_format_code(s5k3t2);
>+
>+ return 0;
>+}
>+
>+static int s5k3t2_enum_frame_size(struct v4l2_subdev *sd,
>+ struct v4l2_subdev_state *sd_state,
>+ struct v4l2_subdev_frame_size_enum *fse)
>+{
>+ struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
>+ unsigned int i, index = fse->index;
>+
>+ if (fse->code != s5k3t2_get_format_code(s5k3t2))
>+ return -EINVAL;
>+
>+ for (i = 0; i < ARRAY_SIZE(s5k3t2_supported_modes); i++) {
>+ const struct s5k3t2_mode *mode = &s5k3t2_supported_modes[i];
>+
>+ if (!(s5k3t2->link_freq_bitmap & BIT(mode->link_freq_index)))
>+ continue;
>+
>+ if (index--)
>+ continue;
>+
>+ fse->min_width = mode->width;
>+ fse->max_width = fse->min_width;
>+ fse->min_height = mode->height;
>+ fse->max_height = fse->min_height;
>+
>+ return 0;
>+ }
>+
>+ return -EINVAL;
>+}
>+
>+static int s5k3t2_get_selection(struct v4l2_subdev *sd,
>+ struct v4l2_subdev_state *sd_state,
>+ struct v4l2_subdev_selection *sel)
>+{
>+ struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
>+ const struct s5k3t2_mode *mode = s5k3t2->mode;
>+ const struct v4l2_mbus_framefmt *fmt;
>+
Please align as reverse christmas tree order
>+ switch (sel->target) {
>+ case V4L2_SEL_TGT_CROP:
>+ if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
>+ fmt = v4l2_subdev_state_get_format(sd_state, 0);
>+ mode = v4l2_find_nearest_size(s5k3t2_supported_modes,
>+ ARRAY_SIZE(s5k3t2_supported_modes),
>+ width, height,
>+ fmt->width, fmt->height);
>+ }
>+ sel->r = mode->crop;
>+ return 0;
>+ case V4L2_SEL_TGT_NATIVE_SIZE:
>+ sel->r.left = 0;
>+ sel->r.top = 0;
>+ sel->r.width = S5K3T2_NATIVE_WIDTH;
>+ sel->r.height = S5K3T2_NATIVE_HEIGHT;
>+ return 0;
>+ case V4L2_SEL_TGT_CROP_DEFAULT:
>+ case V4L2_SEL_TGT_CROP_BOUNDS:
>+ sel->r.left = 8;
>+ sel->r.top = 8;
>+ sel->r.width = 5184;
>+ sel->r.height = 3880;
>+ return 0;
>+ default:
>+ return -EINVAL;
>+ }
>+}
>+
>+static int s5k3t2_init_state(struct v4l2_subdev *sd,
>+ struct v4l2_subdev_state *state)
>+{
>+ struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
>+ struct v4l2_subdev_format fmt = {
>+ .which = V4L2_SUBDEV_FORMAT_TRY,
>+ .pad = 0,
>+ .format = {
>+ .width = s5k3t2->mode->width,
>+ .height = s5k3t2->mode->height,
>+ },
>+ };
>+
>+ s5k3t2_set_pad_format(sd, state, &fmt);
>+
>+ return 0;
>+}
Please return s5k3t2_set_pad_format(sd, state, &fmt);
since this function always returns 0
>+
>+static const struct v4l2_subdev_video_ops s5k3t2_video_ops = {
>+ .s_stream = v4l2_subdev_s_stream_helper,
>+};
>+
>+static const struct v4l2_subdev_pad_ops s5k3t2_pad_ops = {
>+ .set_fmt = s5k3t2_set_pad_format,
>+ .get_fmt = v4l2_subdev_get_fmt,
>+ .get_selection = s5k3t2_get_selection,
>+ .enum_mbus_code = s5k3t2_enum_mbus_code,
>+ .enum_frame_size = s5k3t2_enum_frame_size,
>+ .enable_streams = s5k3t2_enable_streams,
>+ .disable_streams = s5k3t2_disable_streams,
>+};
>+
>+static const struct v4l2_subdev_ops s5k3t2_subdev_ops = {
>+ .video = &s5k3t2_video_ops,
>+ .pad = &s5k3t2_pad_ops,
>+};
>+
>+static const struct v4l2_subdev_internal_ops s5k3t2_internal_ops = {
>+ .init_state = s5k3t2_init_state,
>+};
>+
>+static const struct media_entity_operations s5k3t2_subdev_entity_ops = {
>+ .link_validate = v4l2_subdev_link_validate,
>+};
>+
>+static int s5k3t2_identify_sensor(struct s5k3t2 *s5k3t2)
>+{
>+ u64 val;
>+ int ret;
>+
>+ ret = cci_read(s5k3t2->regmap, S5K3T2_REG_CHIP_ID, &val, NULL);
>+ if (ret) {
>+ dev_err(s5k3t2->dev, "failed to read chip id: %d\n", ret);
>+ return ret;
>+ }
>+
>+ if (val != S5K3T2_CHIP_ID) {
>+ dev_err(s5k3t2->dev, "chip id mismatch: %x!=%llx\n",
>+ S5K3T2_CHIP_ID, val);
>+ return -ENODEV;
>+ }
>+
>+ return 0;
>+}
>+
>+static int s5k3t2_check_hwcfg(struct s5k3t2 *s5k3t2)
>+{
>+ struct fwnode_handle *fwnode = dev_fwnode(s5k3t2->dev), *ep;
>+ struct v4l2_fwnode_endpoint bus_cfg = {
>+ .bus = {
>+ .mipi_csi2 = {
>+ .num_data_lanes = S5K3T2_DATA_LANES,
>+ },
>+ },
>+ .bus_type = V4L2_MBUS_CSI2_DPHY,
>+ };
>+ int ret;
>+
>+ if (!fwnode)
>+ return -ENODEV;
>+
>+ ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
>+ if (!ep)
>+ return -EINVAL;
>+
>+ ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
>+ fwnode_handle_put(ep);
>+ if (ret)
>+ return ret;
>+
>+ if (bus_cfg.bus.mipi_csi2.num_data_lanes != S5K3T2_DATA_LANES) {
>+ dev_err(s5k3t2->dev, "invalid number of data lanes: %u\n",
>+ bus_cfg.bus.mipi_csi2.num_data_lanes);
>+ ret = -EINVAL;
>+ goto endpoint_free;
>+ }
>+
>+ ret = v4l2_link_freq_to_bitmap(s5k3t2->dev, bus_cfg.link_frequencies,
>+ bus_cfg.nr_of_link_frequencies,
>+ s5k3t2_link_freq_menu,
>+ ARRAY_SIZE(s5k3t2_link_freq_menu),
>+ &s5k3t2->link_freq_bitmap);
>+
>+endpoint_free:
>+ v4l2_fwnode_endpoint_free(&bus_cfg);
>+
>+ return ret;
>+}
>+
>+static int s5k3t2_power_on(struct device *dev)
>+{
>+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
>+ struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
>+ int ret;
>+
>+ ret = regulator_bulk_enable(S5K3T2_NUM_SUPPLIES, s5k3t2->supplies);
>+ if (ret)
>+ return ret;
>+
>+ ret = clk_prepare_enable(s5k3t2->mclk);
>+ if (ret)
>+ goto disable_regulators;
>+
>+ usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC);
>+ gpiod_set_value_cansleep(s5k3t2->reset_gpio, 0);
>+ usleep_range(10 * USEC_PER_MSEC, 15 * USEC_PER_MSEC);
>+
>+ return 0;
>+
>+disable_regulators:
>+ regulator_bulk_disable(S5K3T2_NUM_SUPPLIES, s5k3t2->supplies);
>+
>+ return ret;
>+}
>+
>+static int s5k3t2_power_off(struct device *dev)
>+{
>+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
>+ struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
>+
>+ gpiod_set_value_cansleep(s5k3t2->reset_gpio, 1);
>+
>+ clk_disable_unprepare(s5k3t2->mclk);
>+
>+ regulator_bulk_disable(S5K3T2_NUM_SUPPLIES, s5k3t2->supplies);
>+
>+ return 0;
>+}
>+
>+static int s5k3t2_probe(struct i2c_client *client)
>+{
>+ struct s5k3t2 *s5k3t2;
>+ unsigned long freq;
>+ unsigned int i;
>+ int ret;
>+
>+ s5k3t2 = devm_kzalloc(&client->dev, sizeof(*s5k3t2), GFP_KERNEL);
>+ if (!s5k3t2)
>+ return -ENOMEM;
>+
>+ s5k3t2->dev = &client->dev;
>+ v4l2_i2c_subdev_init(&s5k3t2->sd, client, &s5k3t2_subdev_ops);
>+
>+ s5k3t2->regmap = devm_cci_regmap_init_i2c(client, 16);
>+ if (IS_ERR(s5k3t2->regmap))
>+ return dev_err_probe(s5k3t2->dev, PTR_ERR(s5k3t2->regmap),
>+ "failed to init CCI\n");
>+
>+ s5k3t2->mclk = devm_v4l2_sensor_clk_get(s5k3t2->dev, NULL);
>+ if (IS_ERR(s5k3t2->mclk))
>+ return dev_err_probe(s5k3t2->dev, PTR_ERR(s5k3t2->mclk),
>+ "failed to get MCLK clock\n");
>+
>+ freq = clk_get_rate(s5k3t2->mclk);
>+ if (freq != S5K3T2_MCLK_FREQ)
>+ return dev_err_probe(s5k3t2->dev, -EINVAL,
>+ "MCLK clock frequency %lu is not supported\n",
>+ freq);
>+
>+ ret = s5k3t2_check_hwcfg(s5k3t2);
>+ if (ret)
>+ return dev_err_probe(s5k3t2->dev, ret,
>+ "failed to check HW configuration\n");
>+
>+ s5k3t2->reset_gpio = devm_gpiod_get_optional(s5k3t2->dev, "reset",
>+ GPIOD_OUT_HIGH);
>+ if (IS_ERR(s5k3t2->reset_gpio))
>+ return dev_err_probe(s5k3t2->dev, PTR_ERR(s5k3t2->reset_gpio),
>+ "cannot get reset GPIO\n");
>+
>+ for (i = 0; i < S5K3T2_NUM_SUPPLIES; i++)
>+ s5k3t2->supplies[i].supply = s5k3t2_supply_names[i];
>+
>+ ret = devm_regulator_bulk_get(s5k3t2->dev, S5K3T2_NUM_SUPPLIES,
>+ s5k3t2->supplies);
>+ if (ret)
>+ return dev_err_probe(s5k3t2->dev, ret,
>+ "failed to get supply regulators\n");
>+
>+ ret = s5k3t2_power_on(s5k3t2->dev);
>+ if (ret)
>+ return ret;
>+
>+ ret = s5k3t2_identify_sensor(s5k3t2);
>+ if (ret) {
>+ dev_err_probe(s5k3t2->dev, ret, "failed to find sensor\n");
>+ goto power_off;
>+ }
>+
>+ s5k3t2->mode =
>+ v4l2_find_nearest_size_conditional(s5k3t2_supported_modes,
>+ ARRAY_SIZE(s5k3t2_supported_modes),
>+ width, height,
>+ S5K3T2_DEFAULT_WIDTH,
>+ S5K3T2_DEFAULT_HEIGHT,
>+ s5k3t2_filter_by_link_freq,
>+ s5k3t2);
>+ ret = s5k3t2_init_controls(s5k3t2);
>+ if (ret) {
>+ dev_err_probe(s5k3t2->dev, ret, "failed to init controls\n");
>+ goto power_off;
>+ }
>+
>+ s5k3t2->sd.state_lock = s5k3t2->ctrl_handler.lock;
>+ s5k3t2->sd.internal_ops = &s5k3t2_internal_ops;
>+ s5k3t2->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>+ s5k3t2->sd.entity.ops = &s5k3t2_subdev_entity_ops;
>+ s5k3t2->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>+ s5k3t2->pad.flags = MEDIA_PAD_FL_SOURCE;
>+
>+ ret = media_entity_pads_init(&s5k3t2->sd.entity, 1, &s5k3t2->pad);
>+ if (ret) {
>+ dev_err_probe(s5k3t2->dev, ret,
>+ "failed to init media entity pads\n");
>+ goto v4l2_ctrl_handler_free;
>+ }
>+
>+ ret = v4l2_subdev_init_finalize(&s5k3t2->sd);
>+ if (ret < 0) {
>+ dev_err_probe(s5k3t2->dev, ret,
>+ "failed to finalize subdev init\n");
>+ goto media_entity_cleanup;
>+ }
>+
>+ pm_runtime_set_active(s5k3t2->dev);
>+ pm_runtime_enable(s5k3t2->dev);
>+
>+ ret = v4l2_async_register_subdev_sensor(&s5k3t2->sd);
>+ if (ret < 0) {
>+ dev_err_probe(s5k3t2->dev, ret,
>+ "failed to register V4L2 subdev\n");
>+ goto subdev_cleanup;
>+ }
>+
>+ pm_runtime_set_autosuspend_delay(s5k3t2->dev, 1000);
>+ pm_runtime_use_autosuspend(s5k3t2->dev);
>+ pm_runtime_idle(s5k3t2->dev);
>+
>+ return 0;
>+
>+subdev_cleanup:
>+ v4l2_subdev_cleanup(&s5k3t2->sd);
>+ pm_runtime_disable(s5k3t2->dev);
>+ pm_runtime_set_suspended(s5k3t2->dev);
>+
>+media_entity_cleanup:
>+ media_entity_cleanup(&s5k3t2->sd.entity);
>+
>+v4l2_ctrl_handler_free:
>+ v4l2_ctrl_handler_free(s5k3t2->sd.ctrl_handler);
>+
>+power_off:
>+ s5k3t2_power_off(s5k3t2->dev);
>+
>+ return ret;
>+}
>+
>+static void s5k3t2_remove(struct i2c_client *client)
>+{
>+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
>+ struct s5k3t2 *s5k3t2 = to_s5k3t2(sd);
>+
>+ v4l2_async_unregister_subdev(sd);
>+ v4l2_subdev_cleanup(sd);
>+ media_entity_cleanup(&sd->entity);
>+ v4l2_ctrl_handler_free(sd->ctrl_handler);
>+ pm_runtime_disable(s5k3t2->dev);
>+
>+ if (!pm_runtime_status_suspended(s5k3t2->dev)) {
>+ s5k3t2_power_off(s5k3t2->dev);
>+ pm_runtime_set_suspended(s5k3t2->dev);
>+ }
>+}
>+
>+static const struct dev_pm_ops s5k3t2_pm_ops = {
>+ RUNTIME_PM_OPS(s5k3t2_power_off, s5k3t2_power_on, NULL)
>+};
>+
>+static const struct of_device_id s5k3t2_of_match[] = {
>+ { .compatible = "samsung,s5k3t2" },
>+ { }
>+};
>+MODULE_DEVICE_TABLE(of, s5k3t2_of_match);
>+
>+static struct i2c_driver s5k3t2_i2c_driver = {
>+ .driver = {
>+ .name = "s5k3t2",
>+ .pm = pm_ptr(&s5k3t2_pm_ops),
>+ .of_match_table = s5k3t2_of_match,
>+ },
>+ .probe = s5k3t2_probe,
>+ .remove = s5k3t2_remove,
>+};
>+
>+module_i2c_driver(s5k3t2_i2c_driver);
>+
>+MODULE_AUTHOR("Armandas Kvietkus <armandas.kvietkus@proton.me>");
>+MODULE_DESCRIPTION("Samsung S5K3T2 image sensor driver");
>+MODULE_LICENSE("GPL");
>
>--
>2.55.0
Best Regards,
Elgin
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3 2/2] media: i2c: Add Samsung S5K3T2 image sensor driver
2026-10-01 18:41 ` Elgin Perumbilly
@ 2026-10-03 8:27 ` armandas.kvietkus
0 siblings, 0 replies; 9+ messages in thread
From: armandas.kvietkus @ 2026-10-03 8:27 UTC (permalink / raw)
To: Elgin Perumbilly
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, Sakari Ailus
Hi Elgin,
Thanks for the review.
On Thu, Oct 01, 2026 at 06:41:29PM +0000, Elgin Perumbilly wrote:
> Remove the unwanted pointers if they are not referenced,
> as they unnecessarily consume memory.
pixel_rate is not used after it is created, so I'll drop it. link_freq
is used in set_fmt, so it stays.
> Curious why 0x6f12 and 0x602a is used repeatedly in sequence here,
> unlike in other drivers, Could you explain it.
This is how these Samsung sensors write to their internal memory.
0x6028 selects the page, 0x602a sets the address, and the words written
to 0x6f12 go to that address and the ones after it. s5k3m5 and s5kjn1
do the same.
The first long block writes 210 words to page 0x2000 at 0x3aec. They
decode as ARM Thumb code, so it looks like code for the sensor's own
CPU. s5k3m5 starts its burst_array_setting with the same six words.
> Could you explain what this register CCI_REG16(0x6028) actually does?
It is the page pointer for the access above. s5k3m5 and s5kjn1 write
the same value at the same point before streaming.
> It seems unusual to write the values to S5K3T2_REG_CHIP_ID (0x0000),
>
> The chip ID register is generally read-only.
s5k3m5 and s5kjn1 do the same two writes to 0x0000, with a different
first value, and mark them as setting the version. I don't have
documentation that says more than that.
I'll fix the rest in v4. It should be ready by Monday, or later if I
need to wait for Sakari's answer on the link frequency.
Armandas
^ permalink raw reply [flat|nested] 9+ messages in thread