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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
