Re: [PATCH 1/5] of/device: Add of_ functions for device_link_{add,remove}

[Benjamin GAIGNARD]

On 4/24/19 3:06 PM, Rafael J. Wysocki wrote:
> On 4/24/2019 12:19 PM, Benjamin Gaignard wrote:
>> Allows to create and remove links between consumer and suppliers from
>> device-tree data. Use 'links-add' property from consumer node to setup
>> a link with a list of suppliers.
>
> One immediate question about this one is why stateless links are 
> better here?
They aren't better, it is just because I wanted to keep one of_ function 
for each related device_link_* functions.
>
>> Consumers will be suspend before their suppliers and resume after them.
>>
>> Add devm_of_device_links_add() to automatically remove the links
>> when the device is unbound from the bus.
>
> And this might not be necessary even with managed links.

I guess I could use DL_FLAG_PM_RUNTIME and DL_FLAG_AUTOREMOVE_CONSUMER 
flags and just keep

of_device_links_add() but the naming will not be explicit.

>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxx>
>> ---
>> Â drivers/of/device.cÂÂÂÂÂÂ | 103 
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> Â include/linux/of_device.h |Â 20 +++++++++
>> Â 2 files changed, 123 insertions(+)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index 3717f2a20d0d..011ba9bf7642 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -336,3 +336,106 @@ int of_device_uevent_modalias(struct device 
>> *dev, struct kobj_uevent_env *env)
>> ÂÂÂÂÂ return 0;
>> Â }
>> Â EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
>> +
>> +/**
>> + * of_device_links_add - Create links between consumer and suppliers 
>> from
>> + * device tree data
>> + *
>> + * @consumer: consumer device
>> + *
>> + * Returns 0 on success, < 0 on failure.
>> + */
>> +int of_device_links_add(struct device *consumer)
>> +{
>> +ÂÂÂ struct device_node *np;
>> +ÂÂÂ struct platform_device *pdev;
>> +ÂÂÂ int i = 0;
>> +
>> +ÂÂÂ np = of_parse_phandle(consumer->of_node, "links-add", i++);
>> +ÂÂÂ while (np) {
>> +ÂÂÂÂÂÂÂ pdev = of_find_device_by_node(np);
>> +ÂÂÂÂÂÂÂ of_node_put(np);
>> +ÂÂÂÂÂÂÂ if (!pdev)
>> +ÂÂÂÂÂÂÂÂÂÂÂ return -EINVAL;
>> +
>> +ÂÂÂÂÂÂÂ device_link_add(consumer, &pdev->dev, DL_FLAG_STATELESS);
>> +ÂÂÂÂÂÂÂ platform_device_put(pdev);
>> +
>> +ÂÂÂÂÂÂÂ np = of_parse_phandle(consumer->of_node, "links-add", i++);
>> +ÂÂÂ }
>> +
>> +ÂÂÂ return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_device_links_add);
>> +
>> +/**
>> + * of_device_links_remove - Remove links between consumer and 
>> suppliers from
>> + * device tree data
>> + *
>> + * @consumer: consumer device
>> + *
>> + * Returns 0 on success, < 0 on failure.
>> + */
>> +int of_device_links_remove(struct device *consumer)
>> +{
>> +ÂÂÂ struct device_node *np;
>> +ÂÂÂ struct platform_device *pdev;
>> +ÂÂÂ int i = 0;
>> +
>> +ÂÂÂ np = of_parse_phandle(consumer->of_node, "links-add", i++);
>> +ÂÂÂ while (np) {
>> +ÂÂÂÂÂÂÂ pdev = of_find_device_by_node(np);
>> +ÂÂÂÂÂÂÂ of_node_put(np);
>> +ÂÂÂÂÂÂÂ if (!pdev)
>> +ÂÂÂÂÂÂÂÂÂÂÂ return -EINVAL;
>> +
>> +ÂÂÂÂÂÂÂ device_link_remove(consumer, &pdev->dev);
>> +ÂÂÂÂÂÂÂ platform_device_put(pdev);
>> +
>> +ÂÂÂÂÂÂÂ np = of_parse_phandle(consumer->of_node, "links-add", i++);
>> +ÂÂÂ }
>> +
>> +ÂÂÂ return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_device_links_remove);
>> +
>> +static void devm_of_device_links_remove(struct device *dev, void *res)
>> +{
>> +ÂÂÂ of_device_links_remove(*(struct device **)res);
>> +}
>> +
>> +/**
>> + * devm_of_device_links_add - Create links between consumer and 
>> suppliers
>> + * from device tree data
>> + *
>> + * @consumer: consumer device
>> + *
>> + * Returns 0 on success, < 0 on failure.
>> + *
>> + * Similar to of_device_links_add(), but will automatically call
>> + * of_device_links_remove() when the device is unbound from the bus.
>> + */
>> +int devm_of_device_links_add(struct device *consumer)
>> +{
>> +ÂÂÂÂÂÂÂ struct device **ptr;
>> +ÂÂÂ int ret;
>> +
>> +ÂÂÂ if (!consumer)
>> +ÂÂÂÂÂÂÂ return -EINVAL;
>> +
>> +ÂÂÂ ptr = devres_alloc(devm_of_device_links_remove,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sizeof(*ptr), GFP_KERNEL);
>> +ÂÂÂ if (!ptr)
>> +ÂÂÂÂÂÂÂ return -ENOMEM;
>> +
>> +ÂÂÂ ret = of_device_links_add(consumer);
>> +ÂÂÂ if (ret < 0) {
>> +ÂÂÂÂÂÂÂ devres_free(ptr);
>> +ÂÂÂ } else {
>> +ÂÂÂÂÂÂÂ *ptr = consumer;
>> +ÂÂÂÂÂÂÂ devres_add(consumer, ptr);
>> +ÂÂÂ }
>> +
>> +ÂÂÂ return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_of_device_links_add);
>> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
>> index 8d31e39dd564..ad01db6828e8 100644
>> --- a/include/linux/of_device.h
>> +++ b/include/linux/of_device.h
>> @@ -41,6 +41,11 @@ extern int of_device_request_module(struct device 
>> *dev);
>> Â extern void of_device_uevent(struct device *dev, struct 
>> kobj_uevent_env *env);
>> Â extern int of_device_uevent_modalias(struct device *dev, struct 
>> kobj_uevent_env *env);
>> Â +
>> +extern int of_device_links_add(struct device *consumer);
>> +extern int of_device_links_remove(struct device *consumer);
>> +extern int devm_of_device_links_add(struct device *consumer);
>> +
>> Â static inline void of_device_node_put(struct device *dev)
>> Â {
>> ÂÂÂÂÂ of_node_put(dev->of_node);
>> @@ -91,6 +96,21 @@ static inline int of_device_uevent_modalias(struct 
>> device *dev,
>> ÂÂÂÂÂ return -ENODEV;
>> Â }
>> Â +static int of_device_links_add(struct device *consumer)
>> +{
>> +ÂÂÂ return 0;
>> +}
>> +
>> +static int of_device_links_remove(struct device *consumer)
>> +{
>> +ÂÂÂ return 0;
>> +}
>> +
>> +static int devm_of_device_links_add(struct device *consumer)
>> +{
>> +ÂÂÂ return 0;
>> +}
>> +
>> Â static inline void of_device_node_put(struct device *dev) { }
>> Â Â static inline const struct of_device_id *__of_match_device(
>
>