Re: [PATCH v3 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Finn,

thanks for your comments!

On 17/06/21 6:58 pm, Finn Thain wrote:
>> -#define isa_inb(port)      in_8(isa_itb(port))
>>   #define isa_inw(port)      (ISA_SEX ? in_be16(isa_itw(port)) : in_le16(isa_itw(port)))
>>   #define isa_inl(port)      (ISA_SEX ? in_be32(isa_itl(port)) : in_le32(isa_itl(port)))
>>   #define isa_outb(val,port) out_8(isa_itb(port),(val))
>>   #define isa_outw(val,port) (ISA_SEX ? out_be16(isa_itw(port),(val)) : out_le16(isa_itw(port),(val)))
>>   #define isa_outl(val,port) (ISA_SEX ? out_be32(isa_itl(port),(val)) : out_le32(isa_itl(port),(val)))
>>   
>> +#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG16) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port)))
>> +
>>   #define isa_readb(p)       in_8(isa_mtb((unsigned long)(p)))
>>   #define isa_readw(p)       \
>>   	(ISA_SEX ? in_be16(isa_mtw((unsigned long)(p)))	\
> Was the re-ordering of definitions deliberate?

Yes, it was. I remembered looking at a bug report for curl on the m68k 
kernel that was due to ordering of #defines.

On second thought, there is no prior definition of isa_inw in our case, 
so even this won't really be an issue.

>
>
>> @@ -270,6 +291,9 @@ static inline void isa_delay(void)
>>       case ISA_TYPE_Q40: isa_outb(0,0x80); break;
>>   #endif
>>   #ifdef CONFIG_AMIGA_PCMCIA
>> +#ifdef CONFIG_APNE100MBIT
>> +    case ISA_TYPE_AG16: break;
>> +#endif
>>       case ISA_TYPE_AG: break;
>>   #endif
>>   #ifdef CONFIG_ATARI_ROM_ISA
>>
> I think most of these "#ifdef CONFIG_APNE100MBIT" conditionals are
> redundant. case ISA_TYPE_AG16 should be optimized away as dead code in the
> MULTI_ISA == 0 configuration. And in the MULTI_ISA == 1 configuration, the
> logic used to assign isa_type already depends on
> defined(CONFIG_APNE100MBIT).

You're right there, too. I'll drop those.

Cheers,

     Michael