On 12/10/2021 17:32, Hans Verkuil wrote:
On 05/10/2021 10:57, Tomi Valkeinen wrote:Add a new 'state' field to struct v4l2_subdev to which we can store the active state of a subdev. This will place the subdev configuration into a known place, allowing us to use the state directly from the v4l2 framework, thus simplifying the drivers. Also add functions v4l2_subdev_init_finalize() and v4l2_subdev_cleanup(), which will allocate and free the active state. The functions are named in a generic way so that they can be also used for other subdev initialization work. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> --- drivers/media/v4l2-core/v4l2-subdev.c | 21 +++++++++++ include/media/v4l2-subdev.h | 51 +++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index fe49c86a9b02..bcaf66a1e3d9 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd, v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev); } EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event); + +int v4l2_subdev_init_finalize(struct v4l2_subdev *sd) +{ + struct v4l2_subdev_state *state; + + state = __v4l2_subdev_state_alloc(sd); + if (IS_ERR(state)) + return PTR_ERR(state); + + sd->state = state; + + return 0; +} +EXPORT_SYMBOL_GPL(v4l2_subdev_init_finalize); + +void v4l2_subdev_cleanup(struct v4l2_subdev *sd) +{ + __v4l2_subdev_state_free(sd->state); + sd->state = NULL; +} +EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup); diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index e52bf508c75b..3aaa7146e5ff 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -898,6 +898,8 @@ struct v4l2_subdev_platform_data { * @subdev_notifier: A sub-device notifier implicitly registered for the sub- * device using v4l2_async_register_subdev_sensor(). * @pdata: common part of subdevice platform data + * @state: active state for the subdev (NULL for subdevs tracking the state + * internally) * * Each instance of a subdev driver should create this struct, either * stand-alone or embedded in a larger struct. @@ -929,6 +931,19 @@ struct v4l2_subdev { struct v4l2_async_notifier *notifier; struct v4l2_async_notifier *subdev_notifier; struct v4l2_subdev_platform_data *pdata; + + /* + * The fields below are private, and should only be accessed via + * appropriate functions. + */ + + /* + * TODO: state should most likely be changed from a pointer to an + * embedded field. For the time being it's kept as a pointer to more + * easily catch uses of state in the cases where the driver doesn't + * support it. + */ + struct v4l2_subdev_state *state;Hmm: struct v4l2_subdev_state { struct v4l2_subdev_pad_config *pads; }; and: struct v4l2_subdev_pad_config { struct v4l2_mbus_framefmt try_fmt; struct v4l2_rect try_crop; struct v4l2_rect try_compose; }; So why is this the active state if struct v4l2_subdev_pad_config has try_* fields? At the very least it should be mentioned somewhere in the code (and probably commit log as well) that the try_ prefix is historical, or will be removed later, or something like that.
I'll add a note about it. I did not want to do renames like this yet, as they'll affect multiple drivers, creating a conflict hell when rebasing this series.
Also note that at the moment those try_* fields are only used in try-context. Upstream drivers do not support active state, so they'll only get the TRY version of v4l2_subdev_state. And the new drivers that support active state do not use the 'pads' field nor v4l2_subdev_pad_config.
Tomi
