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.
Regards,
Hans
> };
>
>
> @@ -1217,4 +1232,40 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
> void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
> const struct v4l2_event *ev);
>
> +/**
> + * v4l2_subdev_init_finalize() - Finalize the initialization of the subdevice
> + * @sd: The subdev
> + *
> + * This finalizes the initialization of the subdev, including allocation of
> + * the active state for the subdev.
> + *
> + * This must be called by the subdev drivers that use the centralized active
> + * state, after the subdev struct has been initialized and
> + * media_entity_pads_init() has been called.
> + *
> + * Must call v4l2_subdev_cleanup() when the subdev is being removed.
> + */
> +int v4l2_subdev_init_finalize(struct v4l2_subdev *sd);
> +
> +/**
> + * v4l2_subdev_cleanup() - Release the resources needed by the subdevice
> + * @sd: The subdevice
> + *
> + * This will release the resources allocated in v4l2_subdev_init_finalize.
> + */
> +void v4l2_subdev_cleanup(struct v4l2_subdev *sd);
> +
> +/**
> + * v4l2_subdev_get_active_state() - Return the active subdev state for subdevice
> + * @sd: The subdevice
> + *
> + * Return the active state for the subdevice, or NULL if the subdev does not
> + * support active state.
> + */
> +static inline struct v4l2_subdev_state *
> +v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
> +{
> + return sd->state;
> +}
> +
> #endif
>
