Re: [PATCH v2 09/11] hw/intc/sh_intc: Turn some defines into an enum

[BALATON Zoltan]

On Wed, 27 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/27/21 15:46, BALATON Zoltan wrote:
> > Turn the INTC_MODE defines into an enum (except the one which is a
> > flag) and clean up the function returning these to make it clearer by
> > removing nested ifs and superfluous parenthesis.
> >
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > ---
> >  hw/intc/sh_intc.c | 43 +++++++++++++++++++------------------------
> >  1 file changed, 19 insertions(+), 24 deletions(-)
> >
> > diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
> > index 0bd27aaf4f..18461ff554 100644
> > --- a/hw/intc/sh_intc.c
> > +++ b/hw/intc/sh_intc.c
> > @@ -100,33 +100,27 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, 
> > int imask)
> >      abort();
> >  }
> >
> > -#define INTC_MODE_NONE       0
> > -#define INTC_MODE_DUAL_SET   1
> > -#define INTC_MODE_DUAL_CLR   2
> > -#define INTC_MODE_ENABLE_REG 3
> > -#define INTC_MODE_MASK_REG   4
> > -#define INTC_MODE_IS_PRIO    8
> > -
> > -static unsigned int sh_intc_mode(unsigned long address,
> > -                                 unsigned long set_reg, unsigned long clr_reg)
> > +#define INTC_MODE_IS_PRIO 0x80
>
> Why change 8 -> 0x80 without updating the definition usage?

To better separate it from the enum values as these are OR-ed together 
later so just leave some spare bits inbetween. Should this be a separate 
one line patch or mention it in the commit message?

> > +typedef enum {
> > +    INTC_MODE_NONE,
> > +    INTC_MODE_DUAL_SET,
> > +    INTC_MODE_DUAL_CLR,
> > +    INTC_MODE_ENABLE_REG,
> > +    INTC_MODE_MASK_REG,
>
> If this is defined by the spec, better explicit the enum value,
> otherwise if someone add a misplaced field this would change the
> definitions.

I don't know. It didn't occur to me it could be part of the arch, looked 
more like an implementation detail but have to check the docs if anything 
similar is mentioned.

Regards,
BALATON Zoltan

> > +} SHIntCMode;
> > +
> > +
> > +static SHIntCMode sh_intc_mode(unsigned long address, unsigned long set_reg,
> > +                               unsigned long clr_reg)
> >  {
> > -    if ((address != A7ADDR(set_reg)) &&
> > -        (address != A7ADDR(clr_reg)))
> > +    if (address != A7ADDR(set_reg) && address != A7ADDR(clr_reg)) {
> >          return INTC_MODE_NONE;
> > -
> > -    if (set_reg && clr_reg) {
> > -        if (address == A7ADDR(set_reg)) {
> > -            return INTC_MODE_DUAL_SET;
> > -        } else {
> > -            return INTC_MODE_DUAL_CLR;
> > -        }
> >      }
> > -
> > -    if (set_reg) {
> > -        return INTC_MODE_ENABLE_REG;
> > -    } else {
> > -        return INTC_MODE_MASK_REG;
> > +    if (set_reg && clr_reg) {
> > +        return address == A7ADDR(set_reg) ?
> > +               INTC_MODE_DUAL_SET : INTC_MODE_DUAL_CLR;
> >      }
> > +    return set_reg ? INTC_MODE_ENABLE_REG : INTC_MODE_MASK_REG;
> >  }
> >
> >  static void sh_intc_locate(struct intc_desc *desc,
> > @@ -137,7 +131,8 @@ static void sh_intc_locate(struct intc_desc *desc,
> >                             unsigned int *width,
> >                             unsigned int *modep)
> >  {
> > -    unsigned int i, mode;
> > +    SHIntCMode mode;
> > +    unsigned int i;
> >
> >      /* this is slow but works for now */