Re: [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation

[Philippe Mathieu-Daudé]

On 05/09/2018 12:42 PM, Alistair Francis wrote:
> On Tue, May 8, 2018 at 11:01 PM, Philippe Mathieu-Daudé <address@hidden> 
> wrote:
>> Add more descriptive comments to keep a clear separation
>> between static property vs runtime changeable.
>>
>> Suggested-by: Peter Maydell <address@hidden>
>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>> ---
>>  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 <address@hidden>

Thanks!

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