On 02.07.2020 14:33, Georgi Djakov wrote:
On 7/2/20 15:01, Sylwester Nawrocki wrote:
On 01.07.2020 14:50, Georgi Djakov wrote:
On 5/29/20 19:31, Sylwester Nawrocki wrote:
+static int exynos_generic_icc_remove(struct platform_device *pdev) +{
- struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
- struct icc_node *parent_node, *node = priv->node;
- parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
- if (parent_node && !IS_ERR(parent_node))
Nit: !IS_ERR_OR_NULL?
It was left on purpose that way but I changed it now to IS_ERR_OR_NULL.
Well, i have no strong opinion on that, it's up to you.
I will leave it as you suggested, otherwise we are likely to see "clean up" patches sooner or later.
icc_link_destroy(node, parent_node);- icc_nodes_remove(&priv->provider);
- icc_provider_del(&priv->provider);
- return 0;
+}
+static int exynos_generic_icc_probe(struct platform_device *pdev) +{
- struct device *bus_dev = pdev->dev.parent;
- struct exynos_icc_priv *priv;
- struct icc_provider *provider;
- struct icc_node *icc_node, *icc_parent_node;
- int ret;
- priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return -ENOMEM;- priv->dev = &pdev->dev;
- platform_set_drvdata(pdev, priv);
- provider = &priv->provider;
- provider->set = exynos_generic_icc_set;
- provider->aggregate = icc_std_aggregate;
- provider->xlate = exynos_generic_icc_xlate;
- provider->dev = bus_dev;
- provider->inter_set = true;
- provider->data = priv;
- ret = icc_provider_add(provider);
Nit: Maybe it would be better to move this after the node is created. The idea is to create the nodes first and add the provider when the topology is populated. It's fine either way here, but i am planning to change this in some of the existing provider drivers.
OK, it makes the clean up path a bit less straightforward. And still we need to register the provider before calling icc_node_add(). I made a change as below.
--------------8<------------------ @@ -124,14 +123,14 @@ static int exynos_generic_icc_probe(struct platform_device *pdev) provider->inter_set = true; provider->data = priv;
- icc_node = icc_node_create(pdev->id);
- if (IS_ERR(icc_node))
return PTR_ERR(icc_node);- ret = icc_provider_add(provider);
- if (ret < 0)
- if (ret < 0) {
icc_node_destroy(icc_node->id);
icc_node = icc_node_create(pdev->id);
if (IS_ERR(icc_node)) {
ret = PTR_ERR(icc_node);goto err_prov_del;}
priv->node = icc_node;
@@ -171,9 +170,7 @@ static int exynos_generic_icc_probe(struct platform_device *pdev) icc_link_destroy(icc_node, icc_parent_node); err_node_del: icc_nodes_remove(provider); -err_prov_del: icc_provider_del(provider);
- return ret;
} --------------8<------------------
Actually i need to post some patches first, so maybe keep it as is for now and we will update it afterwards. Sorry for the confusion.
OK, anyway this helped me to remove a memory leak, which was there since icc_nodes_remove() was used before a call to icc_node_add() in order to free the node allocated with icc_node_create(), instead of icc_node_destroy().
All looks good to me, but it seems that the patch-set is not on Rob's radar currently, so please re-send and CC the DT mailing list.
Thanks, indeed I missed some mailing list when posting. I will make sure Rob and DT ML list is on Cc, especially that I have added new "bus-width" property in v6.
Ok, good. I have been thinking about bus-width and we might want to make it even a generic DT property if there are multiple platforms which want to use it - maybe if the bus-width is the same across the whole interconnect provider. But as most of the existing drivers have different bus-widths, i haven't done it yet, but let's see and start a discussion.
I see, it sounds good to me. Having an array property to allow specifying bus width for each node is probably not so good idea.