[PATCH 1/2] firmware: arm_scmi: Keep the discrete clock rates sorted

[Dien Pham]

Hi Sudeep,

I share my build warning and some in-line comment below:

  CC      drivers/firmware/arm_scmi/clock.o
drivers/firmware/arm_scmi/clock.c: In function 'rate_cmp_func':
drivers/firmware/arm_scmi/clock.c:127:12: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  u64 *r1 = _r1, *r2 = _r2;
            ^~~
drivers/firmware/arm_scmi/clock.c:127:23: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  u64 *r1 = _r1, *r2 = _r2;
                       ^~~
  CC      arch/arm64/kernel/vdso.o
drivers/firmware/arm_scmi/clock.c: In function 'scmi_clock_protocol_init':
drivers/firmware/arm_scmi/clock.c:197:3: warning: 'rate' may be used uninitialized in this function [-Wmaybe-uninitialized]
   sort(rate, tot_rate_cnt, sizeof(*rate), rate_cmp_func, NULL);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

>-----Original Message-----
>From: Sudeep Holla <sudeep.holla at arm.com> 
>Sent: Wednesday, July 8, 2020 6:07 PM
>To: linux-arm-kernel at lists.infradead.org; linux-clk at vger.kernel.org; Stephen Boyd <sboyd at kernel.org>
>Cc: Sudeep Holla <sudeep.holla at arm.com>; linux-kernel at vger.kernel.org; Michael Turquette <mturquette at baylibre.com>; Dien Pham <dien.pham.ry at renesas.com>
>Subject: [PATCH 1/2] firmware: arm_scmi: Keep the discrete clock rates sorted
>
>Instead of relying on the firmware to keep the clock rates sorted, let us sort the list. This is not essential for clock layer but it helps to find the min and max rates easily from the list.
>
>Fixes: 5f6c6430e904 ("firmware: arm_scmi: add initial support for clock protocol")
>Reported-by: Dien Pham <dien.pham.ry at renesas.com>
>Signed-off-by: Sudeep Holla <sudeep.holla at arm.com>
>---
> drivers/firmware/arm_scmi/clock.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
>Hi Dien-san,
>
>If you could review/test these patches, I can queue them ASAP.
>I am planning to send the PR for ARM SoC later this week, so I need your tested-by.

I applied the patch,
Although there are some build warnings, but the patch effect is ok.

>
>Regards,
>Sudeep
>
>diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
>index 4c2227662b26..2dd119cdebf6 100644
>--- a/drivers/firmware/arm_scmi/clock.c
>+++ b/drivers/firmware/arm_scmi/clock.c
>@@ -5,6 +5,8 @@
>  * Copyright (C) 2018 ARM Ltd.
>  */
>
>+#include <linux/sort.h>
>+
> #include "common.h"
>
> enum scmi_clock_protocol_cmd {
>@@ -121,6 +123,13 @@ static int scmi_clock_attributes_get(const struct scmi_handle *handle,
> 	return ret;
> }
>
>+static int rate_cmp_func(const void *_r1, const void *_r2) {
>+	u64 *r1 = _r1, *r2 = _r2;

It is better to add 'const' as below to avoid warning.
const u64 *r1 = _r1, *r2 = _r2;

>+
>+	return r1 - r2;

r1 and r2 are u64, but returned value is 'int' type.
Do you think we should improve this ? e.g. return (int)r1 - r2;

>+}
>+
> static int
> scmi_clock_describe_rates_get(const struct scmi_handle *handle, u32 clk_id,
> 			      struct scmi_clock_info *clk)
>@@ -184,8 +193,10 @@ scmi_clock_describe_rates_get(const struct scmi_handle *handle, u32 clk_id,
> 		 */
> 	} while (num_returned && num_remaining);
>
>-	if (rate_discrete)
>+	if (rate_discrete) {
> 		clk->list.num_rates = tot_rate_cnt;
>+		sort(rate, tot_rate_cnt, sizeof(*rate), rate_cmp_func, NULL);

About warning of above line, I think it relates to below snip of code:
                if (tot_rate_cnt + num_returned > SCMI_MAX_NUM_RATES) {
                        dev_err(handle->dev, "No. of rates > MAX_NUM_RATES");
                        break;
                }

I see that in this case is true, it is not proceeded as error case,
If so I think you can update 'rate' for value from 'tot_rate_cnt' to SCMI_MAX_NUM_RATES at here.
How do you think ?

>+	}
>
> 	clk->rate_discrete = rate_discrete;
>
>--
>2.17.1

Best regard.
DIEN Pham