* [PATCH] cpufreq: qcom-hw: Use optional irq API
@ 2021-11-17 2:03 Stephen Boyd
2021-11-18 2:55 ` Thara Gopinath
2021-12-03 4:47 ` Viresh Kumar
0 siblings, 2 replies; 7+ messages in thread
From: Stephen Boyd @ 2021-11-17 2:03 UTC (permalink / raw)
To: Viresh Kumar, Rafael J . Wysocki
Cc: linux-kernel, linux-arm-msm, linux-pm, Thara Gopinath
Use platform_get_irq_optional() to avoid a noisy error message when the
irq isn't specified. The irq is definitely optional given that we only
care about errors that are -EPROBE_DEFER here.
Cc: Thara Gopinath <thara.gopinath@linaro.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
drivers/cpufreq/qcom-cpufreq-hw.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index a2be0df7e174..b442d4983a22 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -382,9 +382,11 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
* Look for LMh interrupt. If no interrupt line is specified /
* if there is an error, allow cpufreq to be enabled as usual.
*/
- data->throttle_irq = platform_get_irq(pdev, index);
- if (data->throttle_irq <= 0)
- return data->throttle_irq == -EPROBE_DEFER ? -EPROBE_DEFER : 0;
+ data->throttle_irq = platform_get_irq_optional(pdev, index);
+ if (data->throttle_irq == -ENXIO)
+ return 0;
+ if (data->throttle_irq < 0)
+ return data->throttle_irq;
data->cancel_throttle = false;
data->policy = policy;
base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
--
https://chromeos.dev
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] cpufreq: qcom-hw: Use optional irq API
2021-11-17 2:03 [PATCH] cpufreq: qcom-hw: Use optional irq API Stephen Boyd
@ 2021-11-18 2:55 ` Thara Gopinath
2021-11-18 4:32 ` Stephen Boyd
2021-12-03 4:47 ` Viresh Kumar
1 sibling, 1 reply; 7+ messages in thread
From: Thara Gopinath @ 2021-11-18 2:55 UTC (permalink / raw)
To: Stephen Boyd, Viresh Kumar, Rafael J . Wysocki
Cc: linux-kernel, linux-arm-msm, linux-pm
Hello Stephen,
Thanks for the patch
On 11/16/21 9:03 PM, Stephen Boyd wrote:
> Use platform_get_irq_optional() to avoid a noisy error message when the
> irq isn't specified. The irq is definitely optional given that we only
> care about errors that are -EPROBE_DEFER here.
>
> Cc: Thara Gopinath <thara.gopinath@linaro.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> drivers/cpufreq/qcom-cpufreq-hw.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index a2be0df7e174..b442d4983a22 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -382,9 +382,11 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
> * Look for LMh interrupt. If no interrupt line is specified /
> * if there is an error, allow cpufreq to be enabled as usual.
> */
> - data->throttle_irq = platform_get_irq(pdev, index);
> - if (data->throttle_irq <= 0)
> - return data->throttle_irq == -EPROBE_DEFER ? -EPROBE_DEFER : 0;
> + data->throttle_irq = platform_get_irq_optional(pdev, index);
> + if (data->throttle_irq == -ENXIO)
> + return 0;
> + if (data->throttle_irq < 0)
> + return data->throttle_irq;
Here the idea is to return only -EPROBE_DEFER error. Else return a 0 ,
so that cpufreq is enabled even if lmh interrupt is inaccessible. The
above check returns errors other than -EPROBE_DEFER as well. So I would
say make irq optional and keep the below check
if (data->throttle_irq <= 0)
return data->throttle_irq == -EPROBE_DEFER ? -EPROBE_DEFER : 0;
>
> data->cancel_throttle = false;
> data->policy = policy;
>
> base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
>
--
Warm Regards
Thara (She/Her/Hers)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cpufreq: qcom-hw: Use optional irq API
2021-11-18 2:55 ` Thara Gopinath
@ 2021-11-18 4:32 ` Stephen Boyd
2021-11-18 14:08 ` Thara Gopinath
0 siblings, 1 reply; 7+ messages in thread
From: Stephen Boyd @ 2021-11-18 4:32 UTC (permalink / raw)
To: Rafael J . Wysocki, Thara Gopinath, Viresh Kumar
Cc: linux-kernel, linux-arm-msm, linux-pm
Quoting Thara Gopinath (2021-11-17 18:55:17)
> Hello Stephen,
>
> Thanks for the patch
>
> On 11/16/21 9:03 PM, Stephen Boyd wrote:
> > Use platform_get_irq_optional() to avoid a noisy error message when the
> > irq isn't specified. The irq is definitely optional given that we only
> > care about errors that are -EPROBE_DEFER here.
> >
> > Cc: Thara Gopinath <thara.gopinath@linaro.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> > drivers/cpufreq/qcom-cpufreq-hw.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > index a2be0df7e174..b442d4983a22 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > @@ -382,9 +382,11 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
> > * Look for LMh interrupt. If no interrupt line is specified /
> > * if there is an error, allow cpufreq to be enabled as usual.
> > */
> > - data->throttle_irq = platform_get_irq(pdev, index);
> > - if (data->throttle_irq <= 0)
> > - return data->throttle_irq == -EPROBE_DEFER ? -EPROBE_DEFER : 0;
> > + data->throttle_irq = platform_get_irq_optional(pdev, index);
> > + if (data->throttle_irq == -ENXIO)
> > + return 0;
> > + if (data->throttle_irq < 0)
> > + return data->throttle_irq;
>
> Here the idea is to return only -EPROBE_DEFER error. Else return a 0 ,
> so that cpufreq is enabled even if lmh interrupt is inaccessible. The
> above check returns errors other than -EPROBE_DEFER as well. So I would
> say make irq optional and keep the below check
>
> if (data->throttle_irq <= 0)
> return data->throttle_irq == -EPROBE_DEFER ? -EPROBE_DEFER : 0;
I'd like to catch other errors, for example, DT has an irq specified
that is outside the range of irqs available. If the DT is correct, then
it will either have a valid irq and this will return a >= 0 value or
nothing will be specified and we'll get back -ENXIO now. Do you have
some scenario where my patch fails to work?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cpufreq: qcom-hw: Use optional irq API
2021-11-18 4:32 ` Stephen Boyd
@ 2021-11-18 14:08 ` Thara Gopinath
2021-11-19 0:57 ` Stephen Boyd
0 siblings, 1 reply; 7+ messages in thread
From: Thara Gopinath @ 2021-11-18 14:08 UTC (permalink / raw)
To: Stephen Boyd, Rafael J . Wysocki, Viresh Kumar
Cc: linux-kernel, linux-arm-msm, linux-pm
On 11/17/21 11:32 PM, Stephen Boyd wrote:
> Quoting Thara Gopinath (2021-11-17 18:55:17)
>> Hello Stephen,
>>
>> Thanks for the patch
>>
>> On 11/16/21 9:03 PM, Stephen Boyd wrote:
>>> Use platform_get_irq_optional() to avoid a noisy error message when the
>>> irq isn't specified. The irq is definitely optional given that we only
>>> care about errors that are -EPROBE_DEFER here.
>>>
>>> Cc: Thara Gopinath <thara.gopinath@linaro.org>
>>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>>> ---
>>> drivers/cpufreq/qcom-cpufreq-hw.c | 8 +++++---
>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>>> index a2be0df7e174..b442d4983a22 100644
>>> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
>>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>>> @@ -382,9 +382,11 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
>>> * Look for LMh interrupt. If no interrupt line is specified /
>>> * if there is an error, allow cpufreq to be enabled as usual.
>>> */
>>> - data->throttle_irq = platform_get_irq(pdev, index);
>>> - if (data->throttle_irq <= 0)
>>> - return data->throttle_irq == -EPROBE_DEFER ? -EPROBE_DEFER : 0;
>>> + data->throttle_irq = platform_get_irq_optional(pdev, index);
>>> + if (data->throttle_irq == -ENXIO)
>>> + return 0;
>>> + if (data->throttle_irq < 0)
>>> + return data->throttle_irq;
>>
>> Here the idea is to return only -EPROBE_DEFER error. Else return a 0 ,
>> so that cpufreq is enabled even if lmh interrupt is inaccessible. The
>> above check returns errors other than -EPROBE_DEFER as well. So I would
>> say make irq optional and keep the below check
>>
>> if (data->throttle_irq <= 0)
>> return data->throttle_irq == -EPROBE_DEFER ? -EPROBE_DEFER : 0;
>
> I'd like to catch other errors, for example, DT has an irq specified
> that is outside the range of irqs available. If the DT is correct, then
> it will either have a valid irq and this will return a >= 0 value or
> nothing will be specified and we'll get back -ENXIO now. Do you have
> some scenario where my patch fails to work?
Exactly. Like in the scenario you mentioned above, I do not want cpufreq
to be disabled. This interrupt is a throttle notification interrupt. The
action taken on basis of this is to send thermal pressure signal to
scheduler so that scheduler places tasks better. Even if the dt has
messed up this interrupt, I think cpufreq should still be enabled. May
be we can print a warn and still return 0 to enable cpufreq.
>
--
Warm Regards
Thara (She/Her/Hers)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cpufreq: qcom-hw: Use optional irq API
2021-11-18 14:08 ` Thara Gopinath
@ 2021-11-19 0:57 ` Stephen Boyd
0 siblings, 0 replies; 7+ messages in thread
From: Stephen Boyd @ 2021-11-19 0:57 UTC (permalink / raw)
To: Rafael J . Wysocki, Thara Gopinath, Viresh Kumar
Cc: linux-kernel, linux-arm-msm, linux-pm
Quoting Thara Gopinath (2021-11-18 06:08:04)
>
>
> On 11/17/21 11:32 PM, Stephen Boyd wrote:
> > Quoting Thara Gopinath (2021-11-17 18:55:17)
> >> Hello Stephen,
> >>
> >> Thanks for the patch
> >>
> >> On 11/16/21 9:03 PM, Stephen Boyd wrote:
> >>> Use platform_get_irq_optional() to avoid a noisy error message when the
> >>> irq isn't specified. The irq is definitely optional given that we only
> >>> care about errors that are -EPROBE_DEFER here.
> >>>
> >>> Cc: Thara Gopinath <thara.gopinath@linaro.org>
> >>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> >>> ---
> >>> drivers/cpufreq/qcom-cpufreq-hw.c | 8 +++++---
> >>> 1 file changed, 5 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> >>> index a2be0df7e174..b442d4983a22 100644
> >>> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> >>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> >>> @@ -382,9 +382,11 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
> >>> * Look for LMh interrupt. If no interrupt line is specified /
> >>> * if there is an error, allow cpufreq to be enabled as usual.
> >>> */
> >>> - data->throttle_irq = platform_get_irq(pdev, index);
> >>> - if (data->throttle_irq <= 0)
> >>> - return data->throttle_irq == -EPROBE_DEFER ? -EPROBE_DEFER : 0;
> >>> + data->throttle_irq = platform_get_irq_optional(pdev, index);
> >>> + if (data->throttle_irq == -ENXIO)
> >>> + return 0;
> >>> + if (data->throttle_irq < 0)
> >>> + return data->throttle_irq;
> >>
> >> Here the idea is to return only -EPROBE_DEFER error. Else return a 0 ,
> >> so that cpufreq is enabled even if lmh interrupt is inaccessible. The
> >> above check returns errors other than -EPROBE_DEFER as well. So I would
> >> say make irq optional and keep the below check
> >>
> >> if (data->throttle_irq <= 0)
> >> return data->throttle_irq == -EPROBE_DEFER ? -EPROBE_DEFER : 0;
> >
> > I'd like to catch other errors, for example, DT has an irq specified
> > that is outside the range of irqs available. If the DT is correct, then
> > it will either have a valid irq and this will return a >= 0 value or
> > nothing will be specified and we'll get back -ENXIO now. Do you have
> > some scenario where my patch fails to work?
>
> Exactly. Like in the scenario you mentioned above, I do not want cpufreq
> to be disabled. This interrupt is a throttle notification interrupt. The
> action taken on basis of this is to send thermal pressure signal to
> scheduler so that scheduler places tasks better. Even if the dt has
> messed up this interrupt, I think cpufreq should still be enabled. May
> be we can print a warn and still return 0 to enable cpufreq.
>
If the DT is messed up then we have problems that we should fix instead
of silently ignore in this driver. Is there some bad DT out there that
you're worried about? I don't believe this patch breaks anything but if
you can point to something that it breaks I'd be glad to investigate.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cpufreq: qcom-hw: Use optional irq API
2021-11-17 2:03 [PATCH] cpufreq: qcom-hw: Use optional irq API Stephen Boyd
2021-11-18 2:55 ` Thara Gopinath
@ 2021-12-03 4:47 ` Viresh Kumar
2021-12-25 16:37 ` Andy Shevchenko
1 sibling, 1 reply; 7+ messages in thread
From: Viresh Kumar @ 2021-12-03 4:47 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rafael J . Wysocki, linux-kernel, linux-arm-msm, linux-pm,
Thara Gopinath
On 16-11-21, 18:03, Stephen Boyd wrote:
> Use platform_get_irq_optional() to avoid a noisy error message when the
> irq isn't specified. The irq is definitely optional given that we only
> care about errors that are -EPROBE_DEFER here.
>
> Cc: Thara Gopinath <thara.gopinath@linaro.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> drivers/cpufreq/qcom-cpufreq-hw.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index a2be0df7e174..b442d4983a22 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -382,9 +382,11 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
> * Look for LMh interrupt. If no interrupt line is specified /
> * if there is an error, allow cpufreq to be enabled as usual.
> */
> - data->throttle_irq = platform_get_irq(pdev, index);
> - if (data->throttle_irq <= 0)
> - return data->throttle_irq == -EPROBE_DEFER ? -EPROBE_DEFER : 0;
> + data->throttle_irq = platform_get_irq_optional(pdev, index);
> + if (data->throttle_irq == -ENXIO)
> + return 0;
> + if (data->throttle_irq < 0)
> + return data->throttle_irq;
>
> data->cancel_throttle = false;
> data->policy = policy;
Applied. Thanks.
--
viresh
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cpufreq: qcom-hw: Use optional irq API
2021-12-03 4:47 ` Viresh Kumar
@ 2021-12-25 16:37 ` Andy Shevchenko
0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2021-12-25 16:37 UTC (permalink / raw)
To: Viresh Kumar
Cc: Stephen Boyd, Rafael J . Wysocki, Linux Kernel Mailing List,
linux-arm-msm, Linux PM, Thara Gopinath
On Mon, Dec 6, 2021 at 5:26 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 16-11-21, 18:03, Stephen Boyd wrote:
> > Use platform_get_irq_optional() to avoid a noisy error message when the
> > irq isn't specified. The irq is definitely optional given that we only
> > care about errors that are -EPROBE_DEFER here.
> > + data->throttle_irq = platform_get_irq_optional(pdev, index);
> > + if (data->throttle_irq == -ENXIO)
> > + return 0;
> > + if (data->throttle_irq < 0)
> > + return data->throttle_irq;
This adds more work for the future.
The best approach is
ret = platform_get_irq_optional(...);
if (ret < 0 && ret != -ENXIO)
return ret;
if (ret > 0)
...we got it...
It will allow the future API fix of platform_get_irq_optional() to be
really optional.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-12-25 16:37 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-17 2:03 [PATCH] cpufreq: qcom-hw: Use optional irq API Stephen Boyd
2021-11-18 2:55 ` Thara Gopinath
2021-11-18 4:32 ` Stephen Boyd
2021-11-18 14:08 ` Thara Gopinath
2021-11-19 0:57 ` Stephen Boyd
2021-12-03 4:47 ` Viresh Kumar
2021-12-25 16:37 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).