Re: [PATCH RFC 1/2] m68k/atari: add platform device for Falcon IDE port

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Michael,

On Thu, Jun 20, 2019 at 10:47 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Autoloading of Falcon IDE driver modules requires converting
> these drivers to platform drivers.
>
> Add platform device for Falcon IDE interface in Atari platform
> setup code in preparation for this.
>
> Add Falcon IDE base address in Atari hardware address header.
>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

Thanks for your patch!

> --- a/arch/m68k/atari/config.c
> +++ b/arch/m68k/atari/config.c
> @@ -896,6 +896,21 @@ static void isp1160_delay(struct device *dev, int delay)
>  };
>  #endif
>
> +#if IS_ENABLED(CONFIG_PATA_FALCON)

I wouldn't bother making this depend on a config symbol, as it is
builtin hardware (EtherNEC/NAT isn't), and prevents compiling a module
later.
arch/m68k/amiga/platform.c has everything unconditional.
I know there's such a dependency for SCSI, perhaps it should be removed?

> +static const struct resource atari_falconide_rsrc[] __initconst = {
> +       {
> +               .flags = IORESOURCE_MEM,
> +               .start = FALCON_IDE_BASE,
> +               .end   = FALCON_IDE_BASE+0x40,
> +       },
> +       {
> +               .flags = IORESOURCE_IRQ,
> +               .start = IRQ_MFP_FSCSI,
> +               .end   = IRQ_MFP_FSCSI,
> +       },
> +};
> +#endif
> +
>  int __init atari_platform_init(void)
>  {
>         int rv = 0;
> @@ -939,6 +954,11 @@ int __init atari_platform_init(void)
>                         atari_scsi_tt_rsrc, ARRAY_SIZE(atari_scsi_tt_rsrc));
>  #endif
>
> +#if IS_ENABLED(CONFIG_PATA_FALCON)
> +       if (ATARIHW_PRESENT(IDE))
> +               platform_device_register_simple("pata_falcon", -1,
> +                       atari_falconide_rsrc, ARRAY_SIZE(atari_falconide_rsrc));
> +#endif
>         return rv;
>  }
>
> diff --git a/arch/m68k/include/asm/atarihw.h b/arch/m68k/include/asm/atarihw.h
> index 5330082..4bea923 100644
> --- a/arch/m68k/include/asm/atarihw.h
> +++ b/arch/m68k/include/asm/atarihw.h
> @@ -813,6 +813,12 @@ struct MSTE_RTC {
>  #define mste_rtc ((*(volatile struct MSTE_RTC *)MSTE_RTC_BAS))
>
>  /*
> +** Falcon IDE interface
> +*/
> +
> +#define FALCON_IDE_BASE        0xfff00000

Is it worth having this as a #define in a global header file?
You still need a hardcoded region size in config.c.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds