Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Finn Thain <fthain@linux-m68k.org>]

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.