On Fri, 4 Jun 2021, Michael Schmitz wrote:
of the Q40 branch and out of the later Amiga Gayle PCMCIA branch to the
head of the file and add a comment explaining the rationale? Too much
churn for now?
Right, it could be too much churn for a bug-fix patch.
#ifdef CONFIG_AMIGA_PCMCIA
@@ -135,9 +139,12 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
#endif
#ifdef CONFIG_ATARI_ROM_ISA
- case ISA_TYPE_ENEC: return (u8 __iomem *)ENEC_ISA_IO_B(addr);
+ case ISA_TYPE_ENEC: if (addr < 1024)
+ return (u8 __iomem *)ENEC_ISA_IO_B(addr);
+ else
+ return (u8 __iomem *)(addr);
There is some trailing whitespace added here that 'git am' complains
about.
Also, I think a 'fallthrough' statement would be better than adding a new
else branch that duplicates the new default case below.
I'm still unsure whether changing the default branch for the sake of
fixing Atari behaviour is a sane idea - I was hoping for comments either
way.
You mean, what happens if a random m68k platform (other than amiga, atari
and q40) were to adopt CONFIG_ISA? I guess it would be nice if that would
'just work' but it's probably never going to be needed.
But if it's changed, you are correct that having the control flow fall
through instead of taking a redundant else branch is better.
Essentially, changing that hunk to
#ifdef CONFIG_ATARI_ROM_ISA
- case ISA_TYPE_ENEC: return (u8 __iomem *)ENEC_ISA_IO_B(addr);
+ case ISA_TYPE_ENEC: if (addr < 1024)
+ return (u8 __iomem *)ENEC_ISA_IO_B(addr);
here (and elsewhere below)?
I worry about the static checkers that look for missing 'fallthrough' and
'break' statements. So I was thinking of something like this:
static inline u8 __iomem *isa_itb(unsigned long addr)
{
switch(ISA_TYPE)
{
#ifdef CONFIG_Q40
case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
#endif
#ifdef CONFIG_AMIGA_PCMCIA
case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
#endif
#ifdef CONFIG_ATARI_ROM_ISA
case ISA_TYPE_ENEC:
if (addr < 1024)
return (u8 __iomem *)ENEC_ISA_IO_B(addr);
fallthrough;
#endif
default: return (u8 __iomem *)(addr);
}
}
Alternatively you could just move the default out of the switch:
static inline u8 __iomem *isa_itb(unsigned long addr)
{
switch(ISA_TYPE)
{
#ifdef CONFIG_Q40
case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
#endif
#ifdef CONFIG_AMIGA_PCMCIA
case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
#endif
#ifdef CONFIG_ATARI_ROM_ISA
case ISA_TYPE_ENEC:
if (addr < 1024)
return (u8 __iomem *)ENEC_ISA_IO_B(addr);
break;
#endif
}
return (u8 __iomem *)(addr);
}
The latter is probably the more flexible style because 'break' doesn't
care about the order of case labels.
