Hi Finn,
Am 04.06.2021 um 17:55 schrieb Finn Thain:
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.
I'll leave that for later (but add a comment in the lines inserted).
#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.
The NULL default was meant to catch incorrect use of config options
related to CONFIG_ISA. My repurposing the default branch for Atari's
needs (no translation for IDE) defeats that. But the chance that we run
into such incorrect use in the future are pretty slim indeed.
Still, my patch changes behaviour in existing drivers. We're quite
certain it does not matter, but is that good enough to be accepted?
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
Never ran into 'fallthrough' except as comment annotation, but I now see
that really is a thing these days. Amazing.
'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);
}
}
This one makes it easy to eventually add another ISA_TYPE_ATARI case
before the default case (which could then revert to NULL if desired).
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.
Yes, but it rules out reverting the default case to NULL.
On balance, I'll go with the fallback (and will annotate that other
Atari drivers fall back to the clause below on purpose).
Cheers,
Michael
