1. Patchew
  2. linux
  3. [PATCH 0/4] media: platform: nxp: Trivial cleanup pathces
  4. View patch

[PATCH 4/4] media: nxp: use cleanup __free(fwnode_handle) simplify code

Frank Li posted 4 patches 2 weeks ago
There is a newer version of this series
[PATCH 4/4] media: nxp: use cleanup __free(fwnode_handle) simplify code
Posted by Frank Li 2 weeks ago 
Use cleanup __free(fwnode_handle) simplify code. Change to dev_err_probe()
because replace goto with return.

Add missed "\n" at error message.

No functional change.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c    | 31 +++++++++------------------
 drivers/media/platform/nxp/imx8mq-mipi-csi2.c | 31 +++++++++------------------
 2 files changed, 20 insertions(+), 42 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index ce93d868746f002c22e2f86b1e0aa84ec1a76061..d924adb406a30797b66f0094ab17e98ad44fefac 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -12,6 +12,7 @@
  *
  */
 
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/debugfs.h>
 #include <linux/delay.h>
@@ -1349,28 +1350,25 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
 		.bus_type = V4L2_MBUS_CSI2_DPHY,
 	};
 	struct v4l2_async_connection *asd;
-	struct fwnode_handle *ep;
 	unsigned int i;
 	int ret;
 
 	v4l2_async_subdev_nf_init(&csis->notifier, &csis->sd);
 
-	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(csis->dev), 0, 0,
-					     FWNODE_GRAPH_ENDPOINT_NEXT);
+	struct fwnode_handle *ep __free(fwnode_handle) =
+		fwnode_graph_get_endpoint_by_id(dev_fwnode(csis->dev), 0, 0,
+						FWNODE_GRAPH_ENDPOINT_NEXT);
 	if (!ep)
 		return -ENOTCONN;
 
 	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
 	if (ret)
-		goto err_parse;
+		return ret;
 
 	for (i = 0; i < vep.bus.mipi_csi2.num_data_lanes; ++i) {
-		if (vep.bus.mipi_csi2.data_lanes[i] != i + 1) {
-			dev_err(csis->dev,
-				"data lanes reordering is not supported");
-			ret = -EINVAL;
-			goto err_parse;
-		}
+		if (vep.bus.mipi_csi2.data_lanes[i] != i + 1)
+			return dev_err_probe(csis->dev, -EINVAL,
+					     "data lanes reordering is not supported\n");
 	}
 
 	csis->bus = vep.bus.mipi_csi2;
@@ -1381,12 +1379,8 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
 
 	asd = v4l2_async_nf_add_fwnode_remote(&csis->notifier, ep,
 					      struct v4l2_async_connection);
-	if (IS_ERR(asd)) {
-		ret = PTR_ERR(asd);
-		goto err_parse;
-	}
-
-	fwnode_handle_put(ep);
+	if (IS_ERR(asd))
+		return PTR_ERR(asd);
 
 	csis->notifier.ops = &mipi_csis_notify_ops;
 
@@ -1395,11 +1389,6 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
 		return ret;
 
 	return v4l2_async_register_subdev(&csis->sd);
-
-err_parse:
-	fwnode_handle_put(ep);
-
-	return ret;
 }
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c
index 75709161fb26a61239b94430365849e022fdc14f..94882568405db55593c5c51722db2233a64d53e4 100644
--- a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c
+++ b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/delay.h>
@@ -717,28 +718,25 @@ static int imx8mq_mipi_csi_async_register(struct csi_state *state)
 		.bus_type = V4L2_MBUS_CSI2_DPHY,
 	};
 	struct v4l2_async_connection *asd;
-	struct fwnode_handle *ep;
 	unsigned int i;
 	int ret;
 
 	v4l2_async_subdev_nf_init(&state->notifier, &state->sd);
 
-	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(state->dev), 0, 0,
-					     FWNODE_GRAPH_ENDPOINT_NEXT);
+	struct fwnode_handle *ep __free(fwnode_handle) =
+		fwnode_graph_get_endpoint_by_id(dev_fwnode(state->dev), 0, 0,
+						FWNODE_GRAPH_ENDPOINT_NEXT);
 	if (!ep)
 		return -ENOTCONN;
 
 	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
 	if (ret)
-		goto err_parse;
+		return ret;
 
 	for (i = 0; i < vep.bus.mipi_csi2.num_data_lanes; ++i) {
-		if (vep.bus.mipi_csi2.data_lanes[i] != i + 1) {
-			dev_err(state->dev,
-				"data lanes reordering is not supported");
-			ret = -EINVAL;
-			goto err_parse;
-		}
+		if (vep.bus.mipi_csi2.data_lanes[i] != i + 1)
+			return dev_err_probe(state->dev, -EINVAL,
+					     "data lanes reordering is not supported\n");
 	}
 
 	state->bus = vep.bus.mipi_csi2;
@@ -749,12 +747,8 @@ static int imx8mq_mipi_csi_async_register(struct csi_state *state)
 
 	asd = v4l2_async_nf_add_fwnode_remote(&state->notifier, ep,
 					      struct v4l2_async_connection);
-	if (IS_ERR(asd)) {
-		ret = PTR_ERR(asd);
-		goto err_parse;
-	}
-
-	fwnode_handle_put(ep);
+	if (IS_ERR(asd))
+		return PTR_ERR(asd);
 
 	state->notifier.ops = &imx8mq_mipi_csi_notify_ops;
 
@@ -763,11 +757,6 @@ static int imx8mq_mipi_csi_async_register(struct csi_state *state)
 		return ret;
 
 	return v4l2_async_register_subdev(&state->sd);
-
-err_parse:
-	fwnode_handle_put(ep);
-
-	return ret;
 }
 
 /* -----------------------------------------------------------------------------

-- 
2.34.1
Re: [PATCH 4/4] media: nxp: use cleanup __free(fwnode_handle) simplify code
Posted by Laurent Pinchart 1 week, 5 days ago 
Hi Frank,

Thank you for the patch.

On Mon, Nov 17, 2025 at 01:58:14PM -0500, Frank Li wrote:
> Use cleanup __free(fwnode_handle) simplify code. Change to dev_err_probe()
> because replace goto with return.
> 
> Add missed "\n" at error message.
> 
> No functional change.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c    | 31 +++++++++------------------
>  drivers/media/platform/nxp/imx8mq-mipi-csi2.c | 31 +++++++++------------------
>  2 files changed, 20 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index ce93d868746f002c22e2f86b1e0aa84ec1a76061..d924adb406a30797b66f0094ab17e98ad44fefac 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -12,6 +12,7 @@
>   *
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/debugfs.h>
>  #include <linux/delay.h>
> @@ -1349,28 +1350,25 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
>  		.bus_type = V4L2_MBUS_CSI2_DPHY,
>  	};
>  	struct v4l2_async_connection *asd;
uuu> -	struct fwnode_handle *ep;
>  	unsigned int i;
>  	int ret;
>  
>  	v4l2_async_subdev_nf_init(&csis->notifier, &csis->sd);
>  
> -	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(csis->dev), 0, 0,
> -					     FWNODE_GRAPH_ENDPOINT_NEXT);
> +	struct fwnode_handle *ep __free(fwnode_handle) =
> +		fwnode_graph_get_endpoint_by_id(dev_fwnode(csis->dev), 0, 0,
> +						FWNODE_GRAPH_ENDPOINT_NEXT);

Let's avoid mixing variable declarations and code, this is a style
change that is not widely accepted (yet). You can write

	struct fwnode_handle *ep __free(fwnode_handle) = NULL;
 	struct v4l2_async_connection *asd;
	unsigned int i;
	int ret;

  	v4l2_async_subdev_nf_init(&csis->notifier, &csis->sd);

	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(csis->dev), 0, 0,
					     FWNODE_GRAPH_ENDPOINT_NEXT);

>  	if (!ep)
>  		return -ENOTCONN;
>  
>  	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
>  	if (ret)
> -		goto err_parse;
> +		return ret;
>  
>  	for (i = 0; i < vep.bus.mipi_csi2.num_data_lanes; ++i) {
> -		if (vep.bus.mipi_csi2.data_lanes[i] != i + 1) {
> -			dev_err(csis->dev,
> -				"data lanes reordering is not supported");
> -			ret = -EINVAL;
> -			goto err_parse;
> -		}
> +		if (vep.bus.mipi_csi2.data_lanes[i] != i + 1)
> +			return dev_err_probe(csis->dev, -EINVAL,
> +					     "data lanes reordering is not supported\n");

To switch to dev_err_probe(), we should drop the error message in the
probe() function when mipi_csis_async_register() fails, and make sure
every error path in this function prints a message. I'd prefer splitting
that to a separate patch.

>  	}
>  
>  	csis->bus = vep.bus.mipi_csi2;
> @@ -1381,12 +1379,8 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
>  
>  	asd = v4l2_async_nf_add_fwnode_remote(&csis->notifier, ep,
>  					      struct v4l2_async_connection);
> -	if (IS_ERR(asd)) {
> -		ret = PTR_ERR(asd);
> -		goto err_parse;
> -	}
> -
> -	fwnode_handle_put(ep);
> +	if (IS_ERR(asd))
> +		return PTR_ERR(asd);
>  
>  	csis->notifier.ops = &mipi_csis_notify_ops;
>  
> @@ -1395,11 +1389,6 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
>  		return ret;
>  
>  	return v4l2_async_register_subdev(&csis->sd);
> -
> -err_parse:
> -	fwnode_handle_put(ep);
> -
> -	return ret;
>  }
>  
>  /* -----------------------------------------------------------------------------
> diff --git a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c
> index 75709161fb26a61239b94430365849e022fdc14f..94882568405db55593c5c51722db2233a64d53e4 100644
> --- a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c
> +++ b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/delay.h>
> @@ -717,28 +718,25 @@ static int imx8mq_mipi_csi_async_register(struct csi_state *state)
>  		.bus_type = V4L2_MBUS_CSI2_DPHY,
>  	};
>  	struct v4l2_async_connection *asd;
> -	struct fwnode_handle *ep;
>  	unsigned int i;
>  	int ret;
>  
>  	v4l2_async_subdev_nf_init(&state->notifier, &state->sd);
>  
> -	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(state->dev), 0, 0,
> -					     FWNODE_GRAPH_ENDPOINT_NEXT);
> +	struct fwnode_handle *ep __free(fwnode_handle) =
> +		fwnode_graph_get_endpoint_by_id(dev_fwnode(state->dev), 0, 0,
> +						FWNODE_GRAPH_ENDPOINT_NEXT);

Same comment as above.

>  	if (!ep)
>  		return -ENOTCONN;
>  
>  	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
>  	if (ret)
> -		goto err_parse;
> +		return ret;
>  
>  	for (i = 0; i < vep.bus.mipi_csi2.num_data_lanes; ++i) {
> -		if (vep.bus.mipi_csi2.data_lanes[i] != i + 1) {
> -			dev_err(state->dev,
> -				"data lanes reordering is not supported");
> -			ret = -EINVAL;
> -			goto err_parse;
> -		}
> +		if (vep.bus.mipi_csi2.data_lanes[i] != i + 1)
> +			return dev_err_probe(state->dev, -EINVAL,
> +					     "data lanes reordering is not supported\n");

And here too.

Usage of __free(fwnode_handle) looks good, it just needs to be split to
a patch of its own.

>  	}
>  
>  	state->bus = vep.bus.mipi_csi2;
> @@ -749,12 +747,8 @@ static int imx8mq_mipi_csi_async_register(struct csi_state *state)
>  
>  	asd = v4l2_async_nf_add_fwnode_remote(&state->notifier, ep,
>  					      struct v4l2_async_connection);
> -	if (IS_ERR(asd)) {
> -		ret = PTR_ERR(asd);
> -		goto err_parse;
> -	}
> -
> -	fwnode_handle_put(ep);
> +	if (IS_ERR(asd))
> +		return PTR_ERR(asd);
>  
>  	state->notifier.ops = &imx8mq_mipi_csi_notify_ops;
>  
> @@ -763,11 +757,6 @@ static int imx8mq_mipi_csi_async_register(struct csi_state *state)
>  		return ret;
>  
>  	return v4l2_async_register_subdev(&state->sd);
> -
> -err_parse:
> -	fwnode_handle_put(ep);
> -
> -	return ret;
>  }
>  
>  /* -----------------------------------------------------------------------------

-- 
Regards,

Laurent Pinchart
Re: [PATCH 4/4] media: nxp: use cleanup __free(fwnode_handle) simplify code
Posted by Laurent Pinchart 1 week, 5 days ago 
On Wed, Nov 19, 2025 at 01:25:29PM +0900, Laurent Pinchart wrote:
> On Mon, Nov 17, 2025 at 01:58:14PM -0500, Frank Li wrote:
> > Use cleanup __free(fwnode_handle) simplify code. Change to dev_err_probe()
> > because replace goto with return.
> > 
> > Add missed "\n" at error message.
> > 
> > No functional change.
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/media/platform/nxp/imx-mipi-csis.c    | 31 +++++++++------------------
> >  drivers/media/platform/nxp/imx8mq-mipi-csi2.c | 31 +++++++++------------------
> >  2 files changed, 20 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > index ce93d868746f002c22e2f86b1e0aa84ec1a76061..d924adb406a30797b66f0094ab17e98ad44fefac 100644
> > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > @@ -12,6 +12,7 @@
> >   *
> >   */
> >  
> > +#include <linux/cleanup.h>
> >  #include <linux/clk.h>
> >  #include <linux/debugfs.h>
> >  #include <linux/delay.h>
> > @@ -1349,28 +1350,25 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
> >  		.bus_type = V4L2_MBUS_CSI2_DPHY,
> >  	};
> >  	struct v4l2_async_connection *asd;
> uuu> -	struct fwnode_handle *ep;
> >  	unsigned int i;
> >  	int ret;
> >  
> >  	v4l2_async_subdev_nf_init(&csis->notifier, &csis->sd);
> >  
> > -	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(csis->dev), 0, 0,
> > -					     FWNODE_GRAPH_ENDPOINT_NEXT);
> > +	struct fwnode_handle *ep __free(fwnode_handle) =
> > +		fwnode_graph_get_endpoint_by_id(dev_fwnode(csis->dev), 0, 0,
> > +						FWNODE_GRAPH_ENDPOINT_NEXT);
> 
> Let's avoid mixing variable declarations and code, this is a style
> change that is not widely accepted (yet). You can write
> 
> 	struct fwnode_handle *ep __free(fwnode_handle) = NULL;
>  	struct v4l2_async_connection *asd;
> 	unsigned int i;
> 	int ret;
> 
>   	v4l2_async_subdev_nf_init(&csis->notifier, &csis->sd);
> 
> 	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(csis->dev), 0, 0,
> 					     FWNODE_GRAPH_ENDPOINT_NEXT);

Quite a coincidence, there's an ongoing mail thread on LKML about this
topic:

http://lore.kernel.org/r/CAHk-=whPZoi03ZwphxiW6cuWPtC3nyKYS8_BThgztCdgPWP1WA@mail.gmail.com

> >  	if (!ep)
> >  		return -ENOTCONN;
> >  
> >  	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> >  	if (ret)
> > -		goto err_parse;
> > +		return ret;
> >  
> >  	for (i = 0; i < vep.bus.mipi_csi2.num_data_lanes; ++i) {
> > -		if (vep.bus.mipi_csi2.data_lanes[i] != i + 1) {
> > -			dev_err(csis->dev,
> > -				"data lanes reordering is not supported");
> > -			ret = -EINVAL;
> > -			goto err_parse;
> > -		}
> > +		if (vep.bus.mipi_csi2.data_lanes[i] != i + 1)
> > +			return dev_err_probe(csis->dev, -EINVAL,
> > +					     "data lanes reordering is not supported\n");
> 
> To switch to dev_err_probe(), we should drop the error message in the
> probe() function when mipi_csis_async_register() fails, and make sure
> every error path in this function prints a message. I'd prefer splitting
> that to a separate patch.
> 
> >  	}
> >  
> >  	csis->bus = vep.bus.mipi_csi2;
> > @@ -1381,12 +1379,8 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
> >  
> >  	asd = v4l2_async_nf_add_fwnode_remote(&csis->notifier, ep,
> >  					      struct v4l2_async_connection);
> > -	if (IS_ERR(asd)) {
> > -		ret = PTR_ERR(asd);
> > -		goto err_parse;
> > -	}
> > -
> > -	fwnode_handle_put(ep);
> > +	if (IS_ERR(asd))
> > +		return PTR_ERR(asd);
> >  
> >  	csis->notifier.ops = &mipi_csis_notify_ops;
> >  
> > @@ -1395,11 +1389,6 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
> >  		return ret;
> >  
> >  	return v4l2_async_register_subdev(&csis->sd);
> > -
> > -err_parse:
> > -	fwnode_handle_put(ep);
> > -
> > -	return ret;
> >  }
> >  
> >  /* -----------------------------------------------------------------------------
> > diff --git a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c
> > index 75709161fb26a61239b94430365849e022fdc14f..94882568405db55593c5c51722db2233a64d53e4 100644
> > --- a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c
> > +++ b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c
> > @@ -6,6 +6,7 @@
> >   */
> >  
> >  #include <linux/bitfield.h>
> > +#include <linux/cleanup.h>
> >  #include <linux/clk.h>
> >  #include <linux/clk-provider.h>
> >  #include <linux/delay.h>
> > @@ -717,28 +718,25 @@ static int imx8mq_mipi_csi_async_register(struct csi_state *state)
> >  		.bus_type = V4L2_MBUS_CSI2_DPHY,
> >  	};
> >  	struct v4l2_async_connection *asd;
> > -	struct fwnode_handle *ep;
> >  	unsigned int i;
> >  	int ret;
> >  
> >  	v4l2_async_subdev_nf_init(&state->notifier, &state->sd);
> >  
> > -	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(state->dev), 0, 0,
> > -					     FWNODE_GRAPH_ENDPOINT_NEXT);
> > +	struct fwnode_handle *ep __free(fwnode_handle) =
> > +		fwnode_graph_get_endpoint_by_id(dev_fwnode(state->dev), 0, 0,
> > +						FWNODE_GRAPH_ENDPOINT_NEXT);
> 
> Same comment as above.
> 
> >  	if (!ep)
> >  		return -ENOTCONN;
> >  
> >  	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> >  	if (ret)
> > -		goto err_parse;
> > +		return ret;
> >  
> >  	for (i = 0; i < vep.bus.mipi_csi2.num_data_lanes; ++i) {
> > -		if (vep.bus.mipi_csi2.data_lanes[i] != i + 1) {
> > -			dev_err(state->dev,
> > -				"data lanes reordering is not supported");
> > -			ret = -EINVAL;
> > -			goto err_parse;
> > -		}
> > +		if (vep.bus.mipi_csi2.data_lanes[i] != i + 1)
> > +			return dev_err_probe(state->dev, -EINVAL,
> > +					     "data lanes reordering is not supported\n");
> 
> And here too.
> 
> Usage of __free(fwnode_handle) looks good, it just needs to be split to
> a patch of its own.
> 
> >  	}
> >  
> >  	state->bus = vep.bus.mipi_csi2;
> > @@ -749,12 +747,8 @@ static int imx8mq_mipi_csi_async_register(struct csi_state *state)
> >  
> >  	asd = v4l2_async_nf_add_fwnode_remote(&state->notifier, ep,
> >  					      struct v4l2_async_connection);
> > -	if (IS_ERR(asd)) {
> > -		ret = PTR_ERR(asd);
> > -		goto err_parse;
> > -	}
> > -
> > -	fwnode_handle_put(ep);
> > +	if (IS_ERR(asd))
> > +		return PTR_ERR(asd);
> >  
> >  	state->notifier.ops = &imx8mq_mipi_csi_notify_ops;
> >  
> > @@ -763,11 +757,6 @@ static int imx8mq_mipi_csi_async_register(struct csi_state *state)
> >  		return ret;
> >  
> >  	return v4l2_async_register_subdev(&state->sd);
> > -
> > -err_parse:
> > -	fwnode_handle_put(ep);
> > -
> > -	return ret;
> >  }
> >  
> >  /* -----------------------------------------------------------------------------

-- 
Regards,

Laurent Pinchart

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.