Re: [RFC 24/24] m68k: Dispatch nvram_ops calls to Atari or Mac functions

[Geert Uytterhoeven]

Hi Finn,

On Sun, May 31, 2015 at 3:01 AM, Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx> wrote:
> A multi-platform kernel binary needs to decide at run-time how to dispatch
> the arch_nvram_ops calls. Add platform-independent arch_nvram_ops, for use
> when multiple platform-specific NVRAM ops implementations are needed.

Thanks for your series!

> Signed-off-by: Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>
>
> ---
>  arch/m68k/Kconfig                 |    2
>  arch/m68k/atari/nvram.c           |   22 +++++----
>  arch/m68k/include/asm/atarihw.h   |    6 ++
>  arch/m68k/include/asm/macintosh.h |    4 +
>  arch/m68k/kernel/setup_mm.c       |   89 ++++++++++++++++++++++++++++++++++++++
>  arch/m68k/mac/misc.c              |    8 ++-
>  6 files changed, 117 insertions(+), 14 deletions(-)
>
> Index: linux/arch/m68k/atari/nvram.c
> ===================================================================
> --- linux.orig/arch/m68k/atari/nvram.c  2015-05-31 11:01:21.000000000 +1000
> +++ linux/arch/m68k/atari/nvram.c       2015-05-31 11:01:29.000000000 +1000
> @@ -73,7 +73,7 @@ static void __nvram_set_checksum(void)

> +#ifndef CONFIG_MAC
>  const struct nvram_ops arch_nvram_ops = {
> -       .read           = nvram_read,
> -       .write          = nvram_write,
> -       .get_size       = nvram_get_size,
> -       .set_checksum   = nvram_set_checksum,
> -       .initialize     = nvram_initialize,
> +       .read           = atari_nvram_read,
> +       .write          = atari_nvram_write,
> +       .get_size       = atari_nvram_get_size,
> +       .set_checksum   = atari_nvram_set_checksum,
> +       .initialize     = atari_nvram_initialize,
>  };
>  EXPORT_SYMBOL(arch_nvram_ops);
> +#endif

IMHO, the #ifdef is ugly.

>  #ifdef CONFIG_PROC_FS
>  static struct {
> Index: linux/arch/m68k/mac/misc.c
> ===================================================================
> --- linux.orig/arch/m68k/mac/misc.c     2015-05-31 11:01:28.000000000 +1000
> +++ linux/arch/m68k/mac/misc.c  2015-05-31 11:01:29.000000000 +1000
> @@ -489,7 +489,7 @@ void pmu_shutdown(void)

> +#ifndef CONFIG_ATARI
>  const struct nvram_ops arch_nvram_ops = {
>         .read_byte      = mac_pram_read_byte,
>         .write_byte     = mac_pram_write_byte,
>         .get_size       = mac_pram_get_size,
>  };
>  EXPORT_SYMBOL(arch_nvram_ops);
> +#endif
>  #endif /* CONFIG_NVRAM */

Same here.

> Index: linux/arch/m68k/kernel/setup_mm.c
> ===================================================================
> --- linux.orig/arch/m68k/kernel/setup_mm.c      2015-05-31 11:00:59.000000000 +1000
> +++ linux/arch/m68k/kernel/setup_mm.c   2015-05-31 11:01:29.000000000 +1000

> @@ -568,3 +572,88 @@ static int __init adb_probe_sync_enable
>
>  __setup("adb_sync", adb_probe_sync_enable);
>  #endif /* CONFIG_ADB */
> +
> +#if IS_ENABLED(CONFIG_NVRAM) && defined(CONFIG_ATARI) && defined(CONFIG_MAC)

Likewise.

> +static ssize_t m68k_nvram_read(char *buf, size_t count, loff_t *ppos)
> +{
> +       if (MACH_IS_ATARI)
> +               return atari_nvram_read(buf, count, ppos);
> +       else if (MACH_IS_MAC) {
> +               ssize_t size = mac_pram_get_size();
> +               char *p = buf;
> +               loff_t i;
> +
> +               for (i = *ppos; count > 0 && i < size; --count, ++i, ++p)
> +                       *p = mac_pram_read_byte(i);
> +
> +               *ppos = i;
> +               return p - buf;
> +       }
> +       return -EINVAL;

Isn't this handled already by the nvram core, based on the available operations
in nvram_ops? Same for the other nvram abstractions in this file.

> +const struct nvram_ops arch_nvram_ops = {
> +       .read           = m68k_nvram_read,
> +       .write          = m68k_nvram_write,
> +       .read_byte      = m68k_nvram_read_byte,
> +       .write_byte     = m68k_nvram_write_byte,
> +       .get_size       = m68k_nvram_get_size,
> +       .set_checksum   = m68k_nvram_set_checksum,
> +       .initialize     = m68k_nvram_initialize,
> +};
> +EXPORT_SYMBOL(arch_nvram_ops);

Can't you just fill in the mach specific pointers in the generic structure,
and be done with it?

If you handle this right, I think you can do without the temporary "def_bool
(ATARI && !MAC) || (MAC && !ATARI)" in "[RFC 22/24] m68k/mac: Adopt nvram
module", too.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/