[PATCH] regulator: core: Don't link consumers on the same device

[PATCH] regulator: core: Don't link consumers on the same device

[Mark Brown]

In some cases a device may end up supplying itself, for example when a
DCDC is used to improve the efficiency of LDOs or when a LDO is provided
to clean up sensitive supplies.  In these cases the driver core will
warn loudly about attempts to add links so suppress them here.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---

Compile tested only.

 drivers/regulator/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b5db0257bc40..1302e66e80ac 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1741,7 +1741,8 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 			rdev->use_count = 0;
 	}
 
-	device_link_add(dev, &rdev->dev, DL_FLAG_STATELESS);
+	if (dev != &rdev->dev)
+		device_link_add(dev, &rdev->dev, DL_FLAG_STATELESS);
 
 	return regulator;
 }
@@ -1840,7 +1841,7 @@ static void _regulator_put(struct regulator *regulator)
 			if (r->dev == regulator->dev)
 				count++;
 
-		if (count == 1)
+		if (count == 1 && regulator->dev != &rdev->dev)
 			device_link_remove(regulator->dev, &rdev->dev);
 
 		/* remove any sysfs entries */
-- 
2.18.0.rc2

Re: [PATCH] regulator: core: Don't link consumers on the same device

[Marek Szyprowski]

Hi Mark,

On 2018-07-09 14:54, Mark Brown wrote:
> In some cases a device may end up supplying itself, for example when a
> DCDC is used to improve the efficiency of LDOs or when a LDO is provided
> to clean up sensitive supplies.  In these cases the driver core will
> warn loudly about attempts to add links so suppress them here.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>
> Compile tested only.

This patch doesn't help, because wm8994 creates separate struct device for
each registered regulator, so you need to check the regulator parents too,
see my fixes below in the patch.

I don't like such fixes for this specific case. It's not that uncommon
that a complex device provides various functionalities used by its
components, so it is not a bad design to use regulator provided by
its parent/grandparent/other ancestor device.

Frankly, either the device_is_dependent() function should be exported and
used in regulator core or device_link_add() should have another flag:
DL_FLAG_SKIP_DEPENDANT.

>   drivers/regulator/core.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index b5db0257bc40..1302e66e80ac 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1741,7 +1741,8 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
>   			rdev->use_count = 0;
>   	}
>   
> -	device_link_add(dev, &rdev->dev, DL_FLAG_STATELESS);
> +	if (dev != &rdev->dev)

    if (dev != &rdev->dev && dev != rdev->dev.parent)

> +		device_link_add(dev, &rdev->dev, DL_FLAG_STATELESS);
>   
>   	return regulator;
>   }
> @@ -1840,7 +1841,7 @@ static void _regulator_put(struct regulator *regulator)
>   			if (r->dev == regulator->dev)
>   				count++;
>   
> -		if (count == 1)
> +		if (count == 1 && regulator->dev != &rdev->dev)

    if (count == 1 && regulator->dev != &rdev->dev &&
                     regulator->dev != rdev->dev.parent)

>   			device_link_remove(regulator->dev, &rdev->dev);
>   
>   		/* remove any sysfs entries */

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] regulator: core: Don't link consumers on the same device

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]

On Mon, Jul 09, 2018 at 03:32:14PM +0200, Marek Szyprowski wrote:

> This patch doesn't help, because wm8994 creates separate struct device for
> each registered regulator, so you need to check the regulator parents too,
> see my fixes below in the patch.

Ugh, right - I'd not looked at enough of the context to see what was
being checked.

> I don't like such fixes for this specific case. It's not that uncommon
> that a complex device provides various functionalities used by its
> components, so it is not a bad design to use regulator provided by
> its parent/grandparent/other ancestor device.

> Frankly, either the device_is_dependent() function should be exported and
> used in regulator core or device_link_add() should have another flag:
> DL_FLAG_SKIP_DEPENDANT.

Or it should just make the warning optional at the core level.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]