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 <[email protected]>
---
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
Current threshold is too strict and many upstream patch doesn't pass
this test. Relax it.
Signed-off-by: Yingjoe Chen <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
>
> ---
> 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";
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)
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>
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
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
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
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
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