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 <[email protected]>
>Sent: Wednesday, July 8, 2020 6:07 PM
>To: [email protected]; [email protected]; Stephen
>Boyd <[email protected]>
>Cc: Sudeep Holla <[email protected]>; [email protected]; Michael
>Turquette <[email protected]>; Dien Pham <[email protected]>
>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 <[email protected]>
>Signed-off-by: Sudeep Holla <[email protected]>
>---
> 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
