Subject: Re: [PATCH] cpufreq: qcom-hw: Use optional irq API
To:     Stephen Boyd <swboyd@chromium.org>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Viresh Kumar <viresh.kumar@linaro.org>
Cc:     linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-pm@vger.kernel.org
References: <20211117020346.4088302-1-swboyd@chromium.org>
 <76b103ec-7034-e6c1-1ab4-174cf16f9fc8@linaro.org>
 <CAE-0n53HNSRTdADO1dbQTyLafyajUTatMq5tsLeNDLQ4g95YpA@mail.gmail.com>
From:   Thara Gopinath <thara.gopinath@linaro.org>
Message-ID: <f1038a3e-57fb-ec01-26a0-452a11dfcf3a@linaro.org>
Date:   Thu, 18 Nov 2021 09:08:04 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <CAE-0n53HNSRTdADO1dbQTyLafyajUTatMq5tsLeNDLQ4g95YpA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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)