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); >> return ret; >> - >> - 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. -- Regards, Sylwester