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@xxxxxxxxxxxxxxxx>
---
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 | 170 +++++++++++++++++--------------------
1 file changed, 80 insertions(+), 90 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index b01a627c8c03..94d23eea36be 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,31 @@ 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. This includes all controls 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.
+ */
+ switch (ctrl->id) {
+ case V4L2_CID_LINK_FREQ:
+ case V4L2_CID_PIXEL_RATE:
+ case V4L2_CID_VBLANK:
+ case V4L2_CID_HBLANK:
+ 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 +610,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
&ret);
}
break;
+
default:
ret = -EINVAL;
break;
@@ -626,13 +645,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);
@@ -654,7 +666,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
@@ -719,11 +730,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
return ret;
}
- mutex_lock(imx290->ctrls.lock);
- imx290_ctrl_update(imx290, &imx290->current_format,
- imx290->current_mode);
- mutex_unlock(imx290->ctrls.lock);
-
return 0;
}
@@ -732,8 +738,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 */
@@ -746,7 +754,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;
@@ -766,7 +775,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;
@@ -795,39 +804,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)
@@ -862,23 +864,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)
@@ -888,8 +873,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);
@@ -907,7 +890,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;
@@ -918,8 +901,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
*format = fmt->format;
- mutex_unlock(&imx290->lock);
-
return 0;
}
@@ -927,14 +908,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;
@@ -943,7 +921,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;
}
@@ -972,11 +949,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);
@@ -991,7 +970,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,20 +987,12 @@ 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;
- /*
- * 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;
@@ -1033,15 +1004,40 @@ 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;
+ }
+
+ 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;
+
+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);
}
@@ -1272,12 +1268,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) {
@@ -1300,8 +1294,6 @@ static int imx290_probe(struct i2c_client *client)
err_subdev:
imx290_subdev_cleanup(imx290);
-err_mutex:
- mutex_destroy(&imx290->lock);
return ret;
}
@@ -1314,8 +1306,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
