| Message ID | 91e37a12-b393-8ae9-996b-6cbb63ea9255@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | opp: Don't print error message if getting optional regulator fails | expand |
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;
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; >
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.
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;
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(-)