diff mbox series

[v2,01/14] kernel-doc: public/arch-arm.h

Message ID 20201021000011.15351-1-sstabellini@kernel.org
State New
Headers show
Series [v2,01/14] kernel-doc: public/arch-arm.h | expand

Commit Message

Stefano Stabellini Oct. 20, 2020, 11:59 p.m. UTC
Convert in-code comments to kernel-doc format wherever possible.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 xen/include/public/arch-arm.h | 43 ++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 16 deletions(-)

Comments

Andrew Cooper Oct. 21, 2020, 5:47 p.m. UTC | #1
On 21/10/2020 00:59, Stefano Stabellini wrote:
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index c365b1b39e..409697dede 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -201,7 +208,9 @@ typedef uint64_t xen_pfn_t;
>  #define PRI_xen_pfn PRIx64
>  #define PRIu_xen_pfn PRIu64
>  
> -/*
> +/**
> + * DOC: XEN_LEGACY_MAX_VCPUS
> + *
>   * Maximum number of virtual CPUs in legacy multi-processor guests.
>   * Only one. All other VCPUS must use VCPUOP_register_vcpu_info.
>   */

I suppose I don't really want to know why this exists in the ARM ABI? 
It looks to be a misfeature.

Shouldn't it be labelled as obsolete ?  (Is that even a thing you can do
in kernel-doc?  It surely must be...)

> @@ -299,26 +308,28 @@ struct vcpu_guest_context {
>  typedef struct vcpu_guest_context vcpu_guest_context_t;
>  DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>  
> -/*
> +
> +/**
> + * struct xen_arch_domainconfig - arch-specific domain creation params
> + *
>   * struct xen_arch_domainconfig's ABI is covered by
>   * XEN_DOMCTL_INTERFACE_VERSION.
>   */
> +struct xen_arch_domainconfig {
> +    /** @gic_version: IN/OUT parameter */
>  #define XEN_DOMCTL_CONFIG_GIC_NATIVE    0
>  #define XEN_DOMCTL_CONFIG_GIC_V2        1
>  #define XEN_DOMCTL_CONFIG_GIC_V3        2
> -
> +    uint8_t gic_version;

Please can we have a newline in here, and elsewhere separating blocks of
logically connected field/constant/comments.

It will make a world of difference to the readability of the header
files themselves.

~Andrew
Stefano Stabellini Oct. 21, 2020, 10:17 p.m. UTC | #2
On Wed, 21 Oct 2020, Andrew Cooper wrote:
> On 21/10/2020 00:59, Stefano Stabellini wrote:
> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index c365b1b39e..409697dede 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -201,7 +208,9 @@ typedef uint64_t xen_pfn_t;
> >  #define PRI_xen_pfn PRIx64
> >  #define PRIu_xen_pfn PRIu64
> >  
> > -/*
> > +/**
> > + * DOC: XEN_LEGACY_MAX_VCPUS
> > + *
> >   * Maximum number of virtual CPUs in legacy multi-processor guests.
> >   * Only one. All other VCPUS must use VCPUOP_register_vcpu_info.
> >   */
> 
> I suppose I don't really want to know why this exists in the ARM ABI? 
> It looks to be a misfeature.
> 
> Shouldn't it be labelled as obsolete ?  (Is that even a thing you can do
> in kernel-doc?  It surely must be...)

I tried not to make any content changes as part of this series, but as
we are looking into this, I could append patches to the end of the
series to make some additional changes. I.e. I'd prefer to keep the
mechanical patches mechanical.

In regards to XEN_LEGACY_MAX_VCPUS, it is part of struct shared_info so
it must be defined. It makes sense to define it to the smallest number
given that the newer interface (VCPUOP_register_vcpu_info) is preferred.

In regards to labelling things as obsolete, I couldn't find a way to do
it with kernel-doc, but keep in mind that the end goal is to use
doxygen. It might become possible then.


> > @@ -299,26 +308,28 @@ struct vcpu_guest_context {
> >  typedef struct vcpu_guest_context vcpu_guest_context_t;
> >  DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
> >  
> > -/*
> > +
> > +/**
> > + * struct xen_arch_domainconfig - arch-specific domain creation params
> > + *
> >   * struct xen_arch_domainconfig's ABI is covered by
> >   * XEN_DOMCTL_INTERFACE_VERSION.
> >   */
> > +struct xen_arch_domainconfig {
> > +    /** @gic_version: IN/OUT parameter */
> >  #define XEN_DOMCTL_CONFIG_GIC_NATIVE    0
> >  #define XEN_DOMCTL_CONFIG_GIC_V2        1
> >  #define XEN_DOMCTL_CONFIG_GIC_V3        2
> > -
> > +    uint8_t gic_version;
> 
> Please can we have a newline in here, and elsewhere separating blocks of
> logically connected field/constant/comments.
> 
> It will make a world of difference to the readability of the header
> files themselves.

Sure, I can do that
diff mbox series

Patch

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index c365b1b39e..409697dede 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -27,8 +27,8 @@ 
 #ifndef __XEN_PUBLIC_ARCH_ARM_H__
 #define __XEN_PUBLIC_ARCH_ARM_H__
 
-/*
- * `incontents 50 arm_abi Hypercall Calling Convention
+/**
+ * DOC: Hypercall Calling Convention
  *
  * A hypercall is issued using the ARM HVC instruction.
  *
@@ -72,8 +72,8 @@ 
  * Any cache allocation hints are acceptable.
  */
 
-/*
- * `incontents 55 arm_hcall Supported Hypercalls
+/**
+ * DOC: Supported Hypercalls
  *
  * Xen on ARM makes extensive use of hardware facilities and therefore
  * only a subset of the potential hypercalls are required.
@@ -175,10 +175,17 @@ 
     typedef union { type *p; uint64_aligned_t q; }              \
         __guest_handle_64_ ## name
 
-/*
+/**
+ * DOC: XEN_GUEST_HANDLE - a guest pointer in a struct
+ *
  * XEN_GUEST_HANDLE represents a guest pointer, when passed as a field
  * in a struct in memory. On ARM is always 8 bytes sizes and 8 bytes
  * aligned.
+ */
+
+/**
+ * DOC: XEN_GUEST_HANDLE_PARAM - a guest pointer as a hypercall arg
+ *
  * XEN_GUEST_HANDLE_PARAM represents a guest pointer, when passed as an
  * hypercall argument. It is 4 bytes on aarch32 and 8 bytes on aarch64.
  */
@@ -201,7 +208,9 @@  typedef uint64_t xen_pfn_t;
 #define PRI_xen_pfn PRIx64
 #define PRIu_xen_pfn PRIu64
 
-/*
+/**
+ * DOC: XEN_LEGACY_MAX_VCPUS
+ *
  * Maximum number of virtual CPUs in legacy multi-processor guests.
  * Only one. All other VCPUS must use VCPUOP_register_vcpu_info.
  */
@@ -299,26 +308,28 @@  struct vcpu_guest_context {
 typedef struct vcpu_guest_context vcpu_guest_context_t;
 DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
 
-/*
+
+/**
+ * struct xen_arch_domainconfig - arch-specific domain creation params
+ *
  * struct xen_arch_domainconfig's ABI is covered by
  * XEN_DOMCTL_INTERFACE_VERSION.
  */
+struct xen_arch_domainconfig {
+    /** @gic_version: IN/OUT parameter */
 #define XEN_DOMCTL_CONFIG_GIC_NATIVE    0
 #define XEN_DOMCTL_CONFIG_GIC_V2        1
 #define XEN_DOMCTL_CONFIG_GIC_V3        2
-
+    uint8_t gic_version;
+    /** @tee_type: IN parameter */
 #define XEN_DOMCTL_CONFIG_TEE_NONE      0
 #define XEN_DOMCTL_CONFIG_TEE_OPTEE     1
-
-struct xen_arch_domainconfig {
-    /* IN/OUT */
-    uint8_t gic_version;
-    /* IN */
     uint16_t tee_type;
-    /* IN */
+    /** @nr_spis: IN parameter */
     uint32_t nr_spis;
-    /*
-     * OUT
+    /**
+     * @clock_frequency: OUT parameter
+     *
      * Based on the property clock-frequency in the DT timer node.
      * The property may be present when the bootloader/firmware doesn't
      * set correctly CNTFRQ which hold the timer frequency.