On 22-12-20, 22:19, Dmitry Osipenko wrote:

22.12.2020 12:12, Viresh Kumar пишет:

quoted

On 17-12-20, 21:06, Dmitry Osipenko wrote:

quoted

Fix adding OPP entries in a wrong (opposite) order if OPP rate is
unavailable. The OPP comparison is erroneously skipped if OPP rate is
missing, thus OPPs are left unsorted.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c | 23 ++++++++++++-----------
 drivers/opp/opp.h  |  2 +-
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 34f7e530d941..5c7f130a8de2 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c

@@ -1531,9 +1531,10 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
 	return true;
 }
 
-int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
+int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2,
+		     bool rate_not_available)
 {
-	if (opp1->rate != opp2->rate)
+	if (!rate_not_available && opp1->rate != opp2->rate)

rate will be 0 for both the OPPs here if rate_not_available is true and so this
change shouldn't be required.

The rate_not_available is negated in the condition. This change is
required because both rates are 0 and then we should proceed to the
levels comparison.

Won't that happen without this patch ?

I guess it's not clear by looking at this patch, please see a full
version of the function:

int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2,
         bool rate_not_available)
{
  if (!rate_not_available && opp1->rate != opp2->rate)
    return opp1->rate < opp2->rate ? -1 : 1;
  if (opp1->bandwidth && opp2->bandwidth &&
      opp1->bandwidth[0].peak != opp2->bandwidth[0].peak)
    return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 : 1;
  if (opp1->level != opp2->level)
    return opp1->level < opp2->level ? -1 : 1;
  return 0;
}

Perhaps we could check whether opp1->rate=0, like it's done for the
opp1->bandwidth. I'll consider this variant for v3, thanks.

-- 
viresh