| Message ID | 20220127205405.23499-2-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | mos6522: switch to gpios, add control line edge-triggering and extra debugging | expand |
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. Regards, BALATON Zoltan > /* Bits in ACR */ > #define T1MODE 0xc0 /* Timer 1 mode */ >
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. ATB, Mark.
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
On 05/02/2022 12:06, BALATON Zoltan wrote: > 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. I think the best solution here is to just use BIT() for the final shifted values like this: #define CA2_INT_BIT 0 ... ... #define CA2_INT BIT(CA2_INT_BIT) Otherwise I can see there being confusion given that the BIT() macro is used for defines without a _BIT suffix which are also being used elsewhere. I'll update this in v2 accordingly. ATB, Mark.
On Sun, 20 Feb 2022, Mark Cave-Ayland wrote: > On 05/02/2022 12:06, BALATON Zoltan wrote: >> 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. > > I think the best solution here is to just use BIT() for the final shifted > values like this: > > #define CA2_INT_BIT 0 > ... > ... > #define CA2_INT BIT(CA2_INT_BIT) That does not really help much as the idea was to avoid having two set of defines and only have one set for the bit numbers then use the BIT() macro instead of the current values. Using the BIT() macro in the second set of defines does not help reduce the number of defines in code which the reader will have to look up in this header. IMO having defines only for bit numbers and always using BIT(whatever) for values is less confusing assuming one is familiar with what the BIT() macro does. > Otherwise I can see there being confusion given that the BIT() macro is used > for defines without a _BIT suffix which are also being used elsewhere. Maybe it's only confusing to you as you've named the bit numbers *_BIT and the values without BIT and my proposal was to name the bit numbers as the simple names and use BIT(name) for the value which looks kind of opposite naming but it's the simplest. I guess you could also have bit numbers named *_BIT and then use BIT(CA2_INT_BIT) instead of the second set of defines for the values but that looks a bit redundant and maybe more confusing than just using BIT(CA2_INT). > I'll update this in v2 accordingly. I don't have a strong opinion on this so if you prefer the way it is now or using the BIT() macro only for the separate defines then do that. I don't mind either way. Regards, BALATON Zoltan
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) /* Bits in ACR */ #define T1MODE 0xc0 /* Timer 1 mode */
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(-)