Re: [PATCH v3 4/9] media: platform: microchip: use for_each_endpoint_of_node()
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),
> > _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
Re: [PATCH v3 4/9] media: platform: microchip: use for_each_endpoint_of_node()
On Thu, May 30, 2024 at 02:06:05AM +, Kuninori Morimoto wrote:
> 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(>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),
>_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.
regards,
dan carpenter
[PATCH v3 4/9] media: platform: microchip: use for_each_endpoint_of_node()
We already have for_each_endpoint_of_node(), don't use
of_graph_get_next_endpoint() directly. Replace it.
Signed-off-by: Kuninori Morimoto
Reviewed-by: Laurent Pinchart
---
.../microchip/microchip-sama5d2-isc.c | 21 +++
.../microchip/microchip-sama7g5-isc.c | 21 +++
2 files changed, 16 insertions(+), 26 deletions(-)
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(>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),
_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(_entity->list, >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(>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),
_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(_entity->list, >subdev_entities);
}
- of_node_put(epn);
- return ret;
+ return 0;
}
static int microchip_xisc_probe(struct platform_device *pdev)
--
2.43.0
