* [PATCH v3 01/12] media: i2c: imx214: Use subdev active state
2024-12-07 20:47 [PATCH v3 00/12] media: i2c: imx214: Miscellaneous cleanups and improvements André Apitzsch via B4 Relay
@ 2024-12-07 20:47 ` André Apitzsch via B4 Relay
2024-12-07 20:47 ` [PATCH v3 02/12] media: i2c: imx214: Simplify with dev_err_probe() André Apitzsch via B4 Relay
` (10 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: André Apitzsch via B4 Relay @ 2024-12-07 20:47 UTC (permalink / raw)
To: Ricardo Ribalda, Sakari Ailus, Mauro Carvalho Chehab
Cc: ~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht,
André Apitzsch
From: André Apitzsch <git@apitzsch.eu>
Port the imx214 sensor driver to use the subdev active state.
Move all the format configuration to the subdevice state and simplify
the format handling, locking and initialization.
While at it, simplify imx214_start_streaming() by removing unneeded goto
statements and the corresponding error label.
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
drivers/media/i2c/imx214.c | 156 +++++++++++++++------------------------------
1 file changed, 50 insertions(+), 106 deletions(-)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 4962cfe7c83d62425aeccb46a400fa93146f14ea..435d1ee12c3a0959ae751c3e7a8b4e7fa1a4c303 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -59,8 +59,6 @@ struct imx214 {
struct v4l2_subdev sd;
struct media_pad pad;
- struct v4l2_mbus_framefmt fmt;
- struct v4l2_rect crop;
struct v4l2_ctrl_handler ctrls;
struct v4l2_ctrl *pixel_rate;
@@ -68,15 +66,11 @@ struct imx214 {
struct v4l2_ctrl *exposure;
struct v4l2_ctrl *unit_size;
+ const struct imx214_mode *cur_mode;
+
struct regulator_bulk_data supplies[IMX214_NUM_SUPPLIES];
struct gpio_desc *enable_gpio;
-
- /*
- * Serialize control access, get/set format, get selection
- * and start streaming.
- */
- struct mutex mutex;
};
struct reg_8 {
@@ -490,6 +484,22 @@ static int __maybe_unused imx214_power_off(struct device *dev)
return 0;
}
+static void imx214_update_pad_format(struct imx214 *imx214,
+ const struct imx214_mode *mode,
+ struct v4l2_mbus_framefmt *fmt, u32 code)
+{
+ fmt->code = IMX214_MBUS_CODE;
+ fmt->width = mode->width;
+ fmt->height = mode->height;
+ fmt->field = V4L2_FIELD_NONE;
+ fmt->colorspace = V4L2_COLORSPACE_SRGB;
+ fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
+ fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
+ fmt->colorspace,
+ fmt->ycbcr_enc);
+ fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
+}
+
static int imx214_enum_mbus_code(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_mbus_code_enum *code)
@@ -549,52 +559,6 @@ static const struct v4l2_subdev_core_ops imx214_core_ops = {
#endif
};
-static struct v4l2_mbus_framefmt *
-__imx214_get_pad_format(struct imx214 *imx214,
- struct v4l2_subdev_state *sd_state,
- unsigned int pad,
- enum v4l2_subdev_format_whence which)
-{
- switch (which) {
- case V4L2_SUBDEV_FORMAT_TRY:
- return v4l2_subdev_state_get_format(sd_state, pad);
- case V4L2_SUBDEV_FORMAT_ACTIVE:
- return &imx214->fmt;
- default:
- return NULL;
- }
-}
-
-static int imx214_get_format(struct v4l2_subdev *sd,
- struct v4l2_subdev_state *sd_state,
- struct v4l2_subdev_format *format)
-{
- struct imx214 *imx214 = to_imx214(sd);
-
- mutex_lock(&imx214->mutex);
- format->format = *__imx214_get_pad_format(imx214, sd_state,
- format->pad,
- format->which);
- mutex_unlock(&imx214->mutex);
-
- return 0;
-}
-
-static struct v4l2_rect *
-__imx214_get_pad_crop(struct imx214 *imx214,
- struct v4l2_subdev_state *sd_state,
- unsigned int pad, enum v4l2_subdev_format_whence which)
-{
- switch (which) {
- case V4L2_SUBDEV_FORMAT_TRY:
- return v4l2_subdev_state_get_crop(sd_state, pad);
- case V4L2_SUBDEV_FORMAT_ACTIVE:
- return &imx214->crop;
- default:
- return NULL;
- }
-}
-
static int imx214_set_format(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_format *format)
@@ -604,34 +568,22 @@ static int imx214_set_format(struct v4l2_subdev *sd,
struct v4l2_rect *__crop;
const struct imx214_mode *mode;
- mutex_lock(&imx214->mutex);
-
- __crop = __imx214_get_pad_crop(imx214, sd_state, format->pad,
- format->which);
-
mode = v4l2_find_nearest_size(imx214_modes,
ARRAY_SIZE(imx214_modes), width, height,
format->format.width,
format->format.height);
- __crop->width = mode->width;
- __crop->height = mode->height;
+ imx214_update_pad_format(imx214, mode, &format->format, format->format.code);
+ __format = v4l2_subdev_state_get_format(sd_state, 0);
- __format = __imx214_get_pad_format(imx214, sd_state, format->pad,
- format->which);
- __format->width = __crop->width;
- __format->height = __crop->height;
- __format->code = IMX214_MBUS_CODE;
- __format->field = V4L2_FIELD_NONE;
- __format->colorspace = V4L2_COLORSPACE_SRGB;
- __format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(__format->colorspace);
- __format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
- __format->colorspace, __format->ycbcr_enc);
- __format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(__format->colorspace);
+ *__format = format->format;
- format->format = *__format;
+ __crop = v4l2_subdev_state_get_crop(sd_state, 0);
+ __crop->width = mode->width;
+ __crop->height = mode->height;
- mutex_unlock(&imx214->mutex);
+ if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+ imx214->cur_mode = mode;
return 0;
}
@@ -640,14 +592,9 @@ static int imx214_get_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_selection *sel)
{
- struct imx214 *imx214 = to_imx214(sd);
-
switch (sel->target) {
case V4L2_SEL_TGT_CROP:
- mutex_lock(&imx214->mutex);
- sel->r = *__imx214_get_pad_crop(imx214, sd_state, sel->pad,
- sel->which);
- mutex_unlock(&imx214->mutex);
+ sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
return 0;
case V4L2_SEL_TGT_NATIVE_SIZE:
@@ -826,40 +773,28 @@ static int imx214_write_table(struct imx214 *imx214,
static int imx214_start_streaming(struct imx214 *imx214)
{
- const struct imx214_mode *mode;
int ret;
- mutex_lock(&imx214->mutex);
ret = imx214_write_table(imx214, mode_table_common);
if (ret < 0) {
dev_err(imx214->dev, "could not sent common table %d\n", ret);
- goto error;
+ return ret;
}
- mode = v4l2_find_nearest_size(imx214_modes,
- ARRAY_SIZE(imx214_modes), width, height,
- imx214->fmt.width, imx214->fmt.height);
- ret = imx214_write_table(imx214, mode->reg_table);
+ ret = imx214_write_table(imx214, imx214->cur_mode->reg_table);
if (ret < 0) {
dev_err(imx214->dev, "could not sent mode table %d\n", ret);
- goto error;
+ return ret;
}
ret = __v4l2_ctrl_handler_setup(&imx214->ctrls);
if (ret < 0) {
dev_err(imx214->dev, "could not sync v4l2 controls\n");
- goto error;
+ return ret;
}
ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STREAMING);
- if (ret < 0) {
+ if (ret < 0)
dev_err(imx214->dev, "could not sent start table %d\n", ret);
- goto error;
- }
- mutex_unlock(&imx214->mutex);
- return 0;
-
-error:
- mutex_unlock(&imx214->mutex);
return ret;
}
@@ -877,6 +812,7 @@ static int imx214_stop_streaming(struct imx214 *imx214)
static int imx214_s_stream(struct v4l2_subdev *subdev, int enable)
{
struct imx214 *imx214 = to_imx214(subdev);
+ struct v4l2_subdev_state *state;
int ret;
if (enable) {
@@ -884,7 +820,9 @@ static int imx214_s_stream(struct v4l2_subdev *subdev, int enable)
if (ret < 0)
return ret;
+ state = v4l2_subdev_lock_and_get_active_state(subdev);
ret = imx214_start_streaming(imx214);
+ v4l2_subdev_unlock_state(state);
if (ret < 0)
goto err_rpm_put;
} else {
@@ -948,7 +886,7 @@ static const struct v4l2_subdev_pad_ops imx214_subdev_pad_ops = {
.enum_mbus_code = imx214_enum_mbus_code,
.enum_frame_size = imx214_enum_frame_size,
.enum_frame_interval = imx214_enum_frame_interval,
- .get_fmt = imx214_get_format,
+ .get_fmt = v4l2_subdev_get_fmt,
.set_fmt = imx214_set_format,
.get_selection = imx214_get_selection,
.get_frame_interval = imx214_get_frame_interval,
@@ -1079,13 +1017,13 @@ static int imx214_probe(struct i2c_client *client)
pm_runtime_enable(imx214->dev);
pm_runtime_idle(imx214->dev);
+ /* Set default mode to max resolution */
+ imx214->cur_mode = &imx214_modes[0];
+
ret = imx214_ctrls_init(imx214);
if (ret < 0)
goto error_power_off;
- mutex_init(&imx214->mutex);
- imx214->ctrls.lock = &imx214->mutex;
-
imx214->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
imx214->pad.flags = MEDIA_PAD_FL_SOURCE;
imx214->sd.dev = &client->dev;
@@ -1097,20 +1035,27 @@ static int imx214_probe(struct i2c_client *client)
goto free_ctrl;
}
- imx214_entity_init_state(&imx214->sd, NULL);
+ imx214->sd.state_lock = imx214->ctrls.lock;
+ ret = v4l2_subdev_init_finalize(&imx214->sd);
+ if (ret < 0) {
+ dev_err(dev, "subdev init error: %d\n", ret);
+ goto free_entity;
+ }
ret = v4l2_async_register_subdev_sensor(&imx214->sd);
if (ret < 0) {
dev_err(dev, "could not register v4l2 device\n");
- goto free_entity;
+ goto error_subdev_cleanup;
}
return 0;
+error_subdev_cleanup:
+ v4l2_subdev_cleanup(&imx214->sd);
+
free_entity:
media_entity_cleanup(&imx214->sd.entity);
free_ctrl:
- mutex_destroy(&imx214->mutex);
v4l2_ctrl_handler_free(&imx214->ctrls);
error_power_off:
pm_runtime_disable(imx214->dev);
@@ -1125,13 +1070,12 @@ static void imx214_remove(struct i2c_client *client)
struct imx214 *imx214 = to_imx214(sd);
v4l2_async_unregister_subdev(&imx214->sd);
+ v4l2_subdev_cleanup(sd);
media_entity_cleanup(&imx214->sd.entity);
v4l2_ctrl_handler_free(&imx214->ctrls);
pm_runtime_disable(&client->dev);
pm_runtime_set_suspended(&client->dev);
-
- mutex_destroy(&imx214->mutex);
}
static const struct of_device_id imx214_of_match[] = {
--
2.47.1
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v3 02/12] media: i2c: imx214: Simplify with dev_err_probe()
2024-12-07 20:47 [PATCH v3 00/12] media: i2c: imx214: Miscellaneous cleanups and improvements André Apitzsch via B4 Relay
2024-12-07 20:47 ` [PATCH v3 01/12] media: i2c: imx214: Use subdev active state André Apitzsch via B4 Relay
@ 2024-12-07 20:47 ` André Apitzsch via B4 Relay
2024-12-07 20:47 ` [PATCH v3 03/12] media: i2c: imx214: Convert to CCI register access helpers André Apitzsch via B4 Relay
` (9 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: André Apitzsch via B4 Relay @ 2024-12-07 20:47 UTC (permalink / raw)
To: Ricardo Ribalda, Sakari Ailus, Mauro Carvalho Chehab
Cc: ~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht,
André Apitzsch, Ricardo Ribalda
From: André Apitzsch <git@apitzsch.eu>
Error handling in probe() can be a bit simpler with dev_err_probe().
Acked-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
drivers/media/i2c/imx214.c | 52 ++++++++++++++++++++--------------------------
1 file changed, 22 insertions(+), 30 deletions(-)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 435d1ee12c3a0959ae751c3e7a8b4e7fa1a4c303..09f5078f341c467f083c0294212dad0f354e6185 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -930,14 +930,12 @@ static int imx214_parse_fwnode(struct device *dev)
int ret;
endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
- if (!endpoint) {
- dev_err(dev, "endpoint node not found\n");
- return -EINVAL;
- }
+ if (!endpoint)
+ return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");
ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
if (ret) {
- dev_err(dev, "parsing endpoint node failed\n");
+ dev_err_probe(dev, ret, "parsing endpoint node failed\n");
goto done;
}
@@ -946,8 +944,9 @@ static int imx214_parse_fwnode(struct device *dev)
break;
if (i == bus_cfg.nr_of_link_frequencies) {
- dev_err(dev, "link-frequencies %d not supported, Please review your DT\n",
- IMX214_DEFAULT_LINK_FREQ);
+ dev_err_probe(dev, -EINVAL,
+ "link-frequencies %d not supported, Please review your DT\n",
+ IMX214_DEFAULT_LINK_FREQ);
ret = -EINVAL;
goto done;
}
@@ -975,34 +974,27 @@ static int imx214_probe(struct i2c_client *client)
imx214->dev = dev;
imx214->xclk = devm_clk_get(dev, NULL);
- if (IS_ERR(imx214->xclk)) {
- dev_err(dev, "could not get xclk");
- return PTR_ERR(imx214->xclk);
- }
+ if (IS_ERR(imx214->xclk))
+ return dev_err_probe(dev, PTR_ERR(imx214->xclk),
+ "failed to get xclk\n");
ret = clk_set_rate(imx214->xclk, IMX214_DEFAULT_CLK_FREQ);
- if (ret) {
- dev_err(dev, "could not set xclk frequency\n");
- return ret;
- }
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to set xclk frequency\n");
ret = imx214_get_regulators(dev, imx214);
- if (ret < 0) {
- dev_err(dev, "cannot get regulators\n");
- return ret;
- }
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "failed to get regulators\n");
imx214->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
- if (IS_ERR(imx214->enable_gpio)) {
- dev_err(dev, "cannot get enable gpio\n");
- return PTR_ERR(imx214->enable_gpio);
- }
+ if (IS_ERR(imx214->enable_gpio))
+ return dev_err_probe(dev, PTR_ERR(imx214->enable_gpio),
+ "failed to get enable gpio\n");
imx214->regmap = devm_regmap_init_i2c(client, &sensor_regmap_config);
- if (IS_ERR(imx214->regmap)) {
- dev_err(dev, "regmap init failed\n");
- return PTR_ERR(imx214->regmap);
- }
+ if (IS_ERR(imx214->regmap))
+ return dev_err_probe(dev, PTR_ERR(imx214->regmap),
+ "regmap init failed\n");
v4l2_i2c_subdev_init(&imx214->sd, client, &imx214_subdev_ops);
imx214->sd.internal_ops = &imx214_internal_ops;
@@ -1031,20 +1023,20 @@ static int imx214_probe(struct i2c_client *client)
ret = media_entity_pads_init(&imx214->sd.entity, 1, &imx214->pad);
if (ret < 0) {
- dev_err(dev, "could not register media entity\n");
+ dev_err_probe(dev, ret, "failed to init entity pads\n");
goto free_ctrl;
}
imx214->sd.state_lock = imx214->ctrls.lock;
ret = v4l2_subdev_init_finalize(&imx214->sd);
if (ret < 0) {
- dev_err(dev, "subdev init error: %d\n", ret);
+ dev_err_probe(dev, ret, "subdev init error\n");
goto free_entity;
}
ret = v4l2_async_register_subdev_sensor(&imx214->sd);
if (ret < 0) {
- dev_err(dev, "could not register v4l2 device\n");
+ dev_err_probe(dev, ret, "failed to register sensor sub-device\n");
goto error_subdev_cleanup;
}
--
2.47.1
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v3 03/12] media: i2c: imx214: Convert to CCI register access helpers
2024-12-07 20:47 [PATCH v3 00/12] media: i2c: imx214: Miscellaneous cleanups and improvements André Apitzsch via B4 Relay
2024-12-07 20:47 ` [PATCH v3 01/12] media: i2c: imx214: Use subdev active state André Apitzsch via B4 Relay
2024-12-07 20:47 ` [PATCH v3 02/12] media: i2c: imx214: Simplify with dev_err_probe() André Apitzsch via B4 Relay
@ 2024-12-07 20:47 ` André Apitzsch via B4 Relay
2024-12-07 20:47 ` [PATCH v3 04/12] media: i2c: imx214: Replace register addresses with macros André Apitzsch via B4 Relay
` (8 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: André Apitzsch via B4 Relay @ 2024-12-07 20:47 UTC (permalink / raw)
To: Ricardo Ribalda, Sakari Ailus, Mauro Carvalho Chehab
Cc: ~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht,
André Apitzsch, Ricardo Ribalda
From: André Apitzsch <git@apitzsch.eu>
Use the new common CCI register access helpers to replace the private
register access helpers in the imx214 driver. This simplifies the driver
by reducing the amount of code.
Acked-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
drivers/media/i2c/Kconfig | 1 +
drivers/media/i2c/imx214.c | 672 +++++++++++++++++++++------------------------
2 files changed, 310 insertions(+), 363 deletions(-)
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 8ba096b8ebca241239a327ab3af0d9bce3ee9962..85ecb2aeefdbfff744c8de86866560518abeace1 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -140,6 +140,7 @@ config VIDEO_IMX214
tristate "Sony IMX214 sensor support"
depends on GPIOLIB
select REGMAP_I2C
+ select V4L2_CCI_I2C
help
This is a Video4Linux2 sensor driver for the Sony
IMX214 camera.
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 09f5078f341c467f083c0294212dad0f354e6185..9b077f73dc21a534a0342919f81d7672bd267015 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -15,11 +15,12 @@
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
#include <media/media-entity.h>
+#include <media/v4l2-cci.h>
#include <media/v4l2-ctrls.h>
#include <media/v4l2-fwnode.h>
#include <media/v4l2-subdev.h>
-#define IMX214_REG_MODE_SELECT 0x0100
+#define IMX214_REG_MODE_SELECT CCI_REG8(0x0100)
#define IMX214_MODE_STANDBY 0x00
#define IMX214_MODE_STREAMING 0x01
@@ -30,7 +31,7 @@
#define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
/* Exposure control */
-#define IMX214_REG_EXPOSURE 0x0202
+#define IMX214_REG_EXPOSURE CCI_REG16(0x0202)
#define IMX214_EXPOSURE_MIN 0
#define IMX214_EXPOSURE_MAX 3184
#define IMX214_EXPOSURE_STEP 1
@@ -73,345 +74,324 @@ struct imx214 {
struct gpio_desc *enable_gpio;
};
-struct reg_8 {
- u16 addr;
- u8 val;
-};
-
-enum {
- IMX214_TABLE_WAIT_MS = 0,
- IMX214_TABLE_END,
- IMX214_MAX_RETRIES,
- IMX214_WAIT_MS
-};
-
/*From imx214_mode_tbls.h*/
-static const struct reg_8 mode_4096x2304[] = {
- {0x0114, 0x03},
- {0x0220, 0x00},
- {0x0221, 0x11},
- {0x0222, 0x01},
- {0x0340, 0x0C},
- {0x0341, 0x7A},
- {0x0342, 0x13},
- {0x0343, 0x90},
- {0x0344, 0x00},
- {0x0345, 0x38},
- {0x0346, 0x01},
- {0x0347, 0x98},
- {0x0348, 0x10},
- {0x0349, 0x37},
- {0x034A, 0x0A},
- {0x034B, 0x97},
- {0x0381, 0x01},
- {0x0383, 0x01},
- {0x0385, 0x01},
- {0x0387, 0x01},
- {0x0900, 0x00},
- {0x0901, 0x00},
- {0x0902, 0x00},
- {0x3000, 0x35},
- {0x3054, 0x01},
- {0x305C, 0x11},
-
- {0x0112, 0x0A},
- {0x0113, 0x0A},
- {0x034C, 0x10},
- {0x034D, 0x00},
- {0x034E, 0x09},
- {0x034F, 0x00},
- {0x0401, 0x00},
- {0x0404, 0x00},
- {0x0405, 0x10},
- {0x0408, 0x00},
- {0x0409, 0x00},
- {0x040A, 0x00},
- {0x040B, 0x00},
- {0x040C, 0x10},
- {0x040D, 0x00},
- {0x040E, 0x09},
- {0x040F, 0x00},
-
- {0x0301, 0x05},
- {0x0303, 0x02},
- {0x0305, 0x03},
- {0x0306, 0x00},
- {0x0307, 0x96},
- {0x0309, 0x0A},
- {0x030B, 0x01},
- {0x0310, 0x00},
-
- {0x0820, 0x12},
- {0x0821, 0xC0},
- {0x0822, 0x00},
- {0x0823, 0x00},
-
- {0x3A03, 0x09},
- {0x3A04, 0x50},
- {0x3A05, 0x01},
-
- {0x0B06, 0x01},
- {0x30A2, 0x00},
-
- {0x30B4, 0x00},
-
- {0x3A02, 0xFF},
-
- {0x3011, 0x00},
- {0x3013, 0x01},
-
- {0x0202, 0x0C},
- {0x0203, 0x70},
- {0x0224, 0x01},
- {0x0225, 0xF4},
-
- {0x0204, 0x00},
- {0x0205, 0x00},
- {0x020E, 0x01},
- {0x020F, 0x00},
- {0x0210, 0x01},
- {0x0211, 0x00},
- {0x0212, 0x01},
- {0x0213, 0x00},
- {0x0214, 0x01},
- {0x0215, 0x00},
- {0x0216, 0x00},
- {0x0217, 0x00},
-
- {0x4170, 0x00},
- {0x4171, 0x10},
- {0x4176, 0x00},
- {0x4177, 0x3C},
- {0xAE20, 0x04},
- {0xAE21, 0x5C},
-
- {IMX214_TABLE_WAIT_MS, 10},
- {0x0138, 0x01},
- {IMX214_TABLE_END, 0x00}
+static const struct cci_reg_sequence mode_4096x2304[] = {
+ { CCI_REG8(0x0114), 0x03 },
+ { CCI_REG8(0x0220), 0x00 },
+ { CCI_REG8(0x0221), 0x11 },
+ { CCI_REG8(0x0222), 0x01 },
+ { CCI_REG8(0x0340), 0x0C },
+ { CCI_REG8(0x0341), 0x7A },
+ { CCI_REG8(0x0342), 0x13 },
+ { CCI_REG8(0x0343), 0x90 },
+ { CCI_REG8(0x0344), 0x00 },
+ { CCI_REG8(0x0345), 0x38 },
+ { CCI_REG8(0x0346), 0x01 },
+ { CCI_REG8(0x0347), 0x98 },
+ { CCI_REG8(0x0348), 0x10 },
+ { CCI_REG8(0x0349), 0x37 },
+ { CCI_REG8(0x034A), 0x0A },
+ { CCI_REG8(0x034B), 0x97 },
+ { CCI_REG8(0x0381), 0x01 },
+ { CCI_REG8(0x0383), 0x01 },
+ { CCI_REG8(0x0385), 0x01 },
+ { CCI_REG8(0x0387), 0x01 },
+ { CCI_REG8(0x0900), 0x00 },
+ { CCI_REG8(0x0901), 0x00 },
+ { CCI_REG8(0x0902), 0x00 },
+ { CCI_REG8(0x3000), 0x35 },
+ { CCI_REG8(0x3054), 0x01 },
+ { CCI_REG8(0x305C), 0x11 },
+
+ { CCI_REG8(0x0112), 0x0A },
+ { CCI_REG8(0x0113), 0x0A },
+ { CCI_REG8(0x034C), 0x10 },
+ { CCI_REG8(0x034D), 0x00 },
+ { CCI_REG8(0x034E), 0x09 },
+ { CCI_REG8(0x034F), 0x00 },
+ { CCI_REG8(0x0401), 0x00 },
+ { CCI_REG8(0x0404), 0x00 },
+ { CCI_REG8(0x0405), 0x10 },
+ { CCI_REG8(0x0408), 0x00 },
+ { CCI_REG8(0x0409), 0x00 },
+ { CCI_REG8(0x040A), 0x00 },
+ { CCI_REG8(0x040B), 0x00 },
+ { CCI_REG8(0x040C), 0x10 },
+ { CCI_REG8(0x040D), 0x00 },
+ { CCI_REG8(0x040E), 0x09 },
+ { CCI_REG8(0x040F), 0x00 },
+
+ { CCI_REG8(0x0301), 0x05 },
+ { CCI_REG8(0x0303), 0x02 },
+ { CCI_REG8(0x0305), 0x03 },
+ { CCI_REG8(0x0306), 0x00 },
+ { CCI_REG8(0x0307), 0x96 },
+ { CCI_REG8(0x0309), 0x0A },
+ { CCI_REG8(0x030B), 0x01 },
+ { CCI_REG8(0x0310), 0x00 },
+
+ { CCI_REG8(0x0820), 0x12 },
+ { CCI_REG8(0x0821), 0xC0 },
+ { CCI_REG8(0x0822), 0x00 },
+ { CCI_REG8(0x0823), 0x00 },
+
+ { CCI_REG8(0x3A03), 0x09 },
+ { CCI_REG8(0x3A04), 0x50 },
+ { CCI_REG8(0x3A05), 0x01 },
+
+ { CCI_REG8(0x0B06), 0x01 },
+ { CCI_REG8(0x30A2), 0x00 },
+
+ { CCI_REG8(0x30B4), 0x00 },
+
+ { CCI_REG8(0x3A02), 0xFF },
+
+ { CCI_REG8(0x3011), 0x00 },
+ { CCI_REG8(0x3013), 0x01 },
+
+ { CCI_REG8(0x0202), 0x0C },
+ { CCI_REG8(0x0203), 0x70 },
+ { CCI_REG8(0x0224), 0x01 },
+ { CCI_REG8(0x0225), 0xF4 },
+
+ { CCI_REG8(0x0204), 0x00 },
+ { CCI_REG8(0x0205), 0x00 },
+ { CCI_REG8(0x020E), 0x01 },
+ { CCI_REG8(0x020F), 0x00 },
+ { CCI_REG8(0x0210), 0x01 },
+ { CCI_REG8(0x0211), 0x00 },
+ { CCI_REG8(0x0212), 0x01 },
+ { CCI_REG8(0x0213), 0x00 },
+ { CCI_REG8(0x0214), 0x01 },
+ { CCI_REG8(0x0215), 0x00 },
+ { CCI_REG8(0x0216), 0x00 },
+ { CCI_REG8(0x0217), 0x00 },
+
+ { CCI_REG8(0x4170), 0x00 },
+ { CCI_REG8(0x4171), 0x10 },
+ { CCI_REG8(0x4176), 0x00 },
+ { CCI_REG8(0x4177), 0x3C },
+ { CCI_REG8(0xAE20), 0x04 },
+ { CCI_REG8(0xAE21), 0x5C },
};
-static const struct reg_8 mode_1920x1080[] = {
- {0x0114, 0x03},
- {0x0220, 0x00},
- {0x0221, 0x11},
- {0x0222, 0x01},
- {0x0340, 0x0C},
- {0x0341, 0x7A},
- {0x0342, 0x13},
- {0x0343, 0x90},
- {0x0344, 0x04},
- {0x0345, 0x78},
- {0x0346, 0x03},
- {0x0347, 0xFC},
- {0x0348, 0x0B},
- {0x0349, 0xF7},
- {0x034A, 0x08},
- {0x034B, 0x33},
- {0x0381, 0x01},
- {0x0383, 0x01},
- {0x0385, 0x01},
- {0x0387, 0x01},
- {0x0900, 0x00},
- {0x0901, 0x00},
- {0x0902, 0x00},
- {0x3000, 0x35},
- {0x3054, 0x01},
- {0x305C, 0x11},
-
- {0x0112, 0x0A},
- {0x0113, 0x0A},
- {0x034C, 0x07},
- {0x034D, 0x80},
- {0x034E, 0x04},
- {0x034F, 0x38},
- {0x0401, 0x00},
- {0x0404, 0x00},
- {0x0405, 0x10},
- {0x0408, 0x00},
- {0x0409, 0x00},
- {0x040A, 0x00},
- {0x040B, 0x00},
- {0x040C, 0x07},
- {0x040D, 0x80},
- {0x040E, 0x04},
- {0x040F, 0x38},
-
- {0x0301, 0x05},
- {0x0303, 0x02},
- {0x0305, 0x03},
- {0x0306, 0x00},
- {0x0307, 0x96},
- {0x0309, 0x0A},
- {0x030B, 0x01},
- {0x0310, 0x00},
-
- {0x0820, 0x12},
- {0x0821, 0xC0},
- {0x0822, 0x00},
- {0x0823, 0x00},
-
- {0x3A03, 0x04},
- {0x3A04, 0xF8},
- {0x3A05, 0x02},
-
- {0x0B06, 0x01},
- {0x30A2, 0x00},
-
- {0x30B4, 0x00},
-
- {0x3A02, 0xFF},
-
- {0x3011, 0x00},
- {0x3013, 0x01},
-
- {0x0202, 0x0C},
- {0x0203, 0x70},
- {0x0224, 0x01},
- {0x0225, 0xF4},
-
- {0x0204, 0x00},
- {0x0205, 0x00},
- {0x020E, 0x01},
- {0x020F, 0x00},
- {0x0210, 0x01},
- {0x0211, 0x00},
- {0x0212, 0x01},
- {0x0213, 0x00},
- {0x0214, 0x01},
- {0x0215, 0x00},
- {0x0216, 0x00},
- {0x0217, 0x00},
-
- {0x4170, 0x00},
- {0x4171, 0x10},
- {0x4176, 0x00},
- {0x4177, 0x3C},
- {0xAE20, 0x04},
- {0xAE21, 0x5C},
-
- {IMX214_TABLE_WAIT_MS, 10},
- {0x0138, 0x01},
- {IMX214_TABLE_END, 0x00}
+static const struct cci_reg_sequence mode_1920x1080[] = {
+ { CCI_REG8(0x0114), 0x03 },
+ { CCI_REG8(0x0220), 0x00 },
+ { CCI_REG8(0x0221), 0x11 },
+ { CCI_REG8(0x0222), 0x01 },
+ { CCI_REG8(0x0340), 0x0C },
+ { CCI_REG8(0x0341), 0x7A },
+ { CCI_REG8(0x0342), 0x13 },
+ { CCI_REG8(0x0343), 0x90 },
+ { CCI_REG8(0x0344), 0x04 },
+ { CCI_REG8(0x0345), 0x78 },
+ { CCI_REG8(0x0346), 0x03 },
+ { CCI_REG8(0x0347), 0xFC },
+ { CCI_REG8(0x0348), 0x0B },
+ { CCI_REG8(0x0349), 0xF7 },
+ { CCI_REG8(0x034A), 0x08 },
+ { CCI_REG8(0x034B), 0x33 },
+ { CCI_REG8(0x0381), 0x01 },
+ { CCI_REG8(0x0383), 0x01 },
+ { CCI_REG8(0x0385), 0x01 },
+ { CCI_REG8(0x0387), 0x01 },
+ { CCI_REG8(0x0900), 0x00 },
+ { CCI_REG8(0x0901), 0x00 },
+ { CCI_REG8(0x0902), 0x00 },
+ { CCI_REG8(0x3000), 0x35 },
+ { CCI_REG8(0x3054), 0x01 },
+ { CCI_REG8(0x305C), 0x11 },
+
+ { CCI_REG8(0x0112), 0x0A },
+ { CCI_REG8(0x0113), 0x0A },
+ { CCI_REG8(0x034C), 0x07 },
+ { CCI_REG8(0x034D), 0x80 },
+ { CCI_REG8(0x034E), 0x04 },
+ { CCI_REG8(0x034F), 0x38 },
+ { CCI_REG8(0x0401), 0x00 },
+ { CCI_REG8(0x0404), 0x00 },
+ { CCI_REG8(0x0405), 0x10 },
+ { CCI_REG8(0x0408), 0x00 },
+ { CCI_REG8(0x0409), 0x00 },
+ { CCI_REG8(0x040A), 0x00 },
+ { CCI_REG8(0x040B), 0x00 },
+ { CCI_REG8(0x040C), 0x07 },
+ { CCI_REG8(0x040D), 0x80 },
+ { CCI_REG8(0x040E), 0x04 },
+ { CCI_REG8(0x040F), 0x38 },
+
+ { CCI_REG8(0x0301), 0x05 },
+ { CCI_REG8(0x0303), 0x02 },
+ { CCI_REG8(0x0305), 0x03 },
+ { CCI_REG8(0x0306), 0x00 },
+ { CCI_REG8(0x0307), 0x96 },
+ { CCI_REG8(0x0309), 0x0A },
+ { CCI_REG8(0x030B), 0x01 },
+ { CCI_REG8(0x0310), 0x00 },
+
+ { CCI_REG8(0x0820), 0x12 },
+ { CCI_REG8(0x0821), 0xC0 },
+ { CCI_REG8(0x0822), 0x00 },
+ { CCI_REG8(0x0823), 0x00 },
+
+ { CCI_REG8(0x3A03), 0x04 },
+ { CCI_REG8(0x3A04), 0xF8 },
+ { CCI_REG8(0x3A05), 0x02 },
+
+ { CCI_REG8(0x0B06), 0x01 },
+ { CCI_REG8(0x30A2), 0x00 },
+
+ { CCI_REG8(0x30B4), 0x00 },
+
+ { CCI_REG8(0x3A02), 0xFF },
+
+ { CCI_REG8(0x3011), 0x00 },
+ { CCI_REG8(0x3013), 0x01 },
+
+ { CCI_REG8(0x0202), 0x0C },
+ { CCI_REG8(0x0203), 0x70 },
+ { CCI_REG8(0x0224), 0x01 },
+ { CCI_REG8(0x0225), 0xF4 },
+
+ { CCI_REG8(0x0204), 0x00 },
+ { CCI_REG8(0x0205), 0x00 },
+ { CCI_REG8(0x020E), 0x01 },
+ { CCI_REG8(0x020F), 0x00 },
+ { CCI_REG8(0x0210), 0x01 },
+ { CCI_REG8(0x0211), 0x00 },
+ { CCI_REG8(0x0212), 0x01 },
+ { CCI_REG8(0x0213), 0x00 },
+ { CCI_REG8(0x0214), 0x01 },
+ { CCI_REG8(0x0215), 0x00 },
+ { CCI_REG8(0x0216), 0x00 },
+ { CCI_REG8(0x0217), 0x00 },
+
+ { CCI_REG8(0x4170), 0x00 },
+ { CCI_REG8(0x4171), 0x10 },
+ { CCI_REG8(0x4176), 0x00 },
+ { CCI_REG8(0x4177), 0x3C },
+ { CCI_REG8(0xAE20), 0x04 },
+ { CCI_REG8(0xAE21), 0x5C },
};
-static const struct reg_8 mode_table_common[] = {
+static const struct cci_reg_sequence mode_table_common[] = {
/* software reset */
/* software standby settings */
- {0x0100, 0x00},
+ { CCI_REG8(0x0100), 0x00 },
/* ATR setting */
- {0x9300, 0x02},
+ { CCI_REG8(0x9300), 0x02 },
/* external clock setting */
- {0x0136, 0x18},
- {0x0137, 0x00},
+ { CCI_REG8(0x0136), 0x18 },
+ { CCI_REG8(0x0137), 0x00 },
/* global setting */
/* basic config */
- {0x0101, 0x00},
- {0x0105, 0x01},
- {0x0106, 0x01},
- {0x4550, 0x02},
- {0x4601, 0x00},
- {0x4642, 0x05},
- {0x6227, 0x11},
- {0x6276, 0x00},
- {0x900E, 0x06},
- {0xA802, 0x90},
- {0xA803, 0x11},
- {0xA804, 0x62},
- {0xA805, 0x77},
- {0xA806, 0xAE},
- {0xA807, 0x34},
- {0xA808, 0xAE},
- {0xA809, 0x35},
- {0xA80A, 0x62},
- {0xA80B, 0x83},
- {0xAE33, 0x00},
+ { CCI_REG8(0x0101), 0x00 },
+ { CCI_REG8(0x0105), 0x01 },
+ { CCI_REG8(0x0106), 0x01 },
+ { CCI_REG8(0x4550), 0x02 },
+ { CCI_REG8(0x4601), 0x00 },
+ { CCI_REG8(0x4642), 0x05 },
+ { CCI_REG8(0x6227), 0x11 },
+ { CCI_REG8(0x6276), 0x00 },
+ { CCI_REG8(0x900E), 0x06 },
+ { CCI_REG8(0xA802), 0x90 },
+ { CCI_REG8(0xA803), 0x11 },
+ { CCI_REG8(0xA804), 0x62 },
+ { CCI_REG8(0xA805), 0x77 },
+ { CCI_REG8(0xA806), 0xAE },
+ { CCI_REG8(0xA807), 0x34 },
+ { CCI_REG8(0xA808), 0xAE },
+ { CCI_REG8(0xA809), 0x35 },
+ { CCI_REG8(0xA80A), 0x62 },
+ { CCI_REG8(0xA80B), 0x83 },
+ { CCI_REG8(0xAE33), 0x00 },
/* analog setting */
- {0x4174, 0x00},
- {0x4175, 0x11},
- {0x4612, 0x29},
- {0x461B, 0x12},
- {0x461F, 0x06},
- {0x4635, 0x07},
- {0x4637, 0x30},
- {0x463F, 0x18},
- {0x4641, 0x0D},
- {0x465B, 0x12},
- {0x465F, 0x11},
- {0x4663, 0x11},
- {0x4667, 0x0F},
- {0x466F, 0x0F},
- {0x470E, 0x09},
- {0x4909, 0xAB},
- {0x490B, 0x95},
- {0x4915, 0x5D},
- {0x4A5F, 0xFF},
- {0x4A61, 0xFF},
- {0x4A73, 0x62},
- {0x4A85, 0x00},
- {0x4A87, 0xFF},
+ { CCI_REG8(0x4174), 0x00 },
+ { CCI_REG8(0x4175), 0x11 },
+ { CCI_REG8(0x4612), 0x29 },
+ { CCI_REG8(0x461B), 0x12 },
+ { CCI_REG8(0x461F), 0x06 },
+ { CCI_REG8(0x4635), 0x07 },
+ { CCI_REG8(0x4637), 0x30 },
+ { CCI_REG8(0x463F), 0x18 },
+ { CCI_REG8(0x4641), 0x0D },
+ { CCI_REG8(0x465B), 0x12 },
+ { CCI_REG8(0x465F), 0x11 },
+ { CCI_REG8(0x4663), 0x11 },
+ { CCI_REG8(0x4667), 0x0F },
+ { CCI_REG8(0x466F), 0x0F },
+ { CCI_REG8(0x470E), 0x09 },
+ { CCI_REG8(0x4909), 0xAB },
+ { CCI_REG8(0x490B), 0x95 },
+ { CCI_REG8(0x4915), 0x5D },
+ { CCI_REG8(0x4A5F), 0xFF },
+ { CCI_REG8(0x4A61), 0xFF },
+ { CCI_REG8(0x4A73), 0x62 },
+ { CCI_REG8(0x4A85), 0x00 },
+ { CCI_REG8(0x4A87), 0xFF },
/* embedded data */
- {0x5041, 0x04},
- {0x583C, 0x04},
- {0x620E, 0x04},
- {0x6EB2, 0x01},
- {0x6EB3, 0x00},
- {0x9300, 0x02},
+ { CCI_REG8(0x5041), 0x04 },
+ { CCI_REG8(0x583C), 0x04 },
+ { CCI_REG8(0x620E), 0x04 },
+ { CCI_REG8(0x6EB2), 0x01 },
+ { CCI_REG8(0x6EB3), 0x00 },
+ { CCI_REG8(0x9300), 0x02 },
/* imagequality */
/* HDR setting */
- {0x3001, 0x07},
- {0x6D12, 0x3F},
- {0x6D13, 0xFF},
- {0x9344, 0x03},
- {0x9706, 0x10},
- {0x9707, 0x03},
- {0x9708, 0x03},
- {0x9E04, 0x01},
- {0x9E05, 0x00},
- {0x9E0C, 0x01},
- {0x9E0D, 0x02},
- {0x9E24, 0x00},
- {0x9E25, 0x8C},
- {0x9E26, 0x00},
- {0x9E27, 0x94},
- {0x9E28, 0x00},
- {0x9E29, 0x96},
+ { CCI_REG8(0x3001), 0x07 },
+ { CCI_REG8(0x6D12), 0x3F },
+ { CCI_REG8(0x6D13), 0xFF },
+ { CCI_REG8(0x9344), 0x03 },
+ { CCI_REG8(0x9706), 0x10 },
+ { CCI_REG8(0x9707), 0x03 },
+ { CCI_REG8(0x9708), 0x03 },
+ { CCI_REG8(0x9E04), 0x01 },
+ { CCI_REG8(0x9E05), 0x00 },
+ { CCI_REG8(0x9E0C), 0x01 },
+ { CCI_REG8(0x9E0D), 0x02 },
+ { CCI_REG8(0x9E24), 0x00 },
+ { CCI_REG8(0x9E25), 0x8C },
+ { CCI_REG8(0x9E26), 0x00 },
+ { CCI_REG8(0x9E27), 0x94 },
+ { CCI_REG8(0x9E28), 0x00 },
+ { CCI_REG8(0x9E29), 0x96 },
/* CNR parameter setting */
- {0x69DB, 0x01},
+ { CCI_REG8(0x69DB), 0x01 },
/* Moire reduction */
- {0x6957, 0x01},
+ { CCI_REG8(0x6957), 0x01 },
/* image enhancement */
- {0x6987, 0x17},
- {0x698A, 0x03},
- {0x698B, 0x03},
+ { CCI_REG8(0x6987), 0x17 },
+ { CCI_REG8(0x698A), 0x03 },
+ { CCI_REG8(0x698B), 0x03 },
/* white balanace */
- {0x0B8E, 0x01},
- {0x0B8F, 0x00},
- {0x0B90, 0x01},
- {0x0B91, 0x00},
- {0x0B92, 0x01},
- {0x0B93, 0x00},
- {0x0B94, 0x01},
- {0x0B95, 0x00},
+ { CCI_REG8(0x0B8E), 0x01 },
+ { CCI_REG8(0x0B8F), 0x00 },
+ { CCI_REG8(0x0B90), 0x01 },
+ { CCI_REG8(0x0B91), 0x00 },
+ { CCI_REG8(0x0B92), 0x01 },
+ { CCI_REG8(0x0B93), 0x00 },
+ { CCI_REG8(0x0B94), 0x01 },
+ { CCI_REG8(0x0B95), 0x00 },
/* ATR setting */
- {0x6E50, 0x00},
- {0x6E51, 0x32},
- {0x9340, 0x00},
- {0x9341, 0x3C},
- {0x9342, 0x03},
- {0x9343, 0xFF},
- {IMX214_TABLE_END, 0x00}
+ { CCI_REG8(0x6E50), 0x00 },
+ { CCI_REG8(0x6E51), 0x32 },
+ { CCI_REG8(0x9340), 0x00 },
+ { CCI_REG8(0x9341), 0x3C },
+ { CCI_REG8(0x9342), 0x03 },
+ { CCI_REG8(0x9343), 0xFF },
};
/*
@@ -421,16 +401,19 @@ static const struct reg_8 mode_table_common[] = {
static const struct imx214_mode {
u32 width;
u32 height;
- const struct reg_8 *reg_table;
+ unsigned int num_of_regs;
+ const struct cci_reg_sequence *reg_table;
} imx214_modes[] = {
{
.width = 4096,
.height = 2304,
+ .num_of_regs = ARRAY_SIZE(mode_4096x2304),
.reg_table = mode_4096x2304,
},
{
.width = 1920,
.height = 1080,
+ .num_of_regs = ARRAY_SIZE(mode_1920x1080),
.reg_table = mode_1920x1080,
},
};
@@ -634,7 +617,6 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
{
struct imx214 *imx214 = container_of(ctrl->handler,
struct imx214, ctrls);
- u8 vals[2];
int ret;
/*
@@ -646,12 +628,7 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
switch (ctrl->id) {
case V4L2_CID_EXPOSURE:
- vals[1] = ctrl->val;
- vals[0] = ctrl->val >> 8;
- ret = regmap_bulk_write(imx214->regmap, IMX214_REG_EXPOSURE, vals, 2);
- if (ret < 0)
- dev_err(imx214->dev, "Error %d\n", ret);
- ret = 0;
+ cci_write(imx214->regmap, IMX214_REG_EXPOSURE, ctrl->val, &ret);
break;
default:
@@ -737,61 +714,35 @@ static int imx214_ctrls_init(struct imx214 *imx214)
return 0;
};
-#define MAX_CMD 4
-static int imx214_write_table(struct imx214 *imx214,
- const struct reg_8 table[])
-{
- u8 vals[MAX_CMD];
- int i;
- int ret;
-
- for (; table->addr != IMX214_TABLE_END ; table++) {
- if (table->addr == IMX214_TABLE_WAIT_MS) {
- usleep_range(table->val * 1000,
- table->val * 1000 + 500);
- continue;
- }
-
- for (i = 0; i < MAX_CMD; i++) {
- if (table[i].addr != (table[0].addr + i))
- break;
- vals[i] = table[i].val;
- }
-
- ret = regmap_bulk_write(imx214->regmap, table->addr, vals, i);
-
- if (ret) {
- dev_err(imx214->dev, "write_table error: %d\n", ret);
- return ret;
- }
-
- table += i - 1;
- }
-
- return 0;
-}
-
static int imx214_start_streaming(struct imx214 *imx214)
{
int ret;
- ret = imx214_write_table(imx214, mode_table_common);
+ ret = cci_multi_reg_write(imx214->regmap, mode_table_common,
+ ARRAY_SIZE(mode_table_common), NULL);
if (ret < 0) {
dev_err(imx214->dev, "could not sent common table %d\n", ret);
return ret;
}
- ret = imx214_write_table(imx214, imx214->cur_mode->reg_table);
+ ret = cci_multi_reg_write(imx214->regmap, imx214->cur_mode->reg_table,
+ imx214->cur_mode->num_of_regs, NULL);
if (ret < 0) {
dev_err(imx214->dev, "could not sent mode table %d\n", ret);
return ret;
}
+
+ usleep_range(10000, 10500);
+
+ cci_write(imx214->regmap, CCI_REG8(0x0138), 0x01, NULL);
+
ret = __v4l2_ctrl_handler_setup(&imx214->ctrls);
if (ret < 0) {
dev_err(imx214->dev, "could not sync v4l2 controls\n");
return ret;
}
- ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STREAMING);
+ ret = cci_write(imx214->regmap, IMX214_REG_MODE_SELECT,
+ IMX214_MODE_STREAMING, NULL);
if (ret < 0)
dev_err(imx214->dev, "could not sent start table %d\n", ret);
@@ -802,7 +753,8 @@ static int imx214_stop_streaming(struct imx214 *imx214)
{
int ret;
- ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STANDBY);
+ ret = cci_write(imx214->regmap, IMX214_REG_MODE_SELECT,
+ IMX214_MODE_STANDBY, NULL);
if (ret < 0)
dev_err(imx214->dev, "could not sent stop table %d\n", ret);
@@ -903,12 +855,6 @@ static const struct v4l2_subdev_internal_ops imx214_internal_ops = {
.init_state = imx214_entity_init_state,
};
-static const struct regmap_config sensor_regmap_config = {
- .reg_bits = 16,
- .val_bits = 8,
- .cache_type = REGCACHE_MAPLE,
-};
-
static int imx214_get_regulators(struct device *dev, struct imx214 *imx214)
{
unsigned int i;
@@ -991,10 +937,10 @@ static int imx214_probe(struct i2c_client *client)
return dev_err_probe(dev, PTR_ERR(imx214->enable_gpio),
"failed to get enable gpio\n");
- imx214->regmap = devm_regmap_init_i2c(client, &sensor_regmap_config);
+ imx214->regmap = devm_cci_regmap_init_i2c(client, 16);
if (IS_ERR(imx214->regmap))
return dev_err_probe(dev, PTR_ERR(imx214->regmap),
- "regmap init failed\n");
+ "failed to initialize CCI\n");
v4l2_i2c_subdev_init(&imx214->sd, client, &imx214_subdev_ops);
imx214->sd.internal_ops = &imx214_internal_ops;
--
2.47.1
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v3 04/12] media: i2c: imx214: Replace register addresses with macros
2024-12-07 20:47 [PATCH v3 00/12] media: i2c: imx214: Miscellaneous cleanups and improvements André Apitzsch via B4 Relay
` (2 preceding siblings ...)
2024-12-07 20:47 ` [PATCH v3 03/12] media: i2c: imx214: Convert to CCI register access helpers André Apitzsch via B4 Relay
@ 2024-12-07 20:47 ` André Apitzsch via B4 Relay
2024-12-07 20:47 ` [PATCH v3 05/12] media: i2c: imx214: Drop IMX214_REG_EXPOSURE from mode reg arrays André Apitzsch via B4 Relay
` (7 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: André Apitzsch via B4 Relay @ 2024-12-07 20:47 UTC (permalink / raw)
To: Ricardo Ribalda, Sakari Ailus, Mauro Carvalho Chehab
Cc: ~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht,
André Apitzsch, Ricardo Ribalda
From: André Apitzsch <git@apitzsch.eu>
Define macros for all the known registers used in the register arrays,
and use them to replace the numerical addresses. This improves
readability.
Acked-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
drivers/media/i2c/imx214.c | 407 ++++++++++++++++++++++++++-------------------
1 file changed, 236 insertions(+), 171 deletions(-)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 9b077f73dc21a534a0342919f81d7672bd267015..41b2a098957a8a0636b95b7985be96dc508e01f9 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -24,18 +24,141 @@
#define IMX214_MODE_STANDBY 0x00
#define IMX214_MODE_STREAMING 0x01
+#define IMX214_REG_FAST_STANDBY_CTRL CCI_REG8(0x0106)
+
#define IMX214_DEFAULT_CLK_FREQ 24000000
#define IMX214_DEFAULT_LINK_FREQ 480000000
#define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10)
#define IMX214_FPS 30
#define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
+/* V-TIMING internal */
+#define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340)
+
/* Exposure control */
#define IMX214_REG_EXPOSURE CCI_REG16(0x0202)
#define IMX214_EXPOSURE_MIN 0
#define IMX214_EXPOSURE_MAX 3184
#define IMX214_EXPOSURE_STEP 1
#define IMX214_EXPOSURE_DEFAULT 3184
+#define IMX214_REG_EXPOSURE_RATIO CCI_REG8(0x0222)
+#define IMX214_REG_SHORT_EXPOSURE CCI_REG16(0x0224)
+
+/* Analog gain control */
+#define IMX214_REG_ANALOG_GAIN CCI_REG16(0x0204)
+#define IMX214_REG_SHORT_ANALOG_GAIN CCI_REG16(0x0216)
+
+/* Digital gain control */
+#define IMX214_REG_DIG_GAIN_GREENR CCI_REG16(0x020e)
+#define IMX214_REG_DIG_GAIN_RED CCI_REG16(0x0210)
+#define IMX214_REG_DIG_GAIN_BLUE CCI_REG16(0x0212)
+#define IMX214_REG_DIG_GAIN_GREENB CCI_REG16(0x0214)
+
+#define IMX214_REG_ORIENTATION CCI_REG8(0x0101)
+
+#define IMX214_REG_MASK_CORR_FRAMES CCI_REG8(0x0105)
+#define IMX214_CORR_FRAMES_TRANSMIT 0
+#define IMX214_CORR_FRAMES_MASK 1
+
+#define IMX214_REG_CSI_DATA_FORMAT CCI_REG16(0x0112)
+#define IMX214_CSI_DATA_FORMAT_RAW8 0x0808
+#define IMX214_CSI_DATA_FORMAT_RAW10 0x0A0A
+#define IMX214_CSI_DATA_FORMAT_COMP6 0x0A06
+#define IMX214_CSI_DATA_FORMAT_COMP8 0x0A08
+
+#define IMX214_REG_CSI_LANE_MODE CCI_REG8(0x0114)
+#define IMX214_CSI_2_LANE_MODE 1
+#define IMX214_CSI_4_LANE_MODE 3
+
+#define IMX214_REG_EXCK_FREQ CCI_REG16(0x0136)
+#define IMX214_EXCK_FREQ(n) ((n) * 256) /* n expressed in MHz */
+
+#define IMX214_REG_TEMP_SENSOR_CONTROL CCI_REG8(0x0138)
+
+#define IMX214_REG_HDR_MODE CCI_REG8(0x0220)
+#define IMX214_HDR_MODE_OFF 0
+#define IMX214_HDR_MODE_ON 1
+
+#define IMX214_REG_HDR_RES_REDUCTION CCI_REG8(0x0221)
+#define IMX214_HDR_RES_REDU_THROUGH 0x11
+#define IMX214_HDR_RES_REDU_2_BINNING 0x22
+
+/* PLL settings */
+#define IMX214_REG_VTPXCK_DIV CCI_REG8(0x0301)
+#define IMX214_REG_VTSYCK_DIV CCI_REG8(0x0303)
+#define IMX214_REG_PREPLLCK_VT_DIV CCI_REG8(0x0305)
+#define IMX214_REG_PLL_VT_MPY CCI_REG16(0x0306)
+#define IMX214_REG_OPPXCK_DIV CCI_REG8(0x0309)
+#define IMX214_REG_OPSYCK_DIV CCI_REG8(0x030b)
+#define IMX214_REG_PLL_MULT_DRIV CCI_REG8(0x0310)
+#define IMX214_PLL_SINGLE 0
+#define IMX214_PLL_DUAL 1
+
+#define IMX214_REG_LINE_LENGTH_PCK CCI_REG16(0x0342)
+#define IMX214_REG_X_ADD_STA CCI_REG16(0x0344)
+#define IMX214_REG_Y_ADD_STA CCI_REG16(0x0346)
+#define IMX214_REG_X_ADD_END CCI_REG16(0x0348)
+#define IMX214_REG_Y_ADD_END CCI_REG16(0x034a)
+#define IMX214_REG_X_OUTPUT_SIZE CCI_REG16(0x034c)
+#define IMX214_REG_Y_OUTPUT_SIZE CCI_REG16(0x034e)
+#define IMX214_REG_X_EVEN_INC CCI_REG8(0x0381)
+#define IMX214_REG_X_ODD_INC CCI_REG8(0x0383)
+#define IMX214_REG_Y_EVEN_INC CCI_REG8(0x0385)
+#define IMX214_REG_Y_ODD_INC CCI_REG8(0x0387)
+
+#define IMX214_REG_SCALE_MODE CCI_REG8(0x0401)
+#define IMX214_SCALE_NONE 0
+#define IMX214_SCALE_HORIZONTAL 1
+#define IMX214_SCALE_FULL 2
+#define IMX214_REG_SCALE_M CCI_REG16(0x0404)
+
+#define IMX214_REG_DIG_CROP_X_OFFSET CCI_REG16(0x0408)
+#define IMX214_REG_DIG_CROP_Y_OFFSET CCI_REG16(0x040a)
+#define IMX214_REG_DIG_CROP_WIDTH CCI_REG16(0x040c)
+#define IMX214_REG_DIG_CROP_HEIGHT CCI_REG16(0x040e)
+
+#define IMX214_REG_REQ_LINK_BIT_RATE CCI_REG32(0x0820)
+#define IMX214_LINK_BIT_RATE_MBPS(n) ((n) << 16)
+
+/* Binning mode */
+#define IMX214_REG_BINNING_MODE CCI_REG8(0x0900)
+#define IMX214_BINNING_NONE 0
+#define IMX214_BINNING_ENABLE 1
+#define IMX214_REG_BINNING_TYPE CCI_REG8(0x0901)
+#define IMX214_REG_BINNING_WEIGHTING CCI_REG8(0x0902)
+#define IMX214_BINNING_AVERAGE 0x00
+#define IMX214_BINNING_SUMMED 0x01
+#define IMX214_BINNING_BAYER 0x02
+
+#define IMX214_REG_SING_DEF_CORR_EN CCI_REG8(0x0b06)
+#define IMX214_SING_DEF_CORR_OFF 0
+#define IMX214_SING_DEF_CORR_ON 1
+
+/* AWB control */
+#define IMX214_REG_ABS_GAIN_GREENR CCI_REG16(0x0b8e)
+#define IMX214_REG_ABS_GAIN_RED CCI_REG16(0x0b90)
+#define IMX214_REG_ABS_GAIN_BLUE CCI_REG16(0x0b92)
+#define IMX214_REG_ABS_GAIN_GREENB CCI_REG16(0x0b94)
+
+#define IMX214_REG_RMSC_NR_MODE CCI_REG8(0x3001)
+#define IMX214_REG_STATS_OUT_EN CCI_REG8(0x3013)
+#define IMX214_STATS_OUT_OFF 0
+#define IMX214_STATS_OUT_ON 1
+
+/* Chroma noise reduction */
+#define IMX214_REG_NML_NR_EN CCI_REG8(0x30a2)
+#define IMX214_NML_NR_OFF 0
+#define IMX214_NML_NR_ON 1
+
+#define IMX214_REG_EBD_SIZE_V CCI_REG8(0x5041)
+#define IMX214_EBD_NO 0
+#define IMX214_EBD_4_LINE 4
+
+#define IMX214_REG_RG_STATS_LMT CCI_REG16(0x6d12)
+#define IMX214_RG_STATS_LMT_10_BIT 0x03FF
+#define IMX214_RG_STATS_LMT_14_BIT 0x3FFF
+
+#define IMX214_REG_ATR_FAST_MOVE CCI_REG8(0x9300)
/* IMX214 native and active pixel array size */
#define IMX214_NATIVE_WIDTH 4224U
@@ -76,96 +199,70 @@ struct imx214 {
/*From imx214_mode_tbls.h*/
static const struct cci_reg_sequence mode_4096x2304[] = {
- { CCI_REG8(0x0114), 0x03 },
- { CCI_REG8(0x0220), 0x00 },
- { CCI_REG8(0x0221), 0x11 },
- { CCI_REG8(0x0222), 0x01 },
- { CCI_REG8(0x0340), 0x0C },
- { CCI_REG8(0x0341), 0x7A },
- { CCI_REG8(0x0342), 0x13 },
- { CCI_REG8(0x0343), 0x90 },
- { CCI_REG8(0x0344), 0x00 },
- { CCI_REG8(0x0345), 0x38 },
- { CCI_REG8(0x0346), 0x01 },
- { CCI_REG8(0x0347), 0x98 },
- { CCI_REG8(0x0348), 0x10 },
- { CCI_REG8(0x0349), 0x37 },
- { CCI_REG8(0x034A), 0x0A },
- { CCI_REG8(0x034B), 0x97 },
- { CCI_REG8(0x0381), 0x01 },
- { CCI_REG8(0x0383), 0x01 },
- { CCI_REG8(0x0385), 0x01 },
- { CCI_REG8(0x0387), 0x01 },
- { CCI_REG8(0x0900), 0x00 },
- { CCI_REG8(0x0901), 0x00 },
- { CCI_REG8(0x0902), 0x00 },
+ { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE },
+ { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
+ { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
+ { IMX214_REG_EXPOSURE_RATIO, 1 },
+ { IMX214_REG_FRM_LENGTH_LINES, 3194 },
+ { IMX214_REG_LINE_LENGTH_PCK, 5008 },
+ { IMX214_REG_X_ADD_STA, 56 },
+ { IMX214_REG_Y_ADD_STA, 408 },
+ { IMX214_REG_X_ADD_END, 4151 },
+ { IMX214_REG_Y_ADD_END, 2711 },
+ { IMX214_REG_X_EVEN_INC, 1 },
+ { IMX214_REG_X_ODD_INC, 1 },
+ { IMX214_REG_Y_EVEN_INC, 1 },
+ { IMX214_REG_Y_ODD_INC, 1 },
+ { IMX214_REG_BINNING_MODE, IMX214_BINNING_NONE },
+ { IMX214_REG_BINNING_TYPE, 0 },
+ { IMX214_REG_BINNING_WEIGHTING, IMX214_BINNING_AVERAGE },
{ CCI_REG8(0x3000), 0x35 },
{ CCI_REG8(0x3054), 0x01 },
{ CCI_REG8(0x305C), 0x11 },
- { CCI_REG8(0x0112), 0x0A },
- { CCI_REG8(0x0113), 0x0A },
- { CCI_REG8(0x034C), 0x10 },
- { CCI_REG8(0x034D), 0x00 },
- { CCI_REG8(0x034E), 0x09 },
- { CCI_REG8(0x034F), 0x00 },
- { CCI_REG8(0x0401), 0x00 },
- { CCI_REG8(0x0404), 0x00 },
- { CCI_REG8(0x0405), 0x10 },
- { CCI_REG8(0x0408), 0x00 },
- { CCI_REG8(0x0409), 0x00 },
- { CCI_REG8(0x040A), 0x00 },
- { CCI_REG8(0x040B), 0x00 },
- { CCI_REG8(0x040C), 0x10 },
- { CCI_REG8(0x040D), 0x00 },
- { CCI_REG8(0x040E), 0x09 },
- { CCI_REG8(0x040F), 0x00 },
-
- { CCI_REG8(0x0301), 0x05 },
- { CCI_REG8(0x0303), 0x02 },
- { CCI_REG8(0x0305), 0x03 },
- { CCI_REG8(0x0306), 0x00 },
- { CCI_REG8(0x0307), 0x96 },
- { CCI_REG8(0x0309), 0x0A },
- { CCI_REG8(0x030B), 0x01 },
- { CCI_REG8(0x0310), 0x00 },
-
- { CCI_REG8(0x0820), 0x12 },
- { CCI_REG8(0x0821), 0xC0 },
- { CCI_REG8(0x0822), 0x00 },
- { CCI_REG8(0x0823), 0x00 },
+ { IMX214_REG_CSI_DATA_FORMAT, IMX214_CSI_DATA_FORMAT_RAW10 },
+ { IMX214_REG_X_OUTPUT_SIZE, 4096 },
+ { IMX214_REG_Y_OUTPUT_SIZE, 2304 },
+ { IMX214_REG_SCALE_MODE, IMX214_SCALE_NONE },
+ { IMX214_REG_SCALE_M, 2 },
+ { IMX214_REG_DIG_CROP_X_OFFSET, 0 },
+ { IMX214_REG_DIG_CROP_Y_OFFSET, 0 },
+ { IMX214_REG_DIG_CROP_WIDTH, 4096 },
+ { IMX214_REG_DIG_CROP_HEIGHT, 2304 },
+
+ { IMX214_REG_VTPXCK_DIV, 5 },
+ { IMX214_REG_VTSYCK_DIV, 2 },
+ { IMX214_REG_PREPLLCK_VT_DIV, 3 },
+ { IMX214_REG_PLL_VT_MPY, 150 },
+ { IMX214_REG_OPPXCK_DIV, 10 },
+ { IMX214_REG_OPSYCK_DIV, 1 },
+ { IMX214_REG_PLL_MULT_DRIV, IMX214_PLL_SINGLE },
+
+ { IMX214_REG_REQ_LINK_BIT_RATE, IMX214_LINK_BIT_RATE_MBPS(4800) },
{ CCI_REG8(0x3A03), 0x09 },
{ CCI_REG8(0x3A04), 0x50 },
{ CCI_REG8(0x3A05), 0x01 },
- { CCI_REG8(0x0B06), 0x01 },
- { CCI_REG8(0x30A2), 0x00 },
+ { IMX214_REG_SING_DEF_CORR_EN, IMX214_SING_DEF_CORR_ON },
+ { IMX214_REG_NML_NR_EN, IMX214_NML_NR_OFF },
{ CCI_REG8(0x30B4), 0x00 },
{ CCI_REG8(0x3A02), 0xFF },
{ CCI_REG8(0x3011), 0x00 },
- { CCI_REG8(0x3013), 0x01 },
-
- { CCI_REG8(0x0202), 0x0C },
- { CCI_REG8(0x0203), 0x70 },
- { CCI_REG8(0x0224), 0x01 },
- { CCI_REG8(0x0225), 0xF4 },
-
- { CCI_REG8(0x0204), 0x00 },
- { CCI_REG8(0x0205), 0x00 },
- { CCI_REG8(0x020E), 0x01 },
- { CCI_REG8(0x020F), 0x00 },
- { CCI_REG8(0x0210), 0x01 },
- { CCI_REG8(0x0211), 0x00 },
- { CCI_REG8(0x0212), 0x01 },
- { CCI_REG8(0x0213), 0x00 },
- { CCI_REG8(0x0214), 0x01 },
- { CCI_REG8(0x0215), 0x00 },
- { CCI_REG8(0x0216), 0x00 },
- { CCI_REG8(0x0217), 0x00 },
+ { IMX214_REG_STATS_OUT_EN, IMX214_STATS_OUT_ON },
+
+ { IMX214_REG_EXPOSURE, IMX214_EXPOSURE_DEFAULT },
+ { IMX214_REG_SHORT_EXPOSURE, 500 },
+
+ { IMX214_REG_ANALOG_GAIN, 0 },
+ { IMX214_REG_DIG_GAIN_GREENR, 256 },
+ { IMX214_REG_DIG_GAIN_RED, 256 },
+ { IMX214_REG_DIG_GAIN_BLUE, 256 },
+ { IMX214_REG_DIG_GAIN_GREENB, 256 },
+ { IMX214_REG_SHORT_ANALOG_GAIN, 0 },
{ CCI_REG8(0x4170), 0x00 },
{ CCI_REG8(0x4171), 0x10 },
@@ -176,96 +273,70 @@ static const struct cci_reg_sequence mode_4096x2304[] = {
};
static const struct cci_reg_sequence mode_1920x1080[] = {
- { CCI_REG8(0x0114), 0x03 },
- { CCI_REG8(0x0220), 0x00 },
- { CCI_REG8(0x0221), 0x11 },
- { CCI_REG8(0x0222), 0x01 },
- { CCI_REG8(0x0340), 0x0C },
- { CCI_REG8(0x0341), 0x7A },
- { CCI_REG8(0x0342), 0x13 },
- { CCI_REG8(0x0343), 0x90 },
- { CCI_REG8(0x0344), 0x04 },
- { CCI_REG8(0x0345), 0x78 },
- { CCI_REG8(0x0346), 0x03 },
- { CCI_REG8(0x0347), 0xFC },
- { CCI_REG8(0x0348), 0x0B },
- { CCI_REG8(0x0349), 0xF7 },
- { CCI_REG8(0x034A), 0x08 },
- { CCI_REG8(0x034B), 0x33 },
- { CCI_REG8(0x0381), 0x01 },
- { CCI_REG8(0x0383), 0x01 },
- { CCI_REG8(0x0385), 0x01 },
- { CCI_REG8(0x0387), 0x01 },
- { CCI_REG8(0x0900), 0x00 },
- { CCI_REG8(0x0901), 0x00 },
- { CCI_REG8(0x0902), 0x00 },
+ { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE },
+ { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
+ { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
+ { IMX214_REG_EXPOSURE_RATIO, 1 },
+ { IMX214_REG_FRM_LENGTH_LINES, 3194 },
+ { IMX214_REG_LINE_LENGTH_PCK, 5008 },
+ { IMX214_REG_X_ADD_STA, 1144 },
+ { IMX214_REG_Y_ADD_STA, 1020 },
+ { IMX214_REG_X_ADD_END, 3063 },
+ { IMX214_REG_Y_ADD_END, 2099 },
+ { IMX214_REG_X_EVEN_INC, 1 },
+ { IMX214_REG_X_ODD_INC, 1 },
+ { IMX214_REG_Y_EVEN_INC, 1 },
+ { IMX214_REG_Y_ODD_INC, 1 },
+ { IMX214_REG_BINNING_MODE, IMX214_BINNING_NONE },
+ { IMX214_REG_BINNING_TYPE, 0 },
+ { IMX214_REG_BINNING_WEIGHTING, IMX214_BINNING_AVERAGE },
{ CCI_REG8(0x3000), 0x35 },
{ CCI_REG8(0x3054), 0x01 },
{ CCI_REG8(0x305C), 0x11 },
- { CCI_REG8(0x0112), 0x0A },
- { CCI_REG8(0x0113), 0x0A },
- { CCI_REG8(0x034C), 0x07 },
- { CCI_REG8(0x034D), 0x80 },
- { CCI_REG8(0x034E), 0x04 },
- { CCI_REG8(0x034F), 0x38 },
- { CCI_REG8(0x0401), 0x00 },
- { CCI_REG8(0x0404), 0x00 },
- { CCI_REG8(0x0405), 0x10 },
- { CCI_REG8(0x0408), 0x00 },
- { CCI_REG8(0x0409), 0x00 },
- { CCI_REG8(0x040A), 0x00 },
- { CCI_REG8(0x040B), 0x00 },
- { CCI_REG8(0x040C), 0x07 },
- { CCI_REG8(0x040D), 0x80 },
- { CCI_REG8(0x040E), 0x04 },
- { CCI_REG8(0x040F), 0x38 },
-
- { CCI_REG8(0x0301), 0x05 },
- { CCI_REG8(0x0303), 0x02 },
- { CCI_REG8(0x0305), 0x03 },
- { CCI_REG8(0x0306), 0x00 },
- { CCI_REG8(0x0307), 0x96 },
- { CCI_REG8(0x0309), 0x0A },
- { CCI_REG8(0x030B), 0x01 },
- { CCI_REG8(0x0310), 0x00 },
-
- { CCI_REG8(0x0820), 0x12 },
- { CCI_REG8(0x0821), 0xC0 },
- { CCI_REG8(0x0822), 0x00 },
- { CCI_REG8(0x0823), 0x00 },
+ { IMX214_REG_CSI_DATA_FORMAT, IMX214_CSI_DATA_FORMAT_RAW10 },
+ { IMX214_REG_X_OUTPUT_SIZE, 1920 },
+ { IMX214_REG_Y_OUTPUT_SIZE, 1080 },
+ { IMX214_REG_SCALE_MODE, IMX214_SCALE_NONE },
+ { IMX214_REG_SCALE_M, 2 },
+ { IMX214_REG_DIG_CROP_X_OFFSET, 0 },
+ { IMX214_REG_DIG_CROP_Y_OFFSET, 0 },
+ { IMX214_REG_DIG_CROP_WIDTH, 1920 },
+ { IMX214_REG_DIG_CROP_HEIGHT, 1080 },
+
+ { IMX214_REG_VTPXCK_DIV, 5 },
+ { IMX214_REG_VTSYCK_DIV, 2 },
+ { IMX214_REG_PREPLLCK_VT_DIV, 3 },
+ { IMX214_REG_PLL_VT_MPY, 150 },
+ { IMX214_REG_OPPXCK_DIV, 10 },
+ { IMX214_REG_OPSYCK_DIV, 1 },
+ { IMX214_REG_PLL_MULT_DRIV, IMX214_PLL_SINGLE },
+
+ { IMX214_REG_REQ_LINK_BIT_RATE, IMX214_LINK_BIT_RATE_MBPS(4800) },
{ CCI_REG8(0x3A03), 0x04 },
{ CCI_REG8(0x3A04), 0xF8 },
{ CCI_REG8(0x3A05), 0x02 },
- { CCI_REG8(0x0B06), 0x01 },
- { CCI_REG8(0x30A2), 0x00 },
+ { IMX214_REG_SING_DEF_CORR_EN, IMX214_SING_DEF_CORR_ON },
+ { IMX214_REG_NML_NR_EN, IMX214_NML_NR_OFF },
{ CCI_REG8(0x30B4), 0x00 },
{ CCI_REG8(0x3A02), 0xFF },
{ CCI_REG8(0x3011), 0x00 },
- { CCI_REG8(0x3013), 0x01 },
-
- { CCI_REG8(0x0202), 0x0C },
- { CCI_REG8(0x0203), 0x70 },
- { CCI_REG8(0x0224), 0x01 },
- { CCI_REG8(0x0225), 0xF4 },
-
- { CCI_REG8(0x0204), 0x00 },
- { CCI_REG8(0x0205), 0x00 },
- { CCI_REG8(0x020E), 0x01 },
- { CCI_REG8(0x020F), 0x00 },
- { CCI_REG8(0x0210), 0x01 },
- { CCI_REG8(0x0211), 0x00 },
- { CCI_REG8(0x0212), 0x01 },
- { CCI_REG8(0x0213), 0x00 },
- { CCI_REG8(0x0214), 0x01 },
- { CCI_REG8(0x0215), 0x00 },
- { CCI_REG8(0x0216), 0x00 },
- { CCI_REG8(0x0217), 0x00 },
+ { IMX214_REG_STATS_OUT_EN, IMX214_STATS_OUT_ON },
+
+ { IMX214_REG_EXPOSURE, IMX214_EXPOSURE_DEFAULT },
+ { IMX214_REG_SHORT_EXPOSURE, 500 },
+
+ { IMX214_REG_ANALOG_GAIN, 0 },
+ { IMX214_REG_DIG_GAIN_GREENR, 256 },
+ { IMX214_REG_DIG_GAIN_RED, 256 },
+ { IMX214_REG_DIG_GAIN_BLUE, 256 },
+ { IMX214_REG_DIG_GAIN_GREENB, 256 },
+ { IMX214_REG_SHORT_ANALOG_GAIN, 0 },
{ CCI_REG8(0x4170), 0x00 },
{ CCI_REG8(0x4171), 0x10 },
@@ -279,20 +350,19 @@ static const struct cci_reg_sequence mode_table_common[] = {
/* software reset */
/* software standby settings */
- { CCI_REG8(0x0100), 0x00 },
+ { IMX214_REG_MODE_SELECT, IMX214_MODE_STANDBY },
/* ATR setting */
- { CCI_REG8(0x9300), 0x02 },
+ { IMX214_REG_ATR_FAST_MOVE, 2 },
/* external clock setting */
- { CCI_REG8(0x0136), 0x18 },
- { CCI_REG8(0x0137), 0x00 },
+ { IMX214_REG_EXCK_FREQ, IMX214_EXCK_FREQ(IMX214_DEFAULT_CLK_FREQ / 1000000) },
/* global setting */
/* basic config */
- { CCI_REG8(0x0101), 0x00 },
- { CCI_REG8(0x0105), 0x01 },
- { CCI_REG8(0x0106), 0x01 },
+ { IMX214_REG_ORIENTATION, 0 },
+ { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK },
+ { IMX214_REG_FAST_STANDBY_CTRL, 1 },
{ CCI_REG8(0x4550), 0x02 },
{ CCI_REG8(0x4601), 0x00 },
{ CCI_REG8(0x4642), 0x05 },
@@ -337,18 +407,17 @@ static const struct cci_reg_sequence mode_table_common[] = {
{ CCI_REG8(0x4A87), 0xFF },
/* embedded data */
- { CCI_REG8(0x5041), 0x04 },
+ { IMX214_REG_EBD_SIZE_V, IMX214_EBD_4_LINE },
{ CCI_REG8(0x583C), 0x04 },
{ CCI_REG8(0x620E), 0x04 },
{ CCI_REG8(0x6EB2), 0x01 },
{ CCI_REG8(0x6EB3), 0x00 },
- { CCI_REG8(0x9300), 0x02 },
+ { IMX214_REG_ATR_FAST_MOVE, 2 },
/* imagequality */
/* HDR setting */
- { CCI_REG8(0x3001), 0x07 },
- { CCI_REG8(0x6D12), 0x3F },
- { CCI_REG8(0x6D13), 0xFF },
+ { IMX214_REG_RMSC_NR_MODE, 0x07 },
+ { IMX214_REG_RG_STATS_LMT, IMX214_RG_STATS_LMT_14_BIT },
{ CCI_REG8(0x9344), 0x03 },
{ CCI_REG8(0x9706), 0x10 },
{ CCI_REG8(0x9707), 0x03 },
@@ -376,14 +445,10 @@ static const struct cci_reg_sequence mode_table_common[] = {
{ CCI_REG8(0x698B), 0x03 },
/* white balanace */
- { CCI_REG8(0x0B8E), 0x01 },
- { CCI_REG8(0x0B8F), 0x00 },
- { CCI_REG8(0x0B90), 0x01 },
- { CCI_REG8(0x0B91), 0x00 },
- { CCI_REG8(0x0B92), 0x01 },
- { CCI_REG8(0x0B93), 0x00 },
- { CCI_REG8(0x0B94), 0x01 },
- { CCI_REG8(0x0B95), 0x00 },
+ { IMX214_REG_ABS_GAIN_GREENR, 0x0100 },
+ { IMX214_REG_ABS_GAIN_RED, 0x0100 },
+ { IMX214_REG_ABS_GAIN_BLUE, 0x0100 },
+ { IMX214_REG_ABS_GAIN_GREENB, 0x0100 },
/* ATR setting */
{ CCI_REG8(0x6E50), 0x00 },
@@ -734,7 +799,7 @@ static int imx214_start_streaming(struct imx214 *imx214)
usleep_range(10000, 10500);
- cci_write(imx214->regmap, CCI_REG8(0x0138), 0x01, NULL);
+ cci_write(imx214->regmap, IMX214_REG_TEMP_SENSOR_CONTROL, 0x01, NULL);
ret = __v4l2_ctrl_handler_setup(&imx214->ctrls);
if (ret < 0) {
--
2.47.1
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v3 05/12] media: i2c: imx214: Drop IMX214_REG_EXPOSURE from mode reg arrays
2024-12-07 20:47 [PATCH v3 00/12] media: i2c: imx214: Miscellaneous cleanups and improvements André Apitzsch via B4 Relay
` (3 preceding siblings ...)
2024-12-07 20:47 ` [PATCH v3 04/12] media: i2c: imx214: Replace register addresses with macros André Apitzsch via B4 Relay
@ 2024-12-07 20:47 ` André Apitzsch via B4 Relay
2024-12-07 20:47 ` [PATCH v3 06/12] media: i2c: imx214: Check number of lanes from device tree André Apitzsch via B4 Relay
` (6 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: André Apitzsch via B4 Relay @ 2024-12-07 20:47 UTC (permalink / raw)
To: Ricardo Ribalda, Sakari Ailus, Mauro Carvalho Chehab
Cc: ~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht,
André Apitzsch, Ricardo Ribalda
From: André Apitzsch <git@apitzsch.eu>
The IMX214_REG_EXPOSURE is configured twice, once with a hardcoded value
in the mode_<res> registers arrays, and once via v4l2_ctrl_ops. The
latter is enough, drop the former.
Acked-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
drivers/media/i2c/imx214.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 41b2a098957a8a0636b95b7985be96dc508e01f9..07926f7257877155548f7bcf0e7ee80037b4ce6c 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -254,7 +254,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = {
{ CCI_REG8(0x3011), 0x00 },
{ IMX214_REG_STATS_OUT_EN, IMX214_STATS_OUT_ON },
- { IMX214_REG_EXPOSURE, IMX214_EXPOSURE_DEFAULT },
{ IMX214_REG_SHORT_EXPOSURE, 500 },
{ IMX214_REG_ANALOG_GAIN, 0 },
@@ -328,7 +327,6 @@ static const struct cci_reg_sequence mode_1920x1080[] = {
{ CCI_REG8(0x3011), 0x00 },
{ IMX214_REG_STATS_OUT_EN, IMX214_STATS_OUT_ON },
- { IMX214_REG_EXPOSURE, IMX214_EXPOSURE_DEFAULT },
{ IMX214_REG_SHORT_EXPOSURE, 500 },
{ IMX214_REG_ANALOG_GAIN, 0 },
--
2.47.1
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v3 06/12] media: i2c: imx214: Check number of lanes from device tree
2024-12-07 20:47 [PATCH v3 00/12] media: i2c: imx214: Miscellaneous cleanups and improvements André Apitzsch via B4 Relay
` (4 preceding siblings ...)
2024-12-07 20:47 ` [PATCH v3 05/12] media: i2c: imx214: Drop IMX214_REG_EXPOSURE from mode reg arrays André Apitzsch via B4 Relay
@ 2024-12-07 20:47 ` André Apitzsch via B4 Relay
2024-12-08 21:07 ` Ricardo Ribalda Delgado
2024-12-10 12:35 ` Sakari Ailus
2024-12-07 20:47 ` [PATCH v3 07/12] media: i2c: imx214: Add vblank and hblank controls André Apitzsch via B4 Relay
` (5 subsequent siblings)
11 siblings, 2 replies; 27+ messages in thread
From: André Apitzsch via B4 Relay @ 2024-12-07 20:47 UTC (permalink / raw)
To: Ricardo Ribalda, Sakari Ailus, Mauro Carvalho Chehab
Cc: ~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht,
André Apitzsch
From: André Apitzsch <git@apitzsch.eu>
The imx214 camera is capable of either two-lane or four-lane operation.
Currently only the four-lane mode is supported, as proper pixel rates
and link frequences for the two-lane mode are unknown.
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
drivers/media/i2c/imx214.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 07926f7257877155548f7bcf0e7ee80037b4ce6c..f1c72db0775eaf4810f762e8798d301c5ad9923c 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -199,7 +199,6 @@ struct imx214 {
/*From imx214_mode_tbls.h*/
static const struct cci_reg_sequence mode_4096x2304[] = {
- { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE },
{ IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
{ IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
{ IMX214_REG_EXPOSURE_RATIO, 1 },
@@ -272,7 +271,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = {
};
static const struct cci_reg_sequence mode_1920x1080[] = {
- { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE },
{ IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
{ IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
{ IMX214_REG_EXPOSURE_RATIO, 1 },
@@ -788,6 +786,13 @@ static int imx214_start_streaming(struct imx214 *imx214)
return ret;
}
+ ret = cci_write(imx214->regmap, IMX214_REG_CSI_LANE_MODE,
+ IMX214_CSI_4_LANE_MODE, NULL);
+ if (ret) {
+ dev_err(imx214->dev, "%s failed to configure lanes\n", __func__);
+ return ret;
+ }
+
ret = cci_multi_reg_write(imx214->regmap, imx214->cur_mode->reg_table,
imx214->cur_mode->num_of_regs, NULL);
if (ret < 0) {
@@ -948,6 +953,13 @@ static int imx214_parse_fwnode(struct device *dev)
goto done;
}
+ /* Check the number of MIPI CSI2 data lanes */
+ if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
+ ret = dev_err_probe(dev, -EINVAL,
+ "only 4 data lanes are currently supported\n");
+ goto done;
+ }
+
for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ)
break;
--
2.47.1
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v3 06/12] media: i2c: imx214: Check number of lanes from device tree
2024-12-07 20:47 ` [PATCH v3 06/12] media: i2c: imx214: Check number of lanes from device tree André Apitzsch via B4 Relay
@ 2024-12-08 21:07 ` Ricardo Ribalda Delgado
2024-12-10 12:35 ` Sakari Ailus
1 sibling, 0 replies; 27+ messages in thread
From: Ricardo Ribalda Delgado @ 2024-12-08 21:07 UTC (permalink / raw)
To: git
Cc: Sakari Ailus, Mauro Carvalho Chehab, ~postmarketos/upstreaming,
phone-devel, linux-media, linux-kernel, Dave Stevenson,
Vincent Knecht
Acked-by: Ricardo Ribalda <ribalda@chromium.org>
On Sat, Dec 7, 2024 at 9:48 PM André Apitzsch via B4 Relay
<devnull+git.apitzsch.eu@kernel.org> wrote:
>
> From: André Apitzsch <git@apitzsch.eu>
>
> The imx214 camera is capable of either two-lane or four-lane operation.
>
> Currently only the four-lane mode is supported, as proper pixel rates
> and link frequences for the two-lane mode are unknown.
>
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
> drivers/media/i2c/imx214.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 07926f7257877155548f7bcf0e7ee80037b4ce6c..f1c72db0775eaf4810f762e8798d301c5ad9923c 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -199,7 +199,6 @@ struct imx214 {
>
> /*From imx214_mode_tbls.h*/
> static const struct cci_reg_sequence mode_4096x2304[] = {
> - { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE },
> { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
> { IMX214_REG_EXPOSURE_RATIO, 1 },
> @@ -272,7 +271,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = {
> };
>
> static const struct cci_reg_sequence mode_1920x1080[] = {
> - { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE },
> { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
> { IMX214_REG_EXPOSURE_RATIO, 1 },
> @@ -788,6 +786,13 @@ static int imx214_start_streaming(struct imx214 *imx214)
> return ret;
> }
>
> + ret = cci_write(imx214->regmap, IMX214_REG_CSI_LANE_MODE,
> + IMX214_CSI_4_LANE_MODE, NULL);
> + if (ret) {
> + dev_err(imx214->dev, "%s failed to configure lanes\n", __func__);
> + return ret;
> + }
> +
> ret = cci_multi_reg_write(imx214->regmap, imx214->cur_mode->reg_table,
> imx214->cur_mode->num_of_regs, NULL);
> if (ret < 0) {
> @@ -948,6 +953,13 @@ static int imx214_parse_fwnode(struct device *dev)
> goto done;
> }
>
nit: I'd have only added this check, and do not introduce any of the
other changes until we support more lanes
But if you prefer this way and works, I have no objection.
Thanks!
> + /* Check the number of MIPI CSI2 data lanes */
> + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> + ret = dev_err_probe(dev, -EINVAL,
> + "only 4 data lanes are currently supported\n");
> + goto done;
> + }
> +
> for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ)
> break;
>
> --
> 2.47.1
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v3 06/12] media: i2c: imx214: Check number of lanes from device tree
2024-12-07 20:47 ` [PATCH v3 06/12] media: i2c: imx214: Check number of lanes from device tree André Apitzsch via B4 Relay
2024-12-08 21:07 ` Ricardo Ribalda Delgado
@ 2024-12-10 12:35 ` Sakari Ailus
1 sibling, 0 replies; 27+ messages in thread
From: Sakari Ailus @ 2024-12-10 12:35 UTC (permalink / raw)
To: git
Cc: Ricardo Ribalda, Mauro Carvalho Chehab,
~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht
Hi André,
On Sat, Dec 07, 2024 at 09:47:55PM +0100, André Apitzsch via B4 Relay wrote:
> From: André Apitzsch <git@apitzsch.eu>
>
> The imx214 camera is capable of either two-lane or four-lane operation.
>
> Currently only the four-lane mode is supported, as proper pixel rates
> and link frequences for the two-lane mode are unknown.
>
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
> drivers/media/i2c/imx214.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 07926f7257877155548f7bcf0e7ee80037b4ce6c..f1c72db0775eaf4810f762e8798d301c5ad9923c 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -199,7 +199,6 @@ struct imx214 {
>
> /*From imx214_mode_tbls.h*/
> static const struct cci_reg_sequence mode_4096x2304[] = {
> - { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE },
> { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
> { IMX214_REG_EXPOSURE_RATIO, 1 },
> @@ -272,7 +271,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = {
> };
>
> static const struct cci_reg_sequence mode_1920x1080[] = {
> - { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE },
> { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
> { IMX214_REG_EXPOSURE_RATIO, 1 },
> @@ -788,6 +786,13 @@ static int imx214_start_streaming(struct imx214 *imx214)
> return ret;
> }
>
> + ret = cci_write(imx214->regmap, IMX214_REG_CSI_LANE_MODE,
> + IMX214_CSI_4_LANE_MODE, NULL);
> + if (ret) {
> + dev_err(imx214->dev, "%s failed to configure lanes\n", __func__);
You can drop the function name here.
> + return ret;
> + }
> +
> ret = cci_multi_reg_write(imx214->regmap, imx214->cur_mode->reg_table,
> imx214->cur_mode->num_of_regs, NULL);
> if (ret < 0) {
> @@ -948,6 +953,13 @@ static int imx214_parse_fwnode(struct device *dev)
> goto done;
> }
>
> + /* Check the number of MIPI CSI2 data lanes */
> + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> + ret = dev_err_probe(dev, -EINVAL,
> + "only 4 data lanes are currently supported\n");
> + goto done;
> + }
> +
> for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ)
> break;
>
--
Sakari Ailus
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 07/12] media: i2c: imx214: Add vblank and hblank controls
2024-12-07 20:47 [PATCH v3 00/12] media: i2c: imx214: Miscellaneous cleanups and improvements André Apitzsch via B4 Relay
` (5 preceding siblings ...)
2024-12-07 20:47 ` [PATCH v3 06/12] media: i2c: imx214: Check number of lanes from device tree André Apitzsch via B4 Relay
@ 2024-12-07 20:47 ` André Apitzsch via B4 Relay
2024-12-08 20:59 ` Ricardo Ribalda Delgado
2024-12-07 20:47 ` [PATCH v3 08/12] media: i2c: imx214: Implement vflip/hflip controls André Apitzsch via B4 Relay
` (4 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: André Apitzsch via B4 Relay @ 2024-12-07 20:47 UTC (permalink / raw)
To: Ricardo Ribalda, Sakari Ailus, Mauro Carvalho Chehab
Cc: ~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht,
André Apitzsch
From: André Apitzsch <git@apitzsch.eu>
Add vblank control to allow changing the framerate /
higher exposure values.
The vblank and hblank controls are needed for libcamera support.
While at it, fix the minimal exposure time according to the datasheet.
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
drivers/media/i2c/imx214.c | 106 ++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 94 insertions(+), 12 deletions(-)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index f1c72db0775eaf4810f762e8798d301c5ad9923c..a7f49dbafe0f54af3c02f5534460fdee88a22fe2 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -34,11 +34,17 @@
/* V-TIMING internal */
#define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340)
+#define IMX214_VTS_MAX 0xffff
+
+#define IMX214_VBLANK_MIN 890
+
+/* HBLANK control - read only */
+#define IMX214_PPL_DEFAULT 5008
/* Exposure control */
#define IMX214_REG_EXPOSURE CCI_REG16(0x0202)
-#define IMX214_EXPOSURE_MIN 0
-#define IMX214_EXPOSURE_MAX 3184
+#define IMX214_EXPOSURE_OFFSET 10
+#define IMX214_EXPOSURE_MIN 1
#define IMX214_EXPOSURE_STEP 1
#define IMX214_EXPOSURE_DEFAULT 3184
#define IMX214_REG_EXPOSURE_RATIO CCI_REG8(0x0222)
@@ -187,6 +193,8 @@ struct imx214 {
struct v4l2_ctrl_handler ctrls;
struct v4l2_ctrl *pixel_rate;
struct v4l2_ctrl *link_freq;
+ struct v4l2_ctrl *vblank;
+ struct v4l2_ctrl *hblank;
struct v4l2_ctrl *exposure;
struct v4l2_ctrl *unit_size;
@@ -202,8 +210,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = {
{ IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
{ IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
{ IMX214_REG_EXPOSURE_RATIO, 1 },
- { IMX214_REG_FRM_LENGTH_LINES, 3194 },
- { IMX214_REG_LINE_LENGTH_PCK, 5008 },
{ IMX214_REG_X_ADD_STA, 56 },
{ IMX214_REG_Y_ADD_STA, 408 },
{ IMX214_REG_X_ADD_END, 4151 },
@@ -274,8 +280,6 @@ static const struct cci_reg_sequence mode_1920x1080[] = {
{ IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
{ IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
{ IMX214_REG_EXPOSURE_RATIO, 1 },
- { IMX214_REG_FRM_LENGTH_LINES, 3194 },
- { IMX214_REG_LINE_LENGTH_PCK, 5008 },
{ IMX214_REG_X_ADD_STA, 1144 },
{ IMX214_REG_Y_ADD_STA, 1020 },
{ IMX214_REG_X_ADD_END, 3063 },
@@ -359,6 +363,7 @@ static const struct cci_reg_sequence mode_table_common[] = {
{ IMX214_REG_ORIENTATION, 0 },
{ IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK },
{ IMX214_REG_FAST_STANDBY_CTRL, 1 },
+ { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT },
{ CCI_REG8(0x4550), 0x02 },
{ CCI_REG8(0x4601), 0x00 },
{ CCI_REG8(0x4642), 0x05 },
@@ -462,18 +467,24 @@ static const struct cci_reg_sequence mode_table_common[] = {
static const struct imx214_mode {
u32 width;
u32 height;
+
+ /* V-timing */
+ unsigned int vts_def;
+
unsigned int num_of_regs;
const struct cci_reg_sequence *reg_table;
} imx214_modes[] = {
{
.width = 4096,
.height = 2304,
+ .vts_def = 3194,
.num_of_regs = ARRAY_SIZE(mode_4096x2304),
.reg_table = mode_4096x2304,
},
{
.width = 1920,
.height = 1080,
+ .vts_def = 3194,
.num_of_regs = ARRAY_SIZE(mode_1920x1080),
.reg_table = mode_1920x1080,
},
@@ -626,9 +637,36 @@ static int imx214_set_format(struct v4l2_subdev *sd,
__crop->width = mode->width;
__crop->height = mode->height;
- if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+ if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+ int exposure_max;
+ int exposure_def;
+ int hblank;
+
imx214->cur_mode = mode;
+ /* Update FPS limits */
+ __v4l2_ctrl_modify_range(imx214->vblank, IMX214_VBLANK_MIN,
+ IMX214_VTS_MAX - mode->height, 2,
+ mode->vts_def - mode->height);
+
+ /* Update max exposure while meeting expected vblanking */
+ exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
+ exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
+ __v4l2_ctrl_modify_range(imx214->exposure,
+ imx214->exposure->minimum,
+ exposure_max, imx214->exposure->step,
+ exposure_def);
+
+ /*
+ * Currently PPL is fixed to IMX214_PPL_DEFAULT, so hblank
+ * depends on mode->width only, and is not changeable in any
+ * way other than changing the mode.
+ */
+ hblank = IMX214_PPL_DEFAULT - mode->width;
+ __v4l2_ctrl_modify_range(imx214->hblank, hblank, hblank, 1,
+ hblank);
+ }
+
return 0;
}
@@ -678,8 +716,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
{
struct imx214 *imx214 = container_of(ctrl->handler,
struct imx214, ctrls);
+ const struct v4l2_mbus_framefmt *format;
+ struct v4l2_subdev_state *state;
int ret;
+ if (ctrl->id == V4L2_CID_VBLANK) {
+ int exposure_max, exposure_def;
+
+ state = v4l2_subdev_get_locked_active_state(&imx214->sd);
+ format = v4l2_subdev_state_get_format(state, 0);
+
+ /* Update max exposure while meeting expected vblanking */
+ exposure_max = format->height + ctrl->val - IMX214_EXPOSURE_OFFSET;
+ exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
+ __v4l2_ctrl_modify_range(imx214->exposure,
+ imx214->exposure->minimum,
+ exposure_max, imx214->exposure->step,
+ exposure_def);
+ }
+
/*
* Applying V4L2 control value only happens
* when power is up for streaming
@@ -691,7 +746,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
case V4L2_CID_EXPOSURE:
cci_write(imx214->regmap, IMX214_REG_EXPOSURE, ctrl->val, &ret);
break;
-
+ case V4L2_CID_VBLANK:
+ cci_write(imx214->regmap, IMX214_REG_FRM_LENGTH_LINES,
+ format->height + ctrl->val, &ret);
+ break;
default:
ret = -EINVAL;
}
@@ -714,8 +772,11 @@ static int imx214_ctrls_init(struct imx214 *imx214)
.width = 1120,
.height = 1120,
};
+ const struct imx214_mode *mode = &imx214_modes[0];
struct v4l2_fwnode_device_properties props;
struct v4l2_ctrl_handler *ctrl_hdlr;
+ int exposure_max, exposure_def;
+ int hblank;
int ret;
ret = v4l2_fwnode_device_parse(imx214->dev, &props);
@@ -723,7 +784,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
return ret;
ctrl_hdlr = &imx214->ctrls;
- ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6);
+ ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8);
if (ret)
return ret;
@@ -749,12 +810,27 @@ static int imx214_ctrls_init(struct imx214 *imx214)
*
* Yours sincerely, Ricardo.
*/
+
+ /* Initial vblank/hblank/exposure parameters based on current mode */
+ imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
+ V4L2_CID_VBLANK, IMX214_VBLANK_MIN,
+ IMX214_VTS_MAX - mode->height, 2,
+ mode->vts_def - mode->height);
+
+ hblank = IMX214_PPL_DEFAULT - mode->width;
+ imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
+ V4L2_CID_HBLANK, hblank, hblank,
+ 1, hblank);
+ if (imx214->hblank)
+ imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+ exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
+ exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
V4L2_CID_EXPOSURE,
- IMX214_EXPOSURE_MIN,
- IMX214_EXPOSURE_MAX,
+ IMX214_EXPOSURE_MIN, exposure_max,
IMX214_EXPOSURE_STEP,
- IMX214_EXPOSURE_DEFAULT);
+ exposure_def);
imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr,
NULL,
@@ -876,6 +952,12 @@ static int imx214_get_frame_interval(struct v4l2_subdev *subdev,
return 0;
}
+/*
+ * Raw sensors should be using the VBLANK and HBLANK controls to determine
+ * the frame rate. However this driver was initially added using the
+ * [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of 30fps.
+ * Retain the frame_interval ops for backwards compatibility, but they do nothing.
+ */
static int imx214_enum_frame_interval(struct v4l2_subdev *subdev,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_frame_interval_enum *fie)
--
2.47.1
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v3 07/12] media: i2c: imx214: Add vblank and hblank controls
2024-12-07 20:47 ` [PATCH v3 07/12] media: i2c: imx214: Add vblank and hblank controls André Apitzsch via B4 Relay
@ 2024-12-08 20:59 ` Ricardo Ribalda Delgado
2024-12-08 21:19 ` Ricardo Ribalda Delgado
` (2 more replies)
0 siblings, 3 replies; 27+ messages in thread
From: Ricardo Ribalda Delgado @ 2024-12-08 20:59 UTC (permalink / raw)
To: git
Cc: Sakari Ailus, Mauro Carvalho Chehab, ~postmarketos/upstreaming,
phone-devel, linux-media, linux-kernel, Dave Stevenson,
Vincent Knecht
In general it looks good to me (besides the comments, ignore the nits
if you want to).
I'd recommend that you test with lockdep to make sure that we are not
missing anything, and I'd like to hear back from Sakari regarding the
get_locked_active
Thanks!
On Sat, Dec 7, 2024 at 9:48 PM André Apitzsch via B4 Relay
<devnull+git.apitzsch.eu@kernel.org> wrote:
>
> From: André Apitzsch <git@apitzsch.eu>
>
> Add vblank control to allow changing the framerate /
> higher exposure values.
>
> The vblank and hblank controls are needed for libcamera support.
>
> While at it, fix the minimal exposure time according to the datasheet.
>
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
> drivers/media/i2c/imx214.c | 106 ++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 94 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index f1c72db0775eaf4810f762e8798d301c5ad9923c..a7f49dbafe0f54af3c02f5534460fdee88a22fe2 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -34,11 +34,17 @@
>
> /* V-TIMING internal */
> #define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340)
> +#define IMX214_VTS_MAX 0xffff
> +
> +#define IMX214_VBLANK_MIN 890
> +
> +/* HBLANK control - read only */
> +#define IMX214_PPL_DEFAULT 5008
>
> /* Exposure control */
> #define IMX214_REG_EXPOSURE CCI_REG16(0x0202)
> -#define IMX214_EXPOSURE_MIN 0
> -#define IMX214_EXPOSURE_MAX 3184
> +#define IMX214_EXPOSURE_OFFSET 10
> +#define IMX214_EXPOSURE_MIN 1
> #define IMX214_EXPOSURE_STEP 1
> #define IMX214_EXPOSURE_DEFAULT 3184
> #define IMX214_REG_EXPOSURE_RATIO CCI_REG8(0x0222)
> @@ -187,6 +193,8 @@ struct imx214 {
> struct v4l2_ctrl_handler ctrls;
> struct v4l2_ctrl *pixel_rate;
> struct v4l2_ctrl *link_freq;
> + struct v4l2_ctrl *vblank;
> + struct v4l2_ctrl *hblank;
> struct v4l2_ctrl *exposure;
> struct v4l2_ctrl *unit_size;
>
> @@ -202,8 +210,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = {
> { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
> { IMX214_REG_EXPOSURE_RATIO, 1 },
> - { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> - { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> { IMX214_REG_X_ADD_STA, 56 },
> { IMX214_REG_Y_ADD_STA, 408 },
> { IMX214_REG_X_ADD_END, 4151 },
> @@ -274,8 +280,6 @@ static const struct cci_reg_sequence mode_1920x1080[] = {
> { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
> { IMX214_REG_EXPOSURE_RATIO, 1 },
> - { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> - { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> { IMX214_REG_X_ADD_STA, 1144 },
> { IMX214_REG_Y_ADD_STA, 1020 },
> { IMX214_REG_X_ADD_END, 3063 },
> @@ -359,6 +363,7 @@ static const struct cci_reg_sequence mode_table_common[] = {
> { IMX214_REG_ORIENTATION, 0 },
> { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK },
> { IMX214_REG_FAST_STANDBY_CTRL, 1 },
> + { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT },
> { CCI_REG8(0x4550), 0x02 },
> { CCI_REG8(0x4601), 0x00 },
> { CCI_REG8(0x4642), 0x05 },
> @@ -462,18 +467,24 @@ static const struct cci_reg_sequence mode_table_common[] = {
> static const struct imx214_mode {
> u32 width;
> u32 height;
> +
> + /* V-timing */
> + unsigned int vts_def;
> +
> unsigned int num_of_regs;
> const struct cci_reg_sequence *reg_table;
> } imx214_modes[] = {
> {
> .width = 4096,
> .height = 2304,
> + .vts_def = 3194,
> .num_of_regs = ARRAY_SIZE(mode_4096x2304),
> .reg_table = mode_4096x2304,
> },
> {
> .width = 1920,
> .height = 1080,
> + .vts_def = 3194,
> .num_of_regs = ARRAY_SIZE(mode_1920x1080),
> .reg_table = mode_1920x1080,
> },
> @@ -626,9 +637,36 @@ static int imx214_set_format(struct v4l2_subdev *sd,
> __crop->width = mode->width;
> __crop->height = mode->height;
>
> - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> + int exposure_max;
> + int exposure_def;
> + int hblank;
> +
> imx214->cur_mode = mode;
>
> + /* Update FPS limits */
nit: Update blank limits
> + __v4l2_ctrl_modify_range(imx214->vblank, IMX214_VBLANK_MIN,
> + IMX214_VTS_MAX - mode->height, 2,
> + mode->vts_def - mode->height);
Is the handler->lock held when we call this function? Can you try
running the code with lockdep?
> +
> + /* Update max exposure while meeting expected vblanking */
> + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> + __v4l2_ctrl_modify_range(imx214->exposure,
> + imx214->exposure->minimum,
> + exposure_max, imx214->exposure->step,
> + exposure_def);
> +
> + /*
> + * Currently PPL is fixed to IMX214_PPL_DEFAULT, so hblank
> + * depends on mode->width only, and is not changeable in any
> + * way other than changing the mode.
> + */
> + hblank = IMX214_PPL_DEFAULT - mode->width;
> + __v4l2_ctrl_modify_range(imx214->hblank, hblank, hblank, 1,
> + hblank);
> + }
> +
> return 0;
> }
>
> @@ -678,8 +716,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct imx214 *imx214 = container_of(ctrl->handler,
> struct imx214, ctrls);
> + const struct v4l2_mbus_framefmt *format;
> + struct v4l2_subdev_state *state;
> int ret;
>
> + if (ctrl->id == V4L2_CID_VBLANK) {
> + int exposure_max, exposure_def;
> +
> + state = v4l2_subdev_get_locked_active_state(&imx214->sd);
Sakari, I see that other drivers assume that the active is locked in
set_ctrl. Is this correct?
> + format = v4l2_subdev_state_get_format(state, 0);
> +
> + /* Update max exposure while meeting expected vblanking */
> + exposure_max = format->height + ctrl->val - IMX214_EXPOSURE_OFFSET;
> + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> + __v4l2_ctrl_modify_range(imx214->exposure,
> + imx214->exposure->minimum,
> + exposure_max, imx214->exposure->step,
> + exposure_def);
> + }
> +
> /*
> * Applying V4L2 control value only happens
> * when power is up for streaming
> @@ -691,7 +746,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
> case V4L2_CID_EXPOSURE:
> cci_write(imx214->regmap, IMX214_REG_EXPOSURE, ctrl->val, &ret);
> break;
> -
> + case V4L2_CID_VBLANK:
No need to read the format here?
format = v4l2_subdev_state_get_format(state, 0);
> + cci_write(imx214->regmap, IMX214_REG_FRM_LENGTH_LINES,
> + format->height + ctrl->val, &ret);
> + break;
> default:
> ret = -EINVAL;
> }
> @@ -714,8 +772,11 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> .width = 1120,
> .height = 1120,
> };
> + const struct imx214_mode *mode = &imx214_modes[0];
> struct v4l2_fwnode_device_properties props;
> struct v4l2_ctrl_handler *ctrl_hdlr;
> + int exposure_max, exposure_def;
> + int hblank;
> int ret;
>
> ret = v4l2_fwnode_device_parse(imx214->dev, &props);
> @@ -723,7 +784,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> return ret;
>
> ctrl_hdlr = &imx214->ctrls;
> - ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6);
> + ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8);
> if (ret)
> return ret;
>
> @@ -749,12 +810,27 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> *
> * Yours sincerely, Ricardo.
> */
> +
> + /* Initial vblank/hblank/exposure parameters based on current mode */
> + imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> + V4L2_CID_VBLANK, IMX214_VBLANK_MIN,
> + IMX214_VTS_MAX - mode->height, 2,
> + mode->vts_def - mode->height);
> +
> + hblank = IMX214_PPL_DEFAULT - mode->width;
> + imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> + V4L2_CID_HBLANK, hblank, hblank,
> + 1, hblank);
> + if (imx214->hblank)
> + imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> V4L2_CID_EXPOSURE,
> - IMX214_EXPOSURE_MIN,
> - IMX214_EXPOSURE_MAX,
> + IMX214_EXPOSURE_MIN, exposure_max,
nit: I think it looks nicer with exposure_max in the next line, but
ignore if you prefer this way :)
> IMX214_EXPOSURE_STEP,
> - IMX214_EXPOSURE_DEFAULT);
> + exposure_def);
>
> imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr,
> NULL,
> @@ -876,6 +952,12 @@ static int imx214_get_frame_interval(struct v4l2_subdev *subdev,
> return 0;
> }
>
> +/*
> + * Raw sensors should be using the VBLANK and HBLANK controls to determine
> + * the frame rate. However this driver was initially added using the
> + * [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of 30fps.
> + * Retain the frame_interval ops for backwards compatibility, but they do nothing.
> + */
Now that these controls are useless... maybe we can do a dev_warn_once
when the user calls it to leave some output in dmesg?
> static int imx214_enum_frame_interval(struct v4l2_subdev *subdev,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_frame_interval_enum *fie)
>
> --
> 2.47.1
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v3 07/12] media: i2c: imx214: Add vblank and hblank controls
2024-12-08 20:59 ` Ricardo Ribalda Delgado
@ 2024-12-08 21:19 ` Ricardo Ribalda Delgado
2024-12-10 12:34 ` Sakari Ailus
2024-12-08 21:35 ` André Apitzsch
2024-12-15 22:35 ` André Apitzsch
2 siblings, 1 reply; 27+ messages in thread
From: Ricardo Ribalda Delgado @ 2024-12-08 21:19 UTC (permalink / raw)
To: git
Cc: Sakari Ailus, Mauro Carvalho Chehab, ~postmarketos/upstreaming,
phone-devel, linux-media, linux-kernel, Dave Stevenson,
Vincent Knecht
On Sun, Dec 8, 2024 at 9:59 PM Ricardo Ribalda Delgado
<ribalda@kernel.org> wrote:
>
> In general it looks good to me (besides the comments, ignore the nits
> if you want to).
>
> I'd recommend that you test with lockdep to make sure that we are not
> missing anything, and I'd like to hear back from Sakari regarding the
> get_locked_active
>
> Thanks!
>
> On Sat, Dec 7, 2024 at 9:48 PM André Apitzsch via B4 Relay
> <devnull+git.apitzsch.eu@kernel.org> wrote:
> >
> > From: André Apitzsch <git@apitzsch.eu>
> >
> > Add vblank control to allow changing the framerate /
> > higher exposure values.
> >
> > The vblank and hblank controls are needed for libcamera support.
> >
> > While at it, fix the minimal exposure time according to the datasheet.
> >
> > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > ---
> > drivers/media/i2c/imx214.c | 106 ++++++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 94 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > index f1c72db0775eaf4810f762e8798d301c5ad9923c..a7f49dbafe0f54af3c02f5534460fdee88a22fe2 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -34,11 +34,17 @@
> >
> > /* V-TIMING internal */
> > #define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340)
> > +#define IMX214_VTS_MAX 0xffff
> > +
> > +#define IMX214_VBLANK_MIN 890
> > +
> > +/* HBLANK control - read only */
> > +#define IMX214_PPL_DEFAULT 5008
> >
> > /* Exposure control */
> > #define IMX214_REG_EXPOSURE CCI_REG16(0x0202)
> > -#define IMX214_EXPOSURE_MIN 0
> > -#define IMX214_EXPOSURE_MAX 3184
> > +#define IMX214_EXPOSURE_OFFSET 10
> > +#define IMX214_EXPOSURE_MIN 1
> > #define IMX214_EXPOSURE_STEP 1
> > #define IMX214_EXPOSURE_DEFAULT 3184
> > #define IMX214_REG_EXPOSURE_RATIO CCI_REG8(0x0222)
> > @@ -187,6 +193,8 @@ struct imx214 {
> > struct v4l2_ctrl_handler ctrls;
> > struct v4l2_ctrl *pixel_rate;
> > struct v4l2_ctrl *link_freq;
> > + struct v4l2_ctrl *vblank;
> > + struct v4l2_ctrl *hblank;
> > struct v4l2_ctrl *exposure;
> > struct v4l2_ctrl *unit_size;
> >
> > @@ -202,8 +210,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = {
> > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
> > { IMX214_REG_EXPOSURE_RATIO, 1 },
> > - { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > - { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > { IMX214_REG_X_ADD_STA, 56 },
> > { IMX214_REG_Y_ADD_STA, 408 },
> > { IMX214_REG_X_ADD_END, 4151 },
> > @@ -274,8 +280,6 @@ static const struct cci_reg_sequence mode_1920x1080[] = {
> > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
> > { IMX214_REG_EXPOSURE_RATIO, 1 },
> > - { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > - { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > { IMX214_REG_X_ADD_STA, 1144 },
> > { IMX214_REG_Y_ADD_STA, 1020 },
> > { IMX214_REG_X_ADD_END, 3063 },
> > @@ -359,6 +363,7 @@ static const struct cci_reg_sequence mode_table_common[] = {
> > { IMX214_REG_ORIENTATION, 0 },
> > { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK },
> > { IMX214_REG_FAST_STANDBY_CTRL, 1 },
> > + { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT },
> > { CCI_REG8(0x4550), 0x02 },
> > { CCI_REG8(0x4601), 0x00 },
> > { CCI_REG8(0x4642), 0x05 },
> > @@ -462,18 +467,24 @@ static const struct cci_reg_sequence mode_table_common[] = {
> > static const struct imx214_mode {
> > u32 width;
> > u32 height;
> > +
> > + /* V-timing */
> > + unsigned int vts_def;
> > +
> > unsigned int num_of_regs;
> > const struct cci_reg_sequence *reg_table;
> > } imx214_modes[] = {
> > {
> > .width = 4096,
> > .height = 2304,
> > + .vts_def = 3194,
> > .num_of_regs = ARRAY_SIZE(mode_4096x2304),
> > .reg_table = mode_4096x2304,
> > },
> > {
> > .width = 1920,
> > .height = 1080,
> > + .vts_def = 3194,
> > .num_of_regs = ARRAY_SIZE(mode_1920x1080),
> > .reg_table = mode_1920x1080,
> > },
> > @@ -626,9 +637,36 @@ static int imx214_set_format(struct v4l2_subdev *sd,
> > __crop->width = mode->width;
> > __crop->height = mode->height;
> >
> > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + int exposure_max;
> > + int exposure_def;
> > + int hblank;
> > +
> > imx214->cur_mode = mode;
> >
> > + /* Update FPS limits */
> nit: Update blank limits
> > + __v4l2_ctrl_modify_range(imx214->vblank, IMX214_VBLANK_MIN,
> > + IMX214_VTS_MAX - mode->height, 2,
> > + mode->vts_def - mode->height);
>
> Is the handler->lock held when we call this function? Can you try
> running the code with lockdep?
> > +
> > + /* Update max exposure while meeting expected vblanking */
> > + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> > + __v4l2_ctrl_modify_range(imx214->exposure,
> > + imx214->exposure->minimum,
> > + exposure_max, imx214->exposure->step,
> > + exposure_def);
> > +
> > + /*
> > + * Currently PPL is fixed to IMX214_PPL_DEFAULT, so hblank
> > + * depends on mode->width only, and is not changeable in any
> > + * way other than changing the mode.
> > + */
> > + hblank = IMX214_PPL_DEFAULT - mode->width;
> > + __v4l2_ctrl_modify_range(imx214->hblank, hblank, hblank, 1,
> > + hblank);
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -678,8 +716,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
> > {
> > struct imx214 *imx214 = container_of(ctrl->handler,
> > struct imx214, ctrls);
> > + const struct v4l2_mbus_framefmt *format;
> > + struct v4l2_subdev_state *state;
> > int ret;
> >
> > + if (ctrl->id == V4L2_CID_VBLANK) {
> > + int exposure_max, exposure_def;
> > +
> > + state = v4l2_subdev_get_locked_active_state(&imx214->sd);
>
> Sakari, I see that other drivers assume that the active is locked in
> set_ctrl. Is this correct?
imx214->sd.state_lock = imx214->ctrls.lock;
So I guess it is fine :)
>
> > + format = v4l2_subdev_state_get_format(state, 0);
> > +
> > + /* Update max exposure while meeting expected vblanking */
> > + exposure_max = format->height + ctrl->val - IMX214_EXPOSURE_OFFSET;
> > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> > + __v4l2_ctrl_modify_range(imx214->exposure,
> > + imx214->exposure->minimum,
> > + exposure_max, imx214->exposure->step,
> > + exposure_def);
> > + }
> > +
> > /*
> > * Applying V4L2 control value only happens
> > * when power is up for streaming
> > @@ -691,7 +746,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
> > case V4L2_CID_EXPOSURE:
> > cci_write(imx214->regmap, IMX214_REG_EXPOSURE, ctrl->val, &ret);
> > break;
> > -
> > + case V4L2_CID_VBLANK:
> No need to read the format here?
> format = v4l2_subdev_state_get_format(state, 0);
> > + cci_write(imx214->regmap, IMX214_REG_FRM_LENGTH_LINES,
> > + format->height + ctrl->val, &ret);
> > + break;
> > default:
> > ret = -EINVAL;
> > }
> > @@ -714,8 +772,11 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> > .width = 1120,
> > .height = 1120,
> > };
> > + const struct imx214_mode *mode = &imx214_modes[0];
> > struct v4l2_fwnode_device_properties props;
> > struct v4l2_ctrl_handler *ctrl_hdlr;
> > + int exposure_max, exposure_def;
> > + int hblank;
> > int ret;
> >
> > ret = v4l2_fwnode_device_parse(imx214->dev, &props);
> > @@ -723,7 +784,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> > return ret;
> >
> > ctrl_hdlr = &imx214->ctrls;
> > - ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6);
> > + ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8);
> > if (ret)
> > return ret;
> >
> > @@ -749,12 +810,27 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> > *
> > * Yours sincerely, Ricardo.
> > */
> > +
> > + /* Initial vblank/hblank/exposure parameters based on current mode */
> > + imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> > + V4L2_CID_VBLANK, IMX214_VBLANK_MIN,
> > + IMX214_VTS_MAX - mode->height, 2,
> > + mode->vts_def - mode->height);
> > +
> > + hblank = IMX214_PPL_DEFAULT - mode->width;
> > + imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> > + V4L2_CID_HBLANK, hblank, hblank,
> > + 1, hblank);
> > + if (imx214->hblank)
> > + imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> > imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> > V4L2_CID_EXPOSURE,
> > - IMX214_EXPOSURE_MIN,
> > - IMX214_EXPOSURE_MAX,
> > + IMX214_EXPOSURE_MIN, exposure_max,
> nit: I think it looks nicer with exposure_max in the next line, but
> ignore if you prefer this way :)
> > IMX214_EXPOSURE_STEP,
> > - IMX214_EXPOSURE_DEFAULT);
> > + exposure_def);
> >
> > imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr,
> > NULL,
> > @@ -876,6 +952,12 @@ static int imx214_get_frame_interval(struct v4l2_subdev *subdev,
> > return 0;
> > }
> >
> > +/*
> > + * Raw sensors should be using the VBLANK and HBLANK controls to determine
> > + * the frame rate. However this driver was initially added using the
> > + * [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of 30fps.
> > + * Retain the frame_interval ops for backwards compatibility, but they do nothing.
> > + */
>
> Now that these controls are useless... maybe we can do a dev_warn_once
> when the user calls it to leave some output in dmesg?
> > static int imx214_enum_frame_interval(struct v4l2_subdev *subdev,
> > struct v4l2_subdev_state *sd_state,
> > struct v4l2_subdev_frame_interval_enum *fie)
> >
> > --
> > 2.47.1
> >
> >
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v3 07/12] media: i2c: imx214: Add vblank and hblank controls
2024-12-08 21:19 ` Ricardo Ribalda Delgado
@ 2024-12-10 12:34 ` Sakari Ailus
2024-12-14 22:45 ` André Apitzsch
0 siblings, 1 reply; 27+ messages in thread
From: Sakari Ailus @ 2024-12-10 12:34 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: git, Mauro Carvalho Chehab, ~postmarketos/upstreaming,
phone-devel, linux-media, linux-kernel, Dave Stevenson,
Vincent Knecht
On Sun, Dec 08, 2024 at 10:19:51PM +0100, Ricardo Ribalda Delgado wrote:
> > > + state = v4l2_subdev_get_locked_active_state(&imx214->sd);
> >
> > Sakari, I see that other drivers assume that the active is locked in
> > set_ctrl. Is this correct?
>
> imx214->sd.state_lock = imx214->ctrls.lock;
>
> So I guess it is fine :)
Yes, it is.
Please also run this on the set:
$ ./scripts/checkpatch.pl --strict --max-line-length=80
--
Sakari Ailus
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 07/12] media: i2c: imx214: Add vblank and hblank controls
2024-12-10 12:34 ` Sakari Ailus
@ 2024-12-14 22:45 ` André Apitzsch
2024-12-15 12:10 ` Sakari Ailus
0 siblings, 1 reply; 27+ messages in thread
From: André Apitzsch @ 2024-12-14 22:45 UTC (permalink / raw)
To: Sakari Ailus, Ricardo Ribalda Delgado
Cc: Mauro Carvalho Chehab, ~postmarketos/upstreaming, phone-devel,
linux-media, linux-kernel, Dave Stevenson, Vincent Knecht
Hi,
Am Dienstag, dem 10.12.2024 um 12:34 +0000 schrieb Sakari Ailus:
> On Sun, Dec 08, 2024 at 10:19:51PM +0100, Ricardo Ribalda Delgado
> wrote:
> > > > + state =
> > > > v4l2_subdev_get_locked_active_state(&imx214->sd);
> > >
> > > Sakari, I see that other drivers assume that the active is locked
> > > in
> > > set_ctrl. Is this correct?
> >
> > imx214->sd.state_lock = imx214->ctrls.lock;
> >
> > So I guess it is fine :)
>
> Yes, it is.
>
> Please also run this on the set:
>
> $ ./scripts/checkpatch.pl --strict --max-line-length=80
>
there are a few lines that exceed 80 columns:
WARNING: line length of 85 exceeds 80 columns
#163: FILE: drivers/media/i2c/imx214.c:576:
+ imx214_update_pad_format(imx214, mode, &format->format, format->format.code);
--
WARNING: line length of 81 exceeds 80 columns
#88: FILE: drivers/media/i2c/imx214.c:983:
+ return dev_err_probe(dev, ret, "failed to set xclk frequency\n");
--
WARNING: line length of 82 exceeds 80 columns
#138: FILE: drivers/media/i2c/imx214.c:1039:
+ dev_err_probe(dev, ret, "failed to register sensor sub-device\n");
--
WARNING: line length of 86 exceeds 80 columns
#491: FILE: drivers/media/i2c/imx214.c:359:
+ { IMX214_REG_EXCK_FREQ, IMX214_EXCK_FREQ(IMX214_DEFAULT_CLK_FREQ / 1000000) },
--
WARNING: line length of 83 exceeds 80 columns
#177: FILE: drivers/media/i2c/imx214.c:730:
+ exposure_max = format->height + ctrl->val - IMX214_EXPOSURE_OFFSET;
--
WARNING: line length of 85 exceeds 80 columns
#85: FILE: drivers/media/i2c/imx214.c:1231:
+ if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ_LEGACY) {
Is the strict 80 columns limit really necessary, as it would decrease
readability?
Best regards,
André
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v3 07/12] media: i2c: imx214: Add vblank and hblank controls
2024-12-14 22:45 ` André Apitzsch
@ 2024-12-15 12:10 ` Sakari Ailus
0 siblings, 0 replies; 27+ messages in thread
From: Sakari Ailus @ 2024-12-15 12:10 UTC (permalink / raw)
To: André Apitzsch
Cc: Ricardo Ribalda Delgado, Mauro Carvalho Chehab,
~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht
Hi André,
On Sat, Dec 14, 2024 at 11:45:16PM +0100, André Apitzsch wrote:
> Hi,
>
> Am Dienstag, dem 10.12.2024 um 12:34 +0000 schrieb Sakari Ailus:
> > On Sun, Dec 08, 2024 at 10:19:51PM +0100, Ricardo Ribalda Delgado
> > wrote:
> > > > > + state =
> > > > > v4l2_subdev_get_locked_active_state(&imx214->sd);
> > > >
> > > > Sakari, I see that other drivers assume that the active is locked
> > > > in
> > > > set_ctrl. Is this correct?
> > >
> > > imx214->sd.state_lock = imx214->ctrls.lock;
> > >
> > > So I guess it is fine :)
> >
> > Yes, it is.
> >
> > Please also run this on the set:
> >
> > $ ./scripts/checkpatch.pl --strict --max-line-length=80
> >
>
> there are a few lines that exceed 80 columns:
>
> WARNING: line length of 85 exceeds 80 columns
> #163: FILE: drivers/media/i2c/imx214.c:576:
> + imx214_update_pad_format(imx214, mode, &format->format, format->format.code);
> --
> WARNING: line length of 81 exceeds 80 columns
> #88: FILE: drivers/media/i2c/imx214.c:983:
> + return dev_err_probe(dev, ret, "failed to set xclk frequency\n");
> --
> WARNING: line length of 82 exceeds 80 columns
> #138: FILE: drivers/media/i2c/imx214.c:1039:
> + dev_err_probe(dev, ret, "failed to register sensor sub-device\n");
> --
> WARNING: line length of 86 exceeds 80 columns
> #491: FILE: drivers/media/i2c/imx214.c:359:
> + { IMX214_REG_EXCK_FREQ, IMX214_EXCK_FREQ(IMX214_DEFAULT_CLK_FREQ / 1000000) },
> --
> WARNING: line length of 83 exceeds 80 columns
> #177: FILE: drivers/media/i2c/imx214.c:730:
> + exposure_max = format->height + ctrl->val - IMX214_EXPOSURE_OFFSET;
> --
> WARNING: line length of 85 exceeds 80 columns
> #85: FILE: drivers/media/i2c/imx214.c:1231:
> + if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ_LEGACY) {
>
>
> Is the strict 80 columns limit really necessary, as it would decrease
> readability?
For the array that could be the case but I think I'd wrap the rest.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 07/12] media: i2c: imx214: Add vblank and hblank controls
2024-12-08 20:59 ` Ricardo Ribalda Delgado
2024-12-08 21:19 ` Ricardo Ribalda Delgado
@ 2024-12-08 21:35 ` André Apitzsch
2024-12-08 21:47 ` Ricardo Ribalda Delgado
2024-12-15 22:35 ` André Apitzsch
2 siblings, 1 reply; 27+ messages in thread
From: André Apitzsch @ 2024-12-08 21:35 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Sakari Ailus, Mauro Carvalho Chehab, ~postmarketos/upstreaming,
phone-devel, linux-media, linux-kernel, Dave Stevenson,
Vincent Knecht
Hi Ricardo,
Am Sonntag, dem 08.12.2024 um 21:59 +0100 schrieb Ricardo Ribalda
Delgado:
> In general it looks good to me (besides the comments, ignore the nits
> if you want to).
>
> I'd recommend that you test with lockdep to make sure that we are not
> missing anything, and I'd like to hear back from Sakari regarding the
> get_locked_active
>
> Thanks!
>
> On Sat, Dec 7, 2024 at 9:48 PM André Apitzsch via B4 Relay
> <devnull+git.apitzsch.eu@kernel.org> wrote:
> >
> > From: André Apitzsch <git@apitzsch.eu>
> >
> > Add vblank control to allow changing the framerate /
> > higher exposure values.
> >
> > The vblank and hblank controls are needed for libcamera support.
> >
> > While at it, fix the minimal exposure time according to the
> > datasheet.
> >
> > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > ---
> > drivers/media/i2c/imx214.c | 106
> > ++++++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 94 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx214.c
> > b/drivers/media/i2c/imx214.c
> > index
> > f1c72db0775eaf4810f762e8798d301c5ad9923c..a7f49dbafe0f54af3c02f5534
> > 460fdee88a22fe2 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -34,11 +34,17 @@
> >
> > /* V-TIMING internal */
> > #define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340)
> > +#define IMX214_VTS_MAX 0xffff
> > +
> > +#define IMX214_VBLANK_MIN 890
> > +
> > +/* HBLANK control - read only */
> > +#define IMX214_PPL_DEFAULT 5008
> >
> > /* Exposure control */
> > #define IMX214_REG_EXPOSURE CCI_REG16(0x0202)
> > -#define IMX214_EXPOSURE_MIN 0
> > -#define IMX214_EXPOSURE_MAX 3184
> > +#define IMX214_EXPOSURE_OFFSET 10
> > +#define IMX214_EXPOSURE_MIN 1
> > #define IMX214_EXPOSURE_STEP 1
> > #define IMX214_EXPOSURE_DEFAULT 3184
> > #define IMX214_REG_EXPOSURE_RATIO CCI_REG8(0x0222)
> > @@ -187,6 +193,8 @@ struct imx214 {
> > struct v4l2_ctrl_handler ctrls;
> > struct v4l2_ctrl *pixel_rate;
> > struct v4l2_ctrl *link_freq;
> > + struct v4l2_ctrl *vblank;
> > + struct v4l2_ctrl *hblank;
> > struct v4l2_ctrl *exposure;
> > struct v4l2_ctrl *unit_size;
> >
> > @@ -202,8 +210,6 @@ static const struct cci_reg_sequence
> > mode_4096x2304[] = {
> > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH
> > },
> > { IMX214_REG_EXPOSURE_RATIO, 1 },
> > - { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > - { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > { IMX214_REG_X_ADD_STA, 56 },
> > { IMX214_REG_Y_ADD_STA, 408 },
> > { IMX214_REG_X_ADD_END, 4151 },
> > @@ -274,8 +280,6 @@ static const struct cci_reg_sequence
> > mode_1920x1080[] = {
> > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH
> > },
> > { IMX214_REG_EXPOSURE_RATIO, 1 },
> > - { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > - { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > { IMX214_REG_X_ADD_STA, 1144 },
> > { IMX214_REG_Y_ADD_STA, 1020 },
> > { IMX214_REG_X_ADD_END, 3063 },
> > @@ -359,6 +363,7 @@ static const struct cci_reg_sequence
> > mode_table_common[] = {
> > { IMX214_REG_ORIENTATION, 0 },
> > { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK },
> > { IMX214_REG_FAST_STANDBY_CTRL, 1 },
> > + { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT },
> > { CCI_REG8(0x4550), 0x02 },
> > { CCI_REG8(0x4601), 0x00 },
> > { CCI_REG8(0x4642), 0x05 },
> > @@ -462,18 +467,24 @@ static const struct cci_reg_sequence
> > mode_table_common[] = {
> > static const struct imx214_mode {
> > u32 width;
> > u32 height;
> > +
> > + /* V-timing */
> > + unsigned int vts_def;
> > +
> > unsigned int num_of_regs;
> > const struct cci_reg_sequence *reg_table;
> > } imx214_modes[] = {
> > {
> > .width = 4096,
> > .height = 2304,
> > + .vts_def = 3194,
> > .num_of_regs = ARRAY_SIZE(mode_4096x2304),
> > .reg_table = mode_4096x2304,
> > },
> > {
> > .width = 1920,
> > .height = 1080,
> > + .vts_def = 3194,
> > .num_of_regs = ARRAY_SIZE(mode_1920x1080),
> > .reg_table = mode_1920x1080,
> > },
> > @@ -626,9 +637,36 @@ static int imx214_set_format(struct
> > v4l2_subdev *sd,
> > __crop->width = mode->width;
> > __crop->height = mode->height;
> >
> > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + int exposure_max;
> > + int exposure_def;
> > + int hblank;
> > +
> > imx214->cur_mode = mode;
> >
> > + /* Update FPS limits */
> nit: Update blank limits
> > + __v4l2_ctrl_modify_range(imx214->vblank,
> > IMX214_VBLANK_MIN,
> > + IMX214_VTS_MAX - mode-
> > >height, 2,
> > + mode->vts_def - mode-
> > >height);
>
> Is the handler->lock held when we call this function? Can you try
> running the code with lockdep?
> > +
> > + /* Update max exposure while meeting expected
> > vblanking */
> > + exposure_max = mode->vts_def -
> > IMX214_EXPOSURE_OFFSET;
> > + exposure_def = min(exposure_max,
> > IMX214_EXPOSURE_DEFAULT);
> > + __v4l2_ctrl_modify_range(imx214->exposure,
> > + imx214->exposure->minimum,
> > + exposure_max, imx214-
> > >exposure->step,
> > + exposure_def);
> > +
> > + /*
> > + * Currently PPL is fixed to IMX214_PPL_DEFAULT, so
> > hblank
> > + * depends on mode->width only, and is not
> > changeable in any
> > + * way other than changing the mode.
> > + */
> > + hblank = IMX214_PPL_DEFAULT - mode->width;
> > + __v4l2_ctrl_modify_range(imx214->hblank, hblank,
> > hblank, 1,
> > + hblank);
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -678,8 +716,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl
> > *ctrl)
> > {
> > struct imx214 *imx214 = container_of(ctrl->handler,
> > struct imx214, ctrls);
> > + const struct v4l2_mbus_framefmt *format;
> > + struct v4l2_subdev_state *state;
> > int ret;
> >
> > + if (ctrl->id == V4L2_CID_VBLANK) {
> > + int exposure_max, exposure_def;
> > +
> > + state =
> > v4l2_subdev_get_locked_active_state(&imx214->sd);
>
> Sakari, I see that other drivers assume that the active is locked in
> set_ctrl. Is this correct?
>
> > + format = v4l2_subdev_state_get_format(state, 0);
(*) The format is read here.
> > +
> > + /* Update max exposure while meeting expected
> > vblanking */
> > + exposure_max = format->height + ctrl->val -
> > IMX214_EXPOSURE_OFFSET;
> > + exposure_def = min(exposure_max,
> > IMX214_EXPOSURE_DEFAULT);
> > + __v4l2_ctrl_modify_range(imx214->exposure,
> > + imx214->exposure->minimum,
> > + exposure_max, imx214-
> > >exposure->step,
> > + exposure_def);
> > + }
> > +
> > /*
> > * Applying V4L2 control value only happens
> > * when power is up for streaming
> > @@ -691,7 +746,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl
> > *ctrl)
> > case V4L2_CID_EXPOSURE:
> > cci_write(imx214->regmap, IMX214_REG_EXPOSURE,
> > ctrl->val, &ret);
> > break;
> > -
> > + case V4L2_CID_VBLANK:
> No need to read the format here?
> format = v4l2_subdev_state_get_format(state,
> 0);
The format is read above (*), so no need to do it here. Note, in both
locations we have ctrl->id == V4L2_CID_VBLANK.
> > + cci_write(imx214->regmap,
> > IMX214_REG_FRM_LENGTH_LINES,
> > + format->height + ctrl->val, &ret);
> > + break;
> > default:
> > ret = -EINVAL;
> > }
> > @@ -714,8 +772,11 @@ static int imx214_ctrls_init(struct imx214
> > *imx214)
> > .width = 1120,
> > .height = 1120,
> > };
> > + const struct imx214_mode *mode = &imx214_modes[0];
> > struct v4l2_fwnode_device_properties props;
> > struct v4l2_ctrl_handler *ctrl_hdlr;
> > + int exposure_max, exposure_def;
> > + int hblank;
> > int ret;
> >
> > ret = v4l2_fwnode_device_parse(imx214->dev, &props);
> > @@ -723,7 +784,7 @@ static int imx214_ctrls_init(struct imx214
> > *imx214)
> > return ret;
> >
> > ctrl_hdlr = &imx214->ctrls;
> > - ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6);
> > + ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8);
> > if (ret)
> > return ret;
> >
> > @@ -749,12 +810,27 @@ static int imx214_ctrls_init(struct imx214
> > *imx214)
> > *
> > * Yours sincerely, Ricardo.
> > */
> > +
> > + /* Initial vblank/hblank/exposure parameters based on
> > current mode */
> > + imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > &imx214_ctrl_ops,
> > + V4L2_CID_VBLANK,
> > IMX214_VBLANK_MIN,
> > + IMX214_VTS_MAX - mode-
> > >height, 2,
> > + mode->vts_def - mode-
> > >height);
> > +
> > + hblank = IMX214_PPL_DEFAULT - mode->width;
> > + imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > &imx214_ctrl_ops,
> > + V4L2_CID_HBLANK, hblank,
> > hblank,
> > + 1, hblank);
> > + if (imx214->hblank)
> > + imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> > imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr,
> > &imx214_ctrl_ops,
> > V4L2_CID_EXPOSURE,
> > - IMX214_EXPOSURE_MIN,
> > - IMX214_EXPOSURE_MAX,
> > + IMX214_EXPOSURE_MIN,
> > exposure_max,
> nit: I think it looks nicer with exposure_max in the next line, but
> ignore if you prefer this way :)
> > IMX214_EXPOSURE_STEP,
> > -
> > IMX214_EXPOSURE_DEFAULT);
> > + exposure_def);
> >
> > imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr,
> > NULL,
> > @@ -876,6 +952,12 @@ static int imx214_get_frame_interval(struct
> > v4l2_subdev *subdev,
> > return 0;
> > }
> >
> > +/*
> > + * Raw sensors should be using the VBLANK and HBLANK controls to
> > determine
> > + * the frame rate. However this driver was initially added using
> > the
> > + * [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of 30fps.
> > + * Retain the frame_interval ops for backwards compatibility, but
> > they do nothing.
> > + */
>
> Now that these controls are useless... maybe we can do a
> dev_warn_once
> when the user calls it to leave some output in dmesg?
Sure. We the following be a proper warning?
"Use the VBLANK and HBLANK controls to determine the frame rate\n"
> > static int imx214_enum_frame_interval(struct v4l2_subdev *subdev,
> > struct v4l2_subdev_state *sd_state,
> > struct
> > v4l2_subdev_frame_interval_enum *fie)
> >
> > --
> > 2.47.1
> >
> >
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v3 07/12] media: i2c: imx214: Add vblank and hblank controls
2024-12-08 21:35 ` André Apitzsch
@ 2024-12-08 21:47 ` Ricardo Ribalda Delgado
0 siblings, 0 replies; 27+ messages in thread
From: Ricardo Ribalda Delgado @ 2024-12-08 21:47 UTC (permalink / raw)
To: André Apitzsch
Cc: Sakari Ailus, Mauro Carvalho Chehab, ~postmarketos/upstreaming,
phone-devel, linux-media, linux-kernel, Dave Stevenson,
Vincent Knecht
On Sun, Dec 8, 2024 at 10:35 PM André Apitzsch <git@apitzsch.eu> wrote:
>
> Hi Ricardo,
>
> Am Sonntag, dem 08.12.2024 um 21:59 +0100 schrieb Ricardo Ribalda
> Delgado:
> > In general it looks good to me (besides the comments, ignore the nits
> > if you want to).
> >
> > I'd recommend that you test with lockdep to make sure that we are not
> > missing anything, and I'd like to hear back from Sakari regarding the
> > get_locked_active
> >
> > Thanks!
> >
> > On Sat, Dec 7, 2024 at 9:48 PM André Apitzsch via B4 Relay
> > <devnull+git.apitzsch.eu@kernel.org> wrote:
> > >
> > > From: André Apitzsch <git@apitzsch.eu>
> > >
> > > Add vblank control to allow changing the framerate /
> > > higher exposure values.
> > >
> > > The vblank and hblank controls are needed for libcamera support.
> > >
> > > While at it, fix the minimal exposure time according to the
> > > datasheet.
> > >
> > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > ---
> > > drivers/media/i2c/imx214.c | 106
> > > ++++++++++++++++++++++++++++++++++++++++-----
> > > 1 file changed, 94 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx214.c
> > > b/drivers/media/i2c/imx214.c
> > > index
> > > f1c72db0775eaf4810f762e8798d301c5ad9923c..a7f49dbafe0f54af3c02f5534
> > > 460fdee88a22fe2 100644
> > > --- a/drivers/media/i2c/imx214.c
> > > +++ b/drivers/media/i2c/imx214.c
> > > @@ -34,11 +34,17 @@
> > >
> > > /* V-TIMING internal */
> > > #define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340)
> > > +#define IMX214_VTS_MAX 0xffff
> > > +
> > > +#define IMX214_VBLANK_MIN 890
> > > +
> > > +/* HBLANK control - read only */
> > > +#define IMX214_PPL_DEFAULT 5008
> > >
> > > /* Exposure control */
> > > #define IMX214_REG_EXPOSURE CCI_REG16(0x0202)
> > > -#define IMX214_EXPOSURE_MIN 0
> > > -#define IMX214_EXPOSURE_MAX 3184
> > > +#define IMX214_EXPOSURE_OFFSET 10
> > > +#define IMX214_EXPOSURE_MIN 1
> > > #define IMX214_EXPOSURE_STEP 1
> > > #define IMX214_EXPOSURE_DEFAULT 3184
> > > #define IMX214_REG_EXPOSURE_RATIO CCI_REG8(0x0222)
> > > @@ -187,6 +193,8 @@ struct imx214 {
> > > struct v4l2_ctrl_handler ctrls;
> > > struct v4l2_ctrl *pixel_rate;
> > > struct v4l2_ctrl *link_freq;
> > > + struct v4l2_ctrl *vblank;
> > > + struct v4l2_ctrl *hblank;
> > > struct v4l2_ctrl *exposure;
> > > struct v4l2_ctrl *unit_size;
> > >
> > > @@ -202,8 +210,6 @@ static const struct cci_reg_sequence
> > > mode_4096x2304[] = {
> > > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH
> > > },
> > > { IMX214_REG_EXPOSURE_RATIO, 1 },
> > > - { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > > - { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > > { IMX214_REG_X_ADD_STA, 56 },
> > > { IMX214_REG_Y_ADD_STA, 408 },
> > > { IMX214_REG_X_ADD_END, 4151 },
> > > @@ -274,8 +280,6 @@ static const struct cci_reg_sequence
> > > mode_1920x1080[] = {
> > > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH
> > > },
> > > { IMX214_REG_EXPOSURE_RATIO, 1 },
> > > - { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > > - { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > > { IMX214_REG_X_ADD_STA, 1144 },
> > > { IMX214_REG_Y_ADD_STA, 1020 },
> > > { IMX214_REG_X_ADD_END, 3063 },
> > > @@ -359,6 +363,7 @@ static const struct cci_reg_sequence
> > > mode_table_common[] = {
> > > { IMX214_REG_ORIENTATION, 0 },
> > > { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK },
> > > { IMX214_REG_FAST_STANDBY_CTRL, 1 },
> > > + { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT },
> > > { CCI_REG8(0x4550), 0x02 },
> > > { CCI_REG8(0x4601), 0x00 },
> > > { CCI_REG8(0x4642), 0x05 },
> > > @@ -462,18 +467,24 @@ static const struct cci_reg_sequence
> > > mode_table_common[] = {
> > > static const struct imx214_mode {
> > > u32 width;
> > > u32 height;
> > > +
> > > + /* V-timing */
> > > + unsigned int vts_def;
> > > +
> > > unsigned int num_of_regs;
> > > const struct cci_reg_sequence *reg_table;
> > > } imx214_modes[] = {
> > > {
> > > .width = 4096,
> > > .height = 2304,
> > > + .vts_def = 3194,
> > > .num_of_regs = ARRAY_SIZE(mode_4096x2304),
> > > .reg_table = mode_4096x2304,
> > > },
> > > {
> > > .width = 1920,
> > > .height = 1080,
> > > + .vts_def = 3194,
> > > .num_of_regs = ARRAY_SIZE(mode_1920x1080),
> > > .reg_table = mode_1920x1080,
> > > },
> > > @@ -626,9 +637,36 @@ static int imx214_set_format(struct
> > > v4l2_subdev *sd,
> > > __crop->width = mode->width;
> > > __crop->height = mode->height;
> > >
> > > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > + int exposure_max;
> > > + int exposure_def;
> > > + int hblank;
> > > +
> > > imx214->cur_mode = mode;
> > >
> > > + /* Update FPS limits */
> > nit: Update blank limits
> > > + __v4l2_ctrl_modify_range(imx214->vblank,
> > > IMX214_VBLANK_MIN,
> > > + IMX214_VTS_MAX - mode-
> > > >height, 2,
> > > + mode->vts_def - mode-
> > > >height);
> >
> > Is the handler->lock held when we call this function? Can you try
> > running the code with lockdep?
> > > +
> > > + /* Update max exposure while meeting expected
> > > vblanking */
> > > + exposure_max = mode->vts_def -
> > > IMX214_EXPOSURE_OFFSET;
> > > + exposure_def = min(exposure_max,
> > > IMX214_EXPOSURE_DEFAULT);
> > > + __v4l2_ctrl_modify_range(imx214->exposure,
> > > + imx214->exposure->minimum,
> > > + exposure_max, imx214-
> > > >exposure->step,
> > > + exposure_def);
> > > +
> > > + /*
> > > + * Currently PPL is fixed to IMX214_PPL_DEFAULT, so
> > > hblank
> > > + * depends on mode->width only, and is not
> > > changeable in any
> > > + * way other than changing the mode.
> > > + */
> > > + hblank = IMX214_PPL_DEFAULT - mode->width;
> > > + __v4l2_ctrl_modify_range(imx214->hblank, hblank,
> > > hblank, 1,
> > > + hblank);
> > > + }
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -678,8 +716,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl
> > > *ctrl)
> > > {
> > > struct imx214 *imx214 = container_of(ctrl->handler,
> > > struct imx214, ctrls);
> > > + const struct v4l2_mbus_framefmt *format;
> > > + struct v4l2_subdev_state *state;
> > > int ret;
> > >
> > > + if (ctrl->id == V4L2_CID_VBLANK) {
> > > + int exposure_max, exposure_def;
> > > +
> > > + state =
> > > v4l2_subdev_get_locked_active_state(&imx214->sd);
> >
> > Sakari, I see that other drivers assume that the active is locked in
> > set_ctrl. Is this correct?
> >
> > > + format = v4l2_subdev_state_get_format(state, 0);
>
> (*) The format is read here.
>
> > > +
> > > + /* Update max exposure while meeting expected
> > > vblanking */
> > > + exposure_max = format->height + ctrl->val -
> > > IMX214_EXPOSURE_OFFSET;
> > > + exposure_def = min(exposure_max,
> > > IMX214_EXPOSURE_DEFAULT);
> > > + __v4l2_ctrl_modify_range(imx214->exposure,
> > > + imx214->exposure->minimum,
> > > + exposure_max, imx214-
> > > >exposure->step,
> > > + exposure_def);
> > > + }
> > > +
> > > /*
> > > * Applying V4L2 control value only happens
> > > * when power is up for streaming
> > > @@ -691,7 +746,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl
> > > *ctrl)
> > > case V4L2_CID_EXPOSURE:
> > > cci_write(imx214->regmap, IMX214_REG_EXPOSURE,
> > > ctrl->val, &ret);
> > > break;
> > > -
> > > + case V4L2_CID_VBLANK:
> > No need to read the format here?
> > format = v4l2_subdev_state_get_format(state,
> > 0);
>
> The format is read above (*), so no need to do it here. Note, in both
> locations we have ctrl->id == V4L2_CID_VBLANK.
Can you init format to NULL at the declaration part to make the compiler happy?
https://gitlab.freedesktop.org/linux-media/users/patchwork/-/pipelines/1325516/test_report?job_name=build
>
> > > + cci_write(imx214->regmap,
> > > IMX214_REG_FRM_LENGTH_LINES,
> > > + format->height + ctrl->val, &ret);
> > > + break;
> > > default:
> > > ret = -EINVAL;
> > > }
> > > @@ -714,8 +772,11 @@ static int imx214_ctrls_init(struct imx214
> > > *imx214)
> > > .width = 1120,
> > > .height = 1120,
> > > };
> > > + const struct imx214_mode *mode = &imx214_modes[0];
> > > struct v4l2_fwnode_device_properties props;
> > > struct v4l2_ctrl_handler *ctrl_hdlr;
> > > + int exposure_max, exposure_def;
> > > + int hblank;
> > > int ret;
> > >
> > > ret = v4l2_fwnode_device_parse(imx214->dev, &props);
> > > @@ -723,7 +784,7 @@ static int imx214_ctrls_init(struct imx214
> > > *imx214)
> > > return ret;
> > >
> > > ctrl_hdlr = &imx214->ctrls;
> > > - ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6);
> > > + ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8);
> > > if (ret)
> > > return ret;
> > >
> > > @@ -749,12 +810,27 @@ static int imx214_ctrls_init(struct imx214
> > > *imx214)
> > > *
> > > * Yours sincerely, Ricardo.
> > > */
> > > +
> > > + /* Initial vblank/hblank/exposure parameters based on
> > > current mode */
> > > + imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > > &imx214_ctrl_ops,
> > > + V4L2_CID_VBLANK,
> > > IMX214_VBLANK_MIN,
> > > + IMX214_VTS_MAX - mode-
> > > >height, 2,
> > > + mode->vts_def - mode-
> > > >height);
> > > +
> > > + hblank = IMX214_PPL_DEFAULT - mode->width;
> > > + imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > > &imx214_ctrl_ops,
> > > + V4L2_CID_HBLANK, hblank,
> > > hblank,
> > > + 1, hblank);
> > > + if (imx214->hblank)
> > > + imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +
> > > + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> > > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> > > imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr,
> > > &imx214_ctrl_ops,
> > > V4L2_CID_EXPOSURE,
> > > - IMX214_EXPOSURE_MIN,
> > > - IMX214_EXPOSURE_MAX,
> > > + IMX214_EXPOSURE_MIN,
> > > exposure_max,
> > nit: I think it looks nicer with exposure_max in the next line, but
> > ignore if you prefer this way :)
> > > IMX214_EXPOSURE_STEP,
> > > -
> > > IMX214_EXPOSURE_DEFAULT);
> > > + exposure_def);
> > >
> > > imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr,
> > > NULL,
> > > @@ -876,6 +952,12 @@ static int imx214_get_frame_interval(struct
> > > v4l2_subdev *subdev,
> > > return 0;
> > > }
> > >
> > > +/*
> > > + * Raw sensors should be using the VBLANK and HBLANK controls to
> > > determine
> > > + * the frame rate. However this driver was initially added using
> > > the
> > > + * [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of 30fps.
> > > + * Retain the frame_interval ops for backwards compatibility, but
> > > they do nothing.
> > > + */
> >
> > Now that these controls are useless... maybe we can do a
> > dev_warn_once
> > when the user calls it to leave some output in dmesg?
>
> Sure. We the following be a proper warning?
>
nit: What about?
"frame_interval functions returns an unreliable value for
compatibility reasons. Use the VBLANK and HBLANK controls to determine
the correct frame rate.\n"
>
> > > static int imx214_enum_frame_interval(struct v4l2_subdev *subdev,
> > > struct v4l2_subdev_state *sd_state,
> > > struct
> > > v4l2_subdev_frame_interval_enum *fie)
> > >
> > > --
> > > 2.47.1
> > >
> > >
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 07/12] media: i2c: imx214: Add vblank and hblank controls
2024-12-08 20:59 ` Ricardo Ribalda Delgado
2024-12-08 21:19 ` Ricardo Ribalda Delgado
2024-12-08 21:35 ` André Apitzsch
@ 2024-12-15 22:35 ` André Apitzsch
2024-12-16 13:00 ` Ricardo Ribalda Delgado
2024-12-17 7:17 ` Sakari Ailus
2 siblings, 2 replies; 27+ messages in thread
From: André Apitzsch @ 2024-12-15 22:35 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Sakari Ailus, Mauro Carvalho Chehab, ~postmarketos/upstreaming,
phone-devel, linux-media, linux-kernel, Dave Stevenson,
Vincent Knecht
Hi,
Am Sonntag, dem 08.12.2024 um 21:59 +0100 schrieb Ricardo Ribalda
Delgado:
> In general it looks good to me (besides the comments, ignore the nits
> if you want to).
>
> I'd recommend that you test with lockdep to make sure that we are not
> missing anything, and I'd like to hear back from Sakari regarding the
> get_locked_active
>
> Thanks!
>
> On Sat, Dec 7, 2024 at 9:48 PM André Apitzsch via B4 Relay
> <devnull+git.apitzsch.eu@kernel.org> wrote:
> >
> > From: André Apitzsch <git@apitzsch.eu>
> >
> > Add vblank control to allow changing the framerate /
> > higher exposure values.
> >
> > The vblank and hblank controls are needed for libcamera support.
> >
> > While at it, fix the minimal exposure time according to the
> > datasheet.
> >
> > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > ---
> > drivers/media/i2c/imx214.c | 106
> > ++++++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 94 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx214.c
> > b/drivers/media/i2c/imx214.c
> > index
> > f1c72db0775eaf4810f762e8798d301c5ad9923c..a7f49dbafe0f54af3c02f5534
> > 460fdee88a22fe2 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -34,11 +34,17 @@
> >
> > /* V-TIMING internal */
> > #define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340)
> > +#define IMX214_VTS_MAX 0xffff
> > +
> > +#define IMX214_VBLANK_MIN 890
> > +
> > +/* HBLANK control - read only */
> > +#define IMX214_PPL_DEFAULT 5008
> >
> > /* Exposure control */
> > #define IMX214_REG_EXPOSURE CCI_REG16(0x0202)
> > -#define IMX214_EXPOSURE_MIN 0
> > -#define IMX214_EXPOSURE_MAX 3184
> > +#define IMX214_EXPOSURE_OFFSET 10
> > +#define IMX214_EXPOSURE_MIN 1
> > #define IMX214_EXPOSURE_STEP 1
> > #define IMX214_EXPOSURE_DEFAULT 3184
> > #define IMX214_REG_EXPOSURE_RATIO CCI_REG8(0x0222)
> > @@ -187,6 +193,8 @@ struct imx214 {
> > struct v4l2_ctrl_handler ctrls;
> > struct v4l2_ctrl *pixel_rate;
> > struct v4l2_ctrl *link_freq;
> > + struct v4l2_ctrl *vblank;
> > + struct v4l2_ctrl *hblank;
> > struct v4l2_ctrl *exposure;
> > struct v4l2_ctrl *unit_size;
> >
> > @@ -202,8 +210,6 @@ static const struct cci_reg_sequence
> > mode_4096x2304[] = {
> > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH
> > },
> > { IMX214_REG_EXPOSURE_RATIO, 1 },
> > - { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > - { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > { IMX214_REG_X_ADD_STA, 56 },
> > { IMX214_REG_Y_ADD_STA, 408 },
> > { IMX214_REG_X_ADD_END, 4151 },
> > @@ -274,8 +280,6 @@ static const struct cci_reg_sequence
> > mode_1920x1080[] = {
> > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH
> > },
> > { IMX214_REG_EXPOSURE_RATIO, 1 },
> > - { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > - { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > { IMX214_REG_X_ADD_STA, 1144 },
> > { IMX214_REG_Y_ADD_STA, 1020 },
> > { IMX214_REG_X_ADD_END, 3063 },
> > @@ -359,6 +363,7 @@ static const struct cci_reg_sequence
> > mode_table_common[] = {
> > { IMX214_REG_ORIENTATION, 0 },
> > { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK },
> > { IMX214_REG_FAST_STANDBY_CTRL, 1 },
> > + { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT },
> > { CCI_REG8(0x4550), 0x02 },
> > { CCI_REG8(0x4601), 0x00 },
> > { CCI_REG8(0x4642), 0x05 },
> > @@ -462,18 +467,24 @@ static const struct cci_reg_sequence
> > mode_table_common[] = {
> > static const struct imx214_mode {
> > u32 width;
> > u32 height;
> > +
> > + /* V-timing */
> > + unsigned int vts_def;
> > +
> > unsigned int num_of_regs;
> > const struct cci_reg_sequence *reg_table;
> > } imx214_modes[] = {
> > {
> > .width = 4096,
> > .height = 2304,
> > + .vts_def = 3194,
> > .num_of_regs = ARRAY_SIZE(mode_4096x2304),
> > .reg_table = mode_4096x2304,
> > },
> > {
> > .width = 1920,
> > .height = 1080,
> > + .vts_def = 3194,
> > .num_of_regs = ARRAY_SIZE(mode_1920x1080),
> > .reg_table = mode_1920x1080,
> > },
> > @@ -626,9 +637,36 @@ static int imx214_set_format(struct
> > v4l2_subdev *sd,
> > __crop->width = mode->width;
> > __crop->height = mode->height;
> >
> > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + int exposure_max;
> > + int exposure_def;
> > + int hblank;
> > +
> > imx214->cur_mode = mode;
> >
> > + /* Update FPS limits */
> nit: Update blank limits
> > + __v4l2_ctrl_modify_range(imx214->vblank,
> > IMX214_VBLANK_MIN,
> > + IMX214_VTS_MAX - mode-
> > >height, 2,
> > + mode->vts_def - mode-
> > >height);
>
> Is the handler->lock held when we call this function?
I'm not sure how to test this.
> Can you try
> running the code with lockdep?
/proc/lockdep contains
> 00000000f9299231 FD: 89 BD: 6 +.+.: imx214:901:(&imx214->ctrls)->_lock
(no idea how to interpret this line)
and dmesg doesn't print anything, when changing the vblank value.
Best regards,
André
> > +
> > + /* Update max exposure while meeting expected
> > vblanking */
> > + exposure_max = mode->vts_def -
> > IMX214_EXPOSURE_OFFSET;
> > + exposure_def = min(exposure_max,
> > IMX214_EXPOSURE_DEFAULT);
> > + __v4l2_ctrl_modify_range(imx214->exposure,
> > + imx214->exposure->minimum,
> > + exposure_max, imx214-
> > >exposure->step,
> > + exposure_def);
> > +
> > + /*
> > + * Currently PPL is fixed to IMX214_PPL_DEFAULT, so
> > hblank
> > + * depends on mode->width only, and is not
> > changeable in any
> > + * way other than changing the mode.
> > + */
> > + hblank = IMX214_PPL_DEFAULT - mode->width;
> > + __v4l2_ctrl_modify_range(imx214->hblank, hblank,
> > hblank, 1,
> > + hblank);
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -678,8 +716,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl
> > *ctrl)
> > {
> > struct imx214 *imx214 = container_of(ctrl->handler,
> > struct imx214, ctrls);
> > + const struct v4l2_mbus_framefmt *format;
> > + struct v4l2_subdev_state *state;
> > int ret;
> >
> > + if (ctrl->id == V4L2_CID_VBLANK) {
> > + int exposure_max, exposure_def;
> > +
> > + state =
> > v4l2_subdev_get_locked_active_state(&imx214->sd);
>
> Sakari, I see that other drivers assume that the active is locked in
> set_ctrl. Is this correct?
>
> > + format = v4l2_subdev_state_get_format(state, 0);
> > +
> > + /* Update max exposure while meeting expected
> > vblanking */
> > + exposure_max = format->height + ctrl->val -
> > IMX214_EXPOSURE_OFFSET;
> > + exposure_def = min(exposure_max,
> > IMX214_EXPOSURE_DEFAULT);
> > + __v4l2_ctrl_modify_range(imx214->exposure,
> > + imx214->exposure->minimum,
> > + exposure_max, imx214-
> > >exposure->step,
> > + exposure_def);
> > + }
> > +
> > /*
> > * Applying V4L2 control value only happens
> > * when power is up for streaming
> > @@ -691,7 +746,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl
> > *ctrl)
> > case V4L2_CID_EXPOSURE:
> > cci_write(imx214->regmap, IMX214_REG_EXPOSURE,
> > ctrl->val, &ret);
> > break;
> > -
> > + case V4L2_CID_VBLANK:
> No need to read the format here?
> format = v4l2_subdev_state_get_format(state,
> 0);
> > + cci_write(imx214->regmap,
> > IMX214_REG_FRM_LENGTH_LINES,
> > + format->height + ctrl->val, &ret);
> > + break;
> > default:
> > ret = -EINVAL;
> > }
> > @@ -714,8 +772,11 @@ static int imx214_ctrls_init(struct imx214
> > *imx214)
> > .width = 1120,
> > .height = 1120,
> > };
> > + const struct imx214_mode *mode = &imx214_modes[0];
> > struct v4l2_fwnode_device_properties props;
> > struct v4l2_ctrl_handler *ctrl_hdlr;
> > + int exposure_max, exposure_def;
> > + int hblank;
> > int ret;
> >
> > ret = v4l2_fwnode_device_parse(imx214->dev, &props);
> > @@ -723,7 +784,7 @@ static int imx214_ctrls_init(struct imx214
> > *imx214)
> > return ret;
> >
> > ctrl_hdlr = &imx214->ctrls;
> > - ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6);
> > + ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8);
> > if (ret)
> > return ret;
> >
> > @@ -749,12 +810,27 @@ static int imx214_ctrls_init(struct imx214
> > *imx214)
> > *
> > * Yours sincerely, Ricardo.
> > */
> > +
> > + /* Initial vblank/hblank/exposure parameters based on
> > current mode */
> > + imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > &imx214_ctrl_ops,
> > + V4L2_CID_VBLANK,
> > IMX214_VBLANK_MIN,
> > + IMX214_VTS_MAX - mode-
> > >height, 2,
> > + mode->vts_def - mode-
> > >height);
> > +
> > + hblank = IMX214_PPL_DEFAULT - mode->width;
> > + imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > &imx214_ctrl_ops,
> > + V4L2_CID_HBLANK, hblank,
> > hblank,
> > + 1, hblank);
> > + if (imx214->hblank)
> > + imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> > imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr,
> > &imx214_ctrl_ops,
> > V4L2_CID_EXPOSURE,
> > - IMX214_EXPOSURE_MIN,
> > - IMX214_EXPOSURE_MAX,
> > + IMX214_EXPOSURE_MIN,
> > exposure_max,
> nit: I think it looks nicer with exposure_max in the next line, but
> ignore if you prefer this way :)
> > IMX214_EXPOSURE_STEP,
> > -
> > IMX214_EXPOSURE_DEFAULT);
> > + exposure_def);
> >
> > imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr,
> > NULL,
> > @@ -876,6 +952,12 @@ static int imx214_get_frame_interval(struct
> > v4l2_subdev *subdev,
> > return 0;
> > }
> >
> > +/*
> > + * Raw sensors should be using the VBLANK and HBLANK controls to
> > determine
> > + * the frame rate. However this driver was initially added using
> > the
> > + * [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of 30fps.
> > + * Retain the frame_interval ops for backwards compatibility, but
> > they do nothing.
> > + */
>
> Now that these controls are useless... maybe we can do a
> dev_warn_once
> when the user calls it to leave some output in dmesg?
> > static int imx214_enum_frame_interval(struct v4l2_subdev *subdev,
> > struct v4l2_subdev_state *sd_state,
> > struct
> > v4l2_subdev_frame_interval_enum *fie)
> >
> > --
> > 2.47.1
> >
> >
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v3 07/12] media: i2c: imx214: Add vblank and hblank controls
2024-12-15 22:35 ` André Apitzsch
@ 2024-12-16 13:00 ` Ricardo Ribalda Delgado
2024-12-17 7:17 ` Sakari Ailus
1 sibling, 0 replies; 27+ messages in thread
From: Ricardo Ribalda Delgado @ 2024-12-16 13:00 UTC (permalink / raw)
To: André Apitzsch
Cc: Sakari Ailus, Mauro Carvalho Chehab, ~postmarketos/upstreaming,
phone-devel, linux-media, linux-kernel, Dave Stevenson,
Vincent Knecht
> > Is the handler->lock held when we call this function?
>
> I'm not sure how to test this.
>
> > Can you try
> > running the code with lockdep?
>
> /proc/lockdep contains
>
> > 00000000f9299231 FD: 89 BD: 6 +.+.: imx214:901:(&imx214->ctrls)->_lock
>
> (no idea how to interpret this line)
>
> and dmesg doesn't print anything, when changing the vblank value.
>
If dmesg does not complain after using it I am happy.
Thanks! :)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 07/12] media: i2c: imx214: Add vblank and hblank controls
2024-12-15 22:35 ` André Apitzsch
2024-12-16 13:00 ` Ricardo Ribalda Delgado
@ 2024-12-17 7:17 ` Sakari Ailus
1 sibling, 0 replies; 27+ messages in thread
From: Sakari Ailus @ 2024-12-17 7:17 UTC (permalink / raw)
To: André Apitzsch
Cc: Ricardo Ribalda Delgado, Mauro Carvalho Chehab,
~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht
Hi André,
On Sun, Dec 15, 2024 at 11:35:41PM +0100, André Apitzsch wrote:
> > > @@ -626,9 +637,36 @@ static int imx214_set_format(struct
> > > v4l2_subdev *sd,
> > > __crop->width = mode->width;
> > > __crop->height = mode->height;
> > >
> > > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > + int exposure_max;
> > > + int exposure_def;
> > > + int hblank;
> > > +
> > > imx214->cur_mode = mode;
> > >
> > > + /* Update FPS limits */
> > nit: Update blank limits
> > > + __v4l2_ctrl_modify_range(imx214->vblank,
> > > IMX214_VBLANK_MIN,
> > > + IMX214_VTS_MAX - mode-
> > > >height, 2,
> > > + mode->vts_def - mode-
> > > >height);
> >
> > Is the handler->lock held when we call this function?
>
> I'm not sure how to test this.
I'd say "yes" because the first patch sets the control handler lock to the
sub-device state lock.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 08/12] media: i2c: imx214: Implement vflip/hflip controls
2024-12-07 20:47 [PATCH v3 00/12] media: i2c: imx214: Miscellaneous cleanups and improvements André Apitzsch via B4 Relay
` (6 preceding siblings ...)
2024-12-07 20:47 ` [PATCH v3 07/12] media: i2c: imx214: Add vblank and hblank controls André Apitzsch via B4 Relay
@ 2024-12-07 20:47 ` André Apitzsch via B4 Relay
2024-12-07 20:47 ` [PATCH v3 09/12] media: i2c: imx214: Add analogue/digital gain control André Apitzsch via B4 Relay
` (3 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: André Apitzsch via B4 Relay @ 2024-12-07 20:47 UTC (permalink / raw)
To: Ricardo Ribalda, Sakari Ailus, Mauro Carvalho Chehab
Cc: ~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht,
André Apitzsch, Ricardo Ribalda
From: André Apitzsch <git@apitzsch.eu>
The imx214 sensor supports horizontal and vertical flipping. Add
appropriate controls to the driver.
Acked-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
drivers/media/i2c/imx214.c | 69 ++++++++++++++++++++++++++++++++++++++++------
1 file changed, 61 insertions(+), 8 deletions(-)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index a7f49dbafe0f54af3c02f5534460fdee88a22fe2..f7715b13edd09169d81fbbd04dc231be37cc59f0 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -30,7 +30,6 @@
#define IMX214_DEFAULT_LINK_FREQ 480000000
#define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10)
#define IMX214_FPS 30
-#define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
/* V-TIMING internal */
#define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340)
@@ -182,6 +181,22 @@ static const char * const imx214_supply_name[] = {
#define IMX214_NUM_SUPPLIES ARRAY_SIZE(imx214_supply_name)
+/*
+ * The supported formats.
+ * This table MUST contain 4 entries per format, to cover the various flip
+ * combinations in the order
+ * - no flip
+ * - h flip
+ * - v flip
+ * - h&v flips
+ */
+static const u32 imx214_mbus_formats[] = {
+ MEDIA_BUS_FMT_SRGGB10_1X10,
+ MEDIA_BUS_FMT_SGRBG10_1X10,
+ MEDIA_BUS_FMT_SGBRG10_1X10,
+ MEDIA_BUS_FMT_SBGGR10_1X10,
+};
+
struct imx214 {
struct device *dev;
struct clk *xclk;
@@ -197,6 +212,10 @@ struct imx214 {
struct v4l2_ctrl *hblank;
struct v4l2_ctrl *exposure;
struct v4l2_ctrl *unit_size;
+ struct {
+ struct v4l2_ctrl *hflip;
+ struct v4l2_ctrl *vflip;
+ };
const struct imx214_mode *cur_mode;
@@ -360,7 +379,6 @@ static const struct cci_reg_sequence mode_table_common[] = {
/* global setting */
/* basic config */
- { IMX214_REG_ORIENTATION, 0 },
{ IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK },
{ IMX214_REG_FAST_STANDBY_CTRL, 1 },
{ IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT },
@@ -539,11 +557,21 @@ static int __maybe_unused imx214_power_off(struct device *dev)
return 0;
}
+/* Get bayer order based on flip setting. */
+static u32 imx214_get_format_code(struct imx214 *imx214)
+{
+ unsigned int i;
+
+ i = (imx214->vflip->val ? 2 : 0) | (imx214->hflip->val ? 1 : 0);
+
+ return imx214_mbus_formats[i];
+}
+
static void imx214_update_pad_format(struct imx214 *imx214,
const struct imx214_mode *mode,
struct v4l2_mbus_framefmt *fmt, u32 code)
{
- fmt->code = IMX214_MBUS_CODE;
+ fmt->code = imx214_get_format_code(imx214);
fmt->width = mode->width;
fmt->height = mode->height;
fmt->field = V4L2_FIELD_NONE;
@@ -559,10 +587,12 @@ static int imx214_enum_mbus_code(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_mbus_code_enum *code)
{
- if (code->index > 0)
+ struct imx214 *imx214 = to_imx214(sd);
+
+ if (code->index >= (ARRAY_SIZE(imx214_mbus_formats) / 4))
return -EINVAL;
- code->code = IMX214_MBUS_CODE;
+ code->code = imx214_get_format_code(imx214);
return 0;
}
@@ -571,7 +601,11 @@ static int imx214_enum_frame_size(struct v4l2_subdev *subdev,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_frame_size_enum *fse)
{
- if (fse->code != IMX214_MBUS_CODE)
+ struct imx214 *imx214 = to_imx214(subdev);
+ u32 code;
+
+ code = imx214_get_format_code(imx214);
+ if (fse->code != code)
return -EINVAL;
if (fse->index >= ARRAY_SIZE(imx214_modes))
@@ -704,6 +738,7 @@ static int imx214_entity_init_state(struct v4l2_subdev *subdev,
struct v4l2_subdev_format fmt = { };
fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
+ fmt.format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
fmt.format.width = imx214_modes[0].width;
fmt.format.height = imx214_modes[0].height;
@@ -746,6 +781,11 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
case V4L2_CID_EXPOSURE:
cci_write(imx214->regmap, IMX214_REG_EXPOSURE, ctrl->val, &ret);
break;
+ case V4L2_CID_HFLIP:
+ case V4L2_CID_VFLIP:
+ cci_write(imx214->regmap, IMX214_REG_ORIENTATION,
+ imx214->hflip->val | imx214->vflip->val << 1, &ret);
+ break;
case V4L2_CID_VBLANK:
cci_write(imx214->regmap, IMX214_REG_FRM_LENGTH_LINES,
format->height + ctrl->val, &ret);
@@ -784,7 +824,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
return ret;
ctrl_hdlr = &imx214->ctrls;
- ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8);
+ ret = v4l2_ctrl_handler_init(&imx214->ctrls, 10);
if (ret)
return ret;
@@ -832,6 +872,18 @@ static int imx214_ctrls_init(struct imx214 *imx214)
IMX214_EXPOSURE_STEP,
exposure_def);
+ imx214->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
+ V4L2_CID_HFLIP, 0, 1, 1, 0);
+ if (imx214->hflip)
+ imx214->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+ imx214->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
+ V4L2_CID_VFLIP, 0, 1, 1, 0);
+ if (imx214->vflip)
+ imx214->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+ v4l2_ctrl_cluster(2, &imx214->hflip);
+
imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr,
NULL,
V4L2_CID_UNIT_CELL_SIZE,
@@ -962,6 +1014,7 @@ static int imx214_enum_frame_interval(struct v4l2_subdev *subdev,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_frame_interval_enum *fie)
{
+ struct imx214 *imx214 = to_imx214(subdev);
const struct imx214_mode *mode;
if (fie->index != 0)
@@ -971,7 +1024,7 @@ static int imx214_enum_frame_interval(struct v4l2_subdev *subdev,
ARRAY_SIZE(imx214_modes), width, height,
fie->width, fie->height);
- fie->code = IMX214_MBUS_CODE;
+ fie->code = imx214_get_format_code(imx214);
fie->width = mode->width;
fie->height = mode->height;
fie->interval.numerator = 1;
--
2.47.1
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v3 09/12] media: i2c: imx214: Add analogue/digital gain control
2024-12-07 20:47 [PATCH v3 00/12] media: i2c: imx214: Miscellaneous cleanups and improvements André Apitzsch via B4 Relay
` (7 preceding siblings ...)
2024-12-07 20:47 ` [PATCH v3 08/12] media: i2c: imx214: Implement vflip/hflip controls André Apitzsch via B4 Relay
@ 2024-12-07 20:47 ` André Apitzsch via B4 Relay
2024-12-07 20:47 ` [PATCH v3 10/12] media: i2c: imx214: Verify chip ID André Apitzsch via B4 Relay
` (2 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: André Apitzsch via B4 Relay @ 2024-12-07 20:47 UTC (permalink / raw)
To: Ricardo Ribalda, Sakari Ailus, Mauro Carvalho Chehab
Cc: ~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht,
André Apitzsch, Ricardo Ribalda
From: André Apitzsch <git@apitzsch.eu>
The imx214 sensor supports analogue gain up to 8x and digital gain up to
16x. Implement the corresponding controls in the driver. Default gain
values are not modified by this patch.
Acked-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
drivers/media/i2c/imx214.c | 53 +++++++++++++++++++++++++++++++++-------------
1 file changed, 38 insertions(+), 15 deletions(-)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index f7715b13edd09169d81fbbd04dc231be37cc59f0..a35e4e60e55c2b9d9e3090c98e695f78d099cf44 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -52,12 +52,20 @@
/* Analog gain control */
#define IMX214_REG_ANALOG_GAIN CCI_REG16(0x0204)
#define IMX214_REG_SHORT_ANALOG_GAIN CCI_REG16(0x0216)
+#define IMX214_ANA_GAIN_MIN 0
+#define IMX214_ANA_GAIN_MAX 448
+#define IMX214_ANA_GAIN_STEP 1
+#define IMX214_ANA_GAIN_DEFAULT 0x0
/* Digital gain control */
#define IMX214_REG_DIG_GAIN_GREENR CCI_REG16(0x020e)
#define IMX214_REG_DIG_GAIN_RED CCI_REG16(0x0210)
#define IMX214_REG_DIG_GAIN_BLUE CCI_REG16(0x0212)
#define IMX214_REG_DIG_GAIN_GREENB CCI_REG16(0x0214)
+#define IMX214_DGTL_GAIN_MIN 0x0100
+#define IMX214_DGTL_GAIN_MAX 0x0fff
+#define IMX214_DGTL_GAIN_DEFAULT 0x0100
+#define IMX214_DGTL_GAIN_STEP 1
#define IMX214_REG_ORIENTATION CCI_REG8(0x0101)
@@ -280,13 +288,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = {
{ IMX214_REG_SHORT_EXPOSURE, 500 },
- { IMX214_REG_ANALOG_GAIN, 0 },
- { IMX214_REG_DIG_GAIN_GREENR, 256 },
- { IMX214_REG_DIG_GAIN_RED, 256 },
- { IMX214_REG_DIG_GAIN_BLUE, 256 },
- { IMX214_REG_DIG_GAIN_GREENB, 256 },
- { IMX214_REG_SHORT_ANALOG_GAIN, 0 },
-
{ CCI_REG8(0x4170), 0x00 },
{ CCI_REG8(0x4171), 0x10 },
{ CCI_REG8(0x4176), 0x00 },
@@ -350,13 +351,6 @@ static const struct cci_reg_sequence mode_1920x1080[] = {
{ IMX214_REG_SHORT_EXPOSURE, 500 },
- { IMX214_REG_ANALOG_GAIN, 0 },
- { IMX214_REG_DIG_GAIN_GREENR, 256 },
- { IMX214_REG_DIG_GAIN_RED, 256 },
- { IMX214_REG_DIG_GAIN_BLUE, 256 },
- { IMX214_REG_DIG_GAIN_GREENB, 256 },
- { IMX214_REG_SHORT_ANALOG_GAIN, 0 },
-
{ CCI_REG8(0x4170), 0x00 },
{ CCI_REG8(0x4171), 0x10 },
{ CCI_REG8(0x4176), 0x00 },
@@ -747,6 +741,18 @@ static int imx214_entity_init_state(struct v4l2_subdev *subdev,
return 0;
}
+static int imx214_update_digital_gain(struct imx214 *imx214, u32 val)
+{
+ int ret = 0;
+
+ cci_write(imx214->regmap, IMX214_REG_DIG_GAIN_GREENR, val, &ret);
+ cci_write(imx214->regmap, IMX214_REG_DIG_GAIN_RED, val, &ret);
+ cci_write(imx214->regmap, IMX214_REG_DIG_GAIN_BLUE, val, &ret);
+ cci_write(imx214->regmap, IMX214_REG_DIG_GAIN_GREENB, val, &ret);
+
+ return ret;
+}
+
static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
{
struct imx214 *imx214 = container_of(ctrl->handler,
@@ -778,6 +784,15 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
return 0;
switch (ctrl->id) {
+ case V4L2_CID_ANALOGUE_GAIN:
+ cci_write(imx214->regmap, IMX214_REG_ANALOG_GAIN,
+ ctrl->val, &ret);
+ cci_write(imx214->regmap, IMX214_REG_SHORT_ANALOG_GAIN,
+ ctrl->val, &ret);
+ break;
+ case V4L2_CID_DIGITAL_GAIN:
+ ret = imx214_update_digital_gain(imx214, ctrl->val);
+ break;
case V4L2_CID_EXPOSURE:
cci_write(imx214->regmap, IMX214_REG_EXPOSURE, ctrl->val, &ret);
break;
@@ -824,7 +839,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
return ret;
ctrl_hdlr = &imx214->ctrls;
- ret = v4l2_ctrl_handler_init(&imx214->ctrls, 10);
+ ret = v4l2_ctrl_handler_init(&imx214->ctrls, 12);
if (ret)
return ret;
@@ -872,6 +887,14 @@ static int imx214_ctrls_init(struct imx214 *imx214)
IMX214_EXPOSURE_STEP,
exposure_def);
+ v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
+ IMX214_ANA_GAIN_MIN, IMX214_ANA_GAIN_MAX,
+ IMX214_ANA_GAIN_STEP, IMX214_ANA_GAIN_DEFAULT);
+
+ v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
+ IMX214_DGTL_GAIN_MIN, IMX214_DGTL_GAIN_MAX,
+ IMX214_DGTL_GAIN_STEP, IMX214_DGTL_GAIN_DEFAULT);
+
imx214->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
V4L2_CID_HFLIP, 0, 1, 1, 0);
if (imx214->hflip)
--
2.47.1
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v3 10/12] media: i2c: imx214: Verify chip ID
2024-12-07 20:47 [PATCH v3 00/12] media: i2c: imx214: Miscellaneous cleanups and improvements André Apitzsch via B4 Relay
` (8 preceding siblings ...)
2024-12-07 20:47 ` [PATCH v3 09/12] media: i2c: imx214: Add analogue/digital gain control André Apitzsch via B4 Relay
@ 2024-12-07 20:47 ` André Apitzsch via B4 Relay
2024-12-10 12:40 ` Sakari Ailus
2024-12-07 20:48 ` [PATCH v3 11/12] media: i2c: imx214: Add test pattern control André Apitzsch via B4 Relay
2024-12-07 20:48 ` [PATCH v3 12/12] media: i2c: imx214: Fix link frequency André Apitzsch via B4 Relay
11 siblings, 1 reply; 27+ messages in thread
From: André Apitzsch via B4 Relay @ 2024-12-07 20:47 UTC (permalink / raw)
To: Ricardo Ribalda, Sakari Ailus, Mauro Carvalho Chehab
Cc: ~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht,
André Apitzsch, Ricardo Ribalda
From: André Apitzsch <git@apitzsch.eu>
Check the chip ID and stop probing if it is no imx214 sensor.
Acked-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
drivers/media/i2c/imx214.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index a35e4e60e55c2b9d9e3090c98e695f78d099cf44..3cd64bb91795d8490acad5926940c111fb95fae5 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -20,6 +20,10 @@
#include <media/v4l2-fwnode.h>
#include <media/v4l2-subdev.h>
+/* Chip ID */
+#define IMX214_REG_CHIP_ID CCI_REG16(0x0016)
+#define IMX214_CHIP_ID 0x0214
+
#define IMX214_REG_MODE_SELECT CCI_REG8(0x0100)
#define IMX214_MODE_STANDBY 0x00
#define IMX214_MODE_STREAMING 0x01
@@ -1092,6 +1096,27 @@ static int imx214_get_regulators(struct device *dev, struct imx214 *imx214)
imx214->supplies);
}
+/* Verify chip ID */
+static int imx214_identify_module(struct imx214 *imx214)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&imx214->sd);
+ int ret;
+ u64 val;
+
+ ret = cci_read(imx214->regmap, IMX214_REG_CHIP_ID, &val, NULL);
+ if (ret)
+ return dev_err_probe(&client->dev, ret,
+ "failed to read chip id %x\n",
+ IMX214_CHIP_ID);
+
+ if (val != IMX214_CHIP_ID)
+ return dev_err_probe(&client->dev, -EIO,
+ "chip id mismatch: %x!=%llx\n",
+ IMX214_CHIP_ID, val);
+
+ return 0;
+}
+
static int imx214_parse_fwnode(struct device *dev)
{
struct fwnode_handle *endpoint;
@@ -1184,6 +1209,10 @@ static int imx214_probe(struct i2c_client *client)
*/
imx214_power_on(imx214->dev);
+ ret = imx214_identify_module(imx214);
+ if (ret)
+ goto error_power_off;
+
pm_runtime_set_active(imx214->dev);
pm_runtime_enable(imx214->dev);
pm_runtime_idle(imx214->dev);
--
2.47.1
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v3 10/12] media: i2c: imx214: Verify chip ID
2024-12-07 20:47 ` [PATCH v3 10/12] media: i2c: imx214: Verify chip ID André Apitzsch via B4 Relay
@ 2024-12-10 12:40 ` Sakari Ailus
0 siblings, 0 replies; 27+ messages in thread
From: Sakari Ailus @ 2024-12-10 12:40 UTC (permalink / raw)
To: git
Cc: Ricardo Ribalda, Mauro Carvalho Chehab,
~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht, Ricardo Ribalda
Hi André,
Thanks for the update.
On Sat, Dec 07, 2024 at 09:47:59PM +0100, André Apitzsch via B4 Relay wrote:
> From: André Apitzsch <git@apitzsch.eu>
>
> Check the chip ID and stop probing if it is no imx214 sensor.
>
> Acked-by: Ricardo Ribalda <ribalda@chromium.org>
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
> drivers/media/i2c/imx214.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index a35e4e60e55c2b9d9e3090c98e695f78d099cf44..3cd64bb91795d8490acad5926940c111fb95fae5 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -20,6 +20,10 @@
> #include <media/v4l2-fwnode.h>
> #include <media/v4l2-subdev.h>
>
> +/* Chip ID */
> +#define IMX214_REG_CHIP_ID CCI_REG16(0x0016)
> +#define IMX214_CHIP_ID 0x0214
> +
> #define IMX214_REG_MODE_SELECT CCI_REG8(0x0100)
> #define IMX214_MODE_STANDBY 0x00
> #define IMX214_MODE_STREAMING 0x01
> @@ -1092,6 +1096,27 @@ static int imx214_get_regulators(struct device *dev, struct imx214 *imx214)
> imx214->supplies);
> }
>
> +/* Verify chip ID */
> +static int imx214_identify_module(struct imx214 *imx214)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&imx214->sd);
> + int ret;
> + u64 val;
> +
> + ret = cci_read(imx214->regmap, IMX214_REG_CHIP_ID, &val, NULL);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "failed to read chip id %x\n",
> + IMX214_CHIP_ID);
> +
> + if (val != IMX214_CHIP_ID)
> + return dev_err_probe(&client->dev, -EIO,
> + "chip id mismatch: %x!=%llx\n",
> + IMX214_CHIP_ID, val);
> +
> + return 0;
> +}
> +
> static int imx214_parse_fwnode(struct device *dev)
> {
> struct fwnode_handle *endpoint;
> @@ -1184,6 +1209,10 @@ static int imx214_probe(struct i2c_client *client)
> */
> imx214_power_on(imx214->dev);
>
> + ret = imx214_identify_module(imx214);
> + if (ret)
> + goto error_power_off;
The error handling was wrong in the driver, the code under the
error_power_off label only disables the regulator (and runtime PM, which
hasn't been enabled) but it does not disable or unprepare the clock. Could
you fix these in a new patch, please?
> +
> pm_runtime_set_active(imx214->dev);
> pm_runtime_enable(imx214->dev);
> pm_runtime_idle(imx214->dev);
>
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 11/12] media: i2c: imx214: Add test pattern control
2024-12-07 20:47 [PATCH v3 00/12] media: i2c: imx214: Miscellaneous cleanups and improvements André Apitzsch via B4 Relay
` (9 preceding siblings ...)
2024-12-07 20:47 ` [PATCH v3 10/12] media: i2c: imx214: Verify chip ID André Apitzsch via B4 Relay
@ 2024-12-07 20:48 ` André Apitzsch via B4 Relay
2024-12-07 20:48 ` [PATCH v3 12/12] media: i2c: imx214: Fix link frequency André Apitzsch via B4 Relay
11 siblings, 0 replies; 27+ messages in thread
From: André Apitzsch via B4 Relay @ 2024-12-07 20:48 UTC (permalink / raw)
To: Ricardo Ribalda, Sakari Ailus, Mauro Carvalho Chehab
Cc: ~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht,
André Apitzsch, Ricardo Ribalda
From: André Apitzsch <git@apitzsch.eu>
This adds V4L2_CID_TEST_PATTERN control support.
Acked-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
drivers/media/i2c/imx214.c | 77 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 75 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 3cd64bb91795d8490acad5926940c111fb95fae5..1330f13207beec0960c384681bf0b49e99fe860f 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -177,6 +177,23 @@
#define IMX214_REG_ATR_FAST_MOVE CCI_REG8(0x9300)
+/* Test Pattern Control */
+#define IMX214_REG_TEST_PATTERN CCI_REG16(0x0600)
+#define IMX214_TEST_PATTERN_DISABLE 0
+#define IMX214_TEST_PATTERN_SOLID_COLOR 1
+#define IMX214_TEST_PATTERN_COLOR_BARS 2
+#define IMX214_TEST_PATTERN_GREY_COLOR 3
+#define IMX214_TEST_PATTERN_PN9 4
+
+/* Test pattern colour components */
+#define IMX214_REG_TESTP_RED CCI_REG16(0x0602)
+#define IMX214_REG_TESTP_GREENR CCI_REG16(0x0604)
+#define IMX214_REG_TESTP_BLUE CCI_REG16(0x0606)
+#define IMX214_REG_TESTP_GREENB CCI_REG16(0x0608)
+#define IMX214_TESTP_COLOUR_MIN 0
+#define IMX214_TESTP_COLOUR_MAX 0x03ff
+#define IMX214_TESTP_COLOUR_STEP 1
+
/* IMX214 native and active pixel array size */
#define IMX214_NATIVE_WIDTH 4224U
#define IMX214_NATIVE_HEIGHT 3136U
@@ -209,6 +226,22 @@ static const u32 imx214_mbus_formats[] = {
MEDIA_BUS_FMT_SBGGR10_1X10,
};
+static const char * const imx214_test_pattern_menu[] = {
+ "Disabled",
+ "Color Bars",
+ "Solid Color",
+ "Grey Color Bars",
+ "PN9"
+};
+
+static const int imx214_test_pattern_val[] = {
+ IMX214_TEST_PATTERN_DISABLE,
+ IMX214_TEST_PATTERN_COLOR_BARS,
+ IMX214_TEST_PATTERN_SOLID_COLOR,
+ IMX214_TEST_PATTERN_GREY_COLOR,
+ IMX214_TEST_PATTERN_PN9,
+};
+
struct imx214 {
struct device *dev;
struct clk *xclk;
@@ -809,6 +842,26 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
cci_write(imx214->regmap, IMX214_REG_FRM_LENGTH_LINES,
format->height + ctrl->val, &ret);
break;
+ case V4L2_CID_TEST_PATTERN:
+ cci_write(imx214->regmap, IMX214_REG_TEST_PATTERN,
+ imx214_test_pattern_val[ctrl->val], &ret);
+ break;
+ case V4L2_CID_TEST_PATTERN_RED:
+ cci_write(imx214->regmap, IMX214_REG_TESTP_RED,
+ ctrl->val, &ret);
+ break;
+ case V4L2_CID_TEST_PATTERN_GREENR:
+ cci_write(imx214->regmap, IMX214_REG_TESTP_GREENR,
+ ctrl->val, &ret);
+ break;
+ case V4L2_CID_TEST_PATTERN_BLUE:
+ cci_write(imx214->regmap, IMX214_REG_TESTP_BLUE,
+ ctrl->val, &ret);
+ break;
+ case V4L2_CID_TEST_PATTERN_GREENB:
+ cci_write(imx214->regmap, IMX214_REG_TESTP_GREENB,
+ ctrl->val, &ret);
+ break;
default:
ret = -EINVAL;
}
@@ -836,14 +889,14 @@ static int imx214_ctrls_init(struct imx214 *imx214)
struct v4l2_ctrl_handler *ctrl_hdlr;
int exposure_max, exposure_def;
int hblank;
- int ret;
+ int i, ret;
ret = v4l2_fwnode_device_parse(imx214->dev, &props);
if (ret < 0)
return ret;
ctrl_hdlr = &imx214->ctrls;
- ret = v4l2_ctrl_handler_init(&imx214->ctrls, 12);
+ ret = v4l2_ctrl_handler_init(&imx214->ctrls, 13);
if (ret)
return ret;
@@ -911,6 +964,26 @@ static int imx214_ctrls_init(struct imx214 *imx214)
v4l2_ctrl_cluster(2, &imx214->hflip);
+ v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx214_ctrl_ops,
+ V4L2_CID_TEST_PATTERN,
+ ARRAY_SIZE(imx214_test_pattern_menu) - 1,
+ 0, 0, imx214_test_pattern_menu);
+ for (i = 0; i < 4; i++) {
+ /*
+ * The assumption is that
+ * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
+ * V4L2_CID_TEST_PATTERN_BLUE == V4L2_CID_TEST_PATTERN_RED + 2
+ * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
+ */
+ v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
+ V4L2_CID_TEST_PATTERN_RED + i,
+ IMX214_TESTP_COLOUR_MIN,
+ IMX214_TESTP_COLOUR_MAX,
+ IMX214_TESTP_COLOUR_STEP,
+ IMX214_TESTP_COLOUR_MAX);
+ /* The "Solid color" pattern is white by default */
+ }
+
imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr,
NULL,
V4L2_CID_UNIT_CELL_SIZE,
--
2.47.1
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v3 12/12] media: i2c: imx214: Fix link frequency
2024-12-07 20:47 [PATCH v3 00/12] media: i2c: imx214: Miscellaneous cleanups and improvements André Apitzsch via B4 Relay
` (10 preceding siblings ...)
2024-12-07 20:48 ` [PATCH v3 11/12] media: i2c: imx214: Add test pattern control André Apitzsch via B4 Relay
@ 2024-12-07 20:48 ` André Apitzsch via B4 Relay
2024-12-08 21:05 ` Ricardo Ribalda Delgado
11 siblings, 1 reply; 27+ messages in thread
From: André Apitzsch via B4 Relay @ 2024-12-07 20:48 UTC (permalink / raw)
To: Ricardo Ribalda, Sakari Ailus, Mauro Carvalho Chehab
Cc: ~postmarketos/upstreaming, phone-devel, linux-media,
linux-kernel, Dave Stevenson, Vincent Knecht,
André Apitzsch
From: André Apitzsch <git@apitzsch.eu>
The driver defines IMX214_DEFAULT_LINK_FREQ 480000000, and then
IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10),
which works out as 384MPix/s. (The 8 is 4 lanes and DDR.)
Parsing the PLL registers with the defined 24MHz input. We're in single
PLL mode, so MIPI frequency is directly linked to pixel rate. VTCK ends
up being 1200MHz, and VTPXCK and OPPXCK both are 120MHz. Section 5.3
"Frame rate calculation formula" says "Pixel rate
[pixels/s] = VTPXCK [MHz] * 4", so 120 * 4 = 480MPix/s, which basically
agrees with my number above.
3.1.4. MIPI global timing setting says "Output bitrate = OPPXCK * reg
0x113[7:0]", so 120MHz * 10, or 1200Mbit/s. That would be a link
frequency of 600MHz due to DDR.
That also matches to 480MPix/s * 10bpp / 4 lanes / 2 for DDR.
Keep the previous link frequency for backward compatibility.
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
drivers/media/i2c/imx214.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 1330f13207beec0960c384681bf0b49e99fe860f..910ad03cda23345d3d10d13cd30f007954534e80 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -31,7 +31,9 @@
#define IMX214_REG_FAST_STANDBY_CTRL CCI_REG8(0x0106)
#define IMX214_DEFAULT_CLK_FREQ 24000000
-#define IMX214_DEFAULT_LINK_FREQ 480000000
+#define IMX214_DEFAULT_LINK_FREQ 600000000
+/* Keep wrong link frequency for backward compatibility */
+#define IMX214_DEFAULT_LINK_FREQ_LEGACY 480000000
#define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10)
#define IMX214_FPS 30
@@ -1216,18 +1218,22 @@ static int imx214_parse_fwnode(struct device *dev)
goto done;
}
- for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
+ for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ)
break;
-
- if (i == bus_cfg.nr_of_link_frequencies) {
- dev_err_probe(dev, -EINVAL,
- "link-frequencies %d not supported, Please review your DT\n",
- IMX214_DEFAULT_LINK_FREQ);
- ret = -EINVAL;
- goto done;
+ if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ_LEGACY) {
+ dev_warn(dev,
+ "link-frequencies %d not supported, please review your DT. Continuing anyway\n",
+ IMX214_DEFAULT_LINK_FREQ);
+ break;
+ }
}
+ if (i == bus_cfg.nr_of_link_frequencies)
+ ret = dev_err_probe(dev, -EINVAL,
+ "link-frequencies %d not supported, please review your DT\n",
+ IMX214_DEFAULT_LINK_FREQ);
+
done:
v4l2_fwnode_endpoint_free(&bus_cfg);
fwnode_handle_put(endpoint);
--
2.47.1
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v3 12/12] media: i2c: imx214: Fix link frequency
2024-12-07 20:48 ` [PATCH v3 12/12] media: i2c: imx214: Fix link frequency André Apitzsch via B4 Relay
@ 2024-12-08 21:05 ` Ricardo Ribalda Delgado
0 siblings, 0 replies; 27+ messages in thread
From: Ricardo Ribalda Delgado @ 2024-12-08 21:05 UTC (permalink / raw)
To: git
Cc: Sakari Ailus, Mauro Carvalho Chehab, ~postmarketos/upstreaming,
phone-devel, linux-media, linux-kernel, Dave Stevenson,
Vincent Knecht
Acked-by: Ricardo Ribalda <ribalda@chromium.org>
nit: media: i2c: imx214: Fix link frequency validation
On Sat, Dec 7, 2024 at 9:49 PM André Apitzsch via B4 Relay
<devnull+git.apitzsch.eu@kernel.org> wrote:
>
> From: André Apitzsch <git@apitzsch.eu>
>
> The driver defines IMX214_DEFAULT_LINK_FREQ 480000000, and then
> IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10),
> which works out as 384MPix/s. (The 8 is 4 lanes and DDR.)
>
> Parsing the PLL registers with the defined 24MHz input. We're in single
> PLL mode, so MIPI frequency is directly linked to pixel rate. VTCK ends
> up being 1200MHz, and VTPXCK and OPPXCK both are 120MHz. Section 5.3
> "Frame rate calculation formula" says "Pixel rate
> [pixels/s] = VTPXCK [MHz] * 4", so 120 * 4 = 480MPix/s, which basically
> agrees with my number above.
>
> 3.1.4. MIPI global timing setting says "Output bitrate = OPPXCK * reg
> 0x113[7:0]", so 120MHz * 10, or 1200Mbit/s. That would be a link
> frequency of 600MHz due to DDR.
> That also matches to 480MPix/s * 10bpp / 4 lanes / 2 for DDR.
>
> Keep the previous link frequency for backward compatibility.
>
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
> drivers/media/i2c/imx214.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 1330f13207beec0960c384681bf0b49e99fe860f..910ad03cda23345d3d10d13cd30f007954534e80 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -31,7 +31,9 @@
> #define IMX214_REG_FAST_STANDBY_CTRL CCI_REG8(0x0106)
>
> #define IMX214_DEFAULT_CLK_FREQ 24000000
> -#define IMX214_DEFAULT_LINK_FREQ 480000000
> +#define IMX214_DEFAULT_LINK_FREQ 600000000
> +/* Keep wrong link frequency for backward compatibility */
> +#define IMX214_DEFAULT_LINK_FREQ_LEGACY 480000000
> #define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10)
> #define IMX214_FPS 30
>
> @@ -1216,18 +1218,22 @@ static int imx214_parse_fwnode(struct device *dev)
> goto done;
> }
>
Now that I think about this... We only support
buf_cfg.no_of_link_frequencies ==1.
Maybe you can add a check before the loop
if (bus_cfg.nr_of_link_frequencies != 1)
dev_warn(dev, "Only one link-frequency
supported, please review your DT. Continuing anyway\n ")
> - for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> + for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ)
> break;
> -
> - if (i == bus_cfg.nr_of_link_frequencies) {
> - dev_err_probe(dev, -EINVAL,
> - "link-frequencies %d not supported, Please review your DT\n",
> - IMX214_DEFAULT_LINK_FREQ);
> - ret = -EINVAL;
> - goto done;
> + if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ_LEGACY) {
> + dev_warn(dev,
> + "link-frequencies %d not supported, please review your DT. Continuing anyway\n",
> + IMX214_DEFAULT_LINK_FREQ);
> + break;
> + }
> }
>
> + if (i == bus_cfg.nr_of_link_frequencies)
> + ret = dev_err_probe(dev, -EINVAL,
> + "link-frequencies %d not supported, please review your DT\n",
> + IMX214_DEFAULT_LINK_FREQ);
> +
> done:
> v4l2_fwnode_endpoint_free(&bus_cfg);
> fwnode_handle_put(endpoint);
>
> --
> 2.47.1
>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread