[PATCH] clk: fix ccf_clk_get_rate
From: Hou Zhiqiang Zhiqiang.Hou@nxp.com
As the type of return value is 'ulong', when clk_get_by_id() failed, it should return 0 to indicate the get_rate operation doesn't succeed.
Signed-off-by: Hou Zhiqiang Zhiqiang.Hou@nxp.com --- drivers/clk/clk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index b8c2e8d531..4c2c372cd3 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -84,7 +84,8 @@ ulong ccf_clk_get_rate(struct clk *clk) int err = clk_get_by_id(clk->id, &c);
if (err) - return err; + return 0; + return clk_get_rate(c); }
Hi
On Thu, Jul 25, 2024 at 9:16 AM Zhiqiang Hou Zhiqiang.Hou@nxp.com wrote:
From: Hou Zhiqiang Zhiqiang.Hou@nxp.com
As the type of return value is 'ulong', when clk_get_by_id() failed, it should return 0 to indicate the get_rate operation doesn't succeed.
I understand your point here but the clk_get_rate that can call the ccf clk_get_rate can already return -ENOSYS.
Michael
Signed-off-by: Hou Zhiqiang Zhiqiang.Hou@nxp.com
drivers/clk/clk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index b8c2e8d531..4c2c372cd3 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -84,7 +84,8 @@ ulong ccf_clk_get_rate(struct clk *clk) int err = clk_get_by_id(clk->id, &c);
if (err)
return err;
return 0;return clk_get_rate(c);}
-- 2.17.1
Hi Michael,
-----Original Message----- From: Michael Nazzareno Trimarchi michael@amarulasolutions.com Sent: Thursday, July 25, 2024 3:32 PM To: Z.Q. Hou zhiqiang.hou@nxp.com Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; seanga2@gmail.com Subject: Re: [PATCH] clk: fix ccf_clk_get_rate
Hi
On Thu, Jul 25, 2024 at 9:16 AM Zhiqiang Hou Zhiqiang.Hou@nxp.com wrote:
From: Hou Zhiqiang Zhiqiang.Hou@nxp.com
As the type of return value is 'ulong', when clk_get_by_id() failed, it should return 0 to indicate the get_rate operation doesn't succeed.
I understand your point here but the clk_get_rate that can call the ccf clk_get_rate can already return -ENOSYS.
will also fix it.
Thanks, Zhiqiang
Hi
On Thu, Jul 25, 2024 at 10:53 AM Z.Q. Hou zhiqiang.hou@nxp.com wrote:
Hi Michael,
-----Original Message----- From: Michael Nazzareno Trimarchi michael@amarulasolutions.com Sent: Thursday, July 25, 2024 3:32 PM To: Z.Q. Hou zhiqiang.hou@nxp.com Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; seanga2@gmail.com Subject: Re: [PATCH] clk: fix ccf_clk_get_rate
Hi
On Thu, Jul 25, 2024 at 9:16 AM Zhiqiang Hou Zhiqiang.Hou@nxp.com wrote:
From: Hou Zhiqiang Zhiqiang.Hou@nxp.com
As the type of return value is 'ulong', when clk_get_by_id() failed, it should return 0 to indicate the get_rate operation doesn't succeed.
I understand your point here but the clk_get_rate that can call the ccf clk_get_rate can already return -ENOSYS.
will also fix it.
Is there any usage of the error set on the latest bit of the clock? We need to be sure that this is correct use accross the uboot. The clk-uclass define the error that can return
Michael
Thanks, Zhiqiang
-- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 michael@amarulasolutions.com __________________________________
Amarula Solutions BV Joop Geesinkweg 125, 1114 AB, Amsterdam, NL T. +31 (0)85 111 9172 info@amarulasolutions.com www.amarulasolutions.com
Hi Michael,
-----Original Message----- From: Michael Nazzareno Trimarchi michael@amarulasolutions.com Sent: Thursday, July 25, 2024 5:26 PM To: Z.Q. Hou zhiqiang.hou@nxp.com; Simon Glass sjg@chromium.org Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; seanga2@gmail.com Subject: Re: [PATCH] clk: fix ccf_clk_get_rate
Hi
On Thu, Jul 25, 2024 at 10:53 AM Z.Q. Hou zhiqiang.hou@nxp.com wrote:
Hi Michael,
-----Original Message----- From: Michael Nazzareno Trimarchi michael@amarulasolutions.com Sent: Thursday, July 25, 2024 3:32 PM To: Z.Q. Hou zhiqiang.hou@nxp.com Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; seanga2@gmail.com Subject: Re: [PATCH] clk: fix ccf_clk_get_rate
Hi
On Thu, Jul 25, 2024 at 9:16 AM Zhiqiang Hou Zhiqiang.Hou@nxp.com wrote:
From: Hou Zhiqiang Zhiqiang.Hou@nxp.com
As the type of return value is 'ulong', when clk_get_by_id() failed, it should return 0 to indicate the get_rate operation doesn't
succeed.
I understand your point here but the clk_get_rate that can call the ccf clk_get_rate can already return -ENOSYS.
will also fix it.
Is there any usage of the error set on the latest bit of the clock? We need to be sure that this is correct use accross the uboot. The clk-uclass define the error that can return
The problem is this function's return value is 'ulong', if use the error set, the caller will treat it as a good value instead of a error number. There are several APIs have the same problem.
2 methods to fix it: 1. keep the API's prototype, and use '0' to indicate error condition, but a real '0' return value is also considered as error. 2. Change the return type to like 'long long int', and use a negative error number to indicate error condition. Need to update the check of return value for all the callers.
Any suggestion?
Thanks, Zhiqiang
Hi
On Thu, Jul 25, 2024 at 11:58 AM Z.Q. Hou zhiqiang.hou@nxp.com wrote:
Hi Michael,
-----Original Message----- From: Michael Nazzareno Trimarchi michael@amarulasolutions.com Sent: Thursday, July 25, 2024 5:26 PM To: Z.Q. Hou zhiqiang.hou@nxp.com; Simon Glass sjg@chromium.org Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; seanga2@gmail.com Subject: Re: [PATCH] clk: fix ccf_clk_get_rate
Hi
On Thu, Jul 25, 2024 at 10:53 AM Z.Q. Hou zhiqiang.hou@nxp.com wrote:
Hi Michael,
-----Original Message----- From: Michael Nazzareno Trimarchi michael@amarulasolutions.com Sent: Thursday, July 25, 2024 3:32 PM To: Z.Q. Hou zhiqiang.hou@nxp.com Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; seanga2@gmail.com Subject: Re: [PATCH] clk: fix ccf_clk_get_rate
Hi
On Thu, Jul 25, 2024 at 9:16 AM Zhiqiang Hou Zhiqiang.Hou@nxp.com wrote:
From: Hou Zhiqiang Zhiqiang.Hou@nxp.com
As the type of return value is 'ulong', when clk_get_by_id() failed, it should return 0 to indicate the get_rate operation doesn't
succeed.
I understand your point here but the clk_get_rate that can call the ccf clk_get_rate can already return -ENOSYS.
will also fix it.
Is there any usage of the error set on the latest bit of the clock? We need to be sure that this is correct use accross the uboot. The clk-uclass define the error that can return
The problem is this function's return value is 'ulong', if use the error set, the caller will treat it as a good value instead of a error number. There are several APIs have the same problem.
2 methods to fix it:
- keep the API's prototype, and use '0' to indicate error condition, but a real '0' return value is also considered as error.
- Change the return type to like 'long long int', and use a negative error number to indicate error condition. Need to update the check of return value for all the callers.
Any suggestion?
unit test suggest that error condition are evaluated so we have 32 bit on arm of unsigned long so we can not map all the positive clock
rate = clk_set_rate(clk, 80000000); ut_asserteq(rate, -ENOSYS);
Now, can we have a clock greater then 2Ghz if yes this not work always on 32 bit, then the macro IS_ERR_VALUE should help on clk return
Michael
Thanks, Zhiqiang
-- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 michael@amarulasolutions.com __________________________________
Amarula Solutions BV Joop Geesinkweg 125, 1114 AB, Amsterdam, NL T. +31 (0)85 111 9172 info@amarulasolutions.com www.amarulasolutions.com
Hi Michael,
-----Original Message----- From: Michael Nazzareno Trimarchi michael@amarulasolutions.com Sent: Thursday, July 25, 2024 6:21 PM To: Z.Q. Hou zhiqiang.hou@nxp.com Cc: Simon Glass sjg@chromium.org; u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; seanga2@gmail.com Subject: Re: [PATCH] clk: fix ccf_clk_get_rate
Hi
On Thu, Jul 25, 2024 at 11:58 AM Z.Q. Hou zhiqiang.hou@nxp.com wrote:
Hi Michael,
-----Original Message----- From: Michael Nazzareno Trimarchi michael@amarulasolutions.com Sent: Thursday, July 25, 2024 5:26 PM To: Z.Q. Hou zhiqiang.hou@nxp.com; Simon Glass sjg@chromium.org Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; seanga2@gmail.com Subject: Re: [PATCH] clk: fix ccf_clk_get_rate
Hi
On Thu, Jul 25, 2024 at 10:53 AM Z.Q. Hou zhiqiang.hou@nxp.com
wrote:
Hi Michael,
-----Original Message----- From: Michael Nazzareno Trimarchi
Sent: Thursday, July 25, 2024 3:32 PM To: Z.Q. Hou zhiqiang.hou@nxp.com Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; seanga2@gmail.com Subject: Re: [PATCH] clk: fix ccf_clk_get_rate
Hi
On Thu, Jul 25, 2024 at 9:16 AM Zhiqiang Hou Zhiqiang.Hou@nxp.com wrote:
From: Hou Zhiqiang Zhiqiang.Hou@nxp.com
As the type of return value is 'ulong', when clk_get_by_id() failed, it should return 0 to indicate the get_rate operation doesn't
succeed.
I understand your point here but the clk_get_rate that can call the ccf clk_get_rate can already return -ENOSYS.
will also fix it.
Is there any usage of the error set on the latest bit of the clock? We need to be sure that this is correct use accross the uboot. The clk-uclass define the error that can return
The problem is this function's return value is 'ulong', if use the error set, the
caller will treat it as a good value instead of a error number. There are several APIs have the same problem.
2 methods to fix it:
- keep the API's prototype, and use '0' to indicate error condition, but a real
'0' return value is also considered as error.
- Change the return type to like 'long long int', and use a negative error
number to indicate error condition. Need to update the check of return value for all the callers.
Any suggestion?
unit test suggest that error condition are evaluated so we have 32 bit on arm of unsigned long so we can not map all the positive clock
rate = clk_set_rate(clk, 80000000); ut_asserteq(rate, -ENOSYS);
Now, can we have a clock greater then 2Ghz if yes this not work always on 32 bit, then the macro IS_ERR_VALUE should help on clk return
You mean keep it as it's and let the callers evaluate the error condition using IS_ERR_VALUE, right? Current implement also make 'return 0' as error condition, and many callers treat all non-zero return value as good, these need to fix.
ulong clk_get_rate(struct clk *clk) { ... if (!clk_valid(clk)) return 0; ... }
Contrast to reserve 4K errnos, I prefer to 'return 0' as error condition, 0 Hz is almost never a correct value, and all the positive value can be used especially on 32-bit platform, does the caller really cares about the exact error number?
Thanks, Zhiqiang
participants (3)
-
Michael Nazzareno Trimarchi -
Z.Q. Hou -
Zhiqiang Hou