* [PATCH v3 01/17] media: i2c: imx290: Group functions in sections
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 02/17] media: i2c: imx290: Factor out subdev init and cleanup to functions Laurent Pinchart
` (15 subsequent siblings)
16 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
Move functions around to group them in logical sections, in order to
improve readability. As a result, the IMX290_NUM_SUPPLIES macro has to
be changed. No other code change is included, only moves.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
drivers/media/i2c/imx290.c | 626 +++++++++++++++++++------------------
1 file changed, 325 insertions(+), 301 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 218ded13fd80..ca2fa57c28fe 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -152,13 +152,7 @@
#define IMX290_PIXEL_ARRAY_RECORDING_WIDTH 1920
#define IMX290_PIXEL_ARRAY_RECORDING_HEIGHT 1080
-static const char * const imx290_supply_name[] = {
- "vdda",
- "vddd",
- "vdddo",
-};
-
-#define IMX290_NUM_SUPPLIES ARRAY_SIZE(imx290_supply_name)
+#define IMX290_NUM_SUPPLIES 3
struct imx290_regval {
u32 reg;
@@ -199,31 +193,14 @@ struct imx290 {
struct mutex lock;
};
-struct imx290_pixfmt {
- u32 code;
- u8 bpp;
-};
+static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
+{
+ return container_of(_sd, struct imx290, sd);
+}
-static const struct imx290_pixfmt imx290_formats[] = {
- { MEDIA_BUS_FMT_SRGGB10_1X10, 10 },
- { MEDIA_BUS_FMT_SRGGB12_1X12, 12 },
-};
-
-static const struct regmap_config imx290_regmap_config = {
- .reg_bits = 16,
- .val_bits = 8,
-};
-
-static const char * const imx290_test_pattern_menu[] = {
- "Disabled",
- "Sequence Pattern 1",
- "Horizontal Color-bar Chart",
- "Vertical Color-bar Chart",
- "Sequence Pattern 2",
- "Gradation Pattern 1",
- "Gradation Pattern 2",
- "000/555h Toggle Pattern",
-};
+/* -----------------------------------------------------------------------------
+ * Modes and formats
+ */
static const struct imx290_regval imx290_global_init_settings[] = {
{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
@@ -438,10 +415,19 @@ static inline int imx290_modes_num(const struct imx290 *imx290)
return ARRAY_SIZE(imx290_modes_4lanes);
}
-static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
-{
- return container_of(_sd, struct imx290, sd);
-}
+struct imx290_pixfmt {
+ u32 code;
+ u8 bpp;
+};
+
+static const struct imx290_pixfmt imx290_formats[] = {
+ { MEDIA_BUS_FMT_SRGGB10_1X10, 10 },
+ { MEDIA_BUS_FMT_SRGGB12_1X12, 12 },
+};
+
+/* -----------------------------------------------------------------------------
+ * Register access
+ */
static int __always_unused imx290_read(struct imx290 *imx290, u32 addr, u32 *value)
{
@@ -501,18 +487,94 @@ static int imx290_set_register_array(struct imx290 *imx290,
return 0;
}
-/* Stop streaming */
-static int imx290_stop_streaming(struct imx290 *imx290)
+static int imx290_set_data_lanes(struct imx290 *imx290)
{
- int ret = 0;
+ int ret = 0, laneval, frsel;
- imx290_write(imx290, IMX290_STANDBY, 0x01, &ret);
+ switch (imx290->nlanes) {
+ case 2:
+ laneval = 0x01;
+ frsel = 0x02;
+ break;
+ case 4:
+ laneval = 0x03;
+ frsel = 0x01;
+ break;
+ default:
+ /*
+ * We should never hit this since the data lane count is
+ * validated in probe itself
+ */
+ dev_err(imx290->dev, "Lane configuration not supported\n");
+ return -EINVAL;
+ }
- msleep(30);
+ imx290_write(imx290, IMX290_PHY_LANE_NUM, laneval, &ret);
+ imx290_write(imx290, IMX290_CSI_LANE_MODE, laneval, &ret);
+ imx290_write(imx290, IMX290_FR_FDG_SEL, frsel, &ret);
- return imx290_write(imx290, IMX290_XMSTA, 0x01, &ret);
+ return ret;
}
+static int imx290_write_current_format(struct imx290 *imx290)
+{
+ int ret;
+
+ switch (imx290->current_format.code) {
+ case MEDIA_BUS_FMT_SRGGB10_1X10:
+ ret = imx290_set_register_array(imx290, imx290_10bit_settings,
+ ARRAY_SIZE(
+ imx290_10bit_settings));
+ if (ret < 0) {
+ dev_err(imx290->dev, "Could not set format registers\n");
+ return ret;
+ }
+ break;
+ case MEDIA_BUS_FMT_SRGGB12_1X12:
+ ret = imx290_set_register_array(imx290, imx290_12bit_settings,
+ ARRAY_SIZE(
+ imx290_12bit_settings));
+ if (ret < 0) {
+ dev_err(imx290->dev, "Could not set format registers\n");
+ return ret;
+ }
+ break;
+ default:
+ dev_err(imx290->dev, "Unknown pixel format\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static inline u8 imx290_get_link_freq_index(struct imx290 *imx290)
+{
+ return imx290->current_mode->link_freq_index;
+}
+
+static s64 imx290_get_link_freq(struct imx290 *imx290)
+{
+ u8 index = imx290_get_link_freq_index(imx290);
+
+ return *(imx290_link_freqs_ptr(imx290) + index);
+}
+
+static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
+{
+ s64 link_freq = imx290_get_link_freq(imx290);
+ u8 nlanes = imx290->nlanes;
+ u64 pixel_rate;
+
+ /* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
+ pixel_rate = link_freq * 2 * nlanes;
+ do_div(pixel_rate, imx290->bpp);
+ return pixel_rate;
+}
+
+/* ----------------------------------------------------------------------------
+ * Controls
+ */
+
static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
{
struct imx290 *imx290 = container_of(ctrl->handler,
@@ -566,6 +628,187 @@ static const struct v4l2_ctrl_ops imx290_ctrl_ops = {
.s_ctrl = imx290_set_ctrl,
};
+static const char * const imx290_test_pattern_menu[] = {
+ "Disabled",
+ "Sequence Pattern 1",
+ "Horizontal Color-bar Chart",
+ "Vertical Color-bar Chart",
+ "Sequence Pattern 2",
+ "Gradation Pattern 1",
+ "Gradation Pattern 2",
+ "000/555h Toggle Pattern",
+};
+
+static int imx290_ctrl_init(struct imx290 *imx290)
+{
+ struct v4l2_fwnode_device_properties props;
+ unsigned int blank;
+ int ret;
+
+ ret = v4l2_fwnode_device_parse(imx290->dev, &props);
+ if (ret < 0)
+ return ret;
+
+ v4l2_ctrl_handler_init(&imx290->ctrls, 9);
+ imx290->ctrls.lock = &imx290->lock;
+
+ /*
+ * The sensor has an analog gain and a digital gain, both controlled
+ * through a single gain value, expressed in 0.3dB increments. Values
+ * from 0.0dB (0) to 30.0dB (100) apply analog gain only, higher values
+ * up to 72.0dB (240) add further digital gain. Limit the range to
+ * analog gain only, support for digital gain can be added separately
+ * if needed.
+ *
+ * The IMX327 and IMX462 are largely compatible with the IMX290, but
+ * have an analog gain range of 0.0dB to 29.4dB and 42dB of digital
+ * gain. When support for those sensors gets added to the driver, the
+ * gain control should be adjusted accordingly.
+ */
+ v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+ V4L2_CID_ANALOGUE_GAIN, 0, 100, 1, 0);
+
+ v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+ V4L2_CID_EXPOSURE, 1, IMX290_VMAX_DEFAULT - 2, 1,
+ IMX290_VMAX_DEFAULT - 2);
+
+ imx290->link_freq =
+ v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
+ V4L2_CID_LINK_FREQ,
+ imx290_link_freqs_num(imx290) - 1, 0,
+ imx290_link_freqs_ptr(imx290));
+ if (imx290->link_freq)
+ imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+ imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+ V4L2_CID_PIXEL_RATE,
+ 1, INT_MAX, 1,
+ imx290_calc_pixel_rate(imx290));
+
+ v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
+ V4L2_CID_TEST_PATTERN,
+ ARRAY_SIZE(imx290_test_pattern_menu) - 1,
+ 0, 0, imx290_test_pattern_menu);
+
+ blank = imx290->current_mode->hmax - imx290->current_mode->width;
+ imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+ V4L2_CID_HBLANK, blank, blank, 1,
+ blank);
+ if (imx290->hblank)
+ imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+ blank = IMX290_VMAX_DEFAULT - imx290->current_mode->height;
+ imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+ V4L2_CID_VBLANK, blank, blank, 1,
+ blank);
+ if (imx290->vblank)
+ imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+ v4l2_ctrl_new_fwnode_properties(&imx290->ctrls, &imx290_ctrl_ops,
+ &props);
+
+ imx290->sd.ctrl_handler = &imx290->ctrls;
+
+ if (imx290->ctrls.error) {
+ ret = imx290->ctrls.error;
+ v4l2_ctrl_handler_free(&imx290->ctrls);
+ return ret;
+ }
+
+ return 0;
+}
+
+/* ----------------------------------------------------------------------------
+ * Subdev operations
+ */
+
+/* Start streaming */
+static int imx290_start_streaming(struct imx290 *imx290)
+{
+ int ret;
+
+ /* Set init register settings */
+ ret = imx290_set_register_array(imx290, imx290_global_init_settings,
+ ARRAY_SIZE(
+ imx290_global_init_settings));
+ if (ret < 0) {
+ dev_err(imx290->dev, "Could not set init registers\n");
+ return ret;
+ }
+
+ /* Apply the register values related to current frame format */
+ ret = imx290_write_current_format(imx290);
+ if (ret < 0) {
+ dev_err(imx290->dev, "Could not set frame format\n");
+ return ret;
+ }
+
+ /* Apply default values of current mode */
+ ret = imx290_set_register_array(imx290, imx290->current_mode->data,
+ imx290->current_mode->data_size);
+ if (ret < 0) {
+ dev_err(imx290->dev, "Could not set current mode\n");
+ return ret;
+ }
+
+ ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax,
+ NULL);
+ if (ret)
+ return ret;
+
+ /* Apply customized values from user */
+ ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
+ if (ret) {
+ dev_err(imx290->dev, "Could not sync v4l2 controls\n");
+ return ret;
+ }
+
+ imx290_write(imx290, IMX290_STANDBY, 0x00, &ret);
+
+ msleep(30);
+
+ /* Start streaming */
+ return imx290_write(imx290, IMX290_XMSTA, 0x00, &ret);
+}
+
+/* Stop streaming */
+static int imx290_stop_streaming(struct imx290 *imx290)
+{
+ int ret = 0;
+
+ imx290_write(imx290, IMX290_STANDBY, 0x01, &ret);
+
+ msleep(30);
+
+ return imx290_write(imx290, IMX290_XMSTA, 0x01, &ret);
+}
+
+static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
+{
+ struct imx290 *imx290 = to_imx290(sd);
+ int ret = 0;
+
+ if (enable) {
+ ret = pm_runtime_resume_and_get(imx290->dev);
+ if (ret < 0)
+ goto unlock_and_return;
+
+ ret = imx290_start_streaming(imx290);
+ if (ret) {
+ dev_err(imx290->dev, "Start stream failed\n");
+ pm_runtime_put(imx290->dev);
+ goto unlock_and_return;
+ }
+ } else {
+ imx290_stop_streaming(imx290);
+ pm_runtime_put(imx290->dev);
+ }
+
+unlock_and_return:
+
+ return ret;
+}
+
static struct v4l2_mbus_framefmt *
imx290_get_pad_format(struct imx290 *imx290, struct v4l2_subdev_state *state,
u32 which)
@@ -627,30 +870,6 @@ static int imx290_get_fmt(struct v4l2_subdev *sd,
return 0;
}
-static inline u8 imx290_get_link_freq_index(struct imx290 *imx290)
-{
- return imx290->current_mode->link_freq_index;
-}
-
-static s64 imx290_get_link_freq(struct imx290 *imx290)
-{
- u8 index = imx290_get_link_freq_index(imx290);
-
- return *(imx290_link_freqs_ptr(imx290) + index);
-}
-
-static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
-{
- s64 link_freq = imx290_get_link_freq(imx290);
- u8 nlanes = imx290->nlanes;
- u64 pixel_rate;
-
- /* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
- pixel_rate = link_freq * 2 * nlanes;
- do_div(pixel_rate, imx290->bpp);
- return pixel_rate;
-}
-
static int imx290_set_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_format *fmt)
@@ -774,151 +993,31 @@ static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
return 0;
}
-static int imx290_write_current_format(struct imx290 *imx290)
-{
- int ret;
-
- switch (imx290->current_format.code) {
- case MEDIA_BUS_FMT_SRGGB10_1X10:
- ret = imx290_set_register_array(imx290, imx290_10bit_settings,
- ARRAY_SIZE(
- imx290_10bit_settings));
- if (ret < 0) {
- dev_err(imx290->dev, "Could not set format registers\n");
- return ret;
- }
- break;
- case MEDIA_BUS_FMT_SRGGB12_1X12:
- ret = imx290_set_register_array(imx290, imx290_12bit_settings,
- ARRAY_SIZE(
- imx290_12bit_settings));
- if (ret < 0) {
- dev_err(imx290->dev, "Could not set format registers\n");
- return ret;
- }
- break;
- default:
- dev_err(imx290->dev, "Unknown pixel format\n");
- return -EINVAL;
- }
-
- return 0;
-}
-
-/* Start streaming */
-static int imx290_start_streaming(struct imx290 *imx290)
-{
- int ret;
-
- /* Set init register settings */
- ret = imx290_set_register_array(imx290, imx290_global_init_settings,
- ARRAY_SIZE(
- imx290_global_init_settings));
- if (ret < 0) {
- dev_err(imx290->dev, "Could not set init registers\n");
- return ret;
- }
-
- /* Apply the register values related to current frame format */
- ret = imx290_write_current_format(imx290);
- if (ret < 0) {
- dev_err(imx290->dev, "Could not set frame format\n");
- return ret;
- }
-
- /* Apply default values of current mode */
- ret = imx290_set_register_array(imx290, imx290->current_mode->data,
- imx290->current_mode->data_size);
- if (ret < 0) {
- dev_err(imx290->dev, "Could not set current mode\n");
- return ret;
- }
-
- ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax,
- NULL);
- if (ret)
- return ret;
-
- /* Apply customized values from user */
- ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
- if (ret) {
- dev_err(imx290->dev, "Could not sync v4l2 controls\n");
- return ret;
- }
-
- imx290_write(imx290, IMX290_STANDBY, 0x00, &ret);
-
- msleep(30);
-
- /* Start streaming */
- return imx290_write(imx290, IMX290_XMSTA, 0x00, &ret);
-}
-
-static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
-{
- struct imx290 *imx290 = to_imx290(sd);
- int ret = 0;
-
- if (enable) {
- ret = pm_runtime_resume_and_get(imx290->dev);
- if (ret < 0)
- goto unlock_and_return;
-
- ret = imx290_start_streaming(imx290);
- if (ret) {
- dev_err(imx290->dev, "Start stream failed\n");
- pm_runtime_put(imx290->dev);
- goto unlock_and_return;
- }
- } else {
- imx290_stop_streaming(imx290);
- pm_runtime_put(imx290->dev);
- }
-
-unlock_and_return:
-
- return ret;
-}
-
-static int imx290_get_regulators(struct device *dev, struct imx290 *imx290)
-{
- unsigned int i;
-
- for (i = 0; i < ARRAY_SIZE(imx290->supplies); i++)
- imx290->supplies[i].supply = imx290_supply_name[i];
-
- return devm_regulator_bulk_get(dev, ARRAY_SIZE(imx290->supplies),
- imx290->supplies);
-}
-
-static int imx290_set_data_lanes(struct imx290 *imx290)
-{
- int ret = 0, laneval, frsel;
-
- switch (imx290->nlanes) {
- case 2:
- laneval = 0x01;
- frsel = 0x02;
- break;
- case 4:
- laneval = 0x03;
- frsel = 0x01;
- break;
- default:
- /*
- * We should never hit this since the data lane count is
- * validated in probe itself
- */
- dev_err(imx290->dev, "Lane configuration not supported\n");
- return -EINVAL;
- }
-
- imx290_write(imx290, IMX290_PHY_LANE_NUM, laneval, &ret);
- imx290_write(imx290, IMX290_CSI_LANE_MODE, laneval, &ret);
- imx290_write(imx290, IMX290_FR_FDG_SEL, frsel, &ret);
-
- return ret;
-}
+static const struct v4l2_subdev_video_ops imx290_video_ops = {
+ .s_stream = imx290_set_stream,
+};
+
+static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
+ .init_cfg = imx290_entity_init_cfg,
+ .enum_mbus_code = imx290_enum_mbus_code,
+ .enum_frame_size = imx290_enum_frame_size,
+ .get_fmt = imx290_get_fmt,
+ .set_fmt = imx290_set_fmt,
+ .get_selection = imx290_get_selection,
+};
+
+static const struct v4l2_subdev_ops imx290_subdev_ops = {
+ .video = &imx290_video_ops,
+ .pad = &imx290_pad_ops,
+};
+
+static const struct media_entity_operations imx290_subdev_entity_ops = {
+ .link_validate = v4l2_subdev_link_validate,
+};
+
+/* ----------------------------------------------------------------------------
+ * Power management
+ */
static int imx290_power_on(struct device *dev)
{
@@ -966,105 +1065,30 @@ static const struct dev_pm_ops imx290_pm_ops = {
SET_RUNTIME_PM_OPS(imx290_power_off, imx290_power_on, NULL)
};
-static const struct v4l2_subdev_video_ops imx290_video_ops = {
- .s_stream = imx290_set_stream,
-};
-
-static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
- .init_cfg = imx290_entity_init_cfg,
- .enum_mbus_code = imx290_enum_mbus_code,
- .enum_frame_size = imx290_enum_frame_size,
- .get_fmt = imx290_get_fmt,
- .set_fmt = imx290_set_fmt,
- .get_selection = imx290_get_selection,
-};
+/* ----------------------------------------------------------------------------
+ * Probe & remove
+ */
-static const struct v4l2_subdev_ops imx290_subdev_ops = {
- .video = &imx290_video_ops,
- .pad = &imx290_pad_ops,
+static const struct regmap_config imx290_regmap_config = {
+ .reg_bits = 16,
+ .val_bits = 8,
};
-static const struct media_entity_operations imx290_subdev_entity_ops = {
- .link_validate = v4l2_subdev_link_validate,
+static const char * const imx290_supply_name[IMX290_NUM_SUPPLIES] = {
+ "vdda",
+ "vddd",
+ "vdddo",
};
-static int imx290_ctrl_init(struct imx290 *imx290)
+static int imx290_get_regulators(struct device *dev, struct imx290 *imx290)
{
- struct v4l2_fwnode_device_properties props;
- unsigned int blank;
- int ret;
+ unsigned int i;
- ret = v4l2_fwnode_device_parse(imx290->dev, &props);
- if (ret < 0)
- return ret;
+ for (i = 0; i < ARRAY_SIZE(imx290->supplies); i++)
+ imx290->supplies[i].supply = imx290_supply_name[i];
- v4l2_ctrl_handler_init(&imx290->ctrls, 9);
- imx290->ctrls.lock = &imx290->lock;
-
- /*
- * The sensor has an analog gain and a digital gain, both controlled
- * through a single gain value, expressed in 0.3dB increments. Values
- * from 0.0dB (0) to 30.0dB (100) apply analog gain only, higher values
- * up to 72.0dB (240) add further digital gain. Limit the range to
- * analog gain only, support for digital gain can be added separately
- * if needed.
- *
- * The IMX327 and IMX462 are largely compatible with the IMX290, but
- * have an analog gain range of 0.0dB to 29.4dB and 42dB of digital
- * gain. When support for those sensors gets added to the driver, the
- * gain control should be adjusted accordingly.
- */
- v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
- V4L2_CID_ANALOGUE_GAIN, 0, 100, 1, 0);
-
- v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
- V4L2_CID_EXPOSURE, 1, IMX290_VMAX_DEFAULT - 2, 1,
- IMX290_VMAX_DEFAULT - 2);
-
- imx290->link_freq =
- v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
- V4L2_CID_LINK_FREQ,
- imx290_link_freqs_num(imx290) - 1, 0,
- imx290_link_freqs_ptr(imx290));
- if (imx290->link_freq)
- imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-
- imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
- V4L2_CID_PIXEL_RATE,
- 1, INT_MAX, 1,
- imx290_calc_pixel_rate(imx290));
-
- v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
- V4L2_CID_TEST_PATTERN,
- ARRAY_SIZE(imx290_test_pattern_menu) - 1,
- 0, 0, imx290_test_pattern_menu);
-
- blank = imx290->current_mode->hmax - imx290->current_mode->width;
- imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
- V4L2_CID_HBLANK, blank, blank, 1,
- blank);
- if (imx290->hblank)
- imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-
- blank = IMX290_VMAX_DEFAULT - imx290->current_mode->height;
- imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
- V4L2_CID_VBLANK, blank, blank, 1,
- blank);
- if (imx290->vblank)
- imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-
- v4l2_ctrl_new_fwnode_properties(&imx290->ctrls, &imx290_ctrl_ops,
- &props);
-
- imx290->sd.ctrl_handler = &imx290->ctrls;
-
- if (imx290->ctrls.error) {
- ret = imx290->ctrls.error;
- v4l2_ctrl_handler_free(&imx290->ctrls);
- return ret;
- }
-
- return 0;
+ return devm_regulator_bulk_get(dev, ARRAY_SIZE(imx290->supplies),
+ imx290->supplies);
}
/*
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 02/17] media: i2c: imx290: Factor out subdev init and cleanup to functions
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 01/17] media: i2c: imx290: Group functions in sections Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 03/17] media: i2c: imx290: Factor out control update code to a function Laurent Pinchart
` (14 subsequent siblings)
16 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
The probe() function is large. Make it more readable by factoring the
subdev initialization code out. While at it, rename the error labels as
the "free_" prefix isn't accurate.
No functional change intended.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes since v1:
- Free control handler in imx290_subdev_init() error path
---
drivers/media/i2c/imx290.c | 108 +++++++++++++++++++++----------------
1 file changed, 62 insertions(+), 46 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index ca2fa57c28fe..5529bd39238f 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1015,6 +1015,47 @@ static const struct media_entity_operations imx290_subdev_entity_ops = {
.link_validate = v4l2_subdev_link_validate,
};
+static int imx290_subdev_init(struct imx290 *imx290)
+{
+ struct i2c_client *client = to_i2c_client(imx290->dev);
+ int ret;
+
+ /*
+ * Initialize the frame format. In particular, imx290->current_mode
+ * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() call
+ * below relies on these fields.
+ */
+ imx290_entity_init_cfg(&imx290->sd, NULL);
+
+ ret = imx290_ctrl_init(imx290);
+ if (ret < 0) {
+ dev_err(imx290->dev, "Control initialization error %d\n", ret);
+ return ret;
+ }
+
+ v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
+ imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+ imx290->sd.dev = imx290->dev;
+ imx290->sd.entity.ops = &imx290_subdev_entity_ops;
+ imx290->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+
+ imx290->pad.flags = MEDIA_PAD_FL_SOURCE;
+ ret = media_entity_pads_init(&imx290->sd.entity, 1, &imx290->pad);
+ if (ret < 0) {
+ dev_err(imx290->dev, "Could not register media entity\n");
+ v4l2_ctrl_handler_free(&imx290->ctrls);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void imx290_subdev_cleanup(struct imx290 *imx290)
+{
+ media_entity_cleanup(&imx290->sd.entity);
+ v4l2_ctrl_handler_free(&imx290->ctrls);
+}
+
/* ----------------------------------------------------------------------------
* Power management
*/
@@ -1147,10 +1188,10 @@ static int imx290_probe(struct i2c_client *client)
fwnode_handle_put(endpoint);
if (ret == -ENXIO) {
dev_err(dev, "Unsupported bus type, should be CSI2\n");
- goto free_err;
+ goto err_endpoint;
} else if (ret) {
dev_err(dev, "Parsing endpoint node failed\n");
- goto free_err;
+ goto err_endpoint;
}
/* Get number of data lanes */
@@ -1158,7 +1199,7 @@ static int imx290_probe(struct i2c_client *client)
if (imx290->nlanes != 2 && imx290->nlanes != 4) {
dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes);
ret = -EINVAL;
- goto free_err;
+ goto err_endpoint;
}
dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes);
@@ -1166,7 +1207,7 @@ static int imx290_probe(struct i2c_client *client)
if (!ep.nr_of_link_frequencies) {
dev_err(dev, "link-frequency property not found in DT\n");
ret = -EINVAL;
- goto free_err;
+ goto err_endpoint;
}
/* Check that link frequences for all the modes are in device tree */
@@ -1174,7 +1215,7 @@ static int imx290_probe(struct i2c_client *client)
if (fq) {
dev_err(dev, "Link frequency of %lld is not supported\n", fq);
ret = -EINVAL;
- goto free_err;
+ goto err_endpoint;
}
/* get system clock (xclk) */
@@ -1182,14 +1223,14 @@ static int imx290_probe(struct i2c_client *client)
if (IS_ERR(imx290->xclk)) {
dev_err(dev, "Could not get xclk");
ret = PTR_ERR(imx290->xclk);
- goto free_err;
+ goto err_endpoint;
}
ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
&xclk_freq);
if (ret) {
dev_err(dev, "Could not get xclk frequency\n");
- goto free_err;
+ goto err_endpoint;
}
/* external clock must be 37.125 MHz */
@@ -1197,19 +1238,19 @@ static int imx290_probe(struct i2c_client *client)
dev_err(dev, "External clock frequency %u is not supported\n",
xclk_freq);
ret = -EINVAL;
- goto free_err;
+ goto err_endpoint;
}
ret = clk_set_rate(imx290->xclk, xclk_freq);
if (ret) {
dev_err(dev, "Could not set xclk frequency\n");
- goto free_err;
+ goto err_endpoint;
}
ret = imx290_get_regulators(dev, imx290);
if (ret < 0) {
dev_err(dev, "Cannot get regulators\n");
- goto free_err;
+ goto err_endpoint;
}
imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset",
@@ -1217,48 +1258,26 @@ static int imx290_probe(struct i2c_client *client)
if (IS_ERR(imx290->rst_gpio)) {
dev_err(dev, "Cannot get reset gpio\n");
ret = PTR_ERR(imx290->rst_gpio);
- goto free_err;
+ goto err_endpoint;
}
mutex_init(&imx290->lock);
- /*
- * Initialize the frame format. In particular, imx290->current_mode
- * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() call
- * below relies on these fields.
- */
- imx290_entity_init_cfg(&imx290->sd, NULL);
-
- ret = imx290_ctrl_init(imx290);
- if (ret < 0) {
- dev_err(dev, "Control initialization error %d\n", ret);
- goto free_mutex;
- }
-
- v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
- imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
- imx290->sd.dev = &client->dev;
- imx290->sd.entity.ops = &imx290_subdev_entity_ops;
- imx290->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
-
- imx290->pad.flags = MEDIA_PAD_FL_SOURCE;
- ret = media_entity_pads_init(&imx290->sd.entity, 1, &imx290->pad);
- if (ret < 0) {
- dev_err(dev, "Could not register media entity\n");
- goto free_ctrl;
- }
+ ret = imx290_subdev_init(imx290);
+ if (ret)
+ goto err_mutex;
ret = v4l2_async_register_subdev(&imx290->sd);
if (ret < 0) {
dev_err(dev, "Could not register v4l2 device\n");
- goto free_entity;
+ goto err_subdev;
}
/* Power on the device to match runtime PM state below */
ret = imx290_power_on(dev);
if (ret < 0) {
dev_err(dev, "Could not power on the device\n");
- goto free_entity;
+ goto err_subdev;
}
pm_runtime_set_active(dev);
@@ -1269,13 +1288,11 @@ static int imx290_probe(struct i2c_client *client)
return 0;
-free_entity:
- media_entity_cleanup(&imx290->sd.entity);
-free_ctrl:
- v4l2_ctrl_handler_free(&imx290->ctrls);
-free_mutex:
+err_subdev:
+ imx290_subdev_cleanup(imx290);
+err_mutex:
mutex_destroy(&imx290->lock);
-free_err:
+err_endpoint:
v4l2_fwnode_endpoint_free(&ep);
return ret;
@@ -1287,8 +1304,7 @@ static void imx290_remove(struct i2c_client *client)
struct imx290 *imx290 = to_imx290(sd);
v4l2_async_unregister_subdev(sd);
- media_entity_cleanup(&sd->entity);
- v4l2_ctrl_handler_free(sd->ctrl_handler);
+ imx290_subdev_cleanup(imx290);
mutex_destroy(&imx290->lock);
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 03/17] media: i2c: imx290: Factor out control update code to a function
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 01/17] media: i2c: imx290: Group functions in sections Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 02/17] media: i2c: imx290: Factor out subdev init and cleanup to functions Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 04/17] media: i2c: imx290: Access link_freq_index directly Laurent Pinchart
` (13 subsequent siblings)
16 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
Move the control update code to a separate function to group it with all
the control-related code and make imx290_set_fmt() more readable.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes since v1:
- Correctly handle the case where imx290_ctrl_update() gets called
before controls are initialized
---
drivers/media/i2c/imx290.c | 43 ++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 5529bd39238f..991e7285c40c 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -639,6 +639,28 @@ static const char * const imx290_test_pattern_menu[] = {
"000/555h Toggle Pattern",
};
+static void imx290_ctrl_update(struct imx290 *imx290,
+ const struct imx290_mode *mode)
+{
+ unsigned int hblank = mode->hmax - mode->width;
+ unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
+
+ /*
+ * This function may be called from imx290_set_fmt() before controls
+ * get created by imx290_ctrl_init(). Return immediately in that case.
+ */
+ if (!imx290->ctrls.lock)
+ return;
+
+ __v4l2_ctrl_s_ctrl(imx290->link_freq,
+ imx290_get_link_freq_index(imx290));
+ __v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
+ imx290_calc_pixel_rate(imx290));
+
+ __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
+ __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
+}
+
static int imx290_ctrl_init(struct imx290 *imx290)
{
struct v4l2_fwnode_device_properties props;
@@ -904,26 +926,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
imx290->current_mode = mode;
imx290->bpp = imx290_formats[i].bpp;
- if (imx290->link_freq)
- __v4l2_ctrl_s_ctrl(imx290->link_freq,
- imx290_get_link_freq_index(imx290));
- if (imx290->pixel_rate)
- __v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
- imx290_calc_pixel_rate(imx290));
-
- if (imx290->hblank) {
- unsigned int hblank = mode->hmax - mode->width;
-
- __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank,
- 1, hblank);
- }
-
- if (imx290->vblank) {
- unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
-
- __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank,
- 1, vblank);
- }
+ imx290_ctrl_update(imx290, mode);
}
*format = fmt->format;
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 04/17] media: i2c: imx290: Access link_freq_index directly
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
` (2 preceding siblings ...)
2023-01-16 14:44 ` [PATCH v3 03/17] media: i2c: imx290: Factor out control update code to a function Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 05/17] media: i2c: imx290: Pass format and mode to imx290_calc_pixel_rate() Laurent Pinchart
` (12 subsequent siblings)
16 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
The imx290_get_link_freq_index() function hides the fact that it relies
on the imx290 current_mode field, which obfuscates the code instead of
making it more readable. Inline it in the callers, and use the mode
pointer we already have in imx290_ctrl_update() instead of using the
current_mode field.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
drivers/media/i2c/imx290.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 991e7285c40c..4ad6eab4f2e2 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -547,14 +547,9 @@ static int imx290_write_current_format(struct imx290 *imx290)
return 0;
}
-static inline u8 imx290_get_link_freq_index(struct imx290 *imx290)
-{
- return imx290->current_mode->link_freq_index;
-}
-
static s64 imx290_get_link_freq(struct imx290 *imx290)
{
- u8 index = imx290_get_link_freq_index(imx290);
+ u8 index = imx290->current_mode->link_freq_index;
return *(imx290_link_freqs_ptr(imx290) + index);
}
@@ -652,8 +647,7 @@ static void imx290_ctrl_update(struct imx290 *imx290,
if (!imx290->ctrls.lock)
return;
- __v4l2_ctrl_s_ctrl(imx290->link_freq,
- imx290_get_link_freq_index(imx290));
+ __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
imx290_calc_pixel_rate(imx290));
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 05/17] media: i2c: imx290: Pass format and mode to imx290_calc_pixel_rate()
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
` (3 preceding siblings ...)
2023-01-16 14:44 ` [PATCH v3 04/17] media: i2c: imx290: Access link_freq_index directly Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 15:00 ` Alexander Stein
2023-01-16 14:44 ` [PATCH v3 06/17] media: i2c: imx290: Compute pixel rate and blanking in one place Laurent Pinchart
` (11 subsequent siblings)
16 siblings, 1 reply; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
Avoid accessing the imx290 current_format and current_mode fields in
imx290_calc_pixel_rate() to prepare for the removal of those fields.
Among the two callers of the function, imx290_ctrl_update() has an
explicit mode pointer already, and we can also give it a format pointer.
Use those explicitly.
While at it, inline the imx290_get_link_freq() function in
imx290_calc_pixel_rate() as it is only called there.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:
- Really drop format argument from imx290_calc_pixel_rate()
Changes since v1:
- Drop format argument from imx290_calc_pixel_rate()
---
drivers/media/i2c/imx290.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 4ad6eab4f2e2..25671ded7c2a 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -547,21 +547,14 @@ static int imx290_write_current_format(struct imx290 *imx290)
return 0;
}
-static s64 imx290_get_link_freq(struct imx290 *imx290)
+static u64 imx290_calc_pixel_rate(struct imx290 *imx290,
+ const struct imx290_mode *mode)
{
- u8 index = imx290->current_mode->link_freq_index;
-
- return *(imx290_link_freqs_ptr(imx290) + index);
-}
-
-static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
-{
- s64 link_freq = imx290_get_link_freq(imx290);
- u8 nlanes = imx290->nlanes;
+ s64 link_freq = imx290_link_freqs_ptr(imx290)[mode->link_freq_index];
u64 pixel_rate;
/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
- pixel_rate = link_freq * 2 * nlanes;
+ pixel_rate = link_freq * 2 * imx290->nlanes;
do_div(pixel_rate, imx290->bpp);
return pixel_rate;
}
@@ -649,7 +642,7 @@ static void imx290_ctrl_update(struct imx290 *imx290,
__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
- imx290_calc_pixel_rate(imx290));
+ imx290_calc_pixel_rate(imx290, mode));
__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
@@ -659,6 +652,7 @@ static int imx290_ctrl_init(struct imx290 *imx290)
{
struct v4l2_fwnode_device_properties props;
unsigned int blank;
+ u64 pixel_rate;
int ret;
ret = v4l2_fwnode_device_parse(imx290->dev, &props);
@@ -696,10 +690,10 @@ static int imx290_ctrl_init(struct imx290 *imx290)
if (imx290->link_freq)
imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+ pixel_rate = imx290_calc_pixel_rate(imx290, imx290->current_mode);
imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
V4L2_CID_PIXEL_RATE,
- 1, INT_MAX, 1,
- imx290_calc_pixel_rate(imx290));
+ 1, INT_MAX, 1, pixel_rate);
v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
V4L2_CID_TEST_PATTERN,
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3 05/17] media: i2c: imx290: Pass format and mode to imx290_calc_pixel_rate()
2023-01-16 14:44 ` [PATCH v3 05/17] media: i2c: imx290: Pass format and mode to imx290_calc_pixel_rate() Laurent Pinchart
@ 2023-01-16 15:00 ` Alexander Stein
0 siblings, 0 replies; 24+ messages in thread
From: Alexander Stein @ 2023-01-16 15:00 UTC (permalink / raw)
To: linux-media, Laurent Pinchart
Cc: Sakari Ailus, Manivannan Sadhasivam, Dave Stevenson
Hi Laurent,
thanks for the update.
Am Montag, 16. Januar 2023, 15:44:42 CET schrieb Laurent Pinchart:
> Avoid accessing the imx290 current_format and current_mode fields in
> imx290_calc_pixel_rate() to prepare for the removal of those fields.
> Among the two callers of the function, imx290_ctrl_update() has an
> explicit mode pointer already, and we can also give it a format pointer.
> Use those explicitly.
>
> While at it, inline the imx290_get_link_freq() function in
> imx290_calc_pixel_rate() as it is only called there.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes since v2:
>
> - Really drop format argument from imx290_calc_pixel_rate()
>
> Changes since v1:
>
> - Drop format argument from imx290_calc_pixel_rate()
> ---
> drivers/media/i2c/imx290.c | 22 ++++++++--------------
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 4ad6eab4f2e2..25671ded7c2a 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -547,21 +547,14 @@ static int imx290_write_current_format(struct imx290
> *imx290) return 0;
> }
>
> -static s64 imx290_get_link_freq(struct imx290 *imx290)
> +static u64 imx290_calc_pixel_rate(struct imx290 *imx290,
> + const struct imx290_mode *mode)
> {
> - u8 index = imx290->current_mode->link_freq_index;
> -
> - return *(imx290_link_freqs_ptr(imx290) + index);
> -}
> -
> -static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
> -{
> - s64 link_freq = imx290_get_link_freq(imx290);
> - u8 nlanes = imx290->nlanes;
> + s64 link_freq = imx290_link_freqs_ptr(imx290)[mode-
>link_freq_index];
> u64 pixel_rate;
>
> /* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> - pixel_rate = link_freq * 2 * nlanes;
> + pixel_rate = link_freq * 2 * imx290->nlanes;
> do_div(pixel_rate, imx290->bpp);
> return pixel_rate;
> }
> @@ -649,7 +642,7 @@ static void imx290_ctrl_update(struct imx290 *imx290,
>
> __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> __v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
> - imx290_calc_pixel_rate(imx290));
> + imx290_calc_pixel_rate(imx290,
mode));
>
> __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
> __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
> @@ -659,6 +652,7 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> {
> struct v4l2_fwnode_device_properties props;
> unsigned int blank;
> + u64 pixel_rate;
> int ret;
>
> ret = v4l2_fwnode_device_parse(imx290->dev, &props);
> @@ -696,10 +690,10 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> if (imx290->link_freq)
> imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> + pixel_rate = imx290_calc_pixel_rate(imx290, imx290->current_mode);
> imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls,
&imx290_ctrl_ops,
> V4L2_CID_PIXEL_RATE,
> - 1, INT_MAX, 1,
> -
imx290_calc_pixel_rate(imx290));
> + 1, INT_MAX, 1,
pixel_rate);
>
> v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
> V4L2_CID_TEST_PATTERN,
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 06/17] media: i2c: imx290: Compute pixel rate and blanking in one place
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
` (4 preceding siblings ...)
2023-01-16 14:44 ` [PATCH v3 05/17] media: i2c: imx290: Pass format and mode to imx290_calc_pixel_rate() Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 07/17] media: i2c: imx290: Factor out black level setting to a function Laurent Pinchart
` (10 subsequent siblings)
16 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
The hblank, vblank, pixel rate and link frequency values and limits are
currently computed when creating controls, in imx290_ctrl_init(), and
updated in imx290_ctrl_update(). This duplicates the logic in different
places. Simplify the code by setting the control values and limits to
hardcoded values when creating the controls, and call
imx290_ctrl_update() to then update them.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes since v1:
- Fix typo in comment
---
drivers/media/i2c/imx290.c | 43 +++++++++++++++++---------------------
1 file changed, 19 insertions(+), 24 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 25671ded7c2a..d3279d88f253 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -547,18 +547,6 @@ static int imx290_write_current_format(struct imx290 *imx290)
return 0;
}
-static u64 imx290_calc_pixel_rate(struct imx290 *imx290,
- const struct imx290_mode *mode)
-{
- s64 link_freq = imx290_link_freqs_ptr(imx290)[mode->link_freq_index];
- u64 pixel_rate;
-
- /* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
- pixel_rate = link_freq * 2 * imx290->nlanes;
- do_div(pixel_rate, imx290->bpp);
- return pixel_rate;
-}
-
/* ----------------------------------------------------------------------------
* Controls
*/
@@ -632,6 +620,8 @@ static void imx290_ctrl_update(struct imx290 *imx290,
{
unsigned int hblank = mode->hmax - mode->width;
unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
+ s64 link_freq = imx290_link_freqs_ptr(imx290)[mode->link_freq_index];
+ u64 pixel_rate;
/*
* This function may be called from imx290_set_fmt() before controls
@@ -640,9 +630,12 @@ static void imx290_ctrl_update(struct imx290 *imx290,
if (!imx290->ctrls.lock)
return;
+ /* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
+ pixel_rate = link_freq * 2 * imx290->nlanes;
+ do_div(pixel_rate, imx290->bpp);
+
__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
- __v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
- imx290_calc_pixel_rate(imx290, mode));
+ __v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate, pixel_rate);
__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
@@ -651,8 +644,6 @@ static void imx290_ctrl_update(struct imx290 *imx290,
static int imx290_ctrl_init(struct imx290 *imx290)
{
struct v4l2_fwnode_device_properties props;
- unsigned int blank;
- u64 pixel_rate;
int ret;
ret = v4l2_fwnode_device_parse(imx290->dev, &props);
@@ -682,6 +673,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
V4L2_CID_EXPOSURE, 1, IMX290_VMAX_DEFAULT - 2, 1,
IMX290_VMAX_DEFAULT - 2);
+ /*
+ * Set the link frequency, pixel rate, horizontal blanking and vertical
+ * blanking to hardcoded values, they will be updated by
+ * imx290_ctrl_update().
+ */
imx290->link_freq =
v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
V4L2_CID_LINK_FREQ,
@@ -690,27 +686,22 @@ static int imx290_ctrl_init(struct imx290 *imx290)
if (imx290->link_freq)
imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
- pixel_rate = imx290_calc_pixel_rate(imx290, imx290->current_mode);
imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
V4L2_CID_PIXEL_RATE,
- 1, INT_MAX, 1, pixel_rate);
+ 1, INT_MAX, 1, 1);
v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
V4L2_CID_TEST_PATTERN,
ARRAY_SIZE(imx290_test_pattern_menu) - 1,
0, 0, imx290_test_pattern_menu);
- blank = imx290->current_mode->hmax - imx290->current_mode->width;
imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
- V4L2_CID_HBLANK, blank, blank, 1,
- blank);
+ V4L2_CID_HBLANK, 1, 1, 1, 1);
if (imx290->hblank)
imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
- blank = IMX290_VMAX_DEFAULT - imx290->current_mode->height;
imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
- V4L2_CID_VBLANK, blank, blank, 1,
- blank);
+ V4L2_CID_VBLANK, 1, 1, 1, 1);
if (imx290->vblank)
imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
@@ -725,6 +716,10 @@ static int imx290_ctrl_init(struct imx290 *imx290)
return ret;
}
+ mutex_lock(imx290->ctrls.lock);
+ imx290_ctrl_update(imx290, imx290->current_mode);
+ mutex_unlock(imx290->ctrls.lock);
+
return 0;
}
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 07/17] media: i2c: imx290: Factor out black level setting to a function
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
` (5 preceding siblings ...)
2023-01-16 14:44 ` [PATCH v3 06/17] media: i2c: imx290: Compute pixel rate and blanking in one place Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 08/17] media: i2c: imx290: Factor out DT parsing to separate function Laurent Pinchart
` (9 subsequent siblings)
16 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
The black level programmed in the BLKLEVEL register depends on the
output format. The black level value computation is currently performed
in imx290_set_ctrl(), in addition to having different black level values
in the output-specific register value tables. Move it to a separate
function to simplify the imx290_set_ctrl() code.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
Changes since v1:
- Drop bpp variable in imx290_write_current_format()
---
drivers/media/i2c/imx290.c | 50 ++++++++++++++++++++------------------
1 file changed, 26 insertions(+), 24 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index d3279d88f253..e7043e9a8fd5 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -152,6 +152,9 @@
#define IMX290_PIXEL_ARRAY_RECORDING_WIDTH 1920
#define IMX290_PIXEL_ARRAY_RECORDING_HEIGHT 1080
+/* Equivalent value for 16bpp */
+#define IMX290_BLACK_LEVEL_DEFAULT 3840
+
#define IMX290_NUM_SUPPLIES 3
struct imx290_regval {
@@ -315,7 +318,6 @@ static const struct imx290_regval imx290_10bit_settings[] = {
{ IMX290_ADBIT2, IMX290_ADBIT2_10BIT },
{ IMX290_ADBIT3, IMX290_ADBIT3_10BIT },
{ IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW10 },
- { IMX290_BLKLEVEL, 60 },
};
static const struct imx290_regval imx290_12bit_settings[] = {
@@ -325,7 +327,6 @@ static const struct imx290_regval imx290_12bit_settings[] = {
{ IMX290_ADBIT2, IMX290_ADBIT2_12BIT },
{ IMX290_ADBIT3, IMX290_ADBIT3_12BIT },
{ IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW12 },
- { IMX290_BLKLEVEL, 240 },
};
/* supported link frequencies */
@@ -516,35 +517,40 @@ static int imx290_set_data_lanes(struct imx290 *imx290)
return ret;
}
+static int imx290_set_black_level(struct imx290 *imx290,
+ unsigned int black_level, int *err)
+{
+ return imx290_write(imx290, IMX290_BLKLEVEL,
+ black_level >> (16 - imx290->bpp), err);
+}
+
static int imx290_write_current_format(struct imx290 *imx290)
{
+ const struct imx290_regval *regs;
+ unsigned int num_regs;
int ret;
switch (imx290->current_format.code) {
case MEDIA_BUS_FMT_SRGGB10_1X10:
- ret = imx290_set_register_array(imx290, imx290_10bit_settings,
- ARRAY_SIZE(
- imx290_10bit_settings));
- if (ret < 0) {
- dev_err(imx290->dev, "Could not set format registers\n");
- return ret;
- }
+ regs = imx290_10bit_settings;
+ num_regs = ARRAY_SIZE(imx290_10bit_settings);
break;
case MEDIA_BUS_FMT_SRGGB12_1X12:
- ret = imx290_set_register_array(imx290, imx290_12bit_settings,
- ARRAY_SIZE(
- imx290_12bit_settings));
- if (ret < 0) {
- dev_err(imx290->dev, "Could not set format registers\n");
- return ret;
- }
+ regs = imx290_12bit_settings;
+ num_regs = ARRAY_SIZE(imx290_12bit_settings);
break;
default:
dev_err(imx290->dev, "Unknown pixel format\n");
return -EINVAL;
}
- return 0;
+ ret = imx290_set_register_array(imx290, regs, num_regs);
+ if (ret < 0) {
+ dev_err(imx290->dev, "Could not set format registers\n");
+ return ret;
+ }
+
+ return imx290_set_black_level(imx290, IMX290_BLACK_LEVEL_DEFAULT, &ret);
}
/* ----------------------------------------------------------------------------
@@ -573,7 +579,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
case V4L2_CID_TEST_PATTERN:
if (ctrl->val) {
- imx290_write(imx290, IMX290_BLKLEVEL, 0, &ret);
+ imx290_set_black_level(imx290, 0, &ret);
usleep_range(10000, 11000);
imx290_write(imx290, IMX290_PGCTRL,
(u8)(IMX290_PGCTRL_REGEN |
@@ -582,12 +588,8 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
} else {
imx290_write(imx290, IMX290_PGCTRL, 0x00, &ret);
usleep_range(10000, 11000);
- if (imx290->bpp == 10)
- imx290_write(imx290, IMX290_BLKLEVEL, 0x3c,
- &ret);
- else /* 12 bits per pixel */
- imx290_write(imx290, IMX290_BLKLEVEL, 0xf0,
- &ret);
+ imx290_set_black_level(imx290, IMX290_BLACK_LEVEL_DEFAULT,
+ &ret);
}
break;
default:
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 08/17] media: i2c: imx290: Factor out DT parsing to separate function
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
` (6 preceding siblings ...)
2023-01-16 14:44 ` [PATCH v3 07/17] media: i2c: imx290: Factor out black level setting to a function Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 09/17] media: i2c: imx290: Use dev_err_probe() Laurent Pinchart
` (8 subsequent siblings)
16 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
Make the probe() function more readable by factoring out the DT parsing
code to a separate function. No functional change intended.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
drivers/media/i2c/imx290.c | 95 +++++++++++++++++++++-----------------
1 file changed, 52 insertions(+), 43 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index e7043e9a8fd5..530da5b03e61 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1142,111 +1142,124 @@ static s64 imx290_check_link_freqs(const struct imx290 *imx290,
return 0;
}
-static int imx290_probe(struct i2c_client *client)
+static int imx290_parse_dt(struct imx290 *imx290)
{
- struct device *dev = &client->dev;
- struct fwnode_handle *endpoint;
/* Only CSI2 is supported for now: */
struct v4l2_fwnode_endpoint ep = {
.bus_type = V4L2_MBUS_CSI2_DPHY
};
- struct imx290 *imx290;
- u32 xclk_freq;
+ struct fwnode_handle *endpoint;
+ int ret;
s64 fq;
- int ret;
- imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
- if (!imx290)
- return -ENOMEM;
-
- imx290->dev = dev;
- imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config);
- if (IS_ERR(imx290->regmap)) {
- dev_err(dev, "Unable to initialize I2C\n");
- return -ENODEV;
- }
-
- endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
+ endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), NULL);
if (!endpoint) {
- dev_err(dev, "Endpoint node not found\n");
+ dev_err(imx290->dev, "Endpoint node not found\n");
return -EINVAL;
}
ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
fwnode_handle_put(endpoint);
if (ret == -ENXIO) {
- dev_err(dev, "Unsupported bus type, should be CSI2\n");
- goto err_endpoint;
+ dev_err(imx290->dev, "Unsupported bus type, should be CSI2\n");
+ goto done;
} else if (ret) {
- dev_err(dev, "Parsing endpoint node failed\n");
- goto err_endpoint;
+ dev_err(imx290->dev, "Parsing endpoint node failed\n");
+ goto done;
}
/* Get number of data lanes */
imx290->nlanes = ep.bus.mipi_csi2.num_data_lanes;
if (imx290->nlanes != 2 && imx290->nlanes != 4) {
- dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes);
+ dev_err(imx290->dev, "Invalid data lanes: %d\n", imx290->nlanes);
ret = -EINVAL;
- goto err_endpoint;
+ goto done;
}
- dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes);
+ dev_dbg(imx290->dev, "Using %u data lanes\n", imx290->nlanes);
if (!ep.nr_of_link_frequencies) {
- dev_err(dev, "link-frequency property not found in DT\n");
+ dev_err(imx290->dev, "link-frequency property not found in DT\n");
ret = -EINVAL;
- goto err_endpoint;
+ goto done;
}
/* Check that link frequences for all the modes are in device tree */
fq = imx290_check_link_freqs(imx290, &ep);
if (fq) {
- dev_err(dev, "Link frequency of %lld is not supported\n", fq);
+ dev_err(imx290->dev, "Link frequency of %lld is not supported\n",
+ fq);
ret = -EINVAL;
- goto err_endpoint;
+ goto done;
}
+ ret = 0;
+
+done:
+ v4l2_fwnode_endpoint_free(&ep);
+ return ret;
+}
+
+static int imx290_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct imx290 *imx290;
+ u32 xclk_freq;
+ int ret;
+
+ imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
+ if (!imx290)
+ return -ENOMEM;
+
+ imx290->dev = dev;
+ imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config);
+ if (IS_ERR(imx290->regmap)) {
+ dev_err(dev, "Unable to initialize I2C\n");
+ return -ENODEV;
+ }
+
+ ret = imx290_parse_dt(imx290);
+ if (ret)
+ return ret;
+
/* get system clock (xclk) */
imx290->xclk = devm_clk_get(dev, "xclk");
if (IS_ERR(imx290->xclk)) {
dev_err(dev, "Could not get xclk");
- ret = PTR_ERR(imx290->xclk);
- goto err_endpoint;
+ return PTR_ERR(imx290->xclk);
}
ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
&xclk_freq);
if (ret) {
dev_err(dev, "Could not get xclk frequency\n");
- goto err_endpoint;
+ return ret;
}
/* external clock must be 37.125 MHz */
if (xclk_freq != 37125000) {
dev_err(dev, "External clock frequency %u is not supported\n",
xclk_freq);
- ret = -EINVAL;
- goto err_endpoint;
+ return -EINVAL;
}
ret = clk_set_rate(imx290->xclk, xclk_freq);
if (ret) {
dev_err(dev, "Could not set xclk frequency\n");
- goto err_endpoint;
+ return ret;
}
ret = imx290_get_regulators(dev, imx290);
if (ret < 0) {
dev_err(dev, "Cannot get regulators\n");
- goto err_endpoint;
+ return ret;
}
imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset",
GPIOD_OUT_HIGH);
if (IS_ERR(imx290->rst_gpio)) {
dev_err(dev, "Cannot get reset gpio\n");
- ret = PTR_ERR(imx290->rst_gpio);
- goto err_endpoint;
+ return PTR_ERR(imx290->rst_gpio);
}
mutex_init(&imx290->lock);
@@ -1272,16 +1285,12 @@ static int imx290_probe(struct i2c_client *client)
pm_runtime_enable(dev);
pm_runtime_idle(dev);
- v4l2_fwnode_endpoint_free(&ep);
-
return 0;
err_subdev:
imx290_subdev_cleanup(imx290);
err_mutex:
mutex_destroy(&imx290->lock);
-err_endpoint:
- v4l2_fwnode_endpoint_free(&ep);
return ret;
}
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 09/17] media: i2c: imx290: Use dev_err_probe()
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
` (7 preceding siblings ...)
2023-01-16 14:44 ` [PATCH v3 08/17] media: i2c: imx290: Factor out DT parsing to separate function Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 10/17] media: i2c: imx290: Factor out clock initialization to separate function Laurent Pinchart
` (7 subsequent siblings)
16 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
Improve error handling in the probe() function with dev_err_probe().
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
drivers/media/i2c/imx290.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 530da5b03e61..51f430ca3652 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1224,10 +1224,9 @@ static int imx290_probe(struct i2c_client *client)
/* get system clock (xclk) */
imx290->xclk = devm_clk_get(dev, "xclk");
- if (IS_ERR(imx290->xclk)) {
- dev_err(dev, "Could not get xclk");
- return PTR_ERR(imx290->xclk);
- }
+ if (IS_ERR(imx290->xclk))
+ return dev_err_probe(dev, PTR_ERR(imx290->xclk),
+ "Could not get xclk");
ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
&xclk_freq);
@@ -1250,17 +1249,14 @@ static int imx290_probe(struct i2c_client *client)
}
ret = imx290_get_regulators(dev, imx290);
- if (ret < 0) {
- dev_err(dev, "Cannot get regulators\n");
- return ret;
- }
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Cannot get regulators\n");
imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset",
GPIOD_OUT_HIGH);
- if (IS_ERR(imx290->rst_gpio)) {
- dev_err(dev, "Cannot get reset gpio\n");
- return PTR_ERR(imx290->rst_gpio);
- }
+ if (IS_ERR(imx290->rst_gpio))
+ return dev_err_probe(dev, PTR_ERR(imx290->rst_gpio),
+ "Cannot get reset gpio\n");
mutex_init(&imx290->lock);
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 10/17] media: i2c: imx290: Factor out clock initialization to separate function
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
` (8 preceding siblings ...)
2023-01-16 14:44 ` [PATCH v3 09/17] media: i2c: imx290: Use dev_err_probe() Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 11/17] media: i2c: imx290: Use V4L2 subdev active state Laurent Pinchart
` (6 subsequent siblings)
16 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
Move the external clock initialization code from probe() to a separate
function to improve readability. No functional change intended.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
drivers/media/i2c/imx290.c | 57 +++++++++++++++++++++++---------------
1 file changed, 35 insertions(+), 22 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 51f430ca3652..c1d18ec51e41 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1120,6 +1120,34 @@ static int imx290_get_regulators(struct device *dev, struct imx290 *imx290)
imx290->supplies);
}
+static int imx290_init_clk(struct imx290 *imx290)
+{
+ u32 xclk_freq;
+ int ret;
+
+ ret = fwnode_property_read_u32(dev_fwnode(imx290->dev),
+ "clock-frequency", &xclk_freq);
+ if (ret) {
+ dev_err(imx290->dev, "Could not get xclk frequency\n");
+ return ret;
+ }
+
+ /* external clock must be 37.125 MHz */
+ if (xclk_freq != 37125000) {
+ dev_err(imx290->dev, "External clock frequency %u is not supported\n",
+ xclk_freq);
+ return -EINVAL;
+ }
+
+ ret = clk_set_rate(imx290->xclk, xclk_freq);
+ if (ret) {
+ dev_err(imx290->dev, "Could not set xclk frequency\n");
+ return ret;
+ }
+
+ return 0;
+}
+
/*
* Returns 0 if all link frequencies used by the driver for the given number
* of MIPI data lanes are mentioned in the device tree, or the value of the
@@ -1204,7 +1232,6 @@ static int imx290_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
struct imx290 *imx290;
- u32 xclk_freq;
int ret;
imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
@@ -1222,32 +1249,12 @@ static int imx290_probe(struct i2c_client *client)
if (ret)
return ret;
- /* get system clock (xclk) */
+ /* Acquire resources. */
imx290->xclk = devm_clk_get(dev, "xclk");
if (IS_ERR(imx290->xclk))
return dev_err_probe(dev, PTR_ERR(imx290->xclk),
"Could not get xclk");
- ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
- &xclk_freq);
- if (ret) {
- dev_err(dev, "Could not get xclk frequency\n");
- return ret;
- }
-
- /* external clock must be 37.125 MHz */
- if (xclk_freq != 37125000) {
- dev_err(dev, "External clock frequency %u is not supported\n",
- xclk_freq);
- return -EINVAL;
- }
-
- ret = clk_set_rate(imx290->xclk, xclk_freq);
- if (ret) {
- dev_err(dev, "Could not set xclk frequency\n");
- return ret;
- }
-
ret = imx290_get_regulators(dev, imx290);
if (ret < 0)
return dev_err_probe(dev, ret, "Cannot get regulators\n");
@@ -1258,8 +1265,14 @@ static int imx290_probe(struct i2c_client *client)
return dev_err_probe(dev, PTR_ERR(imx290->rst_gpio),
"Cannot get reset gpio\n");
+ /* Initialize external clock frequency. */
+ ret = imx290_init_clk(imx290);
+ if (ret)
+ return ret;
+
mutex_init(&imx290->lock);
+ /* Initialize and register subdev. */
ret = imx290_subdev_init(imx290);
if (ret)
goto err_mutex;
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 11/17] media: i2c: imx290: Use V4L2 subdev active state
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
` (9 preceding siblings ...)
2023-01-16 14:44 ` [PATCH v3 10/17] media: i2c: imx290: Factor out clock initialization to separate function Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 15:05 ` Alexander Stein
2023-01-16 14:44 ` [PATCH v3 12/17] media: i2c: imx290: Rename, extend and expand usage of imx290_pixfmt Laurent Pinchart
` (5 subsequent siblings)
16 siblings, 1 reply; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
Use the V4L2 subdev active state API to store the active format. This
simplifies the driver not only by dropping the imx290 current_format
field, but it also allows dropping the imx290 lock, replaced with the
state lock.
The lock check in imx290_ctrl_update() can be dropped as
imx290_set_fmt() can't be called anywmore with which set to ACTIVE
before controls are initialized.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:
- Test for V4L2_CTRL_FLAG_READ_ONLY
Changes since v1:
- Remove lock check from imx290_ctrl_update()
- Drop unrelated change
- Fix error handling in imx290_subdev_init()
- Handle errors from v4l2_subdev_init_finalize()
- Return immediately from imx290_set_ctrl() for read-only controls
- Call v4l2_subdev_cleanup()
---
drivers/media/i2c/imx290.c | 159 ++++++++++++++++---------------------
1 file changed, 70 insertions(+), 89 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index c1d18ec51e41..74b9a7110709 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -177,12 +177,12 @@ struct imx290 {
struct clk *xclk;
struct regmap *regmap;
u8 nlanes;
- u8 bpp;
struct v4l2_subdev sd;
struct media_pad pad;
- struct v4l2_mbus_framefmt current_format;
+
const struct imx290_mode *current_mode;
+ u8 bpp;
struct regulator_bulk_data supplies[IMX290_NUM_SUPPLIES];
struct gpio_desc *rst_gpio;
@@ -192,8 +192,6 @@ struct imx290 {
struct v4l2_ctrl *pixel_rate;
struct v4l2_ctrl *hblank;
struct v4l2_ctrl *vblank;
-
- struct mutex lock;
};
static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
@@ -524,13 +522,14 @@ static int imx290_set_black_level(struct imx290 *imx290,
black_level >> (16 - imx290->bpp), err);
}
-static int imx290_write_current_format(struct imx290 *imx290)
+static int imx290_setup_format(struct imx290 *imx290,
+ const struct v4l2_mbus_framefmt *format)
{
const struct imx290_regval *regs;
unsigned int num_regs;
int ret;
- switch (imx290->current_format.code) {
+ switch (format->code) {
case MEDIA_BUS_FMT_SRGGB10_1X10:
regs = imx290_10bit_settings;
num_regs = ARRAY_SIZE(imx290_10bit_settings);
@@ -561,12 +560,26 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
{
struct imx290 *imx290 = container_of(ctrl->handler,
struct imx290, ctrls);
+ const struct v4l2_mbus_framefmt *format;
+ struct v4l2_subdev_state *state;
int ret = 0;
+ /*
+ * Return immediately for controls that don't need to be applied to the
+ * device. Those controls are modified in imx290_ctrl_update(), which
+ * is called at probe time before runtime PM is initialized, so we
+ * can't proceed to the pm_runtime_get_if_in_use() call below.
+ */
+ if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
+ return 0;
+
/* V4L2 controls values will be applied only when power is already up */
if (!pm_runtime_get_if_in_use(imx290->dev))
return 0;
+ state = v4l2_subdev_get_locked_active_state(&imx290->sd);
+ format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
+
switch (ctrl->id) {
case V4L2_CID_ANALOGUE_GAIN:
ret = imx290_write(imx290, IMX290_GAIN, ctrl->val, NULL);
@@ -592,6 +605,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
&ret);
}
break;
+
default:
ret = -EINVAL;
break;
@@ -625,13 +639,6 @@ static void imx290_ctrl_update(struct imx290 *imx290,
s64 link_freq = imx290_link_freqs_ptr(imx290)[mode->link_freq_index];
u64 pixel_rate;
- /*
- * This function may be called from imx290_set_fmt() before controls
- * get created by imx290_ctrl_init(). Return immediately in that case.
- */
- if (!imx290->ctrls.lock)
- return;
-
/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
pixel_rate = link_freq * 2 * imx290->nlanes;
do_div(pixel_rate, imx290->bpp);
@@ -653,7 +660,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
return ret;
v4l2_ctrl_handler_init(&imx290->ctrls, 9);
- imx290->ctrls.lock = &imx290->lock;
/*
* The sensor has an analog gain and a digital gain, both controlled
@@ -718,10 +724,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
return ret;
}
- mutex_lock(imx290->ctrls.lock);
- imx290_ctrl_update(imx290, imx290->current_mode);
- mutex_unlock(imx290->ctrls.lock);
-
return 0;
}
@@ -730,8 +732,10 @@ static int imx290_ctrl_init(struct imx290 *imx290)
*/
/* Start streaming */
-static int imx290_start_streaming(struct imx290 *imx290)
+static int imx290_start_streaming(struct imx290 *imx290,
+ struct v4l2_subdev_state *state)
{
+ const struct v4l2_mbus_framefmt *format;
int ret;
/* Set init register settings */
@@ -744,7 +748,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
}
/* Apply the register values related to current frame format */
- ret = imx290_write_current_format(imx290);
+ format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
+ ret = imx290_setup_format(imx290, format);
if (ret < 0) {
dev_err(imx290->dev, "Could not set frame format\n");
return ret;
@@ -764,7 +769,7 @@ static int imx290_start_streaming(struct imx290 *imx290)
return ret;
/* Apply customized values from user */
- ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
+ ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
if (ret) {
dev_err(imx290->dev, "Could not sync v4l2 controls\n");
return ret;
@@ -793,39 +798,32 @@ static int imx290_stop_streaming(struct imx290 *imx290)
static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
{
struct imx290 *imx290 = to_imx290(sd);
+ struct v4l2_subdev_state *state;
int ret = 0;
+ state = v4l2_subdev_lock_and_get_active_state(sd);
+
if (enable) {
ret = pm_runtime_resume_and_get(imx290->dev);
if (ret < 0)
- goto unlock_and_return;
+ goto unlock;
- ret = imx290_start_streaming(imx290);
+ ret = imx290_start_streaming(imx290, state);
if (ret) {
dev_err(imx290->dev, "Start stream failed\n");
pm_runtime_put(imx290->dev);
- goto unlock_and_return;
+ goto unlock;
}
} else {
imx290_stop_streaming(imx290);
pm_runtime_put(imx290->dev);
}
-unlock_and_return:
-
+unlock:
+ v4l2_subdev_unlock_state(state);
return ret;
}
-static struct v4l2_mbus_framefmt *
-imx290_get_pad_format(struct imx290 *imx290, struct v4l2_subdev_state *state,
- u32 which)
-{
- if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
- return &imx290->current_format;
- else
- return v4l2_subdev_get_try_format(&imx290->sd, state, 0);
-}
-
static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_mbus_code_enum *code)
@@ -860,23 +858,6 @@ static int imx290_enum_frame_size(struct v4l2_subdev *sd,
return 0;
}
-static int imx290_get_fmt(struct v4l2_subdev *sd,
- struct v4l2_subdev_state *sd_state,
- struct v4l2_subdev_format *fmt)
-{
- struct imx290 *imx290 = to_imx290(sd);
- struct v4l2_mbus_framefmt *framefmt;
-
- mutex_lock(&imx290->lock);
-
- framefmt = imx290_get_pad_format(imx290, sd_state, fmt->which);
- fmt->format = *framefmt;
-
- mutex_unlock(&imx290->lock);
-
- return 0;
-}
-
static int imx290_set_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_format *fmt)
@@ -886,8 +867,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
struct v4l2_mbus_framefmt *format;
unsigned int i;
- mutex_lock(&imx290->lock);
-
mode = v4l2_find_nearest_size(imx290_modes_ptr(imx290),
imx290_modes_num(imx290), width, height,
fmt->format.width, fmt->format.height);
@@ -905,7 +884,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
fmt->format.code = imx290_formats[i].code;
fmt->format.field = V4L2_FIELD_NONE;
- format = imx290_get_pad_format(imx290, sd_state, fmt->which);
+ format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
imx290->current_mode = mode;
@@ -916,8 +895,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
*format = fmt->format;
- mutex_unlock(&imx290->lock);
-
return 0;
}
@@ -925,14 +902,11 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_selection *sel)
{
- struct imx290 *imx290 = to_imx290(sd);
struct v4l2_mbus_framefmt *format;
switch (sel->target) {
case V4L2_SEL_TGT_CROP: {
- format = imx290_get_pad_format(imx290, sd_state, sel->which);
-
- mutex_lock(&imx290->lock);
+ format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
sel->r.top = IMX920_PIXEL_ARRAY_MARGIN_TOP
+ (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT - format->height) / 2;
@@ -941,7 +915,6 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
sel->r.width = format->width;
sel->r.height = format->height;
- mutex_unlock(&imx290->lock);
return 0;
}
@@ -970,11 +943,13 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
struct v4l2_subdev_state *sd_state)
{
- struct v4l2_subdev_format fmt = { 0 };
-
- fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
- fmt.format.width = 1920;
- fmt.format.height = 1080;
+ struct v4l2_subdev_format fmt = {
+ .which = V4L2_SUBDEV_FORMAT_TRY,
+ .format = {
+ .width = 1920,
+ .height = 1080,
+ },
+ };
imx290_set_fmt(subdev, sd_state, &fmt);
@@ -989,7 +964,7 @@ static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
.init_cfg = imx290_entity_init_cfg,
.enum_mbus_code = imx290_enum_mbus_code,
.enum_frame_size = imx290_enum_frame_size,
- .get_fmt = imx290_get_fmt,
+ .get_fmt = v4l2_subdev_get_fmt,
.set_fmt = imx290_set_fmt,
.get_selection = imx290_get_selection,
};
@@ -1008,18 +983,8 @@ static int imx290_subdev_init(struct imx290 *imx290)
struct i2c_client *client = to_i2c_client(imx290->dev);
int ret;
- /*
- * Initialize the frame format. In particular, imx290->current_mode
- * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() call
- * below relies on these fields.
- */
- imx290_entity_init_cfg(&imx290->sd, NULL);
-
- ret = imx290_ctrl_init(imx290);
- if (ret < 0) {
- dev_err(imx290->dev, "Control initialization error %d\n", ret);
- return ret;
- }
+ imx290->current_mode = &imx290_modes_ptr(imx290)[0];
+ imx290->bpp = imx290_formats[0].bpp;
v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -1031,15 +996,37 @@ static int imx290_subdev_init(struct imx290 *imx290)
ret = media_entity_pads_init(&imx290->sd.entity, 1, &imx290->pad);
if (ret < 0) {
dev_err(imx290->dev, "Could not register media entity\n");
- v4l2_ctrl_handler_free(&imx290->ctrls);
return ret;
}
+ ret = imx290_ctrl_init(imx290);
+ if (ret < 0) {
+ dev_err(imx290->dev, "Control initialization error %d\n", ret);
+ goto err_media;
+ }
+
+ imx290->sd.state_lock = imx290->ctrls.lock;
+
+ ret = v4l2_subdev_init_finalize(&imx290->sd);
+ if (ret < 0) {
+ dev_err(imx290->dev, "subdev initialization error %d\n", ret);
+ goto err_ctrls;
+ }
+
+ imx290_ctrl_update(imx290, imx290->current_mode);
+
return 0;
+
+err_ctrls:
+ v4l2_ctrl_handler_free(&imx290->ctrls);
+err_media:
+ media_entity_cleanup(&imx290->sd.entity);
+ return ret;
}
static void imx290_subdev_cleanup(struct imx290 *imx290)
{
+ v4l2_subdev_cleanup(&imx290->sd);
media_entity_cleanup(&imx290->sd.entity);
v4l2_ctrl_handler_free(&imx290->ctrls);
}
@@ -1270,12 +1257,10 @@ static int imx290_probe(struct i2c_client *client)
if (ret)
return ret;
- mutex_init(&imx290->lock);
-
/* Initialize and register subdev. */
ret = imx290_subdev_init(imx290);
if (ret)
- goto err_mutex;
+ return ret;
ret = v4l2_async_register_subdev(&imx290->sd);
if (ret < 0) {
@@ -1298,8 +1283,6 @@ static int imx290_probe(struct i2c_client *client)
err_subdev:
imx290_subdev_cleanup(imx290);
-err_mutex:
- mutex_destroy(&imx290->lock);
return ret;
}
@@ -1312,8 +1295,6 @@ static void imx290_remove(struct i2c_client *client)
v4l2_async_unregister_subdev(sd);
imx290_subdev_cleanup(imx290);
- mutex_destroy(&imx290->lock);
-
pm_runtime_disable(imx290->dev);
if (!pm_runtime_status_suspended(imx290->dev))
imx290_power_off(imx290->dev);
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3 11/17] media: i2c: imx290: Use V4L2 subdev active state
2023-01-16 14:44 ` [PATCH v3 11/17] media: i2c: imx290: Use V4L2 subdev active state Laurent Pinchart
@ 2023-01-16 15:05 ` Alexander Stein
0 siblings, 0 replies; 24+ messages in thread
From: Alexander Stein @ 2023-01-16 15:05 UTC (permalink / raw)
To: linux-media, Laurent Pinchart
Cc: Sakari Ailus, Manivannan Sadhasivam, Dave Stevenson
Hi Laurent,
thanks for the update.
Am Montag, 16. Januar 2023, 15:44:48 CET schrieb Laurent Pinchart:
> Use the V4L2 subdev active state API to store the active format. This
> simplifies the driver not only by dropping the imx290 current_format
> field, but it also allows dropping the imx290 lock, replaced with the
> state lock.
>
> The lock check in imx290_ctrl_update() can be dropped as
> imx290_set_fmt() can't be called anywmore with which set to ACTIVE
> before controls are initialized.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
LGTM.
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes since v2:
>
> - Test for V4L2_CTRL_FLAG_READ_ONLY
>
> Changes since v1:
>
> - Remove lock check from imx290_ctrl_update()
> - Drop unrelated change
> - Fix error handling in imx290_subdev_init()
> - Handle errors from v4l2_subdev_init_finalize()
> - Return immediately from imx290_set_ctrl() for read-only controls
> - Call v4l2_subdev_cleanup()
> ---
> drivers/media/i2c/imx290.c | 159 ++++++++++++++++---------------------
> 1 file changed, 70 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index c1d18ec51e41..74b9a7110709 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -177,12 +177,12 @@ struct imx290 {
> struct clk *xclk;
> struct regmap *regmap;
> u8 nlanes;
> - u8 bpp;
>
> struct v4l2_subdev sd;
> struct media_pad pad;
> - struct v4l2_mbus_framefmt current_format;
> +
> const struct imx290_mode *current_mode;
> + u8 bpp;
>
> struct regulator_bulk_data supplies[IMX290_NUM_SUPPLIES];
> struct gpio_desc *rst_gpio;
> @@ -192,8 +192,6 @@ struct imx290 {
> struct v4l2_ctrl *pixel_rate;
> struct v4l2_ctrl *hblank;
> struct v4l2_ctrl *vblank;
> -
> - struct mutex lock;
> };
>
> static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
> @@ -524,13 +522,14 @@ static int imx290_set_black_level(struct imx290
> *imx290, black_level >> (16 - imx290->bpp), err);
> }
>
> -static int imx290_write_current_format(struct imx290 *imx290)
> +static int imx290_setup_format(struct imx290 *imx290,
> + const struct v4l2_mbus_framefmt *format)
> {
> const struct imx290_regval *regs;
> unsigned int num_regs;
> int ret;
>
> - switch (imx290->current_format.code) {
> + switch (format->code) {
> case MEDIA_BUS_FMT_SRGGB10_1X10:
> regs = imx290_10bit_settings;
> num_regs = ARRAY_SIZE(imx290_10bit_settings);
> @@ -561,12 +560,26 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct imx290 *imx290 = container_of(ctrl->handler,
> struct imx290, ctrls);
> + const struct v4l2_mbus_framefmt *format;
> + struct v4l2_subdev_state *state;
> int ret = 0;
>
> + /*
> + * Return immediately for controls that don't need to be applied to
the
> + * device. Those controls are modified in imx290_ctrl_update(),
which
> + * is called at probe time before runtime PM is initialized, so we
> + * can't proceed to the pm_runtime_get_if_in_use() call below.
> + */
> + if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
> + return 0;
> +
> /* V4L2 controls values will be applied only when power is already
up */
> if (!pm_runtime_get_if_in_use(imx290->dev))
> return 0;
>
> + state = v4l2_subdev_get_locked_active_state(&imx290->sd);
> + format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
> +
> switch (ctrl->id) {
> case V4L2_CID_ANALOGUE_GAIN:
> ret = imx290_write(imx290, IMX290_GAIN, ctrl->val, NULL);
> @@ -592,6 +605,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> &ret);
> }
> break;
> +
> default:
> ret = -EINVAL;
> break;
> @@ -625,13 +639,6 @@ static void imx290_ctrl_update(struct imx290 *imx290,
> s64 link_freq = imx290_link_freqs_ptr(imx290)[mode-
>link_freq_index];
> u64 pixel_rate;
>
> - /*
> - * This function may be called from imx290_set_fmt() before controls
> - * get created by imx290_ctrl_init(). Return immediately in that
case.
> - */
> - if (!imx290->ctrls.lock)
> - return;
> -
> /* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> pixel_rate = link_freq * 2 * imx290->nlanes;
> do_div(pixel_rate, imx290->bpp);
> @@ -653,7 +660,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> return ret;
>
> v4l2_ctrl_handler_init(&imx290->ctrls, 9);
> - imx290->ctrls.lock = &imx290->lock;
>
> /*
> * The sensor has an analog gain and a digital gain, both controlled
> @@ -718,10 +724,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> return ret;
> }
>
> - mutex_lock(imx290->ctrls.lock);
> - imx290_ctrl_update(imx290, imx290->current_mode);
> - mutex_unlock(imx290->ctrls.lock);
> -
> return 0;
> }
>
> @@ -730,8 +732,10 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> */
>
> /* Start streaming */
> -static int imx290_start_streaming(struct imx290 *imx290)
> +static int imx290_start_streaming(struct imx290 *imx290,
> + struct v4l2_subdev_state *state)
> {
> + const struct v4l2_mbus_framefmt *format;
> int ret;
>
> /* Set init register settings */
> @@ -744,7 +748,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
> }
>
> /* Apply the register values related to current frame format */
> - ret = imx290_write_current_format(imx290);
> + format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
> + ret = imx290_setup_format(imx290, format);
> if (ret < 0) {
> dev_err(imx290->dev, "Could not set frame format\n");
> return ret;
> @@ -764,7 +769,7 @@ static int imx290_start_streaming(struct imx290 *imx290)
> return ret;
>
> /* Apply customized values from user */
> - ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
> + ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
> if (ret) {
> dev_err(imx290->dev, "Could not sync v4l2 controls\n");
> return ret;
> @@ -793,39 +798,32 @@ static int imx290_stop_streaming(struct imx290
> *imx290) static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
> {
> struct imx290 *imx290 = to_imx290(sd);
> + struct v4l2_subdev_state *state;
> int ret = 0;
>
> + state = v4l2_subdev_lock_and_get_active_state(sd);
> +
> if (enable) {
> ret = pm_runtime_resume_and_get(imx290->dev);
> if (ret < 0)
> - goto unlock_and_return;
> + goto unlock;
>
> - ret = imx290_start_streaming(imx290);
> + ret = imx290_start_streaming(imx290, state);
> if (ret) {
> dev_err(imx290->dev, "Start stream failed\n");
> pm_runtime_put(imx290->dev);
> - goto unlock_and_return;
> + goto unlock;
> }
> } else {
> imx290_stop_streaming(imx290);
> pm_runtime_put(imx290->dev);
> }
>
> -unlock_and_return:
> -
> +unlock:
> + v4l2_subdev_unlock_state(state);
> return ret;
> }
>
> -static struct v4l2_mbus_framefmt *
> -imx290_get_pad_format(struct imx290 *imx290, struct v4l2_subdev_state
> *state, - u32 which)
> -{
> - if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
> - return &imx290->current_format;
> - else
> - return v4l2_subdev_get_try_format(&imx290->sd, state, 0);
> -}
> -
> static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_mbus_code_enum
*code)
> @@ -860,23 +858,6 @@ static int imx290_enum_frame_size(struct v4l2_subdev
> *sd, return 0;
> }
>
> -static int imx290_get_fmt(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *fmt)
> -{
> - struct imx290 *imx290 = to_imx290(sd);
> - struct v4l2_mbus_framefmt *framefmt;
> -
> - mutex_lock(&imx290->lock);
> -
> - framefmt = imx290_get_pad_format(imx290, sd_state, fmt->which);
> - fmt->format = *framefmt;
> -
> - mutex_unlock(&imx290->lock);
> -
> - return 0;
> -}
> -
> static int imx290_set_fmt(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_format *fmt)
> @@ -886,8 +867,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> struct v4l2_mbus_framefmt *format;
> unsigned int i;
>
> - mutex_lock(&imx290->lock);
> -
> mode = v4l2_find_nearest_size(imx290_modes_ptr(imx290),
> imx290_modes_num(imx290), width,
height,
> fmt->format.width, fmt-
>format.height);
> @@ -905,7 +884,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> fmt->format.code = imx290_formats[i].code;
> fmt->format.field = V4L2_FIELD_NONE;
>
> - format = imx290_get_pad_format(imx290, sd_state, fmt->which);
> + format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
>
> if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> imx290->current_mode = mode;
> @@ -916,8 +895,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>
> *format = fmt->format;
>
> - mutex_unlock(&imx290->lock);
> -
> return 0;
> }
>
> @@ -925,14 +902,11 @@ static int imx290_get_selection(struct v4l2_subdev
> *sd, struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_selection *sel)
> {
> - struct imx290 *imx290 = to_imx290(sd);
> struct v4l2_mbus_framefmt *format;
>
> switch (sel->target) {
> case V4L2_SEL_TGT_CROP: {
> - format = imx290_get_pad_format(imx290, sd_state, sel-
>which);
> -
> - mutex_lock(&imx290->lock);
> + format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
>
> sel->r.top = IMX920_PIXEL_ARRAY_MARGIN_TOP
> + (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT -
format->height) / 2;
> @@ -941,7 +915,6 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
> sel->r.width = format->width;
> sel->r.height = format->height;
>
> - mutex_unlock(&imx290->lock);
> return 0;
> }
>
> @@ -970,11 +943,13 @@ static int imx290_get_selection(struct v4l2_subdev
> *sd, static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
> struct v4l2_subdev_state *sd_state)
> {
> - struct v4l2_subdev_format fmt = { 0 };
> -
> - fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY :
V4L2_SUBDEV_FORMAT_ACTIVE;
> - fmt.format.width = 1920;
> - fmt.format.height = 1080;
> + struct v4l2_subdev_format fmt = {
> + .which = V4L2_SUBDEV_FORMAT_TRY,
> + .format = {
> + .width = 1920,
> + .height = 1080,
> + },
> + };
>
> imx290_set_fmt(subdev, sd_state, &fmt);
>
> @@ -989,7 +964,7 @@ static const struct v4l2_subdev_pad_ops imx290_pad_ops =
> { .init_cfg = imx290_entity_init_cfg,
> .enum_mbus_code = imx290_enum_mbus_code,
> .enum_frame_size = imx290_enum_frame_size,
> - .get_fmt = imx290_get_fmt,
> + .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = imx290_set_fmt,
> .get_selection = imx290_get_selection,
> };
> @@ -1008,18 +983,8 @@ static int imx290_subdev_init(struct imx290 *imx290)
> struct i2c_client *client = to_i2c_client(imx290->dev);
> int ret;
>
> - /*
> - * Initialize the frame format. In particular, imx290->current_mode
> - * and imx290->bpp are set to defaults: imx290_calc_pixel_rate()
call
> - * below relies on these fields.
> - */
> - imx290_entity_init_cfg(&imx290->sd, NULL);
> -
> - ret = imx290_ctrl_init(imx290);
> - if (ret < 0) {
> - dev_err(imx290->dev, "Control initialization error %d\n",
ret);
> - return ret;
> - }
> + imx290->current_mode = &imx290_modes_ptr(imx290)[0];
> + imx290->bpp = imx290_formats[0].bpp;
>
> v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
> imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> @@ -1031,15 +996,37 @@ static int imx290_subdev_init(struct imx290 *imx290)
> ret = media_entity_pads_init(&imx290->sd.entity, 1, &imx290->pad);
> if (ret < 0) {
> dev_err(imx290->dev, "Could not register media entity\n");
> - v4l2_ctrl_handler_free(&imx290->ctrls);
> return ret;
> }
>
> + ret = imx290_ctrl_init(imx290);
> + if (ret < 0) {
> + dev_err(imx290->dev, "Control initialization error %d\n",
ret);
> + goto err_media;
> + }
> +
> + imx290->sd.state_lock = imx290->ctrls.lock;
> +
> + ret = v4l2_subdev_init_finalize(&imx290->sd);
> + if (ret < 0) {
> + dev_err(imx290->dev, "subdev initialization error %d\n",
ret);
> + goto err_ctrls;
> + }
> +
> + imx290_ctrl_update(imx290, imx290->current_mode);
> +
> return 0;
> +
> +err_ctrls:
> + v4l2_ctrl_handler_free(&imx290->ctrls);
> +err_media:
> + media_entity_cleanup(&imx290->sd.entity);
> + return ret;
> }
>
> static void imx290_subdev_cleanup(struct imx290 *imx290)
> {
> + v4l2_subdev_cleanup(&imx290->sd);
> media_entity_cleanup(&imx290->sd.entity);
> v4l2_ctrl_handler_free(&imx290->ctrls);
> }
> @@ -1270,12 +1257,10 @@ static int imx290_probe(struct i2c_client *client)
> if (ret)
> return ret;
>
> - mutex_init(&imx290->lock);
> -
> /* Initialize and register subdev. */
> ret = imx290_subdev_init(imx290);
> if (ret)
> - goto err_mutex;
> + return ret;
>
> ret = v4l2_async_register_subdev(&imx290->sd);
> if (ret < 0) {
> @@ -1298,8 +1283,6 @@ static int imx290_probe(struct i2c_client *client)
>
> err_subdev:
> imx290_subdev_cleanup(imx290);
> -err_mutex:
> - mutex_destroy(&imx290->lock);
>
> return ret;
> }
> @@ -1312,8 +1295,6 @@ static void imx290_remove(struct i2c_client *client)
> v4l2_async_unregister_subdev(sd);
> imx290_subdev_cleanup(imx290);
>
> - mutex_destroy(&imx290->lock);
> -
> pm_runtime_disable(imx290->dev);
> if (!pm_runtime_status_suspended(imx290->dev))
> imx290_power_off(imx290->dev);
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 12/17] media: i2c: imx290: Rename, extend and expand usage of imx290_pixfmt
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
` (10 preceding siblings ...)
2023-01-16 14:44 ` [PATCH v3 11/17] media: i2c: imx290: Use V4L2 subdev active state Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 13/17] media: i2c: imx290: Use runtime PM autosuspend Laurent Pinchart
` (4 subsequent siblings)
16 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
The imx290_pixfmt structure contains information about formats,
currently limited to the bpp value. Extend it with the register settings
for each format, and rename it to imx290_format_info to make its purpose
clearer. Add a function named imx290_format_info() to look up format
info for a media bus code, and use it through the code. This allows
dropping the imx290 bpp field as the value is now looked up dynamically.
The error handling in imx290_setup_format() can also be dropped, as the
format is guaranteed by imx290_set_fmt() to be valid.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes since v2:
- Pass format to imx290_ctrl_update()
---
drivers/media/i2c/imx290.c | 94 +++++++++++++++++++++-----------------
1 file changed, 53 insertions(+), 41 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 74b9a7110709..7356279822e8 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -182,7 +182,6 @@ struct imx290 {
struct media_pad pad;
const struct imx290_mode *current_mode;
- u8 bpp;
struct regulator_bulk_data supplies[IMX290_NUM_SUPPLIES];
struct gpio_desc *rst_gpio;
@@ -414,16 +413,41 @@ static inline int imx290_modes_num(const struct imx290 *imx290)
return ARRAY_SIZE(imx290_modes_4lanes);
}
-struct imx290_pixfmt {
+struct imx290_format_info {
u32 code;
u8 bpp;
+ const struct imx290_regval *regs;
+ unsigned int num_regs;
};
-static const struct imx290_pixfmt imx290_formats[] = {
- { MEDIA_BUS_FMT_SRGGB10_1X10, 10 },
- { MEDIA_BUS_FMT_SRGGB12_1X12, 12 },
+static const struct imx290_format_info imx290_formats[] = {
+ {
+ .code = MEDIA_BUS_FMT_SRGGB10_1X10,
+ .bpp = 10,
+ .regs = imx290_10bit_settings,
+ .num_regs = ARRAY_SIZE(imx290_10bit_settings),
+ }, {
+ .code = MEDIA_BUS_FMT_SRGGB12_1X12,
+ .bpp = 12,
+ .regs = imx290_12bit_settings,
+ .num_regs = ARRAY_SIZE(imx290_12bit_settings),
+ }
};
+static const struct imx290_format_info *imx290_format_info(u32 code)
+{
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(imx290_formats); ++i) {
+ const struct imx290_format_info *info = &imx290_formats[i];
+
+ if (info->code == code)
+ return info;
+ }
+
+ return NULL;
+}
+
/* -----------------------------------------------------------------------------
* Register access
*/
@@ -516,40 +540,31 @@ static int imx290_set_data_lanes(struct imx290 *imx290)
}
static int imx290_set_black_level(struct imx290 *imx290,
+ const struct v4l2_mbus_framefmt *format,
unsigned int black_level, int *err)
{
+ unsigned int bpp = imx290_format_info(format->code)->bpp;
+
return imx290_write(imx290, IMX290_BLKLEVEL,
- black_level >> (16 - imx290->bpp), err);
+ black_level >> (16 - bpp), err);
}
static int imx290_setup_format(struct imx290 *imx290,
const struct v4l2_mbus_framefmt *format)
{
- const struct imx290_regval *regs;
- unsigned int num_regs;
+ const struct imx290_format_info *info;
int ret;
- switch (format->code) {
- case MEDIA_BUS_FMT_SRGGB10_1X10:
- regs = imx290_10bit_settings;
- num_regs = ARRAY_SIZE(imx290_10bit_settings);
- break;
- case MEDIA_BUS_FMT_SRGGB12_1X12:
- regs = imx290_12bit_settings;
- num_regs = ARRAY_SIZE(imx290_12bit_settings);
- break;
- default:
- dev_err(imx290->dev, "Unknown pixel format\n");
- return -EINVAL;
- }
+ info = imx290_format_info(format->code);
- ret = imx290_set_register_array(imx290, regs, num_regs);
+ ret = imx290_set_register_array(imx290, info->regs, info->num_regs);
if (ret < 0) {
dev_err(imx290->dev, "Could not set format registers\n");
return ret;
}
- return imx290_set_black_level(imx290, IMX290_BLACK_LEVEL_DEFAULT, &ret);
+ return imx290_set_black_level(imx290, format,
+ IMX290_BLACK_LEVEL_DEFAULT, &ret);
}
/* ----------------------------------------------------------------------------
@@ -592,7 +607,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
case V4L2_CID_TEST_PATTERN:
if (ctrl->val) {
- imx290_set_black_level(imx290, 0, &ret);
+ imx290_set_black_level(imx290, format, 0, &ret);
usleep_range(10000, 11000);
imx290_write(imx290, IMX290_PGCTRL,
(u8)(IMX290_PGCTRL_REGEN |
@@ -601,8 +616,8 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
} else {
imx290_write(imx290, IMX290_PGCTRL, 0x00, &ret);
usleep_range(10000, 11000);
- imx290_set_black_level(imx290, IMX290_BLACK_LEVEL_DEFAULT,
- &ret);
+ imx290_set_black_level(imx290, format,
+ IMX290_BLACK_LEVEL_DEFAULT, &ret);
}
break;
@@ -632,6 +647,7 @@ static const char * const imx290_test_pattern_menu[] = {
};
static void imx290_ctrl_update(struct imx290 *imx290,
+ const struct v4l2_mbus_framefmt *format,
const struct imx290_mode *mode)
{
unsigned int hblank = mode->hmax - mode->width;
@@ -641,7 +657,7 @@ static void imx290_ctrl_update(struct imx290 *imx290,
/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
pixel_rate = link_freq * 2 * imx290->nlanes;
- do_div(pixel_rate, imx290->bpp);
+ do_div(pixel_rate, imx290_format_info(format->code)->bpp);
__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate, pixel_rate);
@@ -843,8 +859,7 @@ static int imx290_enum_frame_size(struct v4l2_subdev *sd,
const struct imx290 *imx290 = to_imx290(sd);
const struct imx290_mode *imx290_modes = imx290_modes_ptr(imx290);
- if ((fse->code != imx290_formats[0].code) &&
- (fse->code != imx290_formats[1].code))
+ if (!imx290_format_info(fse->code))
return -EINVAL;
if (fse->index >= imx290_modes_num(imx290))
@@ -865,7 +880,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
struct imx290 *imx290 = to_imx290(sd);
const struct imx290_mode *mode;
struct v4l2_mbus_framefmt *format;
- unsigned int i;
mode = v4l2_find_nearest_size(imx290_modes_ptr(imx290),
imx290_modes_num(imx290), width, height,
@@ -874,23 +888,17 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
fmt->format.width = mode->width;
fmt->format.height = mode->height;
- for (i = 0; i < ARRAY_SIZE(imx290_formats); i++)
- if (imx290_formats[i].code == fmt->format.code)
- break;
+ if (!imx290_format_info(fmt->format.code))
+ fmt->format.code = imx290_formats[0].code;
- if (i >= ARRAY_SIZE(imx290_formats))
- i = 0;
-
- fmt->format.code = imx290_formats[i].code;
fmt->format.field = V4L2_FIELD_NONE;
format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
imx290->current_mode = mode;
- imx290->bpp = imx290_formats[i].bpp;
- imx290_ctrl_update(imx290, mode);
+ imx290_ctrl_update(imx290, &fmt->format, mode);
}
*format = fmt->format;
@@ -981,10 +989,11 @@ static const struct media_entity_operations imx290_subdev_entity_ops = {
static int imx290_subdev_init(struct imx290 *imx290)
{
struct i2c_client *client = to_i2c_client(imx290->dev);
+ const struct v4l2_mbus_framefmt *format;
+ struct v4l2_subdev_state *state;
int ret;
imx290->current_mode = &imx290_modes_ptr(imx290)[0];
- imx290->bpp = imx290_formats[0].bpp;
v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -1013,7 +1022,10 @@ static int imx290_subdev_init(struct imx290 *imx290)
goto err_ctrls;
}
- imx290_ctrl_update(imx290, imx290->current_mode);
+ state = v4l2_subdev_lock_and_get_active_state(&imx290->sd);
+ format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
+ imx290_ctrl_update(imx290, format, imx290->current_mode);
+ v4l2_subdev_unlock_state(state);
return 0;
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 13/17] media: i2c: imx290: Use runtime PM autosuspend
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
` (11 preceding siblings ...)
2023-01-16 14:44 ` [PATCH v3 12/17] media: i2c: imx290: Rename, extend and expand usage of imx290_pixfmt Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 14/17] media: i2c: imx290: Initialize runtime PM before subdev Laurent Pinchart
` (3 subsequent siblings)
16 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
Use runtime PM autosuspend to avoid powering off the sensor during fast
stop-reconfigure-restart cycles. This also fixes runtime PM handling in
the probe function that didn't suspend the device, effectively leaving
it resumed forever.
While at it, improve documentation of power management in probe() and
remove().
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes since v2:
- Use the correct error label when subdev registration fails
Changes since v1:
- Enable autosuspend before registering the subdev
- Decrease the PM usage count after registering the subdev
- Use pm_runtime_put_autosuspend() in imx290_set_ctrl()
---
drivers/media/i2c/imx290.c | 58 +++++++++++++++++++++++++++++---------
1 file changed, 45 insertions(+), 13 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 7356279822e8..324d30ed5617 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -626,7 +626,8 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
break;
}
- pm_runtime_put(imx290->dev);
+ pm_runtime_mark_last_busy(imx290->dev);
+ pm_runtime_put_autosuspend(imx290->dev);
return ret;
}
@@ -827,12 +828,13 @@ static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
ret = imx290_start_streaming(imx290, state);
if (ret) {
dev_err(imx290->dev, "Start stream failed\n");
- pm_runtime_put(imx290->dev);
+ pm_runtime_put_sync(imx290->dev);
goto unlock;
}
} else {
imx290_stop_streaming(imx290);
- pm_runtime_put(imx290->dev);
+ pm_runtime_mark_last_busy(imx290->dev);
+ pm_runtime_put_autosuspend(imx290->dev);
}
unlock:
@@ -1269,33 +1271,59 @@ static int imx290_probe(struct i2c_client *client)
if (ret)
return ret;
- /* Initialize and register subdev. */
+ /* Initialize the V4L2 subdev. */
ret = imx290_subdev_init(imx290);
if (ret)
return ret;
- ret = v4l2_async_register_subdev(&imx290->sd);
- if (ret < 0) {
- dev_err(dev, "Could not register v4l2 device\n");
- goto err_subdev;
- }
-
- /* Power on the device to match runtime PM state below */
+ /*
+ * Enable power management. The driver supports runtime PM, but needs to
+ * work when runtime PM is disabled in the kernel. To that end, power
+ * the sensor on manually here.
+ */
ret = imx290_power_on(dev);
if (ret < 0) {
dev_err(dev, "Could not power on the device\n");
goto err_subdev;
}
+ /*
+ * Enable runtime PM with autosuspend. As the device has been powered
+ * manually, mark it as active, and increase the usage count without
+ * resuming the device.
+ */
pm_runtime_set_active(dev);
+ pm_runtime_get_noresume(dev);
pm_runtime_enable(dev);
- pm_runtime_idle(dev);
+ pm_runtime_set_autosuspend_delay(dev, 1000);
+ pm_runtime_use_autosuspend(dev);
+
+ /*
+ * Finally, register the V4L2 subdev. This must be done after
+ * initializing everything as the subdev can be used immediately after
+ * being registered.
+ */
+ ret = v4l2_async_register_subdev(&imx290->sd);
+ if (ret < 0) {
+ dev_err(dev, "Could not register v4l2 device\n");
+ goto err_pm;
+ }
+
+ /*
+ * Decrease the PM usage count. The device will get suspended after the
+ * autosuspend delay, turning the power off.
+ */
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
return 0;
+err_pm:
+ pm_runtime_disable(dev);
+ pm_runtime_put_noidle(dev);
+ imx290_power_off(dev);
err_subdev:
imx290_subdev_cleanup(imx290);
-
return ret;
}
@@ -1307,6 +1335,10 @@ static void imx290_remove(struct i2c_client *client)
v4l2_async_unregister_subdev(sd);
imx290_subdev_cleanup(imx290);
+ /*
+ * Disable runtime PM. In case runtime PM is disabled in the kernel,
+ * make sure to turn power off manually.
+ */
pm_runtime_disable(imx290->dev);
if (!pm_runtime_status_suspended(imx290->dev))
imx290_power_off(imx290->dev);
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 14/17] media: i2c: imx290: Initialize runtime PM before subdev
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
` (12 preceding siblings ...)
2023-01-16 14:44 ` [PATCH v3 13/17] media: i2c: imx290: Use runtime PM autosuspend Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-02-27 17:52 ` Guenter Roeck
2023-01-16 14:44 ` [PATCH v3 15/17] media: i2c: imx290: Configure data lanes at start time Laurent Pinchart
` (2 subsequent siblings)
16 siblings, 1 reply; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
Initializing the subdev before runtime PM means that no subdev
initialization can interact with the runtime PM framework. This can be
problematic when modifying controls, as the .s_ctrl() handler commonly
calls pm_runtime_get_if_in_use(). These code paths are not trivial,
making the driver fragile and possibly causing subtle bugs.
To make the subdev initialization more robust, initialize runtime PM
first.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes since v1:
- Use the correct error label when subdev registration fails
---
drivers/media/i2c/imx290.c | 59 ++++++++++++++++++++++----------------
1 file changed, 34 insertions(+), 25 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 324d30ed5617..4185835f065d 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -581,9 +581,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
/*
* Return immediately for controls that don't need to be applied to the
- * device. Those controls are modified in imx290_ctrl_update(), which
- * is called at probe time before runtime PM is initialized, so we
- * can't proceed to the pm_runtime_get_if_in_use() call below.
+ * device.
*/
if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
return 0;
@@ -1049,22 +1047,20 @@ static void imx290_subdev_cleanup(struct imx290 *imx290)
* Power management
*/
-static int imx290_power_on(struct device *dev)
+static int imx290_power_on(struct imx290 *imx290)
{
- struct v4l2_subdev *sd = dev_get_drvdata(dev);
- struct imx290 *imx290 = to_imx290(sd);
int ret;
ret = clk_prepare_enable(imx290->xclk);
if (ret) {
- dev_err(dev, "Failed to enable clock\n");
+ dev_err(imx290->dev, "Failed to enable clock\n");
return ret;
}
ret = regulator_bulk_enable(ARRAY_SIZE(imx290->supplies),
imx290->supplies);
if (ret) {
- dev_err(dev, "Failed to enable regulators\n");
+ dev_err(imx290->dev, "Failed to enable regulators\n");
clk_disable_unprepare(imx290->xclk);
return ret;
}
@@ -1079,20 +1075,33 @@ static int imx290_power_on(struct device *dev)
return 0;
}
-static int imx290_power_off(struct device *dev)
+static void imx290_power_off(struct imx290 *imx290)
{
- struct v4l2_subdev *sd = dev_get_drvdata(dev);
- struct imx290 *imx290 = to_imx290(sd);
-
clk_disable_unprepare(imx290->xclk);
gpiod_set_value_cansleep(imx290->rst_gpio, 1);
regulator_bulk_disable(ARRAY_SIZE(imx290->supplies), imx290->supplies);
+}
+
+static int imx290_runtime_resume(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct imx290 *imx290 = to_imx290(sd);
+
+ return imx290_power_on(imx290);
+}
+
+static int imx290_runtime_suspend(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct imx290 *imx290 = to_imx290(sd);
+
+ imx290_power_off(imx290);
return 0;
}
static const struct dev_pm_ops imx290_pm_ops = {
- SET_RUNTIME_PM_OPS(imx290_power_off, imx290_power_on, NULL)
+ SET_RUNTIME_PM_OPS(imx290_runtime_suspend, imx290_runtime_resume, NULL)
};
/* ----------------------------------------------------------------------------
@@ -1271,20 +1280,15 @@ static int imx290_probe(struct i2c_client *client)
if (ret)
return ret;
- /* Initialize the V4L2 subdev. */
- ret = imx290_subdev_init(imx290);
- if (ret)
- return ret;
-
/*
* Enable power management. The driver supports runtime PM, but needs to
* work when runtime PM is disabled in the kernel. To that end, power
* the sensor on manually here.
*/
- ret = imx290_power_on(dev);
+ ret = imx290_power_on(imx290);
if (ret < 0) {
dev_err(dev, "Could not power on the device\n");
- goto err_subdev;
+ return ret;
}
/*
@@ -1298,6 +1302,11 @@ static int imx290_probe(struct i2c_client *client)
pm_runtime_set_autosuspend_delay(dev, 1000);
pm_runtime_use_autosuspend(dev);
+ /* Initialize the V4L2 subdev. */
+ ret = imx290_subdev_init(imx290);
+ if (ret)
+ goto err_pm;
+
/*
* Finally, register the V4L2 subdev. This must be done after
* initializing everything as the subdev can be used immediately after
@@ -1306,7 +1315,7 @@ static int imx290_probe(struct i2c_client *client)
ret = v4l2_async_register_subdev(&imx290->sd);
if (ret < 0) {
dev_err(dev, "Could not register v4l2 device\n");
- goto err_pm;
+ goto err_subdev;
}
/*
@@ -1318,12 +1327,12 @@ static int imx290_probe(struct i2c_client *client)
return 0;
+err_subdev:
+ imx290_subdev_cleanup(imx290);
err_pm:
pm_runtime_disable(dev);
pm_runtime_put_noidle(dev);
- imx290_power_off(dev);
-err_subdev:
- imx290_subdev_cleanup(imx290);
+ imx290_power_off(imx290);
return ret;
}
@@ -1341,7 +1350,7 @@ static void imx290_remove(struct i2c_client *client)
*/
pm_runtime_disable(imx290->dev);
if (!pm_runtime_status_suspended(imx290->dev))
- imx290_power_off(imx290->dev);
+ imx290_power_off(imx290);
pm_runtime_set_suspended(imx290->dev);
}
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3 14/17] media: i2c: imx290: Initialize runtime PM before subdev
2023-01-16 14:44 ` [PATCH v3 14/17] media: i2c: imx290: Initialize runtime PM before subdev Laurent Pinchart
@ 2023-02-27 17:52 ` Guenter Roeck
2023-03-12 13:10 ` Linux regression tracking (Thorsten Leemhuis)
0 siblings, 1 reply; 24+ messages in thread
From: Guenter Roeck @ 2023-02-27 17:52 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Sakari Ailus, Manivannan Sadhasivam,
Alexander Stein, Dave Stevenson, regressions
Hi,
On Mon, Jan 16, 2023 at 04:44:51PM +0200, Laurent Pinchart wrote:
> Initializing the subdev before runtime PM means that no subdev
> initialization can interact with the runtime PM framework. This can be
> problematic when modifying controls, as the .s_ctrl() handler commonly
> calls pm_runtime_get_if_in_use(). These code paths are not trivial,
> making the driver fragile and possibly causing subtle bugs.
>
> To make the subdev initialization more robust, initialize runtime PM
> first.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
This patch results in
Error log:
<stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
drivers/media/i2c/imx290.c:1090:12: error: 'imx290_runtime_suspend' defined but not used [-Werror=unused-function]
1090 | static int imx290_runtime_suspend(struct device *dev)
| ^~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/imx290.c:1082:12: error: 'imx290_runtime_resume' defined but not used [-Werror=unused-function]
1082 | static int imx290_runtime_resume(struct device *dev)
if PM runtime support is disabled( alpha:allmodconfig, csky:allmodconfig,
and others).
Guenter
---
#regzbot ^introduced 02852c01f6540
#regzbot title Build error in drivers/media/i2c/imx290.c if PM support is disabled
---
# bad: [f3a2439f20d918930cc4ae8f76fe1c1afd26958f] Merge tag 'rproc-v6.3' of git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux
# good: [116b41162f8b267987ea9a73eb7e73eaa7c2cce5] Merge tag 'probes-v6.3-2' of git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace
git bisect start 'HEAD' '116b41162f8b'
# good: [d4563201f33a022fc0353033d9dfeb1606a88330] Documentation: simplify and clarify DCO contribution example language
git bisect good d4563201f33a022fc0353033d9dfeb1606a88330
# bad: [bca7822cbc76b22572faf2e17ca9517b68ebeb3e] media: atomisp: ov2680: Drop MAX_FMTS define
git bisect bad bca7822cbc76b22572faf2e17ca9517b68ebeb3e
# bad: [27e45f2e59c9db2c83ed67775e911c8a3c776db2] media: ti: omap4iss: Use media_pipeline_for_each_entity()
git bisect bad 27e45f2e59c9db2c83ed67775e911c8a3c776db2
# good: [8a54644571fed484d55b3807f25f64cba8a9ca77] media: subdev: Require code change to enable [GS]_ROUTING
git bisect good 8a54644571fed484d55b3807f25f64cba8a9ca77
# good: [8508455961d5a9e8907bcfd8dcd58f19d9b6ce47] media: i2c: imx219: Split common registers from mode tables
git bisect good 8508455961d5a9e8907bcfd8dcd58f19d9b6ce47
# good: [10591fe63691bd8199d5e7244029cc065959ffc9] media: i2c: imx290: Rename, extend and expand usage of imx290_pixfmt
git bisect good 10591fe63691bd8199d5e7244029cc065959ffc9
# bad: [e14d3ac81bd2264edc76bf5796305b2dfea44487] media: i2c: Add driver for OmniVision OV8858
git bisect bad e14d3ac81bd2264edc76bf5796305b2dfea44487
# bad: [7d399658f7c666ead4bc3dbe88944bb8ea7746ca] media: i2c: imx290: Configure data lanes at start time
git bisect bad 7d399658f7c666ead4bc3dbe88944bb8ea7746ca
# bad: [02852c01f65402e2fe4a8a5fe5a0b641f245b529] media: i2c: imx290: Initialize runtime PM before subdev
git bisect bad 02852c01f65402e2fe4a8a5fe5a0b641f245b529
# good: [a8c3e0c1bf1e97b5ee094951ed0f1e57e3b378c7] media: i2c: imx290: Use runtime PM autosuspend
git bisect good a8c3e0c1bf1e97b5ee094951ed0f1e57e3b378c7
# first bad commit: [02852c01f65402e2fe4a8a5fe5a0b641f245b529] media: i2c: imx290: Initialize runtime PM before subdev
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 14/17] media: i2c: imx290: Initialize runtime PM before subdev
2023-02-27 17:52 ` Guenter Roeck
@ 2023-03-12 13:10 ` Linux regression tracking (Thorsten Leemhuis)
2023-03-12 13:34 ` Laurent Pinchart
0 siblings, 1 reply; 24+ messages in thread
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-03-12 13:10 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Sakari Ailus, Manivannan Sadhasivam,
Alexander Stein, Dave Stevenson, regressions, Guenter Roeck
On 27.02.23 18:52, Guenter Roeck wrote:
> On Mon, Jan 16, 2023 at 04:44:51PM +0200, Laurent Pinchart wrote:
>> Initializing the subdev before runtime PM means that no subdev
>> initialization can interact with the runtime PM framework. This can be
>> problematic when modifying controls, as the .s_ctrl() handler commonly
>> calls pm_runtime_get_if_in_use(). These code paths are not trivial,
>> making the driver fragile and possibly causing subtle bugs.
>>
>> To make the subdev initialization more robust, initialize runtime PM
>> first.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>> ---
>
> This patch results in
>
> Error log:
> <stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
> drivers/media/i2c/imx290.c:1090:12: error: 'imx290_runtime_suspend' defined but not used [-Werror=unused-function]
> 1090 | static int imx290_runtime_suspend(struct device *dev)
> | ^~~~~~~~~~~~~~~~~~~~~~
> drivers/media/i2c/imx290.c:1082:12: error: 'imx290_runtime_resume' defined but not used [-Werror=unused-function]
> 1082 | static int imx290_runtime_resume(struct device *dev)
>
> if PM runtime support is disabled( alpha:allmodconfig, csky:allmodconfig,
> and others).
Looks like Guenter never got a reply, but from a recent kernelci report
it looks like above warning still happens:
https://lore.kernel.org/all/640bceb7.a70a0220.af8cd.146b@mx.google.com/
Laurent, do you still have it on your radar?
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
#regzbot poke
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 14/17] media: i2c: imx290: Initialize runtime PM before subdev
2023-03-12 13:10 ` Linux regression tracking (Thorsten Leemhuis)
@ 2023-03-12 13:34 ` Laurent Pinchart
2023-03-12 13:59 ` Linux regression tracking (Thorsten Leemhuis)
0 siblings, 1 reply; 24+ messages in thread
From: Laurent Pinchart @ 2023-03-12 13:34 UTC (permalink / raw)
To: Linux regressions mailing list
Cc: linux-media, Sakari Ailus, Manivannan Sadhasivam,
Alexander Stein, Dave Stevenson, Guenter Roeck
Hello Thorsten,
On Sun, Mar 12, 2023 at 02:10:16PM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 27.02.23 18:52, Guenter Roeck wrote:
> > On Mon, Jan 16, 2023 at 04:44:51PM +0200, Laurent Pinchart wrote:
> >> Initializing the subdev before runtime PM means that no subdev
> >> initialization can interact with the runtime PM framework. This can be
> >> problematic when modifying controls, as the .s_ctrl() handler commonly
> >> calls pm_runtime_get_if_in_use(). These code paths are not trivial,
> >> making the driver fragile and possibly causing subtle bugs.
> >>
> >> To make the subdev initialization more robust, initialize runtime PM
> >> first.
> >>
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> >> ---
> >
> > This patch results in
> >
> > Error log:
> > <stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
> > drivers/media/i2c/imx290.c:1090:12: error: 'imx290_runtime_suspend' defined but not used [-Werror=unused-function]
> > 1090 | static int imx290_runtime_suspend(struct device *dev)
> > | ^~~~~~~~~~~~~~~~~~~~~~
> > drivers/media/i2c/imx290.c:1082:12: error: 'imx290_runtime_resume' defined but not used [-Werror=unused-function]
> > 1082 | static int imx290_runtime_resume(struct device *dev)
> >
> > if PM runtime support is disabled( alpha:allmodconfig, csky:allmodconfig,
> > and others).
>
> Looks like Guenter never got a reply, but from a recent kernelci report
> it looks like above warning still happens:
> https://lore.kernel.org/all/640bceb7.a70a0220.af8cd.146b@mx.google.com/
>
> Laurent, do you still have it on your radar?
I don't. Arnd has sent a fix
(https://lore.kernel.org/linux-media/20230207161316.293923-1-arnd@kernel.org),
I've reviewed it, now I expect Sakari to pick it up and get it upstream.
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
>
> #regzbot poke
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 14/17] media: i2c: imx290: Initialize runtime PM before subdev
2023-03-12 13:34 ` Laurent Pinchart
@ 2023-03-12 13:59 ` Linux regression tracking (Thorsten Leemhuis)
0 siblings, 0 replies; 24+ messages in thread
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-03-12 13:59 UTC (permalink / raw)
To: Laurent Pinchart, Linux regressions mailing list
Cc: linux-media, Sakari Ailus, Manivannan Sadhasivam,
Alexander Stein, Dave Stevenson, Guenter Roeck
On 12.03.23 14:34, Laurent Pinchart wrote:
> On Sun, Mar 12, 2023 at 02:10:16PM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
>> On 27.02.23 18:52, Guenter Roeck wrote:
>>> On Mon, Jan 16, 2023 at 04:44:51PM +0200, Laurent Pinchart wrote:
>>>> Initializing the subdev before runtime PM means that no subdev
>>>> initialization can interact with the runtime PM framework. This can be
>>>> problematic when modifying controls, as the .s_ctrl() handler commonly
>>>> calls pm_runtime_get_if_in_use(). These code paths are not trivial,
>>>> making the driver fragile and possibly causing subtle bugs.
>>>>
>>>> To make the subdev initialization more robust, initialize runtime PM
>>>> first.
>>>>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>>> ---
>>>
>>> This patch results in
>>>
>>> Error log:
>>> <stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
>>> drivers/media/i2c/imx290.c:1090:12: error: 'imx290_runtime_suspend' defined but not used [-Werror=unused-function]
>>> 1090 | static int imx290_runtime_suspend(struct device *dev)
>>> | ^~~~~~~~~~~~~~~~~~~~~~
>>> drivers/media/i2c/imx290.c:1082:12: error: 'imx290_runtime_resume' defined but not used [-Werror=unused-function]
>>> 1082 | static int imx290_runtime_resume(struct device *dev)
>>>
>>> if PM runtime support is disabled( alpha:allmodconfig, csky:allmodconfig,
>>> and others).
>>
>> Looks like Guenter never got a reply, but from a recent kernelci report
>> it looks like above warning still happens:
>> https://lore.kernel.org/all/640bceb7.a70a0220.af8cd.146b@mx.google.com/
>>
>> Laurent, do you still have it on your radar?
>
> I don't. Arnd has sent a fix
> (https://lore.kernel.org/linux-media/20230207161316.293923-1-arnd@kernel.org),
> I've reviewed it, now I expect Sakari to pick it up and get it upstream.
Ahh, great, thx for taking the time and letting me know, much appreciated.
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
P.S.: update the status
#regzbot monitor:
https://lore.kernel.org/linux-media/20230207161316.293923-1-arnd@kernel.org/
#regzbot fix: media: i2c: imx290: fix conditional function defintions
#regzbot ignore-activity
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 15/17] media: i2c: imx290: Configure data lanes at start time
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
` (13 preceding siblings ...)
2023-01-16 14:44 ` [PATCH v3 14/17] media: i2c: imx290: Initialize runtime PM before subdev Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 16/17] media: i2c: imx290: Simplify imx290_set_data_lanes() Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 17/17] media: i2c: imx290: Handle error from imx290_set_data_lanes() Laurent Pinchart
16 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
There's no need to configure the data lanes in the runtime PM resume
handler. Do so in imx290_start_streaming() instead.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
drivers/media/i2c/imx290.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 4185835f065d..34278d098218 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -762,6 +762,9 @@ static int imx290_start_streaming(struct imx290 *imx290,
return ret;
}
+ /* Set data lane count */
+ imx290_set_data_lanes(imx290);
+
/* Apply the register values related to current frame format */
format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
ret = imx290_setup_format(imx290, format);
@@ -1069,9 +1072,6 @@ static int imx290_power_on(struct imx290 *imx290)
gpiod_set_value_cansleep(imx290->rst_gpio, 0);
usleep_range(30000, 31000);
- /* Set data lane count */
- imx290_set_data_lanes(imx290);
-
return 0;
}
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 16/17] media: i2c: imx290: Simplify imx290_set_data_lanes()
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
` (14 preceding siblings ...)
2023-01-16 14:44 ` [PATCH v3 15/17] media: i2c: imx290: Configure data lanes at start time Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 17/17] media: i2c: imx290: Handle error from imx290_set_data_lanes() Laurent Pinchart
16 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
There's no need to check for an incorrect number of data lanes in
imx290_set_data_lanes() as the value is validated at probe() time. Drop
the check.
The PHY_LANE_NUM and CSI_LANE_MODE registers are programmed with a value
equal to the number of lanes minus one. Compute it instead of handling
it in the switch/case.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
drivers/media/i2c/imx290.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 34278d098218..bb8713813e29 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -512,28 +512,21 @@ static int imx290_set_register_array(struct imx290 *imx290,
static int imx290_set_data_lanes(struct imx290 *imx290)
{
- int ret = 0, laneval, frsel;
+ int ret = 0;
+ u32 frsel;
switch (imx290->nlanes) {
case 2:
- laneval = 0x01;
+ default:
frsel = 0x02;
break;
case 4:
- laneval = 0x03;
frsel = 0x01;
break;
- default:
- /*
- * We should never hit this since the data lane count is
- * validated in probe itself
- */
- dev_err(imx290->dev, "Lane configuration not supported\n");
- return -EINVAL;
}
- imx290_write(imx290, IMX290_PHY_LANE_NUM, laneval, &ret);
- imx290_write(imx290, IMX290_CSI_LANE_MODE, laneval, &ret);
+ imx290_write(imx290, IMX290_PHY_LANE_NUM, imx290->nlanes - 1, &ret);
+ imx290_write(imx290, IMX290_CSI_LANE_MODE, imx290->nlanes - 1, &ret);
imx290_write(imx290, IMX290_FR_FDG_SEL, frsel, &ret);
return ret;
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 17/17] media: i2c: imx290: Handle error from imx290_set_data_lanes()
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
` (15 preceding siblings ...)
2023-01-16 14:44 ` [PATCH v3 16/17] media: i2c: imx290: Simplify imx290_set_data_lanes() Laurent Pinchart
@ 2023-01-16 14:44 ` Laurent Pinchart
16 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-01-16 14:44 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Manivannan Sadhasivam, Alexander Stein, Dave Stevenson
Check the error status returned by imx290_set_data_lanes() in its
caller and propagate it.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes since v1:
- New patch
---
drivers/media/i2c/imx290.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index bb8713813e29..49d6c8bdec41 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -756,7 +756,11 @@ static int imx290_start_streaming(struct imx290 *imx290,
}
/* Set data lane count */
- imx290_set_data_lanes(imx290);
+ ret = imx290_set_data_lanes(imx290);
+ if (ret < 0) {
+ dev_err(imx290->dev, "Could not set data lanes\n");
+ return ret;
+ }
/* Apply the register values related to current frame format */
format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread