| Message ID | 21f98d137754b1c58de3cec2c3e4a7df7cc936ce.1635342377.git.balaton@eik.bme.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More SH4 clean ups | expand |
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? > +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. > +} 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 */ > >
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 */ >> >> > >
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 +typedef enum { + INTC_MODE_NONE, + INTC_MODE_DUAL_SET, + INTC_MODE_DUAL_CLR, + INTC_MODE_ENABLE_REG, + INTC_MODE_MASK_REG, +} 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 */
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(-)