On Sun, Dec 30, 2018 at 12:43 AM Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx> wrote:
Is there some benefit, or is that just personal taste?
Avoiding changes to call sites avoids code review, but I think 1) the
thinkpad_acpi changes have already been reviewed and 2) the fbdev changes
need review anyway.
Your suggesion would add several new entities and one extra layer of
indirection.
I think indirection harms readability because now the reader now has to go
and look up the meaning of the new entities.
It's not the case that we need to choose between definitions of
nvram_read_byte() at compile time, or stub them out:
#ifdef CONFIG_FOO
static inline unsigned char nvram_read_byte(int addr)
{
return arch_nvram_ops.read_byte(addr);
}
#else
static inline unsigned char nvram_read_byte(int addr) { }
#endif
And I don't anticipate a need for a macro here either:
#define nvram_read_byte(a) random_nvram_read_byte_impl(a)
I think I've used the simplest solution.
Having the indirection would help if the inline function can
encapsulate the NULL pointer check, like
static inline unsigned char nvram_read_byte(loff_t addr)
{
char data;
if (!IS_ENABLED(CONFIG_NVRAM))
return 0xff;
if (arch_nvram_ops.read_byte)
return arch_nvram_ops.read_byte(addr);
if (arch_nvram_ops.read)
return arch_nvram_ops.read(char, 1, &addr);
return 0xff;
}
(the above assumes no #ifdef in the structure definition, if you
keep the #ifdef there they have to be added here as well).
Arnd
