diff mbox series

[08/19] media: max9286: Add support for subdev active state

Message ID 20240430103956.60190-9-jacopo.mondi@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series [01/19] media: adv748x: Add support for active state | expand

Commit Message

Jacopo Mondi April 30, 2024, 10:39 a.m. UTC 
Use the subdev active state in the max9286 driver to store the
image format.

Replace the .open() function call with the .init_state() one and
simplify the set/get_pad_fmt() operations.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 110 ++++++++++++------------------------
 1 file changed, 37 insertions(+), 73 deletions(-)

Comments

Laurent Pinchart May 2, 2024, 6:10 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Tue, Apr 30, 2024 at 12:39:44PM +0200, Jacopo Mondi wrote:
> Use the subdev active state in the max9286 driver to store the
> image format.
> 
> Replace the .open() function call with the .init_state() one and
> simplify the set/get_pad_fmt() operations.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 110 ++++++++++++------------------------
>  1 file changed, 37 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index d685d445cf23..fb13bfde42df 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -19,7 +19,6 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
>  #include <linux/module.h>
> -#include <linux/mutex.h>
>  #include <linux/of_graph.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
> @@ -198,12 +197,8 @@ struct max9286_priv {
>  	struct v4l2_ctrl *pixelrate_ctrl;
>  	unsigned int pixelrate;
>  
> -	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
>  	struct v4l2_fract interval;
>  
> -	/* Protects controls and fmt structures */
> -	struct mutex mutex;
> -
>  	unsigned int nsources;
>  	unsigned int source_mask;
>  	unsigned int route_mask;
> @@ -788,19 +783,23 @@ static void max9286_v4l2_notifier_unregister(struct max9286_priv *priv)
>  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct max9286_priv *priv = sd_to_max9286(sd);
> +	struct v4l2_subdev_state *state;
>  	struct max9286_source *source;
>  	unsigned int i;
>  	bool sync = false;
> -	int ret;
> +	int ret = 0;
> +
> +	state = v4l2_subdev_lock_and_get_active_state(sd);
>  
>  	if (enable) {
>  		const struct v4l2_mbus_framefmt *format;
> +		unsigned int source_idx = __ffs(priv->bound_sources);
>  
>  		/*
>  		 * Get the format from the first used sink pad, as all sink
>  		 * formats must be identical.
>  		 */
> -		format = &priv->fmt[__ffs(priv->bound_sources)];
> +		format = v4l2_subdev_state_get_format(state, source_idx);

It would be simpler to get the format from the source pad, as all
formats are identical unless I'm mistaken. It may become more complex
when introducing streams though.

>  
>  		max9286_set_video_format(priv, format);
>  		max9286_set_fsync_period(priv);
> @@ -816,12 +815,12 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  		for_each_source(priv, source) {
>  			ret = v4l2_subdev_call(source->sd, video, s_stream, 1);
>  			if (ret)
> -				return ret;
> +				goto err_unlock;
>  		}
>  
>  		ret = max9286_check_video_links(priv);
>  		if (ret)
> -			return ret;
> +			goto err_unlock;
>  
>  		/*
>  		 * Wait until frame synchronization is locked.
> @@ -842,7 +841,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  		if (!sync) {
>  			dev_err(&priv->client->dev,
>  				"Failed to get frame synchronization\n");
> -			return -EXDEV; /* Invalid cross-device link */
> +			ret = -EXDEV; /* Invalid cross-device link */
> +			goto err_unlock;
>  		}
>  
>  		/*
> @@ -865,7 +865,10 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  		max9286_i2c_mux_close(priv);
>  	}
>  
> -	return 0;
> +err_unlock:

As this is used in the success path too, name the label 'done' or 'out'
or 'unlock'.

> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
>  }
>  
>  static int max9286_get_frame_interval(struct v4l2_subdev *sd,
> @@ -922,29 +925,16 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> -static struct v4l2_mbus_framefmt *
> -max9286_get_pad_format(struct max9286_priv *priv,
> -		       struct v4l2_subdev_state *sd_state,
> -		       unsigned int pad, u32 which)
> -{
> -	switch (which) {
> -	case V4L2_SUBDEV_FORMAT_TRY:
> -		return v4l2_subdev_state_get_format(sd_state, pad);
> -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> -		return &priv->fmt[pad];
> -	default:
> -		return NULL;
> -	}
> -}
> -
>  static int max9286_set_fmt(struct v4l2_subdev *sd,
> -			   struct v4l2_subdev_state *sd_state,
> +			   struct v4l2_subdev_state *state,
>  			   struct v4l2_subdev_format *format)
>  {
> -	struct max9286_priv *priv = sd_to_max9286(sd);
> -	struct v4l2_mbus_framefmt *cfg_fmt;
>  	unsigned int i;
>  
> +	/*
> +	 * Disable setting format on the source pad: format is propagated
> +	 * from the sinks.
> +	 */
>  	if (format->pad == MAX9286_SRC_PAD)
>  		return -EINVAL;

Return v4l2_subdev_get_fmt().

>  
> @@ -957,42 +947,13 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  	if (i == ARRAY_SIZE(max9286_formats))
>  		format->format.code = max9286_formats[0].code;
>  
> -	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
> -					 format->which);
> -	if (!cfg_fmt)
> -		return -EINVAL;
> -
> -	mutex_lock(&priv->mutex);
> -	*cfg_fmt = format->format;
> -	mutex_unlock(&priv->mutex);
> -
> -	return 0;
> -}
> -
> -static int max9286_get_fmt(struct v4l2_subdev *sd,
> -			   struct v4l2_subdev_state *sd_state,
> -			   struct v4l2_subdev_format *format)
> -{
> -	struct max9286_priv *priv = sd_to_max9286(sd);
> -	struct v4l2_mbus_framefmt *cfg_fmt;
> -	unsigned int pad = format->pad;
> +	*v4l2_subdev_state_get_format(state, format->pad) = format->format;
>  
>  	/*
> -	 * Multiplexed Stream Support: Support link validation by returning the
> -	 * format of the first bound link. All links must have the same format,
> -	 * as we do not support mixing and matching of cameras connected to the
> -	 * max9286.
> +	 * Apply the same format on the source pad: all links must have the
> +	 * same format.
>  	 */
> -	if (pad == MAX9286_SRC_PAD)
> -		pad = __ffs(priv->bound_sources);
> -
> -	cfg_fmt = max9286_get_pad_format(priv, sd_state, pad, format->which);
> -	if (!cfg_fmt)
> -		return -EINVAL;
> -
> -	mutex_lock(&priv->mutex);
> -	format->format = *cfg_fmt;
> -	mutex_unlock(&priv->mutex);
> +	*v4l2_subdev_state_get_format(state, MAX9286_SRC_PAD) = format->format;

If you want to keep the current behaviour, you should propagate the
format to the source pad for the first sink pad only.

>  
>  	return 0;
>  }
> @@ -1003,7 +964,7 @@ static const struct v4l2_subdev_video_ops max9286_video_ops = {
>  
>  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
>  	.enum_mbus_code = max9286_enum_mbus_code,
> -	.get_fmt	= max9286_get_fmt,
> +	.get_fmt	= v4l2_subdev_get_fmt,
>  	.set_fmt	= max9286_set_fmt,
>  	.get_frame_interval = max9286_get_frame_interval,
>  	.set_frame_interval = max9286_set_frame_interval,
> @@ -1030,13 +991,14 @@ static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
>  	*fmt = max9286_default_format;
>  }
>  
> -static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
> +static int max9286_init_state(struct v4l2_subdev *sd,
> +			      struct v4l2_subdev_state *state)
>  {
>  	struct v4l2_mbus_framefmt *format;
>  	unsigned int i;
>  
>  	for (i = 0; i < MAX9286_N_SINKS; i++) {
> -		format = v4l2_subdev_state_get_format(fh->state, i);
> +		format = v4l2_subdev_state_get_format(state, i);
>  		max9286_init_format(format);
>  	}

You need to initialize the source pad format too.

>  
> @@ -1044,7 +1006,7 @@ static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
>  }
>  
>  static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
> -	.open = max9286_open,
> +	.init_state = max9286_init_state,
>  };
>  
>  static const struct media_entity_operations max9286_media_ops = {
> @@ -1079,10 +1041,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  	}
>  
>  	/* Configure V4L2 for the MAX9286 itself */
> -
> -	for (i = 0; i < MAX9286_N_SINKS; i++)
> -		max9286_init_format(&priv->fmt[i]);
> -
>  	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
>  	priv->sd.internal_ops = &max9286_subdev_internal_ops;
>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> @@ -1109,14 +1067,21 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  	if (ret)
>  		goto err_async;
>  
> +	priv->sd.state_lock = priv->ctrls.lock;
> +	ret = v4l2_subdev_init_finalize(&priv->sd);
> +	if (ret)
> +		goto err_async;
> +
>  	ret = v4l2_async_register_subdev(&priv->sd);
>  	if (ret < 0) {
>  		dev_err(dev, "Unable to register subdevice\n");
> -		goto err_async;
> +		goto err_subdev;
>  	}
>  
>  	return 0;
>  
> +err_subdev:
> +	v4l2_subdev_cleanup(&priv->sd);
>  err_async:
>  	v4l2_ctrl_handler_free(&priv->ctrls);
>  	max9286_v4l2_notifier_unregister(priv);
> @@ -1126,6 +1091,7 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  
>  static void max9286_v4l2_unregister(struct max9286_priv *priv)
>  {
> +	v4l2_subdev_cleanup(&priv->sd);
>  	v4l2_ctrl_handler_free(&priv->ctrls);
>  	v4l2_async_unregister_subdev(&priv->sd);
>  	max9286_v4l2_notifier_unregister(priv);
> @@ -1629,8 +1595,6 @@ static int max9286_probe(struct i2c_client *client)
>  	if (!priv)
>  		return -ENOMEM;
>  
> -	mutex_init(&priv->mutex);
> -
>  	priv->client = client;
>  
>  	/* GPIO values default to high */
diff mbox series

Patch

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index d685d445cf23..fb13bfde42df 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -19,7 +19,6 @@ 
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/module.h>
-#include <linux/mutex.h>
 #include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
@@ -198,12 +197,8 @@  struct max9286_priv {
 	struct v4l2_ctrl *pixelrate_ctrl;
 	unsigned int pixelrate;
 
-	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
 	struct v4l2_fract interval;
 
-	/* Protects controls and fmt structures */
-	struct mutex mutex;
-
 	unsigned int nsources;
 	unsigned int source_mask;
 	unsigned int route_mask;
@@ -788,19 +783,23 @@  static void max9286_v4l2_notifier_unregister(struct max9286_priv *priv)
 static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct max9286_priv *priv = sd_to_max9286(sd);
+	struct v4l2_subdev_state *state;
 	struct max9286_source *source;
 	unsigned int i;
 	bool sync = false;
-	int ret;
+	int ret = 0;
+
+	state = v4l2_subdev_lock_and_get_active_state(sd);
 
 	if (enable) {
 		const struct v4l2_mbus_framefmt *format;
+		unsigned int source_idx = __ffs(priv->bound_sources);
 
 		/*
 		 * Get the format from the first used sink pad, as all sink
 		 * formats must be identical.
 		 */
-		format = &priv->fmt[__ffs(priv->bound_sources)];
+		format = v4l2_subdev_state_get_format(state, source_idx);
 
 		max9286_set_video_format(priv, format);
 		max9286_set_fsync_period(priv);
@@ -816,12 +815,12 @@  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 		for_each_source(priv, source) {
 			ret = v4l2_subdev_call(source->sd, video, s_stream, 1);
 			if (ret)
-				return ret;
+				goto err_unlock;
 		}
 
 		ret = max9286_check_video_links(priv);
 		if (ret)
-			return ret;
+			goto err_unlock;
 
 		/*
 		 * Wait until frame synchronization is locked.
@@ -842,7 +841,8 @@  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 		if (!sync) {
 			dev_err(&priv->client->dev,
 				"Failed to get frame synchronization\n");
-			return -EXDEV; /* Invalid cross-device link */
+			ret = -EXDEV; /* Invalid cross-device link */
+			goto err_unlock;
 		}
 
 		/*
@@ -865,7 +865,10 @@  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 		max9286_i2c_mux_close(priv);
 	}
 
-	return 0;
+err_unlock:
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
 }
 
 static int max9286_get_frame_interval(struct v4l2_subdev *sd,
@@ -922,29 +925,16 @@  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static struct v4l2_mbus_framefmt *
-max9286_get_pad_format(struct max9286_priv *priv,
-		       struct v4l2_subdev_state *sd_state,
-		       unsigned int pad, u32 which)
-{
-	switch (which) {
-	case V4L2_SUBDEV_FORMAT_TRY:
-		return v4l2_subdev_state_get_format(sd_state, pad);
-	case V4L2_SUBDEV_FORMAT_ACTIVE:
-		return &priv->fmt[pad];
-	default:
-		return NULL;
-	}
-}
-
 static int max9286_set_fmt(struct v4l2_subdev *sd,
-			   struct v4l2_subdev_state *sd_state,
+			   struct v4l2_subdev_state *state,
 			   struct v4l2_subdev_format *format)
 {
-	struct max9286_priv *priv = sd_to_max9286(sd);
-	struct v4l2_mbus_framefmt *cfg_fmt;
 	unsigned int i;
 
+	/*
+	 * Disable setting format on the source pad: format is propagated
+	 * from the sinks.
+	 */
 	if (format->pad == MAX9286_SRC_PAD)
 		return -EINVAL;
 
@@ -957,42 +947,13 @@  static int max9286_set_fmt(struct v4l2_subdev *sd,
 	if (i == ARRAY_SIZE(max9286_formats))
 		format->format.code = max9286_formats[0].code;
 
-	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
-					 format->which);
-	if (!cfg_fmt)
-		return -EINVAL;
-
-	mutex_lock(&priv->mutex);
-	*cfg_fmt = format->format;
-	mutex_unlock(&priv->mutex);
-
-	return 0;
-}
-
-static int max9286_get_fmt(struct v4l2_subdev *sd,
-			   struct v4l2_subdev_state *sd_state,
-			   struct v4l2_subdev_format *format)
-{
-	struct max9286_priv *priv = sd_to_max9286(sd);
-	struct v4l2_mbus_framefmt *cfg_fmt;
-	unsigned int pad = format->pad;
+	*v4l2_subdev_state_get_format(state, format->pad) = format->format;
 
 	/*
-	 * Multiplexed Stream Support: Support link validation by returning the
-	 * format of the first bound link. All links must have the same format,
-	 * as we do not support mixing and matching of cameras connected to the
-	 * max9286.
+	 * Apply the same format on the source pad: all links must have the
+	 * same format.
 	 */
-	if (pad == MAX9286_SRC_PAD)
-		pad = __ffs(priv->bound_sources);
-
-	cfg_fmt = max9286_get_pad_format(priv, sd_state, pad, format->which);
-	if (!cfg_fmt)
-		return -EINVAL;
-
-	mutex_lock(&priv->mutex);
-	format->format = *cfg_fmt;
-	mutex_unlock(&priv->mutex);
+	*v4l2_subdev_state_get_format(state, MAX9286_SRC_PAD) = format->format;
 
 	return 0;
 }
@@ -1003,7 +964,7 @@  static const struct v4l2_subdev_video_ops max9286_video_ops = {
 
 static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
 	.enum_mbus_code = max9286_enum_mbus_code,
-	.get_fmt	= max9286_get_fmt,
+	.get_fmt	= v4l2_subdev_get_fmt,
 	.set_fmt	= max9286_set_fmt,
 	.get_frame_interval = max9286_get_frame_interval,
 	.set_frame_interval = max9286_set_frame_interval,
@@ -1030,13 +991,14 @@  static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
 	*fmt = max9286_default_format;
 }
 
-static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
+static int max9286_init_state(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_state *state)
 {
 	struct v4l2_mbus_framefmt *format;
 	unsigned int i;
 
 	for (i = 0; i < MAX9286_N_SINKS; i++) {
-		format = v4l2_subdev_state_get_format(fh->state, i);
+		format = v4l2_subdev_state_get_format(state, i);
 		max9286_init_format(format);
 	}
 
@@ -1044,7 +1006,7 @@  static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
 }
 
 static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
-	.open = max9286_open,
+	.init_state = max9286_init_state,
 };
 
 static const struct media_entity_operations max9286_media_ops = {
@@ -1079,10 +1041,6 @@  static int max9286_v4l2_register(struct max9286_priv *priv)
 	}
 
 	/* Configure V4L2 for the MAX9286 itself */
-
-	for (i = 0; i < MAX9286_N_SINKS; i++)
-		max9286_init_format(&priv->fmt[i]);
-
 	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
 	priv->sd.internal_ops = &max9286_subdev_internal_ops;
 	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -1109,14 +1067,21 @@  static int max9286_v4l2_register(struct max9286_priv *priv)
 	if (ret)
 		goto err_async;
 
+	priv->sd.state_lock = priv->ctrls.lock;
+	ret = v4l2_subdev_init_finalize(&priv->sd);
+	if (ret)
+		goto err_async;
+
 	ret = v4l2_async_register_subdev(&priv->sd);
 	if (ret < 0) {
 		dev_err(dev, "Unable to register subdevice\n");
-		goto err_async;
+		goto err_subdev;
 	}
 
 	return 0;
 
+err_subdev:
+	v4l2_subdev_cleanup(&priv->sd);
 err_async:
 	v4l2_ctrl_handler_free(&priv->ctrls);
 	max9286_v4l2_notifier_unregister(priv);
@@ -1126,6 +1091,7 @@  static int max9286_v4l2_register(struct max9286_priv *priv)
 
 static void max9286_v4l2_unregister(struct max9286_priv *priv)
 {
+	v4l2_subdev_cleanup(&priv->sd);
 	v4l2_ctrl_handler_free(&priv->ctrls);
 	v4l2_async_unregister_subdev(&priv->sd);
 	max9286_v4l2_notifier_unregister(priv);
@@ -1629,8 +1595,6 @@  static int max9286_probe(struct i2c_client *client)
 	if (!priv)
 		return -ENOMEM;
 
-	mutex_init(&priv->mutex);
-
 	priv->client = client;
 
 	/* GPIO values default to high */