On Mon, 31 Dec 2018, Arnd Bergmann wrote:
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 semantics of .read_byte and .read are subtly different. For CONFIG_X86 and CONFIG_ATARI, .read implies checksum validation and .read_byte does not. In particular, in the thinkpad_acpi case, checksum validation isn't used, but in the atari_scsi case, it is. So I like to see drivers explicitly call the method they want. I didn't want to obscure this distinction in a helper. --
