Re: [RFC PATCH v5 2/6] interconnect: Add generic interconnect driver for Exynos SoCs

[Sylwester Nawrocki]

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