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

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@st.com>
>> ---
>>   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(
>
>

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@st.com>
>> ---
>>   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(
>
>