[12/21] clk: sunxi: clk-sun6i-ar100: Demote non-conformant kernel-doc header

Commit Message

Lee Jones Jan. 26, 2021, 12:45 p.m. UTC

Fixes the following W=1 kernel build warning(s):

 drivers/clk/sunxi/clk-sun6i-ar100.c:26: warning: Function parameter or member 'req' not described in 'sun6i_get_ar100_factors'

Cc: "Emilio López" <[e-mail]>
Cc: Michael Turquette <[e-mail]>
Cc: Stephen Boyd <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: Chen-Yu Tsai <[e-mail]>
Cc: Jernej Skrabec <[e-mail]>
Cc: Boris BREZILLON <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Lee Jones <[e-mail]>
---
 drivers/clk/sunxi/clk-sun6i-ar100.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Maxime Ripard Jan. 26, 2021, 3:54 p.m. UTC | #1

On Tue, Jan 26, 2021 at 12:45:31PM +0000, Lee Jones wrote:
> Fixes the following W=1 kernel build warning(s):
> 
>  drivers/clk/sunxi/clk-sun6i-ar100.c:26: warning: Function parameter or member 'req' not described in 'sun6i_get_ar100_factors'
> 
> Cc: "Emilio López" <[e-mail]>
> Cc: Michael Turquette <[e-mail]>
> Cc: Stephen Boyd <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Chen-Yu Tsai <[e-mail]>
> Cc: Jernej Skrabec <[e-mail]>
> Cc: Boris BREZILLON <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Lee Jones <[e-mail]>
> ---
>  drivers/clk/sunxi/clk-sun6i-ar100.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/sunxi/clk-sun6i-ar100.c b/drivers/clk/sunxi/clk-sun6i-ar100.c
> index e1b7d0929cf7f..54babc2b4b9ee 100644
> --- a/drivers/clk/sunxi/clk-sun6i-ar100.c
> +++ b/drivers/clk/sunxi/clk-sun6i-ar100.c
> @@ -16,7 +16,7 @@
>  
>  #include "clk-factors.h"
>  
> -/**
> +/*
>   * sun6i_get_ar100_factors - Calculates factors p, m for AR100
>   *
>   * AR100 rate is calculated as follows

This is the sixth patch doing the exact same thing over the files in
that folder you sent. Please fix all the occurences at once

Maxime

Lee Jones Jan. 26, 2021, 4:54 p.m. UTC | #2

On Tue, 26 Jan 2021, Maxime Ripard wrote:

> On Tue, Jan 26, 2021 at 12:45:31PM +0000, Lee Jones wrote:
> > Fixes the following W=1 kernel build warning(s):
> > 
> >  drivers/clk/sunxi/clk-sun6i-ar100.c:26: warning: Function parameter or member 'req' not described in 'sun6i_get_ar100_factors'
> > 
> > Cc: "Emilio López" <[e-mail]>
> > Cc: Michael Turquette <[e-mail]>
> > Cc: Stephen Boyd <[e-mail]>
> > Cc: Maxime Ripard <[e-mail]>
> > Cc: Chen-Yu Tsai <[e-mail]>
> > Cc: Jernej Skrabec <[e-mail]>
> > Cc: Boris BREZILLON <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Signed-off-by: Lee Jones <[e-mail]>
> > ---
> >  drivers/clk/sunxi/clk-sun6i-ar100.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/sunxi/clk-sun6i-ar100.c b/drivers/clk/sunxi/clk-sun6i-ar100.c
> > index e1b7d0929cf7f..54babc2b4b9ee 100644
> > --- a/drivers/clk/sunxi/clk-sun6i-ar100.c
> > +++ b/drivers/clk/sunxi/clk-sun6i-ar100.c
> > @@ -16,7 +16,7 @@
> >  
> >  #include "clk-factors.h"
> >  
> > -/**
> > +/*
> >   * sun6i_get_ar100_factors - Calculates factors p, m for AR100
> >   *
> >   * AR100 rate is calculated as follows
> 
> This is the sixth patch doing the exact same thing over the files in
> that folder you sent. Please fix all the occurences at once

No.  That would make the whole clean-up process 10x harder than it
already is

Before starting this endeavour there were 18,000+ warnings spread over
100's of files and 10's of subsystems that needed addressing (only a
couple thousand left now thankfully).  Some issues vastly different,
some duplicated (much too much copy/pasting going which made things
very frustrating at times).

Anyway, in order to work though them all gracefully and in a sensible
time-frame I had to come up with a workable plan.  Each subsystem is
compiled separately and a script attempts to take out duplicate
warnings and takes me through the build-log one file at a time.  Once
all of the warnings are fixed in a source-file, it moves on to the
next file.  The method is clean and allows me to handle this
gargantuan task in bite-sized chunks.

Going though and pairing up similar changes is unsustainable for a
task like this.  It would add a lot of additional overhead and would
slow down the rate of acceptance since source files tend to have
different reviewers/maintainers - some working faster to review
patches than others, leading to excessive lag times waiting for that
one reviewer who takes weeks to review.  Having each file addressed
in a separate patch also helps revertability and bisectability.  Not
such a big problem with the documentation patches, but still.

Admittedly doing it this way *can* look a bit odd in *some* patch-sets
when they hit the MLs - particularly clock it seems, where there
hasn't even been a vague attempt to document any of the parameters in
the kernel-doc headers - however the alternative would mean nothing
would get done!

Maxime Ripard Feb. 3, 2021, 9:27 a.m. UTC | #3

On Tue, Jan 26, 2021 at 04:54:59PM +0000, Lee Jones wrote:
> On Tue, 26 Jan 2021, Maxime Ripard wrote:
> 
> > On Tue, Jan 26, 2021 at 12:45:31PM +0000, Lee Jones wrote:
> > > Fixes the following W=1 kernel build warning(s):
> > > 
> > >  drivers/clk/sunxi/clk-sun6i-ar100.c:26: warning: Function parameter or member 'req' not described in 'sun6i_get_ar100_factors'
> > > 
> > > Cc: "Emilio López" <[e-mail]>
> > > Cc: Michael Turquette <[e-mail]>
> > > Cc: Stephen Boyd <[e-mail]>
> > > Cc: Maxime Ripard <[e-mail]>
> > > Cc: Chen-Yu Tsai <[e-mail]>
> > > Cc: Jernej Skrabec <[e-mail]>
> > > Cc: Boris BREZILLON <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > Signed-off-by: Lee Jones <[e-mail]>
> > > ---
> > >  drivers/clk/sunxi/clk-sun6i-ar100.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/sunxi/clk-sun6i-ar100.c b/drivers/clk/sunxi/clk-sun6i-ar100.c
> > > index e1b7d0929cf7f..54babc2b4b9ee 100644
> > > --- a/drivers/clk/sunxi/clk-sun6i-ar100.c
> > > +++ b/drivers/clk/sunxi/clk-sun6i-ar100.c
> > > @@ -16,7 +16,7 @@
> > >  
> > >  #include "clk-factors.h"
> > >  
> > > -/**
> > > +/*
> > >   * sun6i_get_ar100_factors - Calculates factors p, m for AR100
> > >   *
> > >   * AR100 rate is calculated as follows
> > 
> > This is the sixth patch doing the exact same thing over the files in
> > that folder you sent. Please fix all the occurences at once
> 
> No.  That would make the whole clean-up process 10x harder than it
> already is
> 
> Before starting this endeavour there were 18,000+ warnings spread over
> 100's of files and 10's of subsystems that needed addressing (only a
> couple thousand left now thankfully).  Some issues vastly different,
> some duplicated (much too much copy/pasting going which made things
> very frustrating at times).
> 
> Anyway, in order to work though them all gracefully and in a sensible
> time-frame I had to come up with a workable plan.  Each subsystem is
> compiled separately and a script attempts to take out duplicate
> warnings and takes me through the build-log one file at a time.  Once
> all of the warnings are fixed in a source-file, it moves on to the
> next file.  The method is clean and allows me to handle this
> gargantuan task in bite-sized chunks.

I mean, you have literally used the same commit log and the same changes
over six different files in the same directory. Sure changes across
different parts of the kernel can be painful, but it's really not what
we're discussing here.

> Going though and pairing up similar changes is unsustainable for a
> task like this.  It would add a lot of additional overhead and would
> slow down the rate of acceptance since source files tend to have
> different reviewers/maintainers - some working faster to review
> patches than others, leading to excessive lag times waiting for that
> one reviewer who takes weeks to review.

Are you arguing that sending the same patch 6 times is easier and faster
to review for the maintainer than the same changes in a single patch?

> Having each file addressed in a separate patch also helps
> revertability and bisectability. Not such a big problem with the
> documentation patches, but still.

There's nothing to revert or bisect, those changes aren't functional
changes.

> Admittedly doing it this way *can* look a bit odd in *some* patch-sets
> when they hit the MLs - particularly clock it seems, where there
> hasn't even been a vague attempt to document any of the parameters in
> the kernel-doc headers - however the alternative would mean nothing
> would get done!

Yeah, and even though properly documenting the functions would have been
the right way to fix those warnings, I didn't ask you to do that since I
was expecting it to be daunting. Surely we can meet half-way

Maxime

Lee Jones Feb. 3, 2021, 10:09 a.m. UTC | #4

On Wed, 03 Feb 2021, Maxime Ripard wrote:

> On Tue, Jan 26, 2021 at 04:54:59PM +0000, Lee Jones wrote:
> > On Tue, 26 Jan 2021, Maxime Ripard wrote:
> > 
> > > On Tue, Jan 26, 2021 at 12:45:31PM +0000, Lee Jones wrote:
> > > > Fixes the following W=1 kernel build warning(s):
> > > > 
> > > >  drivers/clk/sunxi/clk-sun6i-ar100.c:26: warning: Function parameter or member 'req' not described in 'sun6i_get_ar100_factors'
> > > > 
> > > > Cc: "Emilio López" <[e-mail]>
> > > > Cc: Michael Turquette <[e-mail]>
> > > > Cc: Stephen Boyd <[e-mail]>
> > > > Cc: Maxime Ripard <[e-mail]>
> > > > Cc: Chen-Yu Tsai <[e-mail]>
> > > > Cc: Jernej Skrabec <[e-mail]>
> > > > Cc: Boris BREZILLON <[e-mail]>
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > Signed-off-by: Lee Jones <[e-mail]>
> > > > ---
> > > >  drivers/clk/sunxi/clk-sun6i-ar100.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/clk/sunxi/clk-sun6i-ar100.c b/drivers/clk/sunxi/clk-sun6i-ar100.c
> > > > index e1b7d0929cf7f..54babc2b4b9ee 100644
> > > > --- a/drivers/clk/sunxi/clk-sun6i-ar100.c
> > > > +++ b/drivers/clk/sunxi/clk-sun6i-ar100.c
> > > > @@ -16,7 +16,7 @@
> > > >  
> > > >  #include "clk-factors.h"
> > > >  
> > > > -/**
> > > > +/*
> > > >   * sun6i_get_ar100_factors - Calculates factors p, m for AR100
> > > >   *
> > > >   * AR100 rate is calculated as follows
> > > 
> > > This is the sixth patch doing the exact same thing over the files in
> > > that folder you sent. Please fix all the occurences at once
> > 
> > No.  That would make the whole clean-up process 10x harder than it
> > already is
> > 
> > Before starting this endeavour there were 18,000+ warnings spread over
> > 100's of files and 10's of subsystems that needed addressing (only a
> > couple thousand left now thankfully).  Some issues vastly different,
> > some duplicated (much too much copy/pasting going which made things
> > very frustrating at times).
> > 
> > Anyway, in order to work though them all gracefully and in a sensible
> > time-frame I had to come up with a workable plan.  Each subsystem is
> > compiled separately and a script attempts to take out duplicate
> > warnings and takes me through the build-log one file at a time.  Once
> > all of the warnings are fixed in a source-file, it moves on to the
> > next file.  The method is clean and allows me to handle this
> > gargantuan task in bite-sized chunks.
> 
> I mean, you have literally used the same commit log and the same changes
> over six different files in the same directory.

Yes, that happens.  It's an unfortunate side-effect of the same ol'
issues repeating themselves over and over.  Mostly due to copy/paste
of mundane code segments such as function documentation.

> Sure changes across
> different parts of the kernel can be painful, but it's really not what
> we're discussing here.

It would have even been painful to post-process patches within the
same subsystem.  For instance, I've just finished cleaning up GPU
which was a mammoth task where most of the issues were perpetually
duplicated.

I will admit though, that here in Clock, it would be somewhat easier.

> > Going though and pairing up similar changes is unsustainable for a
> > task like this.  It would add a lot of additional overhead and would
> > slow down the rate of acceptance since source files tend to have
> > different reviewers/maintainers - some working faster to review
> > patches than others, leading to excessive lag times waiting for that
> > one reviewer who takes weeks to review.
> 
> Are you arguing that sending the same patch 6 times is easier and faster
> to review for the maintainer than the same changes in a single patch?

The issue I see with the Clock, is that some files are maintained by
individual driver Maintainers and others by subsystem Maintainers.  So
the post-process here is that much more painful (as it can't be
easily scripted using get_maintainer.pl) and the aforementioned
lag-time issues come into play while we wait for sleepy reviewers.

> > Having each file addressed in a separate patch also helps
> > revertability and bisectability. Not such a big problem with the
> > documentation patches, but still.
> 
> There's nothing to revert or bisect, those changes aren't functional
> changes.

Right, I did mention that.

> > Admittedly doing it this way *can* look a bit odd in *some* patch-sets
> > when they hit the MLs - particularly clock it seems, where there
> > hasn't even been a vague attempt to document any of the parameters in
> > the kernel-doc headers - however the alternative would mean nothing
> > would get done!
> 
> Yeah, and even though properly documenting the functions would have been
> the right way to fix those warnings, I didn't ask you to do that since I
> was expecting it to be daunting.

There are a couple of schools of thought on function documentation.
The conflicting one to yours is that Kernel-doc headers should only be
used if they are part of an API and have an accompanying kernel-doc::
tag in Documentation.  The functions touched here do not.

NB: Fortunately the functions we're discussing are all static or else
`scripts/find-unused-docs.sh` would complain about them also.

Personally, I am in the middle.  If authors have had a good go at
documenting functions and their parameters, I'll make the effort to
fix any doc-rot or oversights.  However if, like here, no such effort
has been made, they get demoted.  Nothing stopping authors fixing them
up properly and re-promoting them again though.  Essentially I'm
trying to avoid a situation where authors throw something together
half-heatedly, safe in the knowledge that someone will come fix and
beautify things for them.

> Surely we can meet half-way

I'm always happy to collaborate.  What does half-way look like?

Patch

diff --git a/drivers/clk/sunxi/clk-sun6i-ar100.c b/drivers/clk/sunxi/clk-sun6i-ar100.c
index e1b7d0929cf7f..54babc2b4b9ee 100644
--- a/drivers/clk/sunxi/clk-sun6i-ar100.c
+++ b/drivers/clk/sunxi/clk-sun6i-ar100.c
@@ -16,7 +16,7 @@ 
 
 #include "clk-factors.h"
 
-/**
+/*
  * sun6i_get_ar100_factors - Calculates factors p, m for AR100
  *
  * AR100 rate is calculated as follows