* [PATCH] watchdog: ixp4xx: Use devm_clk_get_enabled() helper
@ 2022-12-31 11:07 Christophe JAILLET
2022-12-31 23:14 ` Guenter Roeck
2023-03-07 17:51 ` Guenter Roeck
0 siblings, 2 replies; 7+ messages in thread
From: Christophe JAILLET @ 2022-12-31 11:07 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: linux-kernel, kernel-janitors, Christophe JAILLET, linux-watchdog
The devm_clk_get_enabled() helper:
- calls devm_clk_get()
- calls clk_prepare_enable() and registers what is needed in order to
call clk_disable_unprepare() when needed, as a managed resource.
This simplifies the code and avoids the need of a dedicated function used
with devm_add_action_or_reset().
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Note that I get a compilation error because read_cpuid_id() is not defined
on my system (x86_64).
So I think that a "depends on ARM<something>" in missing in a KConfig file.
Fixing it could help compilation farms build-bots.
---
drivers/watchdog/ixp4xx_wdt.c | 18 +++---------------
1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/watchdog/ixp4xx_wdt.c b/drivers/watchdog/ixp4xx_wdt.c
index 281a48d9889f..607ce4b8df57 100644
--- a/drivers/watchdog/ixp4xx_wdt.c
+++ b/drivers/watchdog/ixp4xx_wdt.c
@@ -112,12 +112,6 @@ static const struct watchdog_info ixp4xx_wdt_info = {
.identity = KBUILD_MODNAME,
};
-/* Devres-handled clock disablement */
-static void ixp4xx_clock_action(void *d)
-{
- clk_disable_unprepare(d);
-}
-
static int ixp4xx_wdt_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -139,16 +133,10 @@ static int ixp4xx_wdt_probe(struct platform_device *pdev)
* Retrieve rate from a fixed clock from the device tree if
* the parent has that, else use the default clock rate.
*/
- clk = devm_clk_get(dev->parent, NULL);
- if (!IS_ERR(clk)) {
- ret = clk_prepare_enable(clk);
- if (ret)
- return ret;
- ret = devm_add_action_or_reset(dev, ixp4xx_clock_action, clk);
- if (ret)
- return ret;
+ clk = devm_clk_get_enabled(dev->parent, NULL);
+ if (!IS_ERR(clk))
iwdt->rate = clk_get_rate(clk);
- }
+
if (!iwdt->rate)
iwdt->rate = IXP4XX_TIMER_FREQ;
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] watchdog: ixp4xx: Use devm_clk_get_enabled() helper
2022-12-31 11:07 [PATCH] watchdog: ixp4xx: Use devm_clk_get_enabled() helper Christophe JAILLET
@ 2022-12-31 23:14 ` Guenter Roeck
2023-01-01 9:35 ` Christophe JAILLET
2023-03-07 17:51 ` Guenter Roeck
1 sibling, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2022-12-31 23:14 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Wim Van Sebroeck, linux-kernel, kernel-janitors, linux-watchdog
On Sat, Dec 31, 2022 at 12:07:27PM +0100, Christophe JAILLET wrote:
> The devm_clk_get_enabled() helper:
> - calls devm_clk_get()
> - calls clk_prepare_enable() and registers what is needed in order to
> call clk_disable_unprepare() when needed, as a managed resource.
>
> This simplifies the code and avoids the need of a dedicated function used
> with devm_add_action_or_reset().
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Note that I get a compilation error because read_cpuid_id() is not defined
> on my system (x86_64).
> So I think that a "depends on ARM<something>" in missing in a KConfig file.
It has
depends on ARCH_IXP4XX
and CONFIG_IXP4XX_WATCHDOG is not set for me after "make allmodconfig".
>
> Fixing it could help compilation farms build-bots.
Mine doesn't see a problem, and I don't recall ever being alerted about
one. What am I missing ? Do you see a problem reported anywhere ?
Guenter
> ---
> drivers/watchdog/ixp4xx_wdt.c | 18 +++---------------
> 1 file changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/watchdog/ixp4xx_wdt.c b/drivers/watchdog/ixp4xx_wdt.c
> index 281a48d9889f..607ce4b8df57 100644
> --- a/drivers/watchdog/ixp4xx_wdt.c
> +++ b/drivers/watchdog/ixp4xx_wdt.c
> @@ -112,12 +112,6 @@ static const struct watchdog_info ixp4xx_wdt_info = {
> .identity = KBUILD_MODNAME,
> };
>
> -/* Devres-handled clock disablement */
> -static void ixp4xx_clock_action(void *d)
> -{
> - clk_disable_unprepare(d);
> -}
> -
> static int ixp4xx_wdt_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -139,16 +133,10 @@ static int ixp4xx_wdt_probe(struct platform_device *pdev)
> * Retrieve rate from a fixed clock from the device tree if
> * the parent has that, else use the default clock rate.
> */
> - clk = devm_clk_get(dev->parent, NULL);
> - if (!IS_ERR(clk)) {
> - ret = clk_prepare_enable(clk);
> - if (ret)
> - return ret;
> - ret = devm_add_action_or_reset(dev, ixp4xx_clock_action, clk);
> - if (ret)
> - return ret;
> + clk = devm_clk_get_enabled(dev->parent, NULL);
> + if (!IS_ERR(clk))
> iwdt->rate = clk_get_rate(clk);
> - }
> +
> if (!iwdt->rate)
> iwdt->rate = IXP4XX_TIMER_FREQ;
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] watchdog: ixp4xx: Use devm_clk_get_enabled() helper
2022-12-31 23:14 ` Guenter Roeck
@ 2023-01-01 9:35 ` Christophe JAILLET
2023-01-01 15:07 ` Guenter Roeck
0 siblings, 1 reply; 7+ messages in thread
From: Christophe JAILLET @ 2023-01-01 9:35 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, linux-kernel, kernel-janitors, linux-watchdog
Le 01/01/2023 à 00:14, Guenter Roeck a écrit :
> On Sat, Dec 31, 2022 at 12:07:27PM +0100, Christophe JAILLET wrote:
>> The devm_clk_get_enabled() helper:
>> - calls devm_clk_get()
>> - calls clk_prepare_enable() and registers what is needed in order to
>> call clk_disable_unprepare() when needed, as a managed resource.
>>
>> This simplifies the code and avoids the need of a dedicated function used
>> with devm_add_action_or_reset().
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Note that I get a compilation error because read_cpuid_id() is not defined
>> on my system (x86_64).
>> So I think that a "depends on ARM<something>" in missing in a KConfig file.
>
> It has
>
> depends on ARCH_IXP4XX
>
> and CONFIG_IXP4XX_WATCHDOG is not set for me after "make allmodconfig".
Here is what do.
make allmodconfig
make -j8 drivers/watchdog/ixp4xx_wdt.o
And I get:
DESCEND objtool
CALL scripts/checksyscalls.sh
CC drivers/watchdog/ixp4xx_wdt.o
drivers/watchdog/ixp4xx_wdt.c: In function ‘ixp4xx_wdt_probe’:
drivers/watchdog/ixp4xx_wdt.c:122:15: error: implicit declaration of
function ‘read_cpuid_id’ [-Werror=implicit-function-declaration]
122 | if (!(read_cpuid_id() & 0xf) && !cpu_is_ixp46x()) {
| ^~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[3]: *** [scripts/Makefile.build:252 :
drivers/watchdog/ixp4xx_wdt.o] Erreur 1
make[2]: *** [scripts/Makefile.build:504 : drivers/watchdog] Erreur 2
make[1]: *** [scripts/Makefile.build:504 : drivers] Erreur 2
make: *** [Makefile:2011 : .] Erreur 2
I do agree with you that:
- Kconfig looks fine
config IXP4XX_WATCHDOG
tristate "IXP4xx Watchdog"
depends on ARCH_IXP4XX
- Makefile looks fine
obj-$(CONFIG_IXP4XX_WATCHDOG) += ixp4xx_wdt.o
- .config looks fine
IXP4XX_WATCHDOG is NOT defined
- make drivers/watchdog/ looks fine
No error and ixp4xx_wdt.o is NOT built.
However, in the past (if I recollect correctly :) ), a "make
<something_that depends_on_a_config_variable_that_is_not_defined>"
returned an error stating that no rule existed to build the specified
target.
I sometimes needed to tweak the Kconfig files to force some compilation
when I didn't have the right tool chain or configuration.
It was maybe not the best practice, but it worked most of the time.
Now, with the example above, such a compilation attempt is possible. It
is maybe normal (because of a change somewhere in the way the kernel is
built, because of an updated toolchain on my machine, ...)
This is just fine for me, but looked really surprising.
That is why I first thought that something was missing in a Kconfig file.
So my comments are just a surprise to me to something that seems not to
behave the same as before.
As far a just a 'make' works as expected, I won't dig further if this
behavior is expected or not.
It should be a corner case anyway, and for my own use, I would even call
it a feature :) (i.e. it saves me some Kconfig modification to test things)
CJ
>
>>
>> Fixing it could help compilation farms build-bots.
>
> Mine doesn't see a problem, and I don't recall ever being alerted about
> one. What am I missing ? Do you see a problem reported anywhere ?
>
> Guenter
>
>> ---
>> drivers/watchdog/ixp4xx_wdt.c | 18 +++---------------
>> 1 file changed, 3 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/watchdog/ixp4xx_wdt.c b/drivers/watchdog/ixp4xx_wdt.c
>> index 281a48d9889f..607ce4b8df57 100644
>> --- a/drivers/watchdog/ixp4xx_wdt.c
>> +++ b/drivers/watchdog/ixp4xx_wdt.c
>> @@ -112,12 +112,6 @@ static const struct watchdog_info ixp4xx_wdt_info = {
>> .identity = KBUILD_MODNAME,
>> };
>>
>> -/* Devres-handled clock disablement */
>> -static void ixp4xx_clock_action(void *d)
>> -{
>> - clk_disable_unprepare(d);
>> -}
>> -
>> static int ixp4xx_wdt_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> @@ -139,16 +133,10 @@ static int ixp4xx_wdt_probe(struct platform_device *pdev)
>> * Retrieve rate from a fixed clock from the device tree if
>> * the parent has that, else use the default clock rate.
>> */
>> - clk = devm_clk_get(dev->parent, NULL);
>> - if (!IS_ERR(clk)) {
>> - ret = clk_prepare_enable(clk);
>> - if (ret)
>> - return ret;
>> - ret = devm_add_action_or_reset(dev, ixp4xx_clock_action, clk);
>> - if (ret)
>> - return ret;
>> + clk = devm_clk_get_enabled(dev->parent, NULL);
>> + if (!IS_ERR(clk))
>> iwdt->rate = clk_get_rate(clk);
>> - }
>> +
>> if (!iwdt->rate)
>> iwdt->rate = IXP4XX_TIMER_FREQ;
>>
>> --
>> 2.34.1
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] watchdog: ixp4xx: Use devm_clk_get_enabled() helper
2023-01-01 9:35 ` Christophe JAILLET
@ 2023-01-01 15:07 ` Guenter Roeck
2023-01-01 17:52 ` Christophe JAILLET
0 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2023-01-01 15:07 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Wim Van Sebroeck, linux-kernel, kernel-janitors, linux-watchdog
On Sun, Jan 01, 2023 at 10:35:59AM +0100, Christophe JAILLET wrote:
> Le 01/01/2023 à 00:14, Guenter Roeck a écrit :
> > On Sat, Dec 31, 2022 at 12:07:27PM +0100, Christophe JAILLET wrote:
> > > The devm_clk_get_enabled() helper:
> > > - calls devm_clk_get()
> > > - calls clk_prepare_enable() and registers what is needed in order to
> > > call clk_disable_unprepare() when needed, as a managed resource.
> > >
> > > This simplifies the code and avoids the need of a dedicated function used
> > > with devm_add_action_or_reset().
> > >
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > > Note that I get a compilation error because read_cpuid_id() is not defined
> > > on my system (x86_64).
> > > So I think that a "depends on ARM<something>" in missing in a KConfig file.
> >
> > It has
> >
> > depends on ARCH_IXP4XX
> >
> > and CONFIG_IXP4XX_WATCHDOG is not set for me after "make allmodconfig".
>
> Here is what do.
>
> make allmodconfig
> make -j8 drivers/watchdog/ixp4xx_wdt.o
>
> And I get:
> DESCEND objtool
> CALL scripts/checksyscalls.sh
> CC drivers/watchdog/ixp4xx_wdt.o
> drivers/watchdog/ixp4xx_wdt.c: In function ‘ixp4xx_wdt_probe’:
> drivers/watchdog/ixp4xx_wdt.c:122:15: error: implicit declaration of
> function ‘read_cpuid_id’ [-Werror=implicit-function-declaration]
> 122 | if (!(read_cpuid_id() & 0xf) && !cpu_is_ixp46x()) {
> | ^~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> make[3]: *** [scripts/Makefile.build:252 : drivers/watchdog/ixp4xx_wdt.o]
> Erreur 1
> make[2]: *** [scripts/Makefile.build:504 : drivers/watchdog] Erreur 2
> make[1]: *** [scripts/Makefile.build:504 : drivers] Erreur 2
> make: *** [Makefile:2011 : .] Erreur 2
>
>
> I do agree with you that:
>
> - Kconfig looks fine
> config IXP4XX_WATCHDOG
> tristate "IXP4xx Watchdog"
> depends on ARCH_IXP4XX
>
> - Makefile looks fine
> obj-$(CONFIG_IXP4XX_WATCHDOG) += ixp4xx_wdt.o
>
> - .config looks fine
> IXP4XX_WATCHDOG is NOT defined
>
> - make drivers/watchdog/ looks fine
> No error and ixp4xx_wdt.o is NOT built.
>
>
> However, in the past (if I recollect correctly :) ), a "make <something_that
> depends_on_a_config_variable_that_is_not_defined>" returned an error stating
> that no rule existed to build the specified target.
>
This is not correct. It only applies if the target directory Makefile is
excluded by the make flags, or possibly if the target file is a complex
one build from various source files.
> I sometimes needed to tweak the Kconfig files to force some compilation when
> I didn't have the right tool chain or configuration.
> It was maybe not the best practice, but it worked most of the time.
>
>
> Now, with the example above, such a compilation attempt is possible. It is
> maybe normal (because of a change somewhere in the way the kernel is built,
> because of an updated toolchain on my machine, ...)
> This is just fine for me, but looked really surprising.
>
> That is why I first thought that something was missing in a Kconfig file.
>
>
> So my comments are just a surprise to me to something that seems not to
> behave the same as before.
>
I don't think anything changed. It always worked like that for me.
I would suggest to go back to an older kernel and try it there.
You'll see exactly the same error. Maybe you just never encountered
a file like that.
So, in other words, what you should have said is "not compile tested".
Alternatively, you could install cross compilers and compile test the
patches with those.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] watchdog: ixp4xx: Use devm_clk_get_enabled() helper
2023-01-01 15:07 ` Guenter Roeck
@ 2023-01-01 17:52 ` Christophe JAILLET
2023-01-01 19:31 ` Guenter Roeck
0 siblings, 1 reply; 7+ messages in thread
From: Christophe JAILLET @ 2023-01-01 17:52 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, linux-kernel, kernel-janitors, linux-watchdog
Le 01/01/2023 à 16:07, Guenter Roeck a écrit :
> On Sun, Jan 01, 2023 at 10:35:59AM +0100, Christophe JAILLET wrote:
>> Le 01/01/2023 à 00:14, Guenter Roeck a écrit :
>>> On Sat, Dec 31, 2022 at 12:07:27PM +0100, Christophe JAILLET wrote:
>>>> The devm_clk_get_enabled() helper:
>>>> - calls devm_clk_get()
>>>> - calls clk_prepare_enable() and registers what is needed in order to
>>>> call clk_disable_unprepare() when needed, as a managed resource.
>>>>
>>>> This simplifies the code and avoids the need of a dedicated function used
>>>> with devm_add_action_or_reset().
>>>>
>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet-39ZsbGIQGT5GWvitb5QawA@public.gmane.org>
>>>> ---
>>>> Note that I get a compilation error because read_cpuid_id() is not defined
>>>> on my system (x86_64).
>>>> So I think that a "depends on ARM<something>" in missing in a KConfig file.
>>>
>>> It has
>>>
>>> depends on ARCH_IXP4XX
>>>
>>> and CONFIG_IXP4XX_WATCHDOG is not set for me after "make allmodconfig".
>>
>> Here is what do.
>>
>> make allmodconfig
>> make -j8 drivers/watchdog/ixp4xx_wdt.o
>>
>> And I get:
>> DESCEND objtool
>> CALL scripts/checksyscalls.sh
>> CC drivers/watchdog/ixp4xx_wdt.o
>> drivers/watchdog/ixp4xx_wdt.c: In function ‘ixp4xx_wdt_probe’:
>> drivers/watchdog/ixp4xx_wdt.c:122:15: error: implicit declaration of
>> function ‘read_cpuid_id’ [-Werror=implicit-function-declaration]
>> 122 | if (!(read_cpuid_id() & 0xf) && !cpu_is_ixp46x()) {
>> | ^~~~~~~~~~~~~
>> cc1: all warnings being treated as errors
>> make[3]: *** [scripts/Makefile.build:252 : drivers/watchdog/ixp4xx_wdt.o]
>> Erreur 1
>> make[2]: *** [scripts/Makefile.build:504 : drivers/watchdog] Erreur 2
>> make[1]: *** [scripts/Makefile.build:504 : drivers] Erreur 2
>> make: *** [Makefile:2011 : .] Erreur 2
>>
>>
>> I do agree with you that:
>>
>> - Kconfig looks fine
>> config IXP4XX_WATCHDOG
>> tristate "IXP4xx Watchdog"
>> depends on ARCH_IXP4XX
>>
>> - Makefile looks fine
>> obj-$(CONFIG_IXP4XX_WATCHDOG) += ixp4xx_wdt.o
>>
>> - .config looks fine
>> IXP4XX_WATCHDOG is NOT defined
>>
>> - make drivers/watchdog/ looks fine
>> No error and ixp4xx_wdt.o is NOT built.
>>
>>
>> However, in the past (if I recollect correctly :) ), a "make <something_that
>> depends_on_a_config_variable_that_is_not_defined>" returned an error stating
>> that no rule existed to build the specified target.
>>
>
> This is not correct. It only applies if the target directory Makefile is
> excluded by the make flags, or possibly if the target file is a complex
> one build from various source files.
>
>> I sometimes needed to tweak the Kconfig files to force some compilation when
>> I didn't have the right tool chain or configuration.
>> It was maybe not the best practice, but it worked most of the time.
>>
>>
>> Now, with the example above, such a compilation attempt is possible. It is
>> maybe normal (because of a change somewhere in the way the kernel is built,
>> because of an updated toolchain on my machine, ...)
>> This is just fine for me, but looked really surprising.
>>
>> That is why I first thought that something was missing in a Kconfig file.
>>
>>
>> So my comments are just a surprise to me to something that seems not to
>> behave the same as before.
>>
> I don't think anything changed. It always worked like that for me.
> I would suggest to go back to an older kernel and try it there.
> You'll see exactly the same error. Maybe you just never encountered
> a file like that.
git reset --hard next-20210111 (randomly chosen date)
make allmodconfig
make clean
make -j7 drivers/watchdog/ixp4xx_wdt.o (too build most of the needed
stuff to build a kernel)
touch drivers/watchdog/ixp4xx_wdt.c
make -j7 drivers/watchdog/ixp4xx_wdt.o (too build this file only)
DESCEND objtool
CALL scripts/atomic/check-atomics.sh
CALL scripts/checksyscalls.sh
make[3]: *** Aucune règle pour fabriquer la cible «
drivers/watchdog/ixp4xx_wdt.o ». Arrêt.
make[2]: *** [scripts/Makefile.build:471 : __build] Erreur 2
make[1]: *** [scripts/Makefile.build:496 : drivers/watchdog] Erreur 2
make: *** [Makefile:1805 : drivers] Erreur 2
This is what I had in mind.
(Aucune règle pour fabriquer la cible... = no rule to build...)
So something changed somewhere. Maybe in the way Makefile are now
included or not in the build process, as you suggest above.
>
> So, in other words, what you should have said is "not compile tested".
> Alternatively, you could install cross compilers and compile test the
> patches with those.
>
No. The patches HAVE been cross compiled after my initial attempt with
my default x86_64 failing built.
This one was successfully built using:
make ARCH=arm CROSS_COMPILE=/usr/bin/arm-linux-gnueabi- allmodconfig
make ARCH=arm CROSS_COMPILE=/usr/bin/arm-linux-gnueabi- -j7
drivers/watchdog/ixp4xx_wdt.o
The other one was successfully built using:
make ARCH=mips CROSS_COMPILE=mips64-linux-gnuabi64- allmodconfig
Changing CONFIG_MACH_LOONGSON32 to y, instead of
CONFIG_MIPS_GENERIC_KERNEL
make ARCH=mips CROSS_COMPILE=mips64-linux-gnuabi64- -j7
drivers/watchdog/loongson1_wdt.o
I've long been reluctant to use cross-compiler because of low disk space
on my system. But I've changed my mind recently and now I do cross
compile. (see [1] if needed as example also with ARCH=arm)
My comments below the --- in the patches should not be taken as "I've
not managed to build with the patch", but "I've been surprised to get an
issue with x86_64, then cross-compiled with the relevant toolchain.
Then, I reported what looked like a potential issue when building with
x86_64."
CJ
[1]:
https://lore.kernel.org/all/dc5a2157-19c8-4498-6288-d7513ad3dde2@wanadoo.fr/
>
> Thanks,
> Guenter
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] watchdog: ixp4xx: Use devm_clk_get_enabled() helper
2023-01-01 17:52 ` Christophe JAILLET
@ 2023-01-01 19:31 ` Guenter Roeck
0 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2023-01-01 19:31 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Wim Van Sebroeck, linux-kernel, kernel-janitors, linux-watchdog,
Masahiro Yamada
On Sun, Jan 01, 2023 at 06:52:35PM +0100, Christophe JAILLET wrote:
[ ... ]
> > >
> > > Here is what do.
> > >
> > > make allmodconfig
> > > make -j8 drivers/watchdog/ixp4xx_wdt.o
> > >
> > > And I get:
> > > DESCEND objtool
> > > CALL scripts/checksyscalls.sh
> > > CC drivers/watchdog/ixp4xx_wdt.o
> > > drivers/watchdog/ixp4xx_wdt.c: In function ‘ixp4xx_wdt_probe’:
> > > drivers/watchdog/ixp4xx_wdt.c:122:15: error: implicit declaration of
> > > function ‘read_cpuid_id’ [-Werror=implicit-function-declaration]
> > > 122 | if (!(read_cpuid_id() & 0xf) && !cpu_is_ixp46x()) {
> > > | ^~~~~~~~~~~~~
> > > cc1: all warnings being treated as errors
> > > make[3]: *** [scripts/Makefile.build:252 : drivers/watchdog/ixp4xx_wdt.o]
> > > Erreur 1
> > > make[2]: *** [scripts/Makefile.build:504 : drivers/watchdog] Erreur 2
> > > make[1]: *** [scripts/Makefile.build:504 : drivers] Erreur 2
> > > make: *** [Makefile:2011 : .] Erreur 2
> > >
> > >
> > > I do agree with you that:
> > >
> > > - Kconfig looks fine
> > > config IXP4XX_WATCHDOG
> > > tristate "IXP4xx Watchdog"
> > > depends on ARCH_IXP4XX
> > >
> > > - Makefile looks fine
> > > obj-$(CONFIG_IXP4XX_WATCHDOG) += ixp4xx_wdt.o
> > >
> > > - .config looks fine
> > > IXP4XX_WATCHDOG is NOT defined
> > >
> > > - make drivers/watchdog/ looks fine
> > > No error and ixp4xx_wdt.o is NOT built.
> > >
> > >
> > > However, in the past (if I recollect correctly :) ), a "make <something_that
> > > depends_on_a_config_variable_that_is_not_defined>" returned an error stating
> > > that no rule existed to build the specified target.
> > >
> >
> > This is not correct. It only applies if the target directory Makefile is
> > excluded by the make flags, or possibly if the target file is a complex
> > one build from various source files.
> >
> > > I sometimes needed to tweak the Kconfig files to force some compilation when
> > > I didn't have the right tool chain or configuration.
> > > It was maybe not the best practice, but it worked most of the time.
> > >
> > >
> > > Now, with the example above, such a compilation attempt is possible. It is
> > > maybe normal (because of a change somewhere in the way the kernel is built,
> > > because of an updated toolchain on my machine, ...)
> > > This is just fine for me, but looked really surprising.
> > >
> > > That is why I first thought that something was missing in a Kconfig file.
> > >
> > >
> > > So my comments are just a surprise to me to something that seems not to
> > > behave the same as before.
> > >
> > I don't think anything changed. It always worked like that for me.
> > I would suggest to go back to an older kernel and try it there.
> > You'll see exactly the same error. Maybe you just never encountered
> > a file like that.
>
> git reset --hard next-20210111 (randomly chosen date)
> make allmodconfig
> make clean
> make -j7 drivers/watchdog/ixp4xx_wdt.o (too build most of the needed stuff
> to build a kernel)
> touch drivers/watchdog/ixp4xx_wdt.c
> make -j7 drivers/watchdog/ixp4xx_wdt.o (too build this file only)
>
> DESCEND objtool
> CALL scripts/atomic/check-atomics.sh
> CALL scripts/checksyscalls.sh
> make[3]: *** Aucune règle pour fabriquer la cible «
> drivers/watchdog/ixp4xx_wdt.o ». Arrêt.
> make[2]: *** [scripts/Makefile.build:471 : __build] Erreur 2
> make[1]: *** [scripts/Makefile.build:496 : drivers/watchdog] Erreur 2
> make: *** [Makefile:1805 : drivers] Erreur 2
>
Turns out the behavior preferred by you was introduced in v5.4 with
commit 394053f4a4b3 ("kbuild: make single targets work more correctly")
and undone with commit cc306abd19e8 ("kbuild: fix and refactor single
target build") in v6.1. As for what is the expected behavior, I can't say.
I for my part had not noticed that the behavior had changed between v5.4
and v6.0.
I would suggest to discuss details with Yamada-san.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] watchdog: ixp4xx: Use devm_clk_get_enabled() helper
2022-12-31 11:07 [PATCH] watchdog: ixp4xx: Use devm_clk_get_enabled() helper Christophe JAILLET
2022-12-31 23:14 ` Guenter Roeck
@ 2023-03-07 17:51 ` Guenter Roeck
1 sibling, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2023-03-07 17:51 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Wim Van Sebroeck, linux-kernel, kernel-janitors, linux-watchdog
On Sat, Dec 31, 2022 at 12:07:27PM +0100, Christophe JAILLET wrote:
> The devm_clk_get_enabled() helper:
> - calls devm_clk_get()
> - calls clk_prepare_enable() and registers what is needed in order to
> call clk_disable_unprepare() when needed, as a managed resource.
>
> This simplifies the code and avoids the need of a dedicated function used
> with devm_add_action_or_reset().
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Note that I get a compilation error because read_cpuid_id() is not defined
> on my system (x86_64).
> So I think that a "depends on ARM<something>" in missing in a KConfig file.
>
> Fixing it could help compilation farms build-bots.
As mentioned, this not how build dependencies work. The code builds fine
with arm:allmodconfig. It is not enabled with x86_64:allmodconfig since
it depends on ARCH_IXP4XX. It is is not expected to be buildable if
ARCH_IXP4XX is not selected.
Guenter
> ---
> drivers/watchdog/ixp4xx_wdt.c | 18 +++---------------
> 1 file changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/watchdog/ixp4xx_wdt.c b/drivers/watchdog/ixp4xx_wdt.c
> index 281a48d9889f..607ce4b8df57 100644
> --- a/drivers/watchdog/ixp4xx_wdt.c
> +++ b/drivers/watchdog/ixp4xx_wdt.c
> @@ -112,12 +112,6 @@ static const struct watchdog_info ixp4xx_wdt_info = {
> .identity = KBUILD_MODNAME,
> };
>
> -/* Devres-handled clock disablement */
> -static void ixp4xx_clock_action(void *d)
> -{
> - clk_disable_unprepare(d);
> -}
> -
> static int ixp4xx_wdt_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -139,16 +133,10 @@ static int ixp4xx_wdt_probe(struct platform_device *pdev)
> * Retrieve rate from a fixed clock from the device tree if
> * the parent has that, else use the default clock rate.
> */
> - clk = devm_clk_get(dev->parent, NULL);
> - if (!IS_ERR(clk)) {
> - ret = clk_prepare_enable(clk);
> - if (ret)
> - return ret;
> - ret = devm_add_action_or_reset(dev, ixp4xx_clock_action, clk);
> - if (ret)
> - return ret;
> + clk = devm_clk_get_enabled(dev->parent, NULL);
> + if (!IS_ERR(clk))
> iwdt->rate = clk_get_rate(clk);
> - }
> +
> if (!iwdt->rate)
> iwdt->rate = IXP4XX_TIMER_FREQ;
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-03-07 17:57 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-31 11:07 [PATCH] watchdog: ixp4xx: Use devm_clk_get_enabled() helper Christophe JAILLET
2022-12-31 23:14 ` Guenter Roeck
2023-01-01 9:35 ` Christophe JAILLET
2023-01-01 15:07 ` Guenter Roeck
2023-01-01 17:52 ` Christophe JAILLET
2023-01-01 19:31 ` Guenter Roeck
2023-03-07 17:51 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).