Messages in this thread | | | Date | Sun, 30 Dec 2018 18:53:00 +0100 | From | LEROY Christophe <> | Subject | Re: [PATCH v8 13/25] m68k: Dispatch nvram_ops calls to Atari or Mac functions |
| |
Finn Thain <fthain@telegraphics.com.au> a écrit :
> On Sat, 29 Dec 2018, Arnd Bergmann wrote: > >> On Wed, Dec 26, 2018 at 1:43 AM Finn Thain >> <fthain@telegraphics.com.au> wrote: >> >> > + >> > +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
> >> 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. > > -- > >> Arnd >>
| |