Re: [PATCH 1/5] hw/pci-host/bonito: Make PCI_ADDR() macro more readable

[Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>]

On 10/12/20 3:55 PM, BALATON Zoltan wrote:
> On Mon, 12 Oct 2020, Philippe Mathieu-Daudé wrote:
> > From: Philippe Mathieu-Daudé <f4bug@xxxxxxxxx>
> >
> > The PCI_ADDR() macro use generic PCI fields shifted by 8-bit.
> > Rewrite it extracting the shift operation one layer.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@xxxxxxxxx>
> > ---
> > hw/pci-host/bonito.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> > index a99eced0657..abb3ee86769 100644
> > --- a/hw/pci-host/bonito.c
> > +++ b/hw/pci-host/bonito.c
> > @@ -196,8 +196,8 @@ FIELD(BONGENCFG, PCIQUEUE,      12, 1)
> > #define PCI_IDSEL_VIA686B          (1 << PCI_IDSEL_VIA686B_BIT)
> >
> > #define PCI_ADDR(busno , devno , funno , regno)  \
> > -    ((((busno) << 16) & 0xff0000) + (((devno) << 11) & 0xf800) + \
> > -    (((funno) << 8) & 0x700) + (regno))
> > +    ((((busno) << 8) & 0xff00) + (((devno) << 3) & 0xf8) + \
> > +    (((funno) & 0x7) << 8) + (regno))
> Are you missing a << 8 somewhere before + (regno) or both of these are 
> equally unreadable and I've missed something? This seems to be 
> completely replaced by next patch so what's the point of this change?
I might have missed a parenthesis somewhere indeed =)

I'm happy to merge it in the next patch, I thought it would
be easier to review but it isn't.

Thanks for reviewing!

> Regards,
> BALATON Zoltan
>
> > typedef struct BonitoState BonitoState;