| Message ID | 20191211210204.31579-2-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | watchdog: mtx-1: Relax build dependencies | expand |
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
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
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
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.
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;
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(-)