Hi Finn,

On Sun, May 31, 2015 at 3:01 AM, Finn Thain [off-list ref] 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!

+#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.

+#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.

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