diff mbox series

[v3,4/9] media: platform: microchip: use for_each_endpoint_of_node()

Message ID 87fru0oxyq.wl-kuninori.morimoto.gx@renesas.com
State Superseded
Headers show
Series use for_each_endpoint_of_node() | expand

Commit Message

Kuninori Morimoto May 30, 2024, 2:06 a.m. UTC 
We already have for_each_endpoint_of_node(), don't use
of_graph_get_next_endpoint() directly. Replace it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../microchip/microchip-sama5d2-isc.c         | 21 +++++++------------
 .../microchip/microchip-sama7g5-isc.c         | 21 +++++++------------
 2 files changed, 16 insertions(+), 26 deletions(-)

Comments

Kuninori Morimoto May 30, 2024, 11:52 p.m. UTC | #1 
Hi Dan, Rob, Helge

> > -	while (1) {
> > +	for_each_endpoint_of_node(np, epn) {
> >  		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
> > -
> > -		epn = of_graph_get_next_endpoint(np, epn);
> > -		if (!epn)
> > -			return 0;
> > +		int ret;
> >  
> >  		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
> >  						 &v4l2_epn);
> >  		if (ret) {
> > -			ret = -EINVAL;
> > +			of_node_put(epn);
> >  			dev_err(dev, "Could not parse the endpoint\n");
> > -			break;
> > +			return -EINVAL;
> >  		}
> >  
> >  		subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
> >  					     GFP_KERNEL);
> >  		if (!subdev_entity) {
> > -			ret = -ENOMEM;
> > -			break;
> > +			of_node_put(epn);
> > +			return -ENOMEM;
> >  		}
> >  		subdev_entity->epn = epn;
> 
> This code is an example of what Laurent was talking about.  We're taking
> storing "subdev_entity->epn = epn" but then not incrementing the
> refcount.  Perhaps it's not necessary?
> 
> The difference between this and _scoped() would be if we stored epn and
> then returned.  I feel like that's less common.  We could detect that
> sort of thing using static analysis if it turned out to be an issue.

Thank you for pointing good sample, Dan.

But I wonder should this patch-set include and use _scoped() macro ?
If above is just a comment, _scoped() macro will be separate patch-set.
If above is pointing the issue, v4 need to have _scoped() macro.

Thank you for your help !!
Best regards
---
Kuninori Morimoto
diff mbox series

Patch

diff --git a/drivers/media/platform/microchip/microchip-sama5d2-isc.c b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
index 5ac149cf3647f..60b6d922d764e 100644
--- a/drivers/media/platform/microchip/microchip-sama5d2-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
@@ -353,33 +353,29 @@  static const u32 isc_sama5d2_gamma_table[][GAMMA_ENTRIES] = {
 static int isc_parse_dt(struct device *dev, struct isc_device *isc)
 {
 	struct device_node *np = dev->of_node;
-	struct device_node *epn = NULL;
+	struct device_node *epn;
 	struct isc_subdev_entity *subdev_entity;
 	unsigned int flags;
-	int ret;
 
 	INIT_LIST_HEAD(&isc->subdev_entities);
 
-	while (1) {
+	for_each_endpoint_of_node(np, epn) {
 		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
-
-		epn = of_graph_get_next_endpoint(np, epn);
-		if (!epn)
-			return 0;
+		int ret;
 
 		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
 						 &v4l2_epn);
 		if (ret) {
-			ret = -EINVAL;
+			of_node_put(epn);
 			dev_err(dev, "Could not parse the endpoint\n");
-			break;
+			return -EINVAL;
 		}
 
 		subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
 					     GFP_KERNEL);
 		if (!subdev_entity) {
-			ret = -ENOMEM;
-			break;
+			of_node_put(epn);
+			return -ENOMEM;
 		}
 		subdev_entity->epn = epn;
 
@@ -400,9 +396,8 @@  static int isc_parse_dt(struct device *dev, struct isc_device *isc)
 
 		list_add_tail(&subdev_entity->list, &isc->subdev_entities);
 	}
-	of_node_put(epn);
 
-	return ret;
+	return 0;
 }
 
 static int microchip_isc_probe(struct platform_device *pdev)
diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
index 73445f33d26ba..e97abe3e35af0 100644
--- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
@@ -336,36 +336,32 @@  static const u32 isc_sama7g5_gamma_table[][GAMMA_ENTRIES] = {
 static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
 {
 	struct device_node *np = dev->of_node;
-	struct device_node *epn = NULL;
+	struct device_node *epn;
 	struct isc_subdev_entity *subdev_entity;
 	unsigned int flags;
-	int ret;
 	bool mipi_mode;
 
 	INIT_LIST_HEAD(&isc->subdev_entities);
 
 	mipi_mode = of_property_read_bool(np, "microchip,mipi-mode");
 
-	while (1) {
+	for_each_endpoint_of_node(np, epn) {
 		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
-
-		epn = of_graph_get_next_endpoint(np, epn);
-		if (!epn)
-			return 0;
+		int ret;
 
 		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
 						 &v4l2_epn);
 		if (ret) {
-			ret = -EINVAL;
+			of_node_put(epn);
 			dev_err(dev, "Could not parse the endpoint\n");
-			break;
+			return -EINVAL;
 		}
 
 		subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
 					     GFP_KERNEL);
 		if (!subdev_entity) {
-			ret = -ENOMEM;
-			break;
+			of_node_put(epn);
+			return -ENOMEM;
 		}
 		subdev_entity->epn = epn;
 
@@ -389,9 +385,8 @@  static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
 
 		list_add_tail(&subdev_entity->list, &isc->subdev_entities);
 	}
-	of_node_put(epn);
 
-	return ret;
+	return 0;
 }
 
 static int microchip_xisc_probe(struct platform_device *pdev)