| Message ID | 1623224214-4836-2-git-send-email-schmitzmic@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add APNE PCMCIA 100 Mbit support | expand |
On Jun 09 2021, Michael Schmitz wrote: > @@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr) > case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr); > #endif > #ifdef CONFIG_AMIGA_PCMCIA > +#ifdef CONFIG_APNE100MBIT > + case ISA_TYPE_AG16: fallthrough; > +#endif > case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr); Is the fallthrough annotation really needed? Andreas.
Hi Andreas, On 9/06/21 8:04 pm, Andreas Schwab wrote: > On Jun 09 2021, Michael Schmitz wrote: > >> @@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr) >> case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr); >> #endif >> #ifdef CONFIG_AMIGA_PCMCIA >> +#ifdef CONFIG_APNE100MBIT >> + case ISA_TYPE_AG16: fallthrough; >> +#endif >> case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr); > Is the fallthrough annotation really needed? Just to shut up compiler warnings, and even that I haven't seen myself. I have seen a number of patches adding either comments or this annotation in the core NCR5380 driver (which Finn maintains, who suggested this annotation to an earlier version of the Q40/Atari io_mm.h patch), so adding annotations appears to be encouraged now. I personally think these annotations are over the top generally, but I've learned to program when computed goto statements were still en vogue. In this particular case, there can be no doubt that the fallthrough is intentional, so on balance, I'll remove those annotations as well (unless Finn strongly objects?). Cheers, Michael Cheers, Michael > > Andreas. >
On Thu, 10 Jun 2021, Michael Schmitz wrote: > On 9/06/21 8:04 pm, Andreas Schwab wrote: > > On Jun 09 2021, Michael Schmitz wrote: > > > > > @@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr) > > > case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr); > > > #endif > > > #ifdef CONFIG_AMIGA_PCMCIA > > > +#ifdef CONFIG_APNE100MBIT > > > + case ISA_TYPE_AG16: fallthrough; > > > +#endif > > > case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr); > > Is the fallthrough annotation really needed? > > Just to shut up compiler warnings, and even that I haven't seen myself. > > I have seen a number of patches adding either comments or this annotation in > the core NCR5380 driver (which Finn maintains, who suggested this annotation > to an earlier version of the Q40/Atari io_mm.h patch), so adding annotations > appears to be encouraged now. > > I personally think these annotations are over the top generally, but I've > learned to program when computed goto statements were still en vogue. > > In this particular case, there can be no doubt that the fallthrough is > intentional, so on balance, I'll remove those annotations as well (unless Finn > strongly objects?). > I don't object to removing it. On the contrary, in a previous message I also questioned adding this particular 'fallthrough' (though I did recommended adding a different one). In general, there's no way to predict which static checkers are going to complain about any given line of code. They don't all agree about correctness and they are a moving target, just like fashion or reviewers' preferred code styles. The 'fallthrough' that I recommended was merely the one that seemed to improve readability to my eyes (and not to some algorithm). I didn't actually run "make C=1 W=1" or any other checker.
Hi Finn, On Thu, Jun 10, 2021 at 3:09 AM Finn Thain <fthain@linux-m68k.org> wrote: > On Thu, 10 Jun 2021, Michael Schmitz wrote: > > On 9/06/21 8:04 pm, Andreas Schwab wrote: > > > On Jun 09 2021, Michael Schmitz wrote: > > > > > > > @@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr) > > > > case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr); > > > > #endif > > > > #ifdef CONFIG_AMIGA_PCMCIA > > > > +#ifdef CONFIG_APNE100MBIT > > > > + case ISA_TYPE_AG16: fallthrough; > > > > +#endif > > > > case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr); > > > Is the fallthrough annotation really needed? > > > > Just to shut up compiler warnings, and even that I haven't seen myself. > > > > I have seen a number of patches adding either comments or this annotation in > > the core NCR5380 driver (which Finn maintains, who suggested this annotation > > to an earlier version of the Q40/Atari io_mm.h patch), so adding annotations > > appears to be encouraged now. > > > > I personally think these annotations are over the top generally, but I've > > learned to program when computed goto statements were still en vogue. > > > > In this particular case, there can be no doubt that the fallthrough is > > intentional, so on balance, I'll remove those annotations as well (unless Finn > > strongly objects?). > > > > I don't object to removing it. On the contrary, in a previous message I > also questioned adding this particular 'fallthrough' (though I did > recommended adding a different one). > > In general, there's no way to predict which static checkers are going to > complain about any given line of code. They don't all agree about > correctness and they are a moving target, just like fashion or reviewers' > preferred code styles. AFAIK, they only complain when the switch() operates on an enum, and not all enum values are handled. When operating on an int, there's not enough address space on 32-bit machines to handle all cases ;-) Gr{oetje,eeting}s, Geert
On Jun 10 2021, Michael Schmitz wrote: > Hi Andreas, > > On 9/06/21 8:04 pm, Andreas Schwab wrote: >> On Jun 09 2021, Michael Schmitz wrote: >> >>> @@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr) >>> case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr); >>> #endif >>> #ifdef CONFIG_AMIGA_PCMCIA >>> +#ifdef CONFIG_APNE100MBIT >>> + case ISA_TYPE_AG16: fallthrough; >>> +#endif >>> case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr); >> Is the fallthrough annotation really needed? > > Just to shut up compiler warnings, and even that I haven't seen myself. If there is no warning, there is nothing to shut up. > In this particular case, there can be no doubt that the fallthrough is > intentional, so on balance, I'll remove those annotations as well > (unless Finn strongly objects?). There is no fallthrough, because there is no code. Andreas.
Hi Geert, thanks for the explanation - and thanks Finn and Andreas for clarifying. I've removed the annotation in v2. Cheers, Michael On 10/06/21 7:32 pm, Geert Uytterhoeven wrote: > Hi Finn, > > On Thu, Jun 10, 2021 at 3:09 AM Finn Thain <fthain@linux-m68k.org> wrote: >> On Thu, 10 Jun 2021, Michael Schmitz wrote: >>> On 9/06/21 8:04 pm, Andreas Schwab wrote: >>>> On Jun 09 2021, Michael Schmitz wrote: >>>> >>>>> @@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr) >>>>> case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr); >>>>> #endif >>>>> #ifdef CONFIG_AMIGA_PCMCIA >>>>> +#ifdef CONFIG_APNE100MBIT >>>>> + case ISA_TYPE_AG16: fallthrough; >>>>> +#endif >>>>> case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr); >>>> Is the fallthrough annotation really needed? >>> Just to shut up compiler warnings, and even that I haven't seen myself. >>> >>> I have seen a number of patches adding either comments or this annotation in >>> the core NCR5380 driver (which Finn maintains, who suggested this annotation >>> to an earlier version of the Q40/Atari io_mm.h patch), so adding annotations >>> appears to be encouraged now. >>> >>> I personally think these annotations are over the top generally, but I've >>> learned to program when computed goto statements were still en vogue. >>> >>> In this particular case, there can be no doubt that the fallthrough is >>> intentional, so on balance, I'll remove those annotations as well (unless Finn >>> strongly objects?). >>> >> I don't object to removing it. On the contrary, in a previous message I >> also questioned adding this particular 'fallthrough' (though I did >> recommended adding a different one). >> >> In general, there's no way to predict which static checkers are going to >> complain about any given line of code. They don't all agree about >> correctness and they are a moving target, just like fashion or reviewers' >> preferred code styles. > AFAIK, they only complain when the switch() operates on an enum, > and not all enum values are handled. > > When operating on an int, there's not enough address space on > 32-bit machines to handle all cases ;-) > > Gr{oetje,eeting}s, > > Geert >
diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h index 6eae9ec..a965a0e 100644 --- a/arch/m68k/include/asm/io_mm.h +++ b/arch/m68k/include/asm/io_mm.h @@ -101,6 +101,11 @@ #define ISA_TYPE_Q40 (1) #define ISA_TYPE_AG (2) #define ISA_TYPE_ENEC (3) +#define ISA_TYPE_AG16 (4) /* for 100 MBit APNE card */ + +#if defined(CONFIG_APNE100MBIT) +#define MULTI_ISA 1 +#endif #if defined(CONFIG_Q40) && !defined(MULTI_ISA) #define ISA_TYPE ISA_TYPE_Q40 @@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr) case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr); #endif #ifdef CONFIG_AMIGA_PCMCIA +#ifdef CONFIG_APNE100MBIT + case ISA_TYPE_AG16: fallthrough; +#endif case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr); #endif #ifdef CONFIG_ATARI_ROM_ISA @@ -155,6 +163,9 @@ static inline u16 __iomem *isa_itw(unsigned long addr) case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_IO_W(addr); #endif #ifdef CONFIG_AMIGA_PCMCIA +#ifdef CONFIG_APNE100MBIT + case ISA_TYPE_AG16: fallthrough; +#endif case ISA_TYPE_AG: return (u16 __iomem *)AG_ISA_IO_W(addr); #endif #ifdef CONFIG_ATARI_ROM_ISA @@ -171,6 +182,9 @@ static inline u32 __iomem *isa_itl(unsigned long addr) switch(ISA_TYPE) { #ifdef CONFIG_AMIGA_PCMCIA +#ifdef CONFIG_APNE100MBIT + case ISA_TYPE_AG16: fallthrough; +#endif case ISA_TYPE_AG: return (u32 __iomem *)AG_ISA_IO_W(addr); #endif } @@ -184,6 +198,9 @@ static inline u8 __iomem *isa_mtb(unsigned long addr) case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_MEM_B(addr); #endif #ifdef CONFIG_AMIGA_PCMCIA +#ifdef CONFIG_APNE100MBIT + case ISA_TYPE_AG16: fallthrough; +#endif case ISA_TYPE_AG: return (u8 __iomem *)addr; #endif #ifdef CONFIG_ATARI_ROM_ISA @@ -203,6 +220,9 @@ static inline u16 __iomem *isa_mtw(unsigned long addr) case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_MEM_W(addr); #endif #ifdef CONFIG_AMIGA_PCMCIA +#ifdef CONFIG_APNE100MBIT + case ISA_TYPE_AG16: fallthrough; +#endif case ISA_TYPE_AG: return (u16 __iomem *)addr; #endif #ifdef CONFIG_ATARI_ROM_ISA @@ -216,13 +236,14 @@ static inline u16 __iomem *isa_mtw(unsigned long addr) } -#define isa_inb(port) in_8(isa_itb(port)) #define isa_inw(port) (ISA_SEX ? in_be16(isa_itw(port)) : in_le16(isa_itw(port))) #define isa_inl(port) (ISA_SEX ? in_be32(isa_itl(port)) : in_le32(isa_itl(port))) #define isa_outb(val,port) out_8(isa_itb(port),(val)) #define isa_outw(val,port) (ISA_SEX ? out_be16(isa_itw(port),(val)) : out_le16(isa_itw(port),(val))) #define isa_outl(val,port) (ISA_SEX ? out_be32(isa_itl(port),(val)) : out_le32(isa_itl(port),(val))) +#define isa_inb(port) ((ISA_TYPE == ISA_TYPE_AG16) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port))) + #define isa_readb(p) in_8(isa_mtb((unsigned long)(p))) #define isa_readw(p) \ (ISA_SEX ? in_be16(isa_mtw((unsigned long)(p))) \ @@ -270,6 +291,9 @@ static inline void isa_delay(void) case ISA_TYPE_Q40: isa_outb(0,0x80); break; #endif #ifdef CONFIG_AMIGA_PCMCIA +#ifdef CONFIG_APNE100MBIT + case ISA_TYPE_AG16: break; +#endif case ISA_TYPE_AG: break; #endif #ifdef CONFIG_ATARI_ROM_ISA
Add code to support 10 Mbit and 100 Mbit mode for APNE driver. A new ISA type ISA_TYPE_AG16 dynamically switches the Amiga ISA inb accessor to word access as required by the 100 Mbit cards. Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA 100 MBit card support" submitted to netdev 2018/09/16 by Alex Kazik <alex@kazik.de>. Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> -- Changes from RFC: Geert Uytterhoeven: - rename ISA_TYPE_AG100 to ISA_TYPE_AG16 (16 bit cards) - move ISA_TYPE_AG16 case inside #ifdef CONFIG_AMIGA_PCMCIA - change #if defined(CONFIG_APNE_100MBIT) to #ifdef - fix parentheses in isa_inb() define - omit comment about compiler optimization - add ISA_TYPE_AG16 case to isa_delay() --- arch/m68k/include/asm/io_mm.h | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-)