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 */
>>
>>
>
>