[PATCH 1/3] checkpatch: add Kconfig 'default n' test

[PATCH 1/3] checkpatch: add Kconfig 'default n' test

[Yingjoe Chen]

If a Kconfig config option doesn't specify 'default', the default
will be n. Adding 'default n' is unnecessary.

Add a test to warn about this.

Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
 scripts/checkpatch.pl | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d574d13..1c43dc1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2600,6 +2600,13 @@ sub process {
 			     "Use of boolean is deprecated, please use bool instead.\n" . $herecurr);
 		}
 
+# discourage the use of default n
+		if ($realfile =~ /Kconfig/ &&
+		    $line =~ /^\+\s*\bdefault\b\s*n\s*$/) {
+			WARN("CONFIG_DEFAULT_N",
+			     "Use of default n is unnecessary, default is n when omitted.\n" . $herecurr);
+		}
+
 		if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
 		    ($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
 			my $flag = $1;
-- 
1.9.1

[PATCH 2/3] checkpatch: testing more config for Kconfig help text

[Yingjoe Chen]

Current help text check only check a config option if it is followed
by another config.
Adding check for help text if the next entry is menuconfig, choice/
endchoice, comment, menu/endmenu, if/endif, source or end of file.

Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
 scripts/checkpatch.pl | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1c43dc1..2bf4499 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2563,6 +2563,12 @@ sub process {
 				next if ($f =~ /^-/);
 				last if (!$file && $f =~ /^\@\@/);
 
+				if ($f !~ /^[+\- ]/) {
+					# End of file
+					$is_end = 1;
+					last;
+				}
+
 				if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate)\s*\"/) {
 					$is_start = 1;
 				} elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) {
@@ -2573,7 +2579,10 @@ sub process {
 				$f =~ s/#.*//;
 				$f =~ s/^\s+//;
 				next if ($f =~ /^$/);
-				if ($f =~ /^\s*config\s/) {
+				if ($f =~ /^\s*config\s/ || $f =~ /^\s*menuconfig\s/ || $f =~ /^\s*choice\s/ ||
+				    $f =~ /^\s*endchoice$/ || $f =~ /^\s*comment\s/ || $f =~ /^\s*menu\s/ ||
+				    $f =~ /^\s*endmenu$/ || $f =~ /^\s*if\s/ || $f =~ /^\s*endif$/ ||
+				    $f =~ /^\s*source\s/) {
 					$is_end = 1;
 					last;
 				}
-- 
1.9.1

[PATCH 3/3] checkpatch: relax Kconfig help text line number threshold

[Yingjoe Chen]

Current threshold is too strict and many upstream patch doesn't pass
this test. Relax it.

Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>

---
In v4.6-rc1, 171 new config options was added, and 87 of those options
have < 4 lines and 24 options have only 1 line. After this change,
checkpatch only raise warning when help text only contain 1 line.

Some options try to workaround this check by adding 2 lines
template like 'If you have this device...' which doesn't add value.

---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2bf4499..33b9bfc 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -47,7 +47,7 @@ my $configuration_file = ".checkpatch.conf";
 my $max_line_length = 80;
 my $ignore_perl_version = 0;
 my $minimum_perl_version = 5.10.0;
-my $min_conf_desc_length = 4;
+my $min_conf_desc_length = 2;
 my $spelling_file = "$D/spelling.txt";
 my $codespell = 0;
 my $codespellfile = "/usr/share/codespell/dictionary.txt";
-- 
1.9.1

Re: [PATCH 3/3] checkpatch: relax Kconfig help text line number threshold

[Joe Perches]

On Thu, 2016-04-21 at 21:28 +0800, Yingjoe Chen wrote:
> Current threshold is too strict and many upstream patch doesn't pass
> this test. Relax it.

I don't have an issue with this.
Maybe Andi Kleen does though.

> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> 
> ---
> In v4.6-rc1, 171 new config options was added, and 87 of those options
> have < 4 lines and 24 options have only 1 line. After this change,
> checkpatch only raise warning when help text only contain 1 line.
> 
> Some options try to workaround this check by adding 2 lines
> template like 'If you have this device...' which doesn't add value.
> 
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 2bf4499..33b9bfc 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -47,7 +47,7 @@ my $configuration_file = ".checkpatch.conf";
>  my $max_line_length = 80;
>  my $ignore_perl_version = 0;
>  my $minimum_perl_version = 5.10.0;
> -my $min_conf_desc_length = 4;
> +my $min_conf_desc_length = 2;
>  my $spelling_file = "$D/spelling.txt";
>  my $codespell = 0;
>  my $codespellfile = "/usr/share/codespell/dictionary.txt";

Re: [PATCH 2/3] checkpatch: testing more config for Kconfig help text

[Joe Perches]

On Thu, 2016-04-21 at 21:28 +0800, Yingjoe Chen wrote:
> Current help text check only check a config option if it is followed
> by another config.
> Adding check for help text if the next entry is menuconfig, choice/
> endchoice, comment, menu/endmenu, if/endif, source or end of file.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -2563,6 +2563,12 @@ sub process {
>  				next if ($f =~ /^-/);
>  				last if (!$file && $f =~ /^\@\@/);
>  
> +				if ($f !~ /^[+\- ]/) {
> +					# End of file
> +					$is_end = 1;
> +					last;
> +				}
> +
>  				if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate)\s*\"/) {
>  					$is_start = 1;
>  				} elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) {
> @@ -2573,7 +2579,10 @@ sub process {
>  				$f =~ s/#.*//;
>  				$f =~ s/^\s+//;
>  				next if ($f =~ /^$/);
> -				if ($f =~ /^\s*config\s/) {
> +				if ($f =~ /^\s*config\s/ || $f =~ /^\s*menuconfig\s/ || $f =~ /^\s*choice\s/ ||
> +				    $f =~ /^\s*endchoice$/ || $f =~ /^\s*comment\s/ || $f =~ /^\s*menu\s/ ||
> +				    $f =~ /^\s*endmenu$/ || $f =~ /^\s*if\s/ || $f =~ /^\s*endif$/ ||
> +				    $f =~ /^\s*source\s/) {
>  					$is_end = 1;
>  					last;
>  				}

This seems relatively verbose.
Also, because there's a substitution above that strips leading spaces,
the "^\s*" uses are unnecessary and can be simplified to ^

Maybe:
if ($f =~ /^(?:config\s|menuconfig\s|choice\s|endchoice\s*$|comment\s|menu\s|endmenu$|if\s|endif\s*$|source\s)

Re: [PATCH 1/3] checkpatch: add Kconfig 'default n' test

[Joe Perches]

On Thu, 2016-04-21 at 21:28 +0800, Yingjoe Chen wrote:
> If a Kconfig config option doesn't specify 'default', the default
> will be n. Adding 'default n' is unnecessary.
> 
> Add a test to warn about this.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -2600,6 +2600,13 @@ sub process {
>  			     "Use of boolean is deprecated, please use bool instead.\n" . $herecurr);
>  		}
>  
> +# discourage the use of default n
> +		if ($realfile =~ /Kconfig/ &&
> +		    $line =~ /^\+\s*\bdefault\b\s*n\s*$/) {

maybe line =~ /^\+\s*default\s+n/i

Also, there are some oddities like:

arch/mips/cavium-octeon/Kconfig:        default "n"

and a dozen or so uses like

	default n if <foo>

Re: [PATCH 3/3] checkpatch: relax Kconfig help text line number threshold

[Andi Kleen]

On Thu, Apr 21, 2016 at 10:00:07AM -0700, Joe Perches wrote:
> On Thu, 2016-04-21 at 21:28 +0800, Yingjoe Chen wrote:
> > Current threshold is too strict and many upstream patch doesn't pass
> > this test. Relax it.
> 
> I don't have an issue with this.
> Maybe Andi Kleen does though.

So you spend all this time developing your kernel feature and can't be bothered to
write a real description? How should people even find out about it?

-Andi

Re: [PATCH 1/3] checkpatch: add Kconfig 'default n' test

[Paul Bolle]

On do, 2016-04-21 at 10:16 -0700, Joe Perches wrote:
> Also, there are some oddities like:
> 
> arch/mips/cavium-octeon/Kconfig:        default "n"

For v4.6-rc4:
    $ git grep -n -e "default\s\+\"[mny]\"" -- "*Kconfig*"
    arch/mips/Kconfig:2232: default "y"
    arch/mips/Kconfig:2237: default "y"
    arch/mips/Kconfig:2257: default "y"
    arch/mips/Kconfig:2262: default "y"
    arch/mips/cavium-octeon/Kconfig:5:      default "n"
    arch/mips/cavium-octeon/Kconfig:30:     default "n"
    arch/mips/cavium-octeon/Kconfig:39:     default "y"
    arch/mips/cavium-octeon/Kconfig:46:     default "y"
    arch/mips/cavium-octeon/Kconfig:53:     default "y"
    arch/mips/cavium-octeon/Kconfig:60:     default "y"
    arch/mips/cavium-octeon/Kconfig:67:     default "y"
    arch/mips/cavium-octeon/Kconfig:74:     default "y"
    arch/powerpc/Kconfig:435:       default "y" if PPC_POWERNV
    arch/powerpc/Kconfig:658:       default "y" if PPC_POWERNV
    arch/powerpc/Kconfig:869:       default "n"
    drivers/crypto/Kconfig:142:     default "m"
    drivers/misc/Kconfig:784:       default "n"
    drivers/rapidio/devices/Kconfig:8:      default "n"

These appear to behave as intended, but I still think the quotes should
be dropped. (A brave soul might want to dive into the kconfig internals
and see whether they can be made less liberal in their parsing of
Kconfig files in this regard. I'm not volunteering.)


Paul Bolle

Re: [PATCH 3/3] checkpatch: relax Kconfig help text line number threshold

[Yingjoe Chen]

On Thu, 2016-04-21 at 10:39 -0700, Andi Kleen wrote:
> On Thu, Apr 21, 2016 at 10:00:07AM -0700, Joe Perches wrote:
> > On Thu, 2016-04-21 at 21:28 +0800, Yingjoe Chen wrote:
> > > Current threshold is too strict and many upstream patch doesn't pass
> > > this test. Relax it.
> > 
> > I don't have an issue with this.
> > Maybe Andi Kleen does though.
> 
> So you spend all this time developing your kernel feature and can't be bothered to
> write a real description? How should people even find out about it?


I think it depends. For some features it would be helpful to have more
description. However for many device drivers, 2 lines is enough. For
example, I think the following help text is helpful and clean.

+config PINCTRL_IPQ4019
+       tristate "Qualcomm IPQ4019 pin controller driver"
+       depends on GPIOLIB && OF
+       select PINCTRL_MSM
+       help
+         This is the pinctrl, pinmux, pinconf and gpiolib driver for the
+         Qualcomm TLMM block found in the Qualcomm IPQ4019 platform.
+
+config SND_SOC_PCM179X_I2C
+       tristate "Texas Instruments PCM179X CODEC (I2C)"
+       depends on I2C
+       select SND_SOC_PCM179X
+       help
+         Enable support for Texas Instruments PCM179x CODEC.
+         Select this if your PCM179x is connected via an I2C bus.

Joe.C

Re: [PATCH 2/3] checkpatch: testing more config for Kconfig help text

[Yingjoe Chen]

On Thu, 2016-04-21 at 10:06 -0700, Joe Perches wrote:
> On Thu, 2016-04-21 at 21:28 +0800, Yingjoe Chen wrote:
> > Current help text check only check a config option if it is followed
> > by another config.
> > Adding check for help text if the next entry is menuconfig, choice/
> > endchoice, comment, menu/endmenu, if/endif, source or end of file.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -2563,6 +2563,12 @@ sub process {
> >  				next if ($f =~ /^-/);
> >  				last if (!$file && $f =~ /^\@\@/);
> >  
> > +				if ($f !~ /^[+\- ]/) {
> > +					# End of file
> > +					$is_end = 1;
> > +					last;
> > +				}
> > +
> >  				if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate)\s*\"/) {
> >  					$is_start = 1;
> >  				} elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) {
> > @@ -2573,7 +2579,10 @@ sub process {
> >  				$f =~ s/#.*//;
> >  				$f =~ s/^\s+//;
> >  				next if ($f =~ /^$/);
> > -				if ($f =~ /^\s*config\s/) {
> > +				if ($f =~ /^\s*config\s/ || $f =~ /^\s*menuconfig\s/ || $f =~ /^\s*choice\s/ ||
> > +				    $f =~ /^\s*endchoice$/ || $f =~ /^\s*comment\s/ || $f =~ /^\s*menu\s/ ||
> > +				    $f =~ /^\s*endmenu$/ || $f =~ /^\s*if\s/ || $f =~ /^\s*endif$/ ||
> > +				    $f =~ /^\s*source\s/) {
> >  					$is_end = 1;
> >  					last;
> >  				}
> 
> This seems relatively verbose.
> Also, because there's a substitution above that strips leading spaces,
> the "^\s*" uses are unnecessary and can be simplified to ^
> 
> Maybe:
> if ($f =~ /^(?:config\s|menuconfig\s|choice\s|endchoice\s*$|comment\s|menu\s|endmenu$|if\s|endif\s*$|source\s)
> 

Thanks
Strip apply to trailing spaces as well, will change this to:

if ($f =~ /^(?:config\s|menuconfig\s|choice\s|endchoice$|comment\s|menu\s|endmenu$|if\s|endif$|source\s)/)


Joe.C

Re: [PATCH 1/3] checkpatch: add Kconfig 'default n' test

[Yingjoe Chen]

On Thu, 2016-04-21 at 10:16 -0700, Joe Perches wrote:
> On Thu, 2016-04-21 at 21:28 +0800, Yingjoe Chen wrote:
> > If a Kconfig config option doesn't specify 'default', the default
> > will be n. Adding 'default n' is unnecessary.
> > 
> > Add a test to warn about this.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -2600,6 +2600,13 @@ sub process {
> >  			     "Use of boolean is deprecated, please use bool instead.\n" . $herecurr);
> >  		}
> >  
> > +# discourage the use of default n
> > +		if ($realfile =~ /Kconfig/ &&
> > +		    $line =~ /^\+\s*\bdefault\b\s*n\s*$/) {
> 
> maybe line =~ /^\+\s*default\s+n/i


This might match

	default NIOS2

also this won't match lines with comment.
Will change to  $line =~ /^\+\s*default\s*n\s*(#.*|$)/i


> Also, there are some oddities like:
> 
> arch/mips/cavium-octeon/Kconfig:        default "n"


I think this belong to another (new) test: If an option is bool or
tristate, the default value shouldn't be " quoted.


> and a dozen or so uses like
> 
> 	default n if <foo>
> 

I'm not sure about this. With if, it did provide more information.
According to Documentation/kbuild/kconfig-language.txt:
" A config option can have any number of default values. If multiple
  default values are visible, only the first defined one is active."
So the following example will be different when the first one is
removed.

	default n if <foo>
	default y if <bar>

of course we could do the following, but one could argue the above one
is more readable.

	default y if <bar> && ! <foo>

Joe.C