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

[Michael Schmitz <schmitzmic@gmail.com>]

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