Finn Thain [off-list ref] a écrit :

On Sat, 29 Dec 2018, Arnd Bergmann wrote:

quoted

On Wed, Dec 26, 2018 at 1:43 AM Finn Thain  
[off-list ref] wrote:

quoted

+
+static ssize_t m68k_nvram_get_size(void)
+{
+       if (MACH_IS_ATARI)
+               return atari_nvram_get_size();
+       else if (MACH_IS_MAC)
+               return mac_pram_get_size();
+       return -ENODEV;
+}
+
+/* Atari device drivers call .read (to get checksum validation) whereas
+ * Mac and PowerMac device drivers just use .read_byte.
+ */
+const struct nvram_ops arch_nvram_ops = {
+#ifdef CONFIG_MAC
+       .read_byte      = m68k_nvram_read_byte,
+       .write_byte     = m68k_nvram_write_byte,
+#endif
+#ifdef CONFIG_ATARI
+       .read           = m68k_nvram_read,
+       .write          = m68k_nvram_write,
+       .set_checksum   = m68k_nvram_set_checksum,
+       .initialize     = m68k_nvram_initialize,
+#endif
+       .get_size       = m68k_nvram_get_size,
+};
+EXPORT_SYMBOL(arch_nvram_ops);

Since the operations are almost entirely distinct, why not have two
separate 'nvram_ops' instances here that each refer to just
the set they actually need?

The reason for that is that I am alergic to code duplication. But I'll
change it if you think it matters. BTW, this patch has already been acked
by Geert.

I agree it would be cleaner, as it would also avoid this  
m68k_nvram_get_size() wouldn't it ?

I don't see potential code duplication here, do you ?

Christophe

quoted

I was actually expecting one more patch here that would make the
arch_nvram_ops a pointer to one of multiple structures, which would
be easier to do with multiple copies, but I suppose there is no need
for that here (there might be on ppc, I have to look again).

Yes, I considered that too. I picked the variation that makes everything
const.

--

quoted

       Arnd