diff mbox series

[03/13] ppc440: Add a macro to shorten PCIe controller DCR registration

Message ID e8ae82b0b6c10e48acbc297fa15d9e5f4befc9d2.1688421085.git.balaton@eik.bme.hu
State New
Headers show
Series PPC440 devices misc clean up | expand

Commit Message

BALATON Zoltan July 3, 2023, 10:02 p.m. UTC 
It is more readable to wrap the complex call to ppc_dcr_register in a
macro when needed repeatedly.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/ppc440_uc.c | 76 +++++++++++++++++-----------------------------
 1 file changed, 28 insertions(+), 48 deletions(-)

Comments

Philippe Mathieu-Daudé July 4, 2023, 8:49 a.m. UTC | #1 
On 4/7/23 00:02, BALATON Zoltan wrote:
> It is more readable to wrap the complex call to ppc_dcr_register in a
> macro when needed repeatedly.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ppc/ppc440_uc.c | 76 +++++++++++++++++-----------------------------
>   1 file changed, 28 insertions(+), 48 deletions(-)


> +#define PPC440_PCIE_DCR(s, dcrn) \
> +    ppc_dcr_register(&(s)->cpu->env, (s)->dcrn_base + (dcrn), s, \

'(s), \'

> +                     &dcr_read_pcie, &dcr_write_pcie)
> +
> +

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
BALATON Zoltan July 4, 2023, 9:33 a.m. UTC | #2 
On Tue, 4 Jul 2023, Philippe Mathieu-Daudé wrote:
> On 4/7/23 00:02, BALATON Zoltan wrote:
>> It is more readable to wrap the complex call to ppc_dcr_register in a
>> macro when needed repeatedly.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/ppc/ppc440_uc.c | 76 +++++++++++++++++-----------------------------
>>   1 file changed, 28 insertions(+), 48 deletions(-)
>
>
>> +#define PPC440_PCIE_DCR(s, dcrn) \
>> +    ppc_dcr_register(&(s)->cpu->env, (s)->dcrn_base + (dcrn), s, \
>
> '(s), \'

The parenthesis here would be superfluous as it stands alone in a function 
parameter between commas so no matter what you substitue here should not 
have an unwanted side effect (unless it has a comma but that's an error 
anyway) so maybe this is not needed.

>> +                     &dcr_read_pcie, &dcr_write_pcie)
>> +
>> +
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks for the quick review, I'll post a v2 in a few days to wait a bit if 
anobody else has any other request.

Regards,
BALATON Zoltan
Philippe Mathieu-Daudé July 4, 2023, 9:55 a.m. UTC | #3 
On 4/7/23 11:33, BALATON Zoltan wrote:
> On Tue, 4 Jul 2023, Philippe Mathieu-Daudé wrote:
>> On 4/7/23 00:02, BALATON Zoltan wrote:
>>> It is more readable to wrap the complex call to ppc_dcr_register in a
>>> macro when needed repeatedly.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   hw/ppc/ppc440_uc.c | 76 +++++++++++++++++-----------------------------
>>>   1 file changed, 28 insertions(+), 48 deletions(-)
>>
>>
>>> +#define PPC440_PCIE_DCR(s, dcrn) \
>>> +    ppc_dcr_register(&(s)->cpu->env, (s)->dcrn_base + (dcrn), s, \
>>
>> '(s), \'
> 
> The parenthesis here would be superfluous as it stands alone in a 
> function parameter between commas so no matter what you substitue here 
> should not have an unwanted side effect (unless it has a comma but 
> that's an error anyway) so maybe this is not needed.

Well I noticed because you used it for the 2 other cases, so I'm
just trying to be consistent here. Besides, not using parenthesis
for macro arguments is a bad practice. Problems happen when others
copy code.

> 
>>> +                     &dcr_read_pcie, &dcr_write_pcie)
>>> +
>>> +
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Thanks for the quick review, I'll post a v2 in a few days to wait a bit 
> if anobody else has any other request.
> 
> Regards,
> BALATON Zoltan
diff mbox series

Patch

diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index b26c0cee1b..db83a0dec8 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -1002,56 +1002,36 @@  static void ppc460ex_set_irq(void *opaque, int irq_num, int level)
        qemu_set_irq(s->irq[irq_num], level);
 }
 
+#define PPC440_PCIE_DCR(s, dcrn) \
+    ppc_dcr_register(&(s)->cpu->env, (s)->dcrn_base + (dcrn), s, \
+                     &dcr_read_pcie, &dcr_write_pcie)
+
+
 static void ppc460ex_pcie_register_dcrs(PPC460EXPCIEState *s)
 {
-    CPUPPCState *env = &s->cpu->env;
-
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_CFGBAH, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_CFGBAL, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_CFGMSK, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_MSGBAH, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_MSGBAL, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_MSGMSK, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_OMR1BAH, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_OMR1BAL, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_OMR1MSKH, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_OMR1MSKL, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_OMR2BAH, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_OMR2BAL, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_OMR2MSKH, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_OMR2MSKL, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_OMR3BAH, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_OMR3BAL, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_OMR3MSKH, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_OMR3MSKL, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_REGBAH, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_REGBAL, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_REGMSK, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_SPECIAL, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
-    ppc_dcr_register(env, s->dcrn_base + PEGPL_CFG, s,
-                     &dcr_read_pcie, &dcr_write_pcie);
+    PPC440_PCIE_DCR(s, PEGPL_CFGBAH);
+    PPC440_PCIE_DCR(s, PEGPL_CFGBAL);
+    PPC440_PCIE_DCR(s, PEGPL_CFGMSK);
+    PPC440_PCIE_DCR(s, PEGPL_MSGBAH);
+    PPC440_PCIE_DCR(s, PEGPL_MSGBAL);
+    PPC440_PCIE_DCR(s, PEGPL_MSGMSK);
+    PPC440_PCIE_DCR(s, PEGPL_OMR1BAH);
+    PPC440_PCIE_DCR(s, PEGPL_OMR1BAL);
+    PPC440_PCIE_DCR(s, PEGPL_OMR1MSKH);
+    PPC440_PCIE_DCR(s, PEGPL_OMR1MSKL);
+    PPC440_PCIE_DCR(s, PEGPL_OMR2BAH);
+    PPC440_PCIE_DCR(s, PEGPL_OMR2BAL);
+    PPC440_PCIE_DCR(s, PEGPL_OMR2MSKH);
+    PPC440_PCIE_DCR(s, PEGPL_OMR2MSKL);
+    PPC440_PCIE_DCR(s, PEGPL_OMR3BAH);
+    PPC440_PCIE_DCR(s, PEGPL_OMR3BAL);
+    PPC440_PCIE_DCR(s, PEGPL_OMR3MSKH);
+    PPC440_PCIE_DCR(s, PEGPL_OMR3MSKL);
+    PPC440_PCIE_DCR(s, PEGPL_REGBAH);
+    PPC440_PCIE_DCR(s, PEGPL_REGBAL);
+    PPC440_PCIE_DCR(s, PEGPL_REGMSK);
+    PPC440_PCIE_DCR(s, PEGPL_SPECIAL);
+    PPC440_PCIE_DCR(s, PEGPL_CFG);
 }
 
 static void ppc460ex_pcie_realize(DeviceState *dev, Error **errp)