Re: [PATCH 01/11] mos6522: add defines for IFR bit flags

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 3449 bytes --]

On Sat, 5 Feb 2022, Philippe Mathieu-Daudé wrote:
> On 5/2/22 11:51, Mark Cave-Ayland wrote:
>> On 27/01/2022 23:16, BALATON Zoltan wrote:
>> 
>>> On Thu, 27 Jan 2022, Mark Cave-Ayland wrote:
>>>> These are intended to make it easier to see how the physical control 
>>>> lines
>>>> are wired for each instance.
>>>> 
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>> include/hw/misc/mos6522.h | 22 +++++++++++++++-------
>>>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>>> 
>>>> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
>>>> index fc95d22b0f..12abd8b8d2 100644
>>>> --- a/include/hw/misc/mos6522.h
>>>> +++ b/include/hw/misc/mos6522.h
>>>> @@ -41,13 +41,21 @@
>>>> #define IER_SET            0x80    /* set bits in IER */
>>>> #define IER_CLR            0       /* clear bits in IER */
>>>> 
>>>> -#define CA2_INT            0x01
>>>> -#define CA1_INT            0x02
>>>> -#define SR_INT             0x04    /* Shift register full/empty */
>>>> -#define CB2_INT            0x08
>>>> -#define CB1_INT            0x10
>>>> -#define T2_INT             0x20    /* Timer 2 interrupt */
>>>> -#define T1_INT             0x40    /* Timer 1 interrupt */
>>>> +#define CA2_INT_BIT        0
>>>> +#define CA1_INT_BIT        1
>>>> +#define SR_INT_BIT         2       /* Shift register full/empty */
>>>> +#define CB2_INT_BIT        3
>>>> +#define CB1_INT_BIT        4
>>>> +#define T2_INT_BIT         5       /* Timer 2 interrupt */
>>>> +#define T1_INT_BIT         6       /* Timer 1 interrupt */
>>>> +
>>>> +#define CA2_INT            (1 << CA2_INT_BIT)
>>>> +#define CA1_INT            (1 << CA1_INT_BIT)
>>>> +#define SR_INT             (1 << SR_INT_BIT)
>>>> +#define CB2_INT            (1 << CB2_INT_BIT)
>>>> +#define CB1_INT            (1 << CB1_INT_BIT)
>>>> +#define T2_INT             (1 << T2_INT_BIT)
>>>> +#define T1_INT             (1 << T1_INT_BIT)
>>> 
>>> Maybe you could leave the #defines called XX_INT and then use BIT(XX_INT) 
>>> instead of the second set of #defines which would provide same readability 
>>> but with less #defines needed.
>> 
>> I'm not so keen on removing the _INT defines since that would require 
>> updating all users to use BIT() everywhere which I don't think gains us 
>> much. I could certainly replace these definitions with BIT(FOO) instead of 
>> (1 << FOO) if that helps readability though.
>
> Do you mean simply doing this?
>
> -#define T1_INT             0x40    /* Timer 1 interrupt */
> +#define T1_INT             BIT(6)  /* Timer 1 interrupt */

I meant:

#define T1_INT 6

and then replace current usage of T1_INT with BIT(T1_INT) that way we 
don't need both T1_INT_BIT and T1_INT defines which seems redundant as 
BIT(T1_INT) is not much longer and still clear what it refers to. It's 
true that this needs more changes but the result is more readable IMO than 
introducing another set of defines that ome has to look up when encounters 
them as the meaning might not be clear. That's why I think one set of 
defines for bit numbers and using existing BIT(num) for values is better 
but it's just an idea, I don't care that much.

Regards,
BALATON Zoltan