Re: [Qemu-devel] [PATCH v7 06/16] sdhci: add init_readonly_registers() to initialize the CAPAB register

[Marcel Apfelbaum <marcel@redhat.com>]

On 18/01/2018 17:44, Philippe Mathieu-Daudé wrote:
> On 01/18/2018 09:32 AM, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/sd/sdhci.c | 17 ++++++++++++++---
>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index f9264d3be5..08b85558f1 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -1174,12 +1174,19 @@ static inline unsigned int sdhci_get_fifolen(SDHCIState *s)
>>       }
>>   }
>>   
>> +static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
>> +{
>> +    if (s->capareg == UINT64_MAX) {
>> +        s->capareg = SDHC_CAPAB_REG_DEFAULT;
>> +    }
>> +}
>> +
>>   /* --- qdev common --- */
>>   
>>   #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
>> -    /* Capabilities registers provide information on supported features
>> -     * of this specific host controller implementation */ \
>> -    DEFINE_PROP_UINT64("capareg", _state, capareg, SDHC_CAPAB_REG_DEFAULT), \
>> +    /* deprecated: Capabilities registers provide information on supported
>> +     * features of this specific host controller implementation */ \
>> +    DEFINE_PROP_UINT64("capareg", _state, capareg, UINT64_MAX), \
>>       DEFINE_PROP_UINT64("maxcurr", _state, maxcurr, 0)
>>   
>>   static void sdhci_initfn(SDHCIState *s)
>> @@ -1204,6 +1211,10 @@ static void sdhci_uninitfn(SDHCIState *s)
>>   
>>   static void sdhci_common_realize(SDHCIState *s, Error **errp)
>>   {
>> +    sdhci_init_readonly_registers(s, errp);
>> +    if (errp && *errp) {
>

Hi Phillipe,

> Paolo said this is wrong (as in bad pattern?).
> 
> I will respin probably using 'bool sdhci_init_readonly_registers()' instead.
> 

I always use the explanations in include/qapi/error.h
as guidelines.

Thanks,
Marcel

>> +        return;
>> +    }
>>       s->buf_maxsz = sdhci_get_fifolen(s);
>>       s->fifo_buffer = g_malloc0(s->buf_maxsz);
>>   
>>