[PATCH] opp: Don't print error message if getting optional regulator fails

[PATCH] opp: Don't print error message if getting optional regulator fails

[Heiner Kallweit]

The regulators are optional, therefore I see no need to bother users
with an error level message if -ENODEV is returned.

Inspiration was the following error on my system:
lima d00c0000.gpu: dev_pm_opp_set_regulators: no regulator (mali) found: -19

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/opp/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 740407252..8af5979cc 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2020,7 +2020,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
 		reg = regulator_get_optional(dev, names[i]);
 		if (IS_ERR(reg)) {
 			ret = PTR_ERR(reg);
-			if (ret != -EPROBE_DEFER)
+			if (ret == -ENODEV)
+				dev_info(dev, "%s: no regulator (%s) found\n",
+					 __func__, names[i]);
+			else if (ret != -EPROBE_DEFER)
 				dev_err(dev, "%s: no regulator (%s) found: %d\n",
 					__func__, names[i], ret);
 			goto free_regulators;
-- 
2.35.1

Re: [PATCH] opp: Don't print error message if getting optional regulator fails

[Viresh Kumar]

On 06-03-22, 22:46, Heiner Kallweit wrote:
> The regulators are optional, therefore I see no need to bother users
> with an error level message if -ENODEV is returned.
> 
> Inspiration was the following error on my system:
> lima d00c0000.gpu: dev_pm_opp_set_regulators: no regulator (mali) found: -19
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/opp/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 740407252..8af5979cc 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -2020,7 +2020,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,

If this API is called by the platform specific code, then the
regulator should be compulsory. Isn't it ?

Maybe we shouldn't use regulator_get_optional() here.

>  		reg = regulator_get_optional(dev, names[i]);
>  		if (IS_ERR(reg)) {
>  			ret = PTR_ERR(reg);
> -			if (ret != -EPROBE_DEFER)
> +			if (ret == -ENODEV)
> +				dev_info(dev, "%s: no regulator (%s) found\n",
> +					 __func__, names[i]);
> +			else if (ret != -EPROBE_DEFER)
>  				dev_err(dev, "%s: no regulator (%s) found: %d\n",
>  					__func__, names[i], ret);
>  			goto free_regulators;

-- 
viresh

Re: [PATCH] opp: Don't print error message if getting optional regulator fails

[Heiner Kallweit]

On 07.03.2022 06:27, Viresh Kumar wrote:
> On 06-03-22, 22:46, Heiner Kallweit wrote:
>> The regulators are optional, therefore I see no need to bother users
>> with an error level message if -ENODEV is returned.
>>
>> Inspiration was the following error on my system:
>> lima d00c0000.gpu: dev_pm_opp_set_regulators: no regulator (mali) found: -19
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/opp/core.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 740407252..8af5979cc 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -2020,7 +2020,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
> 
> If this API is called by the platform specific code, then the
> regulator should be compulsory. Isn't it ?
> 
> Maybe we shouldn't use regulator_get_optional() here.
> 
There are two types of users of dev(m)_pm_opp_set_regulators():
1. Caller ignores -ENODEV and goes on.
2. Caller treats -ENODEV like any other error and bails out.

For type 1 callers a missing regulator should not result in an error message.
Switching to regulator_get() wouldn't be too nice because then a missing
regulator would result in the dummy regulator being used. This would create
a related warning and a functional change that may break something.

Type 2 callers still have the option to print an own error message in
case of -ENODEV. AFAICS all of them do so already.

>>  		reg = regulator_get_optional(dev, names[i]);
>>  		if (IS_ERR(reg)) {
>>  			ret = PTR_ERR(reg);
>> -			if (ret != -EPROBE_DEFER)
>> +			if (ret == -ENODEV)
>> +				dev_info(dev, "%s: no regulator (%s) found\n",
>> +					 __func__, names[i]);
>> +			else if (ret != -EPROBE_DEFER)
>>  				dev_err(dev, "%s: no regulator (%s) found: %d\n",
>>  					__func__, names[i], ret);
>>  			goto free_regulators;
> 

Re: [PATCH] opp: Don't print error message if getting optional regulator fails

[Viresh Kumar]

On 07-03-22, 18:18, Heiner Kallweit wrote:
> There are two types of users of dev(m)_pm_opp_set_regulators():
> 1. Caller ignores -ENODEV and goes on.

This is abuse of the API as far as I can tell. If the OPP core was
doing something like this automatically for everyone, then we can
agree that missing regulator isn't necessarily a problem.

But in this case the platforms (lima and panfrost) are asking
explicitly to get the regulators added. To me this is a plain and
simple error.

Take cpufreq-dt for example, it makes sure beforehand if the supply is
available or not, and then only calls this API.

> For type 1 callers a missing regulator should not result in an error message.
> Switching to regulator_get() wouldn't be too nice because then a missing
> regulator would result in the dummy regulator being used. This would create
> a related warning and a functional change that may break something.
> 
> Type 2 callers still have the option to print an own error message in
> case of -ENODEV. AFAICS all of them do so already.

-- 
viresh