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

Geert Uytterhoeven
2015-06-01T08:31:55Z

Hi Finn,

On Sun, May 31, 2015 at 3:01 AM, Finn Thain <fthain@telegraphics.com.au> 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@telegraphics.com.au>
>
> ---
> 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@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