Add more descriptive comments to keep a clear separation
between static property vs runtime changeable.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 235e0518d6..5fb4787671 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -90,12 +90,15 @@ struct SDState {
     uint32_t card_status;
     uint8_t sd_status[64];
 
-    /* Configurable properties */
+    /* Static properties */
+
     BlockBackend *blk;
     bool spi;
 
-    uint32_t mode;    /* current card mode, one of SDCardModes */
-    int32_t state;    /* current card state, one of SDCardStates */
+    /* Runtime changeables */
+
+    uint32_t mode;    /** current card mode, one of #SDCardModes */
+    int32_t state;    /** current card state, one of #SDCardStates */
     uint32_t vhs;
     bool wp_switch;
     unsigned long *wp_groups;
@@ -109,8 +112,9 @@ struct SDState {
     uint32_t pwd_len;
     uint8_t function_group[6];
     uint8_t current_cmd;
-    /* True if we will handle the next command as an ACMD. Note that this does
-     * *not* track the APP_CMD status bit!
+    /**
+     * #True if we will handle the next command as an ACMD.
+     * Note that this does *not* track the APP_CMD status bit!
      */
     bool expecting_acmd;
     uint32_t blk_written;
-- 
2.17.0

On Tue, May 8, 2018 at 11:01 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Add more descriptive comments to keep a clear separation
> between static property vs runtime changeable.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 235e0518d6..5fb4787671 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -90,12 +90,15 @@ struct SDState {
>      uint32_t card_status;
>      uint8_t sd_status[64];
>
> -    /* Configurable properties */
> +    /* Static properties */
> +
>      BlockBackend *blk;
>      bool spi;
>
> -    uint32_t mode;    /* current card mode, one of SDCardModes */
> -    int32_t state;    /* current card state, one of SDCardStates */
> +    /* Runtime changeables */
> +
> +    uint32_t mode;    /** current card mode, one of #SDCardModes */
> +    int32_t state;    /** current card state, one of #SDCardStates */
>      uint32_t vhs;
>      bool wp_switch;
>      unsigned long *wp_groups;
> @@ -109,8 +112,9 @@ struct SDState {
>      uint32_t pwd_len;
>      uint8_t function_group[6];
>      uint8_t current_cmd;
> -    /* True if we will handle the next command as an ACMD. Note that this does
> -     * *not* track the APP_CMD status bit!
> +    /**
> +     * #True if we will handle the next command as an ACMD.

Why do we need a # here?

Otherwise:

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair


> +     * Note that this does *not* track the APP_CMD status bit!
>       */
>      bool expecting_acmd;
>      uint32_t blk_written;
> --
> 2.17.0
>
>

On 05/09/2018 12:42 PM, Alistair Francis wrote:
> On Tue, May 8, 2018 at 11:01 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Add more descriptive comments to keep a clear separation
>> between static property vs runtime changeable.
>>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sd.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 235e0518d6..5fb4787671 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -90,12 +90,15 @@ struct SDState {
>>      uint32_t card_status;
>>      uint8_t sd_status[64];
>>
>> -    /* Configurable properties */
>> +    /* Static properties */
>> +
>>      BlockBackend *blk;
>>      bool spi;
>>
>> -    uint32_t mode;    /* current card mode, one of SDCardModes */
>> -    int32_t state;    /* current card state, one of SDCardStates */
>> +    /* Runtime changeables */
>> +
>> +    uint32_t mode;    /** current card mode, one of #SDCardModes */
>> +    int32_t state;    /** current card state, one of #SDCardStates */
>>      uint32_t vhs;
>>      bool wp_switch;
>>      unsigned long *wp_groups;
>> @@ -109,8 +112,9 @@ struct SDState {
>>      uint32_t pwd_len;
>>      uint8_t function_group[6];
>>      uint8_t current_cmd;
>> -    /* True if we will handle the next command as an ACMD. Note that this does
>> -     * *not* track the APP_CMD status bit!
>> +    /**
>> +     * #True if we will handle the next command as an ACMD.
> 
> Why do we need a # here?

I used the CPUState as a documentation example, but this might not be
the correct use...

  /**
   * CPUState:
   * @running: #true if CPU is currently running (lockless).
   ...

$ git grep -Ei '#(true|false)'
include/exec/memory.h:164:         * If present, and returns #false, the
transaction is not accepted
include/qom/cpu.h:280: * @running: #true if CPU is currently running
(lockless).
include/qom/cpu.h:281: * @has_waiter: #true if a CPU is currently
waiting for the cpu_exec_end;

MemoryRegionOps does not use the double '/**' style:

    bool unaligned;
    /*
     * If present, and returns #false, the transaction is not accepted
     * by the device (and results in machine dependent behaviour such
     * as a machine check exception).
     */

Peter if you take this, feel free to drop this '#'.

> 
> Otherwise:
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Thanks!

> 
> Alistair
> 
> 
>> +     * Note that this does *not* track the APP_CMD status bit!
>>       */
>>      bool expecting_acmd;
>>      uint32_t blk_written;
>> --
>> 2.17.0
>>
>>

On 10 May 2018 at 01:24, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 05/09/2018 12:42 PM, Alistair Francis wrote:
>> On Tue, May 8, 2018 at 11:01 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> Add more descriptive comments to keep a clear separation
>>> between static property vs runtime changeable.

>> Why do we need a # here?
>
> I used the CPUState as a documentation example, but this might not be
> the correct use...

Our doc-comment syntax is rather inconsistent, because we've never
actually run a tool over it to autogenerate documentation from the
comments, and so there hasn't been anything syntax-checking them.
The original intention for the syntax was gtkdoc, I think, where
a leading '#' indicates a symbol that isn't a function, constant or
parameter. However, I think the most recent consensus is that we
should use kernel-doc format, which is similar but doesn't use the
'#' markup:
 https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

(The plan being that as we switch over to Sphinx for our docs
tool workflow we can use the kernel's integration of kernel-doc
into sphinx.)

For this structure in particular, it is not a public struct,
but local to a .c file. I think that for those there is not
really any point in having any kind of doc-comment markup in
it at all (ie no '#', no leading '/**').

thanks
-- PMM

On 05/14/2018 12:49 PM, Peter Maydell wrote:
> On 10 May 2018 at 01:24, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 05/09/2018 12:42 PM, Alistair Francis wrote:
>>> On Tue, May 8, 2018 at 11:01 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> Add more descriptive comments to keep a clear separation
>>>> between static property vs runtime changeable.
> 
>>> Why do we need a # here?
>>
>> I used the CPUState as a documentation example, but this might not be
>> the correct use...
> 
> Our doc-comment syntax is rather inconsistent, because we've never
> actually run a tool over it to autogenerate documentation from the
> comments, and so there hasn't been anything syntax-checking them.
> The original intention for the syntax was gtkdoc, I think, where
> a leading '#' indicates a symbol that isn't a function, constant or
> parameter. However, I think the most recent consensus is that we
> should use kernel-doc format, which is similar but doesn't use the
> '#' markup:
>  https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

Ok, I'll use this guide from now on.

> (The plan being that as we switch over to Sphinx for our docs
> tool workflow we can use the kernel's integration of kernel-doc
> into sphinx.)
> 
> For this structure in particular, it is not a public struct,
> but local to a .c file. I think that for those there is not
> really any point in having any kind of doc-comment markup in
> it at all (ie no '#', no leading '/**').

Ok.