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.
