diff mbox series

[v7,06/16] sdhci: add init_readonly_registers() to initialize the CAPAB register

Message ID 20180118123219.19410-7-f4bug@amsat.org
State Superseded, archived
Headers show
Series SDHCI: clean v1/v2 Specs (part 2) | expand

Commit Message

Philippe Mathieu-Daudé Jan. 18, 2018, 12:32 p.m. UTC 
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Philippe Mathieu-Daudé Jan. 18, 2018, 3:44 p.m. UTC | #1 
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) {

Paolo said this is wrong (as in bad pattern?).

I will respin probably using 'bool sdhci_init_readonly_registers()' instead.

> +        return;
> +    }
>      s->buf_maxsz = sdhci_get_fifolen(s);
>      s->fifo_buffer = g_malloc0(s->buf_maxsz);
>  
>
Marcel Apfelbaum Jan. 18, 2018, 3:47 p.m. UTC | #2 
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);
>>   
>>
Paolo Bonzini Jan. 18, 2018, 4 p.m. UTC | #3 
On 18/01/2018 16:44, Philippe Mathieu-Daudé wrote:
>>  static void sdhci_common_realize(SDHCIState *s, Error **errp)
>>  {
>> +    sdhci_init_readonly_registers(s, errp);
>> +    if (errp && *errp) {
> Paolo said this is wrong (as in bad pattern?).
> 
> I will respin probably using 'bool sdhci_init_readonly_registers()' instead.
> 

Please use the local_err idiom instead.

Paolo
Philippe Mathieu-Daudé Jan. 18, 2018, 4:11 p.m. UTC | #4 
On 01/18/2018 12:47 PM, Marcel Apfelbaum wrote:
> 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.

I'll read them more carefully ;)

Just checking with this cocci script,

@@
Error **errp;
@@
*if (errp && *errp) {
    ...
 }

we get:

+++ ./hw/sd/sdhci.c
@@ -1303,7 +1303,6 @@ static void sdhci_pci_realize(PCIDevice

     sdhci_initfn(s);
     sdhci_common_realize(s, errp);
-    if (errp && *errp) {
         return;
     }

@@ -1383,7 +1382,6 @@ static void sdhci_sysbus_realize(DeviceS
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);

     sdhci_common_realize(s, errp);
-    if (errp && *errp) {
         return;
     }

+++ ./hw/mem/pc-dimm.c
@@ -138,7 +138,6 @@ static int pc_existing_dimms_capacity_in
                 cap->errp);
         }

-        if (cap->errp && *cap->errp) {
             return 1;
         }
     }
@@ -320,7 +319,6 @@ uint64_t pc_dimm_get_free_addr(uint64_t
         uint64_t dimm_size = object_property_get_uint(OBJECT(dimm),
                                                       PC_DIMM_SIZE_PROP,
                                                       errp);
-        if (errp && *errp) {
             goto out;
         }

+++ ./exec.c
@@ -1656,7 +1656,6 @@ static void *file_ram_alloc(RAMBlock *bl

     if (mem_prealloc) {
         os_mem_prealloc(fd, area, memory, smp_cpus, errp);
-        if (errp && *errp) {
             qemu_ram_munmap(area, memory);
             return NULL;
         }

not that bad.
Philippe Mathieu-Daudé Jan. 18, 2018, 4:12 p.m. UTC | #5 
On 01/18/2018 01:00 PM, Paolo Bonzini wrote:
> On 18/01/2018 16:44, Philippe Mathieu-Daudé wrote:
>>>  static void sdhci_common_realize(SDHCIState *s, Error **errp)
>>>  {
>>> +    sdhci_init_readonly_registers(s, errp);
>>> +    if (errp && *errp) {
>> Paolo said this is wrong (as in bad pattern?).
>>
>> I will respin probably using 'bool sdhci_init_readonly_registers()' instead.
>>
> 
> Please use the local_err idiom instead.

Will do, thanks!

Phil.
diff mbox series

Patch

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) {
+        return;
+    }
     s->buf_maxsz = sdhci_get_fifolen(s);
     s->fifo_buffer = g_malloc0(s->buf_maxsz);