Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] watchdog: mtx-1: Relax build dependencies
@ 2019-12-11 21:02 Florian Fainelli
  2019-12-11 21:02 ` [PATCH 1/2] watchdog: mtx-1: Drop au1000.h header inclusion Florian Fainelli
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Florian Fainelli @ 2019-12-11 21:02 UTC (permalink / raw)
  To: linux-watchdog
  Cc: Florian Fainelli, Wim Van Sebroeck, Guenter Roeck, linux-kernel,
	linux-mips, Paul Burton, Denis Efremov

Hi Wim, Guenter,

This came up with Denis trying to fix a MIPS-related build failure:

https://lore.kernel.org/linux-mips/20191210172739.27131-1-efremov@linux.com/

Florian Fainelli (2):
  watchdog: mtx-1: Drop au1000.h header inclusion
  watchdog: Relax dependencies for CONFIG_WDT_MTX1

 drivers/watchdog/Kconfig     | 2 +-
 drivers/watchdog/mtx-1_wdt.c | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] watchdog: mtx-1: Drop au1000.h header inclusion
  2019-12-11 21:02 [PATCH 0/2] watchdog: mtx-1: Relax build dependencies Florian Fainelli
@ 2019-12-11 21:02 ` Florian Fainelli
  2019-12-12  1:35   ` Guenter Roeck
  2019-12-11 21:02 ` [PATCH 2/2] watchdog: Relax dependencies for CONFIG_WDT_MTX1 Florian Fainelli
  2019-12-11 23:46 ` [PATCH 0/2] watchdog: mtx-1: Relax build dependencies Denis Efremov
  2 siblings, 1 reply; 10+ messages in thread
From: Florian Fainelli @ 2019-12-11 21:02 UTC (permalink / raw)
  To: linux-watchdog
  Cc: Florian Fainelli, Wim Van Sebroeck, Guenter Roeck, linux-kernel,
	linux-mips, Paul Burton, Denis Efremov

Including au1000.h from the machine specific header directory prevents
this driver from being built on any other platforms (MIPS included).
Since we do not use any definitions, drop it.

Reported-by: Denis Efremov <efremov@linux.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/watchdog/mtx-1_wdt.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/watchdog/mtx-1_wdt.c b/drivers/watchdog/mtx-1_wdt.c
index 25a92857b217..aeca22f7450e 100644
--- a/drivers/watchdog/mtx-1_wdt.c
+++ b/drivers/watchdog/mtx-1_wdt.c
@@ -41,8 +41,6 @@
 #include <linux/uaccess.h>
 #include <linux/gpio/consumer.h>
 
-#include <asm/mach-au1x00/au1000.h>
-
 #define MTX1_WDT_INTERVAL	(5 * HZ)
 
 static int ticks = 100 * HZ;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] watchdog: Relax dependencies for CONFIG_WDT_MTX1
  2019-12-11 21:02 [PATCH 0/2] watchdog: mtx-1: Relax build dependencies Florian Fainelli
  2019-12-11 21:02 ` [PATCH 1/2] watchdog: mtx-1: Drop au1000.h header inclusion Florian Fainelli
@ 2019-12-11 21:02 ` Florian Fainelli
  2019-12-11 23:46 ` [PATCH 0/2] watchdog: mtx-1: Relax build dependencies Denis Efremov
  2 siblings, 0 replies; 10+ messages in thread
From: Florian Fainelli @ 2019-12-11 21:02 UTC (permalink / raw)
  To: linux-watchdog
  Cc: Florian Fainelli, Wim Van Sebroeck, Guenter Roeck, linux-kernel,
	linux-mips, Paul Burton, Denis Efremov

Now that we have dropped the inclusion of a machine specific header, we
can allow the driver to be compile tested beyond just MIPS.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/watchdog/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 1679e0dc869b..982897ff074e 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1651,7 +1651,7 @@ config JZ4740_WDT
 
 config WDT_MTX1
 	tristate "MTX-1 Hardware Watchdog"
-	depends on MIPS_MTX1 || (MIPS && COMPILE_TEST)
+	depends on MIPS_MTX1 || COMPILE_TEST
 	help
 	  Hardware driver for the MTX-1 boards. This is a watchdog timer that
 	  will reboot the machine after a 100 seconds timer expired.
-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] watchdog: mtx-1: Relax build dependencies
  2019-12-11 21:02 [PATCH 0/2] watchdog: mtx-1: Relax build dependencies Florian Fainelli
  2019-12-11 21:02 ` [PATCH 1/2] watchdog: mtx-1: Drop au1000.h header inclusion Florian Fainelli
  2019-12-11 21:02 ` [PATCH 2/2] watchdog: Relax dependencies for CONFIG_WDT_MTX1 Florian Fainelli
@ 2019-12-11 23:46 ` Denis Efremov
  2019-12-12  1:39   ` Guenter Roeck
  2 siblings, 1 reply; 10+ messages in thread
From: Denis Efremov @ 2019-12-11 23:46 UTC (permalink / raw)
  To: Florian Fainelli, linux-watchdog
  Cc: Wim Van Sebroeck, Guenter Roeck, open list, linux-mips, Paul Burton

Hi,

Thanks for the fix.
I tested the compilation with these patches.
You can add my:
Tested-by: Denis Efremov <efremov@linux.com>

Look like this error could be fixed the same way:
In file included from drivers/watchdog/ar7_wdt.c:29:
./arch/mips/include/asm/mach-ar7/ar7.h: In function ‘ar7_is_titan’:
./arch/mips/include/asm/mach-ar7/ar7.h:111:24: error: implicit declaration of function ‘KSEG1ADDR’; did you mean ‘CKSEG1ADDR’? [-Werror=implicit-function-declaration]

On 12.12.2019 00:02, Florian Fainelli wrote:
> Hi Wim, Guenter,
> 
> This came up with Denis trying to fix a MIPS-related build failure:
> 
> https://lore.kernel.org/linux-mips/20191210172739.27131-1-efremov@linux.com/
> 
> Florian Fainelli (2):
>   watchdog: mtx-1: Drop au1000.h header inclusion
>   watchdog: Relax dependencies for CONFIG_WDT_MTX1
> 
>  drivers/watchdog/Kconfig     | 2 +-
>  drivers/watchdog/mtx-1_wdt.c | 2 --
>  2 files changed, 1 insertion(+), 3 deletions(-)
>

Thanks,
Denis
 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] watchdog: mtx-1: Drop au1000.h header inclusion
  2019-12-11 21:02 ` [PATCH 1/2] watchdog: mtx-1: Drop au1000.h header inclusion Florian Fainelli
@ 2019-12-12  1:35   ` Guenter Roeck
  2019-12-12  3:38     ` Florian Fainelli
  0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2019-12-12  1:35 UTC (permalink / raw)
  To: Florian Fainelli, linux-watchdog
  Cc: Wim Van Sebroeck, open list, linux-mips, Paul Burton, Denis Efremov

On 12/11/19 1:02 PM, Florian Fainelli wrote:
> Including au1000.h from the machine specific header directory prevents
> this driver from being built on any other platforms (MIPS included).
> Since we do not use any definitions, drop it.
> 
> Reported-by: Denis Efremov <efremov@linux.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>   drivers/watchdog/mtx-1_wdt.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/watchdog/mtx-1_wdt.c b/drivers/watchdog/mtx-1_wdt.c
> index 25a92857b217..aeca22f7450e 100644
> --- a/drivers/watchdog/mtx-1_wdt.c
> +++ b/drivers/watchdog/mtx-1_wdt.c
> @@ -41,8 +41,6 @@
>   #include <linux/uaccess.h>
>   #include <linux/gpio/consumer.h>
>   
> -#include <asm/mach-au1x00/au1000.h>
> -
>   #define MTX1_WDT_INTERVAL	(5 * HZ)
>   
>   static int ticks = 100 * HZ;
> 

Given that this is nothing but yet another gpio watchdog driver, I'd
personally rather have it merged with gpio_wdt.c. On a higher level,
cleaning up old-style watchdog drivers, without converting them to
using the watchdog core, is a waste of time.

Wim, should we make it a policy to reject patches into old-style drivers
unless they fix a real bug ? It is getting a pain to have to review those
patches.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] watchdog: mtx-1: Relax build dependencies
  2019-12-11 23:46 ` [PATCH 0/2] watchdog: mtx-1: Relax build dependencies Denis Efremov
@ 2019-12-12  1:39   ` Guenter Roeck
  2019-12-12  3:41     ` Florian Fainelli
  0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2019-12-12  1:39 UTC (permalink / raw)
  To: Denis Efremov, Florian Fainelli, linux-watchdog
  Cc: Wim Van Sebroeck, open list, linux-mips, Paul Burton

On 12/11/19 3:46 PM, Denis Efremov wrote:
> Hi,
> 
> Thanks for the fix.
> I tested the compilation with these patches.
> You can add my:
> Tested-by: Denis Efremov <efremov@linux.com>
> 
> Look like this error could be fixed the same way:
> In file included from drivers/watchdog/ar7_wdt.c:29:
> ./arch/mips/include/asm/mach-ar7/ar7.h: In function ‘ar7_is_titan’:
> ./arch/mips/include/asm/mach-ar7/ar7.h:111:24: error: implicit declaration of function ‘KSEG1ADDR’; did you mean ‘CKSEG1ADDR’? [-Werror=implicit-function-declaration]
> 

This is yet another old-style watchdog driver which should be left alone
unless it has a bug that needs to be fixed. Really, if anyone out there
is still using this driver, converting it to use the watchdog core
would make much more sense.

Guenter



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] watchdog: mtx-1: Drop au1000.h header inclusion
  2019-12-12  1:35   ` Guenter Roeck
@ 2019-12-12  3:38     ` Florian Fainelli
  2019-12-12 18:19       ` Guenter Roeck
  0 siblings, 1 reply; 10+ messages in thread
From: Florian Fainelli @ 2019-12-12  3:38 UTC (permalink / raw)
  To: Guenter Roeck, linux-watchdog
  Cc: Wim Van Sebroeck, open list, linux-mips, Paul Burton, Denis Efremov



On 12/11/2019 5:35 PM, Guenter Roeck wrote:
> On 12/11/19 1:02 PM, Florian Fainelli wrote:
>> Including au1000.h from the machine specific header directory prevents
>> this driver from being built on any other platforms (MIPS included).
>> Since we do not use any definitions, drop it.
>>
>> Reported-by: Denis Efremov <efremov@linux.com>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>   drivers/watchdog/mtx-1_wdt.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/watchdog/mtx-1_wdt.c b/drivers/watchdog/mtx-1_wdt.c
>> index 25a92857b217..aeca22f7450e 100644
>> --- a/drivers/watchdog/mtx-1_wdt.c
>> +++ b/drivers/watchdog/mtx-1_wdt.c
>> @@ -41,8 +41,6 @@
>>   #include <linux/uaccess.h>
>>   #include <linux/gpio/consumer.h>
>>   -#include <asm/mach-au1x00/au1000.h>
>> -
>>   #define MTX1_WDT_INTERVAL    (5 * HZ)
>>     static int ticks = 100 * HZ;
>>
> 
> Given that this is nothing but yet another gpio watchdog driver, I'd
> personally rather have it merged with gpio_wdt.c. On a higher level,
> cleaning up old-style watchdog drivers, without converting them to
> using the watchdog core, is a waste of time.

If that makes you feel any better, I was not planning on going further
than that, and yes, removing this driver and using gpio_wdt.c would be
the way to go, this driver greatly predates gpio_wdt.c and I have since
then not had access to my MTX-1 platforms which is why this did not
happen. We can attempt a "blind conversion" without testing, but what
good would that make, not sure.

> 
> Wim, should we make it a policy to reject patches into old-style drivers
> unless they fix a real bug ? It is getting a pain to have to review those
> patches.
> 
> Thanks,
> Guenter

-- 
Florian

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] watchdog: mtx-1: Relax build dependencies
  2019-12-12  1:39   ` Guenter Roeck
@ 2019-12-12  3:41     ` Florian Fainelli
  0 siblings, 0 replies; 10+ messages in thread
From: Florian Fainelli @ 2019-12-12  3:41 UTC (permalink / raw)
  To: Guenter Roeck, Denis Efremov, linux-watchdog
  Cc: Wim Van Sebroeck, open list, linux-mips, Paul Burton



On 12/11/2019 5:39 PM, Guenter Roeck wrote:
> On 12/11/19 3:46 PM, Denis Efremov wrote:
>> Hi,
>>
>> Thanks for the fix.
>> I tested the compilation with these patches.
>> You can add my:
>> Tested-by: Denis Efremov <efremov@linux.com>
>>
>> Look like this error could be fixed the same way:
>> In file included from drivers/watchdog/ar7_wdt.c:29:
>> ./arch/mips/include/asm/mach-ar7/ar7.h: In function ‘ar7_is_titan’:
>> ./arch/mips/include/asm/mach-ar7/ar7.h:111:24: error: implicit
>> declaration of function ‘KSEG1ADDR’; did you mean ‘CKSEG1ADDR’?
>> [-Werror=implicit-function-declaration]
>>
> 
> This is yet another old-style watchdog driver which should be left alone
> unless it has a bug that needs to be fixed. Really, if anyone out there
> is still using this driver, converting it to use the watchdog core
> would make much more sense.

AR7 is still largely available, so we might be able to get some people
within the OpenWrt community to give a watchdog modernization patch a try.
-- 
Florian

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] watchdog: mtx-1: Drop au1000.h header inclusion
  2019-12-12  3:38     ` Florian Fainelli
@ 2019-12-12 18:19       ` Guenter Roeck
  2019-12-12 18:28         ` Florian Fainelli
  0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2019-12-12 18:19 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: linux-watchdog, Wim Van Sebroeck, open list, linux-mips,
	Paul Burton, Denis Efremov

On Wed, Dec 11, 2019 at 07:38:29PM -0800, Florian Fainelli wrote:
> 
> 
> On 12/11/2019 5:35 PM, Guenter Roeck wrote:
> > On 12/11/19 1:02 PM, Florian Fainelli wrote:
> >> Including au1000.h from the machine specific header directory prevents
> >> this driver from being built on any other platforms (MIPS included).
> >> Since we do not use any definitions, drop it.
> >>
> >> Reported-by: Denis Efremov <efremov@linux.com>
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> >>   drivers/watchdog/mtx-1_wdt.c | 2 --
> >>   1 file changed, 2 deletions(-)
> >>
> >> diff --git a/drivers/watchdog/mtx-1_wdt.c b/drivers/watchdog/mtx-1_wdt.c
> >> index 25a92857b217..aeca22f7450e 100644
> >> --- a/drivers/watchdog/mtx-1_wdt.c
> >> +++ b/drivers/watchdog/mtx-1_wdt.c
> >> @@ -41,8 +41,6 @@
> >>   #include <linux/uaccess.h>
> >>   #include <linux/gpio/consumer.h>
> >>   -#include <asm/mach-au1x00/au1000.h>
> >> -
> >>   #define MTX1_WDT_INTERVAL    (5 * HZ)
> >>     static int ticks = 100 * HZ;
> >>
> > 
> > Given that this is nothing but yet another gpio watchdog driver, I'd
> > personally rather have it merged with gpio_wdt.c. On a higher level,
> > cleaning up old-style watchdog drivers, without converting them to
> > using the watchdog core, is a waste of time.
> 
> If that makes you feel any better, I was not planning on going further
> than that, and yes, removing this driver and using gpio_wdt.c would be
> the way to go, this driver greatly predates gpio_wdt.c and I have since
> then not had access to my MTX-1 platforms which is why this did not
> happen. We can attempt a "blind conversion" without testing, but what
> good would that make, not sure.
> 

It sounds like this is a purely cosmetical change to improve test build
coverage for a more or less obsolete driver. No, that doesn't make me feel
better; I get way too many of those lately. Worse, some of those test build
"improvements" actually end up breaking real builds, which then costs me
and others even more time to track down.

We should really discourage that. Is there some challenge going on somewhere,
along the line of "improve test build coverage" ?

Guenter

> > 
> > Wim, should we make it a policy to reject patches into old-style drivers
> > unless they fix a real bug ? It is getting a pain to have to review those
> > patches.
> > 
> > Thanks,
> > Guenter
> 
> -- 
> Florian

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] watchdog: mtx-1: Drop au1000.h header inclusion
  2019-12-12 18:19       ` Guenter Roeck
@ 2019-12-12 18:28         ` Florian Fainelli
  0 siblings, 0 replies; 10+ messages in thread
From: Florian Fainelli @ 2019-12-12 18:28 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: linux-watchdog, Wim Van Sebroeck, open list, linux-mips,
	Paul Burton, Denis Efremov

On 12/12/19 10:19 AM, Guenter Roeck wrote:
> On Wed, Dec 11, 2019 at 07:38:29PM -0800, Florian Fainelli wrote:
>>
>>
>> On 12/11/2019 5:35 PM, Guenter Roeck wrote:
>>> On 12/11/19 1:02 PM, Florian Fainelli wrote:
>>>> Including au1000.h from the machine specific header directory prevents
>>>> this driver from being built on any other platforms (MIPS included).
>>>> Since we do not use any definitions, drop it.
>>>>
>>>> Reported-by: Denis Efremov <efremov@linux.com>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>>>   drivers/watchdog/mtx-1_wdt.c | 2 --
>>>>   1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/mtx-1_wdt.c b/drivers/watchdog/mtx-1_wdt.c
>>>> index 25a92857b217..aeca22f7450e 100644
>>>> --- a/drivers/watchdog/mtx-1_wdt.c
>>>> +++ b/drivers/watchdog/mtx-1_wdt.c
>>>> @@ -41,8 +41,6 @@
>>>>   #include <linux/uaccess.h>
>>>>   #include <linux/gpio/consumer.h>
>>>>   -#include <asm/mach-au1x00/au1000.h>
>>>> -
>>>>   #define MTX1_WDT_INTERVAL    (5 * HZ)
>>>>     static int ticks = 100 * HZ;
>>>>
>>>
>>> Given that this is nothing but yet another gpio watchdog driver, I'd
>>> personally rather have it merged with gpio_wdt.c. On a higher level,
>>> cleaning up old-style watchdog drivers, without converting them to
>>> using the watchdog core, is a waste of time.
>>
>> If that makes you feel any better, I was not planning on going further
>> than that, and yes, removing this driver and using gpio_wdt.c would be
>> the way to go, this driver greatly predates gpio_wdt.c and I have since
>> then not had access to my MTX-1 platforms which is why this did not
>> happen. We can attempt a "blind conversion" without testing, but what
>> good would that make, not sure.
>>
> 
> It sounds like this is a purely cosmetical change to improve test build
> coverage for a more or less obsolete driver. No, that doesn't make me feel
> better; I get way too many of those lately. Worse, some of those test build
> "improvements" actually end up breaking real builds, which then costs me
> and others even more time to track down.
> 
> We should really discourage that. Is there some challenge going on somewhere,
> along the line of "improve test build coverage" ?

Not really, the only challenge would be access to the original hardware
in order to remove the driver and migrate over to gpio_wdt, which is low
risk, but the watchdog on that platform has bitten me before.
-- 
Florian

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-11 21:02 [PATCH 0/2] watchdog: mtx-1: Relax build dependencies Florian Fainelli
2019-12-11 21:02 ` [PATCH 1/2] watchdog: mtx-1: Drop au1000.h header inclusion Florian Fainelli
2019-12-12  1:35   ` Guenter Roeck
2019-12-12  3:38     ` Florian Fainelli
2019-12-12 18:19       ` Guenter Roeck
2019-12-12 18:28         ` Florian Fainelli
2019-12-11 21:02 ` [PATCH 2/2] watchdog: Relax dependencies for CONFIG_WDT_MTX1 Florian Fainelli
2019-12-11 23:46 ` [PATCH 0/2] watchdog: mtx-1: Relax build dependencies Denis Efremov
2019-12-12  1:39   ` Guenter Roeck
2019-12-12  3:41     ` Florian Fainelli

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git