Hi Dien-san, On Thu, Jul 09, 2020 at 08:20:51AM +0000, Dien Pham wrote: > 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); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >
Sorry for this. I noticed yesterday when I built but strangely I had created patches before I fixed these and sent them instead of fixed version. My mistake. > >-----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. > Thanks for testing. > > > >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; > Yes, I have this in the correct version which I sent as v2 this morning. > >+ > >+ 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; > Not changing to const above must suffice. > >+} > >+ > > 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 don't understand your comment and relation to above warning. > 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 ? > -- Regards, Sudeep
