diff mbox series

[02/13] ACPI: CPPC: Fix doxygen comments

Message ID 20210708180851.2311192-3-sudeep.holla@arm.com
State New
Headers show
Series mailbox: pcc: Add support for PCCT extended PCC subspaces | expand

Commit Message

Sudeep Holla July 8, 2021, 6:08 p.m. UTC 
Clang complains about doxygen comments too with W=1 in the build.

  | drivers/acpi/cppc_acpi.c:560: warning: Function parameter or member
  |	'pcc_ss_id' not described in 'pcc_data_alloc'
  | drivers/acpi/cppc_acpi.c:1343: warning: Function parameter or member
  |	'cpu_num' not described in 'cppc_get_transition_latency'

Fix it.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

---
 drivers/acpi/cppc_acpi.c | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.25.1

Comments

Rafael J. Wysocki July 14, 2021, 12:20 p.m. UTC | #1 
On Thu, Jul 8, 2021 at 8:09 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>

> Clang complains about doxygen comments too with W=1 in the build.

>

>   | drivers/acpi/cppc_acpi.c:560: warning: Function parameter or member

>   |     'pcc_ss_id' not described in 'pcc_data_alloc'

>   | drivers/acpi/cppc_acpi.c:1343: warning: Function parameter or member

>   |     'cpu_num' not described in 'cppc_get_transition_latency'

>

> Fix it.

>

> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> ---

>  drivers/acpi/cppc_acpi.c | 7 +++++++

>  1 file changed, 7 insertions(+)

>

> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c

> index a4d4eebba1da..eb5685167d19 100644

> --- a/drivers/acpi/cppc_acpi.c

> +++ b/drivers/acpi/cppc_acpi.c

> @@ -562,6 +562,8 @@ bool __weak cpc_ffh_supported(void)

>  /**

>   * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace

>   *


I would drop this empty line (and analogously below).

> + * @pcc_ss_id: PCC Subspace channel identifier

> + *

>   * Check and allocate the cppc_pcc_data memory.

>   * In some processor configurations it is possible that same subspace

>   * is shared between multiple CPUs. This is seen especially in CPUs

> @@ -1347,10 +1349,15 @@ EXPORT_SYMBOL_GPL(cppc_set_perf);

>  /**

>   * cppc_get_transition_latency - returns frequency transition latency in ns

>   *

> + * @cpu_num: Logical index of the CPU for which latencty is requested

> + *

>   * ACPI CPPC does not explicitly specify how a platform can specify the

>   * transition latency for performance change requests. The closest we have

>   * is the timing information from the PCCT tables which provides the info

>   * on the number and frequency of PCC commands the platform can handle.

> + *

> + * Returns: frequency transition latency on success or CPUFREQ_ETERNAL on

> + * failure


Is this change needed?  The one-line summary already says this.

>   */

>  unsigned int cppc_get_transition_latency(int cpu_num)

>  {

> --
Sudeep Holla July 14, 2021, 3:12 p.m. UTC | #2 
On Wed, Jul 14, 2021 at 02:20:05PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jul 8, 2021 at 8:09 PM Sudeep Holla <sudeep.holla@arm.com> wrote:

> >

> > Clang complains about doxygen comments too with W=1 in the build.

> >

> >   | drivers/acpi/cppc_acpi.c:560: warning: Function parameter or member

> >   |     'pcc_ss_id' not described in 'pcc_data_alloc'

> >   | drivers/acpi/cppc_acpi.c:1343: warning: Function parameter or member

> >   |     'cpu_num' not described in 'cppc_get_transition_latency'

> >

> > Fix it.

> >

> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> > ---

> >  drivers/acpi/cppc_acpi.c | 7 +++++++

> >  1 file changed, 7 insertions(+)

> >

> > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c

> > index a4d4eebba1da..eb5685167d19 100644

> > --- a/drivers/acpi/cppc_acpi.c

> > +++ b/drivers/acpi/cppc_acpi.c

> > @@ -562,6 +562,8 @@ bool __weak cpc_ffh_supported(void)

> >  /**

> >   * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace

> >   *

>

> I would drop this empty line (and analogously below).

>


Sure

> > + * @pcc_ss_id: PCC Subspace channel identifier

> > + *

> >   * Check and allocate the cppc_pcc_data memory.

> >   * In some processor configurations it is possible that same subspace

> >   * is shared between multiple CPUs. This is seen especially in CPUs

> > @@ -1347,10 +1349,15 @@ EXPORT_SYMBOL_GPL(cppc_set_perf);

> >  /**

> >   * cppc_get_transition_latency - returns frequency transition latency in ns

> >   *

> > + * @cpu_num: Logical index of the CPU for which latencty is requested

> > + *

> >   * ACPI CPPC does not explicitly specify how a platform can specify the

> >   * transition latency for performance change requests. The closest we have

> >   * is the timing information from the PCCT tables which provides the info

> >   * on the number and frequency of PCC commands the platform can handle.

> > + *

> > + * Returns: frequency transition latency on success or CPUFREQ_ETERNAL on

> > + * failure

> 

> Is this change needed?  The one-line summary already says this.

>


Right, not required. I must have got confused with other place that expected
return summary.

-- 
Regards,
Sudeep
Cristian Marussi July 14, 2021, 4:07 p.m. UTC | #3 
On Wed, Jul 14, 2021 at 04:12:10PM +0100, Sudeep Holla wrote:
> On Wed, Jul 14, 2021 at 02:20:05PM +0200, Rafael J. Wysocki wrote:

> > On Thu, Jul 8, 2021 at 8:09 PM Sudeep Holla <sudeep.holla@arm.com> wrote:

> > >

> > > Clang complains about doxygen comments too with W=1 in the build.

> > >

> > >   | drivers/acpi/cppc_acpi.c:560: warning: Function parameter or member

> > >   |     'pcc_ss_id' not described in 'pcc_data_alloc'

> > >   | drivers/acpi/cppc_acpi.c:1343: warning: Function parameter or member

> > >   |     'cpu_num' not described in 'cppc_get_transition_latency'

> > >

> > > Fix it.

> > >

> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> > > ---

> > >  drivers/acpi/cppc_acpi.c | 7 +++++++

> > >  1 file changed, 7 insertions(+)

> > >

> > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c

> > > index a4d4eebba1da..eb5685167d19 100644

> > > --- a/drivers/acpi/cppc_acpi.c

> > > +++ b/drivers/acpi/cppc_acpi.c

> > > @@ -562,6 +562,8 @@ bool __weak cpc_ffh_supported(void)

> > >  /**

> > >   * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace

> > >   *

> >

> > I would drop this empty line (and analogously below).

> >

> 

> Sure

> 

> > > + * @pcc_ss_id: PCC Subspace channel identifier

> > > + *

> > >   * Check and allocate the cppc_pcc_data memory.

> > >   * In some processor configurations it is possible that same subspace

> > >   * is shared between multiple CPUs. This is seen especially in CPUs

> > > @@ -1347,10 +1349,15 @@ EXPORT_SYMBOL_GPL(cppc_set_perf);

> > >  /**

> > >   * cppc_get_transition_latency - returns frequency transition latency in ns

> > >   *

> > > + * @cpu_num: Logical index of the CPU for which latencty is requested

> > > + *

> > >   * ACPI CPPC does not explicitly specify how a platform can specify the

> > >   * transition latency for performance change requests. The closest we have

> > >   * is the timing information from the PCCT tables which provides the info

> > >   * on the number and frequency of PCC commands the platform can handle.

> > > + *

> > > + * Returns: frequency transition latency on success or CPUFREQ_ETERNAL on

> > > + * failure

> > 

> > Is this change needed?  The one-line summary already says this.

> >

> 

> Right, not required. I must have got confused with other place that expected

> return summary.

> 

I think kernel-doc complains if no Return: (not Returns:) doxygen clause
is provided while describing a function which do return some values.
(..even though the info is clearly duplicated as it is now in the
one-line summary)

Thanks,
Cristian

> -- 

> Regards,

> Sudeep
Sudeep Holla July 14, 2021, 4:14 p.m. UTC | #4 
On Wed, Jul 14, 2021 at 05:07:02PM +0100, Cristian Marussi wrote:
> On Wed, Jul 14, 2021 at 04:12:10PM +0100, Sudeep Holla wrote:

> > On Wed, Jul 14, 2021 at 02:20:05PM +0200, Rafael J. Wysocki wrote:

> > > On Thu, Jul 8, 2021 at 8:09 PM Sudeep Holla <sudeep.holla@arm.com> wrote:

> > > >

> > > > Clang complains about doxygen comments too with W=1 in the build.

> > > >

> > > >   | drivers/acpi/cppc_acpi.c:560: warning: Function parameter or member

> > > >   |     'pcc_ss_id' not described in 'pcc_data_alloc'

> > > >   | drivers/acpi/cppc_acpi.c:1343: warning: Function parameter or member

> > > >   |     'cpu_num' not described in 'cppc_get_transition_latency'

> > > >

> > > > Fix it.

> > > >

> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> > > > ---

> > > >  drivers/acpi/cppc_acpi.c | 7 +++++++

> > > >  1 file changed, 7 insertions(+)

> > > >

> > > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c

> > > > index a4d4eebba1da..eb5685167d19 100644

> > > > --- a/drivers/acpi/cppc_acpi.c

> > > > +++ b/drivers/acpi/cppc_acpi.c

> > > > @@ -562,6 +562,8 @@ bool __weak cpc_ffh_supported(void)

> > > >  /**

> > > >   * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace

> > > >   *

> > >

> > > I would drop this empty line (and analogously below).

> > >

> > 

> > Sure

> > 

> > > > + * @pcc_ss_id: PCC Subspace channel identifier

> > > > + *

> > > >   * Check and allocate the cppc_pcc_data memory.

> > > >   * In some processor configurations it is possible that same subspace

> > > >   * is shared between multiple CPUs. This is seen especially in CPUs

> > > > @@ -1347,10 +1349,15 @@ EXPORT_SYMBOL_GPL(cppc_set_perf);

> > > >  /**

> > > >   * cppc_get_transition_latency - returns frequency transition latency in ns

> > > >   *

> > > > + * @cpu_num: Logical index of the CPU for which latencty is requested

> > > > + *

> > > >   * ACPI CPPC does not explicitly specify how a platform can specify the

> > > >   * transition latency for performance change requests. The closest we have

> > > >   * is the timing information from the PCCT tables which provides the info

> > > >   * on the number and frequency of PCC commands the platform can handle.

> > > > + *

> > > > + * Returns: frequency transition latency on success or CPUFREQ_ETERNAL on

> > > > + * failure

> > > 

> > > Is this change needed?  The one-line summary already says this.

> > >

> >

> > Right, not required. I must have got confused with other place that expected

> > return summary.

> >

> I think kernel-doc complains if no Return: (not Returns:) doxygen clause

> is provided while describing a function which do return some values.

> (..even though the info is clearly duplicated as it is now in the

> one-line summary)

>


Thanks Cristian, just noticed the same. I was convinced that I did see the
warning before but couldn't recollect the details quickly.

$ ./scripts/kernel-doc -none drivers/acpi/cppc_acpi.c
(no warnings)

$ ./scripts/kernel-doc -v -none drivers/acpi/cppc_acpi.c 
drivers/acpi/cppc_acpi.c:1345: warning: No description found for return value of 'cppc_get_transition_latency'

The build with W=1 may not be using -v. That explains why I got confused as
I initially started with W=1 build but did switch to ./scripts/kernel-doc -v
after Joe pointed out its existence.

--
Regards,
Sudeep
diff mbox series

Patch

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index a4d4eebba1da..eb5685167d19 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -562,6 +562,8 @@  bool __weak cpc_ffh_supported(void)
 /**
  * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
  *
+ * @pcc_ss_id: PCC Subspace channel identifier
+ *
  * Check and allocate the cppc_pcc_data memory.
  * In some processor configurations it is possible that same subspace
  * is shared between multiple CPUs. This is seen especially in CPUs
@@ -1347,10 +1349,15 @@  EXPORT_SYMBOL_GPL(cppc_set_perf);
 /**
  * cppc_get_transition_latency - returns frequency transition latency in ns
  *
+ * @cpu_num: Logical index of the CPU for which latencty is requested
+ *
  * ACPI CPPC does not explicitly specify how a platform can specify the
  * transition latency for performance change requests. The closest we have
  * is the timing information from the PCCT tables which provides the info
  * on the number and frequency of PCC commands the platform can handle.
+ *
+ * Returns: frequency transition latency on success or CPUFREQ_ETERNAL on
+ * failure
  */
 unsigned int cppc_get_transition_latency(int cpu_num)
 {