| Message ID | 1622958877-2026-2-git-send-email-schmitzmic@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add APNE PCMCIA 100 Mbit support | expand |
Hi Michael, On Sun, Jun 6, 2021 at 7:54 AM Michael Schmitz <schmitzmic@gmail.com> wrote: > Add code to support 10 Mbit and 100 Mbit mode for APNE driver. > > A new ISA type ISA_TYPE_AG100 switches the Amiga ISA inb accessor > to word access as required by the 100 Mbit cards. missing "dynamically". > > 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> Thanks for your patch! > --- 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_AG100 (4) /* for 100 MBit APNE card */ As this type may be needed for other cards, perhaps it should be named after what it really does, i.e. always using 16-bit accesses? ISA_TYPE_AG16? > + > +#if defined(CONFIG_APNE100MBIT) #ifdef > +#define MULTI_ISA 1 > +#endif > > #if defined(CONFIG_Q40) && !defined(MULTI_ISA) > #define ISA_TYPE ISA_TYPE_Q40 > @@ -135,6 +140,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr) > #ifdef CONFIG_Q40 > case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr); > #endif > +#if defined(CONFIG_APNE100MBIT) #ifdef > + case ISA_TYPE_AG100: fallthrough; Fallthrough to what? Oh, to ISA_TYPE_AG. Please make this clear, and safer, by moving this inside the #ifdef below (CONFIG_APNE100MBIT depends on CONFIG_AMIGA_PCMCIA). > +#endif > #ifdef CONFIG_AMIGA_PCMCIA > case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr); > #endif > @@ -212,13 +232,16 @@ static inline u16 __iomem *isa_mtw(unsigned long addr) > } > > > -#define isa_inb(port) in_8(isa_itb(port)) Why move it below? Because its comment does not apply to the below? > #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))) > > +/* for APNE 100 Mbit cards - hope the APNE 100 case will be eliminated as > + * dead code if MULTI_ISA is not set */ I don't think this comment is needed. > +#define isa_inb(port) ((ISA_TYPE == ISA_TYPE_AG100) ? ((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))) \ Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for reviewing this one as well! Am 07.06.2021 um 20:01 schrieb Geert Uytterhoeven: > Hi Michael, > > On Sun, Jun 6, 2021 at 7:54 AM Michael Schmitz <schmitzmic@gmail.com> wrote: >> Add code to support 10 Mbit and 100 Mbit mode for APNE driver. >> >> A new ISA type ISA_TYPE_AG100 switches the Amiga ISA inb accessor >> to word access as required by the 100 Mbit cards. > > missing "dynamically". OK... > >> >> 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> > > Thanks for your patch! > >> --- 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_AG100 (4) /* for 100 MBit APNE card */ > > As this type may be needed for other cards, perhaps it should be > named after what it really does, i.e. always using 16-bit accesses? > ISA_TYPE_AG16? Yep, that sounds better. > >> + >> +#if defined(CONFIG_APNE100MBIT) > > #ifdef > >> +#define MULTI_ISA 1 >> +#endif >> >> #if defined(CONFIG_Q40) && !defined(MULTI_ISA) >> #define ISA_TYPE ISA_TYPE_Q40 >> @@ -135,6 +140,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr) >> #ifdef CONFIG_Q40 >> case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr); >> #endif >> +#if defined(CONFIG_APNE100MBIT) > > #ifdef > >> + case ISA_TYPE_AG100: fallthrough; > > Fallthrough to what? Oh, to ISA_TYPE_AG. > Please make this clear, and safer, by moving this inside the #ifdef > below (CONFIG_APNE100MBIT depends on CONFIG_AMIGA_PCMCIA). OK, good point. >> +#endif >> #ifdef CONFIG_AMIGA_PCMCIA >> case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr); >> #endif > >> @@ -212,13 +232,16 @@ static inline u16 __iomem *isa_mtw(unsigned long addr) >> } >> >> >> -#define isa_inb(port) in_8(isa_itb(port)) > > Why move it below? Because its comment does not apply to the below? Because the new form depends on isa_inw()? I've seen order matter in CPP statements before ... >> #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))) >> >> +/* for APNE 100 Mbit cards - hope the APNE 100 case will be eliminated as >> + * dead code if MULTI_ISA is not set */ > > I don't think this comment is needed. I'll remove it, then. Cheers, Michael > >> +#define isa_inb(port) ((ISA_TYPE == ISA_TYPE_AG100) ? ((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))) \ > > Gr{oetje,eeting}s, > > Geert >
Hi Michael, On Sun, Jun 6, 2021 at 7:54 AM Michael Schmitz <schmitzmic@gmail.com> wrote: > Add code to support 10 Mbit and 100 Mbit mode for APNE driver. > > A new ISA type ISA_TYPE_AG100 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> > --- a/arch/m68k/include/asm/io_mm.h > +++ b/arch/m68k/include/asm/io_mm.h > @@ -212,13 +232,16 @@ 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))) > > +/* for APNE 100 Mbit cards - hope the APNE 100 case will be eliminated as > + * dead code if MULTI_ISA is not set */ > +#define isa_inb(port) ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port)) This fails to compile due to a missing closing parenthesis. Gr{oetje,eeting}s, Geert
Hi Geert, On 7/06/21 11:15 pm, Geert Uytterhoeven wrote: > Hi Michael, > > On Sun, Jun 6, 2021 at 7:54 AM Michael Schmitz <schmitzmic@gmail.com> wrote: >> Add code to support 10 Mbit and 100 Mbit mode for APNE driver. >> >> A new ISA type ISA_TYPE_AG100 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> >> --- a/arch/m68k/include/asm/io_mm.h >> +++ b/arch/m68k/include/asm/io_mm.h >> @@ -212,13 +232,16 @@ 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))) >> >> +/* for APNE 100 Mbit cards - hope the APNE 100 case will be eliminated as >> + * dead code if MULTI_ISA is not set */ >> +#define isa_inb(port) ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port)) > This fails to compile due to a missing closing parenthesis. Sorry - looks like brown paper bag time today. (I did say 'entirely untested'? Didn't expect such a thorough review for a first RFC patch ...) Apologies, Michael > > Gr{oetje,eeting}s, > > Geert >
Hi Michael, On Mon, Jun 7, 2021 at 9:58 PM Michael Schmitz <schmitzmic@gmail.com> wrote: > On 7/06/21 11:15 pm, Geert Uytterhoeven wrote: > > On Sun, Jun 6, 2021 at 7:54 AM Michael Schmitz <schmitzmic@gmail.com> wrote: > >> Add code to support 10 Mbit and 100 Mbit mode for APNE driver. > >> > >> A new ISA type ISA_TYPE_AG100 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> > >> --- a/arch/m68k/include/asm/io_mm.h > >> +++ b/arch/m68k/include/asm/io_mm.h > >> @@ -212,13 +232,16 @@ 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))) > >> > >> +/* for APNE 100 Mbit cards - hope the APNE 100 case will be eliminated as > >> + * dead code if MULTI_ISA is not set */ > >> +#define isa_inb(port) ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port)) > > This fails to compile due to a missing closing parenthesis. > > Sorry - looks like brown paper bag time today. (I did say 'entirely > untested'? Didn't expect such a thorough review for a first RFC patch ...) Sorry, I missed that part in the cover letter ;-) Gr{oetje,eeting}s, Geert
Hi Geert, On 8/06/21 6:42 pm, Geert Uytterhoeven wrote: > +#define isa_inb(port) ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port)) >>> This fails to compile due to a missing closing parenthesis. >> Sorry - looks like brown paper bag time today. (I did say 'entirely >> untested'? Didn't expect such a thorough review for a first RFC patch ...) > Sorry, I missed that part in the cover letter ;-) I'll put it more prominently next time. Ran all patches through checkpatch now, and I still get warnings and even a few errors ('trailing statements should be on the next line'). All due to my keeping to the code style used in io_mm.h, as far as I could see. What's your preference - additions in new style, or keep the old style? Cheers, Michael > > Gr{oetje,eeting}s, > > Geert >
Hi Geert, On 8/06/21 6:42 pm, Geert Uytterhoeven wrote: > +#define isa_inb(port) ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port)) >>> This fails to compile due to a missing closing parenthesis. >> Sorry - looks like brown paper bag time today. (I did say 'entirely >> untested'? Didn't expect such a thorough review for a first RFC patch ...) > Sorry, I missed that part in the cover letter ;-) I'll put it more prominently next time. Ran all patches through checkpatch now, and I still get warnings and even a few errors ('trailing statements should be on the next line'). All due to my keeping to the code style used in io_mm.h, as far as I could see. What's your preference - additions in new style, or keep the old style? Cheers, Michael > > Gr{oetje,eeting}s, > > Geert >
Hi Michael, On Tue, Jun 8, 2021 at 11:55 PM Michael Schmitz <schmitzmic@gmail.com> wrote: > On 8/06/21 6:42 pm, Geert Uytterhoeven wrote: > > +#define isa_inb(port) ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port)) > >>> This fails to compile due to a missing closing parenthesis. > >> Sorry - looks like brown paper bag time today. (I did say 'entirely > >> untested'? Didn't expect such a thorough review for a first RFC patch ...) > > Sorry, I missed that part in the cover letter ;-) > > I'll put it more prominently next time. > > Ran all patches through checkpatch now, and I still get warnings and > even a few errors ('trailing statements should be on the next line'). > All due to my keeping to the code style used in io_mm.h, as far as I > could see. > > What's your preference - additions in new style, or keep the old style? Please use the existing style, unless you're willing to modernize the whole file in a separate patch. Gr{oetje,eeting}s, Geert
diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h index f6b487b..71f694a 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_AG100 (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 @@ -135,6 +140,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr) #ifdef CONFIG_Q40 case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr); #endif +#if defined(CONFIG_APNE100MBIT) + case ISA_TYPE_AG100: fallthrough; +#endif #ifdef CONFIG_AMIGA_PCMCIA case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr); #endif @@ -153,6 +161,9 @@ static inline u16 __iomem *isa_itw(unsigned long addr) #ifdef CONFIG_Q40 case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_IO_W(addr); #endif +#if defined(CONFIG_APNE100MBIT) + case ISA_TYPE_AG100: fallthrough; +#endif #ifdef CONFIG_AMIGA_PCMCIA case ISA_TYPE_AG: return (u16 __iomem *)AG_ISA_IO_W(addr); #endif @@ -168,6 +179,9 @@ static inline u32 __iomem *isa_itl(unsigned long addr) { switch(ISA_TYPE) { +#if defined(CONFIG_APNE100MBIT) + case ISA_TYPE_AG100: fallthrough; +#endif #ifdef CONFIG_AMIGA_PCMCIA case ISA_TYPE_AG: return (u32 __iomem *)AG_ISA_IO_W(addr); #endif @@ -181,6 +195,9 @@ static inline u8 __iomem *isa_mtb(unsigned long addr) #ifdef CONFIG_Q40 case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_MEM_B(addr); #endif +#if defined(CONFIG_APNE100MBIT) + case ISA_TYPE_AG100: fallthrough; +#endif #ifdef CONFIG_AMIGA_PCMCIA case ISA_TYPE_AG: return (u8 __iomem *)addr; #endif @@ -199,6 +216,9 @@ static inline u16 __iomem *isa_mtw(unsigned long addr) #ifdef CONFIG_Q40 case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_MEM_W(addr); #endif +#if defined(CONFIG_APNE100MBIT) + case ISA_TYPE_AG100: fallthrough; +#endif #ifdef CONFIG_AMIGA_PCMCIA case ISA_TYPE_AG: return (u16 __iomem *)addr; #endif @@ -212,13 +232,16 @@ 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))) +/* for APNE 100 Mbit cards - hope the APNE 100 case will be eliminated as + * dead code if MULTI_ISA is not set */ +#define isa_inb(port) ((ISA_TYPE == ISA_TYPE_AG100) ? ((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))) \
Add code to support 10 Mbit and 100 Mbit mode for APNE driver. A new ISA type ISA_TYPE_AG100 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> --- arch/m68k/include/asm/io_mm.h | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)