[PATCH v8 20/25] powerpc, fbdev: Use arch_nvram_ops methods instead of nvram_read_byte() and nvram_write_byte()
Make use of arch_nvram_ops in device drivers so that the nvram_* function exports can be removed.
Since they are no longer global symbols, rename the PPC32 nvram_* functions appropriately.
Signed-off-by: Finn Thain fthain@telegraphics.com.au --- arch/powerpc/kernel/setup_32.c | 8 ++++---- drivers/char/generic_nvram.c | 4 ++-- drivers/video/fbdev/controlfb.c | 4 ++-- drivers/video/fbdev/imsttfb.c | 4 ++-- drivers/video/fbdev/matrox/matroxfb_base.c | 2 +- drivers/video/fbdev/platinumfb.c | 4 ++-- drivers/video/fbdev/valkyriefb.c | 4 ++-- 7 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c index e0d045677472..bdbe6acbef11 100644 --- a/arch/powerpc/kernel/setup_32.c +++ b/arch/powerpc/kernel/setup_32.c @@ -152,20 +152,18 @@ __setup("l3cr=", ppc_setup_l3cr);
#ifdef CONFIG_GENERIC_NVRAM
-unsigned char nvram_read_byte(int addr) +static unsigned char ppc_nvram_read_byte(int addr) { if (ppc_md.nvram_read_val) return ppc_md.nvram_read_val(addr); return 0xff; } -EXPORT_SYMBOL(nvram_read_byte);
-void nvram_write_byte(unsigned char val, int addr) +static void ppc_nvram_write_byte(unsigned char val, int addr) { if (ppc_md.nvram_write_val) ppc_md.nvram_write_val(addr, val); } -EXPORT_SYMBOL(nvram_write_byte);
static ssize_t ppc_nvram_get_size(void) { @@ -182,6 +180,8 @@ static long ppc_nvram_sync(void) }
const struct nvram_ops arch_nvram_ops = { + .read_byte = ppc_nvram_read_byte, + .write_byte = ppc_nvram_write_byte, .get_size = ppc_nvram_get_size, .sync = ppc_nvram_sync, }; diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c index f32d5663de95..41b76bf9614e 100644 --- a/drivers/char/generic_nvram.c +++ b/drivers/char/generic_nvram.c @@ -48,7 +48,7 @@ static ssize_t read_nvram(struct file *file, char __user *buf, if (*ppos >= nvram_len) return 0; for (i = *ppos; count > 0 && i < nvram_len; ++i, ++p, --count) - if (__put_user(nvram_read_byte(i), p)) + if (__put_user(arch_nvram_ops.read_byte(i), p)) return -EFAULT; *ppos = i; return p - buf; @@ -68,7 +68,7 @@ static ssize_t write_nvram(struct file *file, const char __user *buf, for (i = *ppos; count > 0 && i < nvram_len; ++i, ++p, --count) { if (__get_user(c, p)) return -EFAULT; - nvram_write_byte(c, i); + arch_nvram_ops.write_byte(c, i); } *ppos = i; return p - buf; diff --git a/drivers/video/fbdev/controlfb.c b/drivers/video/fbdev/controlfb.c index 9cb0ef7ac29e..27ff33ccafcb 100644 --- a/drivers/video/fbdev/controlfb.c +++ b/drivers/video/fbdev/controlfb.c @@ -413,7 +413,7 @@ static int __init init_control(struct fb_info_control *p) /* Try to pick a video mode out of NVRAM if we have one. */ #ifdef CONFIG_NVRAM if (default_cmode == CMODE_NVRAM) { - cmode = nvram_read_byte(NV_CMODE); + cmode = arch_nvram_ops.read_byte(NV_CMODE); if(cmode < CMODE_8 || cmode > CMODE_32) cmode = CMODE_8; } else @@ -421,7 +421,7 @@ static int __init init_control(struct fb_info_control *p) cmode=default_cmode; #ifdef CONFIG_NVRAM if (default_vmode == VMODE_NVRAM) { - vmode = nvram_read_byte(NV_VMODE); + vmode = arch_nvram_ops.read_byte(NV_VMODE); if (vmode < 1 || vmode > VMODE_MAX || control_mac_modes[vmode - 1].m[full] < cmode) { sense = read_control_sense(p); diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c index 8d231591ff0e..e9f3b8914145 100644 --- a/drivers/video/fbdev/imsttfb.c +++ b/drivers/video/fbdev/imsttfb.c @@ -1393,12 +1393,12 @@ static void init_imstt(struct fb_info *info) int vmode = init_vmode, cmode = init_cmode;
if (vmode == -1) { - vmode = nvram_read_byte(NV_VMODE); + vmode = arch_nvram_ops.read_byte(NV_VMODE); if (vmode <= 0 || vmode > VMODE_MAX) vmode = VMODE_640_480_67; } if (cmode == -1) { - cmode = nvram_read_byte(NV_CMODE); + cmode = arch_nvram_ops.read_byte(NV_CMODE); if (cmode < CMODE_8 || cmode > CMODE_32) cmode = CMODE_8; } diff --git a/drivers/video/fbdev/matrox/matroxfb_base.c b/drivers/video/fbdev/matrox/matroxfb_base.c index cac5865d461c..3fecc628752c 100644 --- a/drivers/video/fbdev/matrox/matroxfb_base.c +++ b/drivers/video/fbdev/matrox/matroxfb_base.c @@ -1876,7 +1876,7 @@ static int initMatrox2(struct matrox_fb_info *minfo, struct board *b) default_vmode = VMODE_640_480_60; #if defined(CONFIG_PPC32) && defined(CONFIG_NVRAM) if (default_cmode == CMODE_NVRAM) - default_cmode = nvram_read_byte(NV_CMODE); + default_cmode = arch_nvram_ops.read_byte(NV_CMODE); #endif if (default_cmode < CMODE_8 || default_cmode > CMODE_32) default_cmode = CMODE_8; diff --git a/drivers/video/fbdev/platinumfb.c b/drivers/video/fbdev/platinumfb.c index bf6b7fb83cf4..3efceaf38f98 100644 --- a/drivers/video/fbdev/platinumfb.c +++ b/drivers/video/fbdev/platinumfb.c @@ -347,7 +347,7 @@ static int platinum_init_fb(struct fb_info *info) printk(KERN_INFO "platinumfb: Monitor sense value = 0x%x, ", sense); if (default_vmode == VMODE_NVRAM) { #ifdef CONFIG_NVRAM - default_vmode = nvram_read_byte(NV_VMODE); + default_vmode = arch_nvram_ops.read_byte(NV_VMODE); if (default_vmode <= 0 || default_vmode > VMODE_MAX || !platinum_reg_init[default_vmode-1]) #endif @@ -360,7 +360,7 @@ static int platinum_init_fb(struct fb_info *info) default_vmode = VMODE_640_480_60; #ifdef CONFIG_NVRAM if (default_cmode == CMODE_NVRAM) - default_cmode = nvram_read_byte(NV_CMODE); + default_cmode = arch_nvram_ops.read_byte(NV_CMODE); #endif if (default_cmode < CMODE_8 || default_cmode > CMODE_32) default_cmode = CMODE_8; diff --git a/drivers/video/fbdev/valkyriefb.c b/drivers/video/fbdev/valkyriefb.c index 8022316ee9c9..81f66d69d9dd 100644 --- a/drivers/video/fbdev/valkyriefb.c +++ b/drivers/video/fbdev/valkyriefb.c @@ -283,7 +283,7 @@ static void __init valkyrie_choose_mode(struct fb_info_valkyrie *p) /* Try to pick a video mode out of NVRAM if we have one. */ #if defined(CONFIG_PPC_PMAC) && defined(CONFIG_NVRAM) if (default_vmode == VMODE_NVRAM) { - default_vmode = nvram_read_byte(NV_VMODE); + default_vmode = arch_nvram_ops.read_byte(NV_VMODE); if (default_vmode <= 0 || default_vmode > VMODE_MAX || !valkyrie_reg_init[default_vmode - 1]) @@ -296,7 +296,7 @@ static void __init valkyrie_choose_mode(struct fb_info_valkyrie *p) default_vmode = VMODE_640_480_67; #if defined(CONFIG_PPC_PMAC) && defined(CONFIG_NVRAM) if (default_cmode == CMODE_NVRAM) - default_cmode = nvram_read_byte(NV_CMODE); + default_cmode = arch_nvram_ops.read_byte(NV_CMODE); #endif
/*
Finn Thain fthain@telegraphics.com.au a écrit :
Make use of arch_nvram_ops in device drivers so that the nvram_* function exports can be removed.
Since they are no longer global symbols, rename the PPC32 nvram_* functions appropriately.
Signed-off-by: Finn Thain fthain@telegraphics.com.au
arch/powerpc/kernel/setup_32.c | 8 ++++---- drivers/char/generic_nvram.c | 4 ++-- drivers/video/fbdev/controlfb.c | 4 ++-- drivers/video/fbdev/imsttfb.c | 4 ++-- drivers/video/fbdev/matrox/matroxfb_base.c | 2 +- drivers/video/fbdev/platinumfb.c | 4 ++-- drivers/video/fbdev/valkyriefb.c | 4 ++-- 7 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c index e0d045677472..bdbe6acbef11 100644 --- a/arch/powerpc/kernel/setup_32.c +++ b/arch/powerpc/kernel/setup_32.c @@ -152,20 +152,18 @@ __setup("l3cr=", ppc_setup_l3cr);
#ifdef CONFIG_GENERIC_NVRAM
-unsigned char nvram_read_byte(int addr) +static unsigned char ppc_nvram_read_byte(int addr) { if (ppc_md.nvram_read_val) return ppc_md.nvram_read_val(addr); return 0xff; } -EXPORT_SYMBOL(nvram_read_byte);
-void nvram_write_byte(unsigned char val, int addr) +static void ppc_nvram_write_byte(unsigned char val, int addr) { if (ppc_md.nvram_write_val) ppc_md.nvram_write_val(addr, val); } -EXPORT_SYMBOL(nvram_write_byte);
static ssize_t ppc_nvram_get_size(void) { @@ -182,6 +180,8 @@ static long ppc_nvram_sync(void) }
const struct nvram_ops arch_nvram_ops = {
- .read_byte = ppc_nvram_read_byte,
- .write_byte = ppc_nvram_write_byte, .get_size = ppc_nvram_get_size, .sync = ppc_nvram_sync,
}; diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c index f32d5663de95..41b76bf9614e 100644 --- a/drivers/char/generic_nvram.c +++ b/drivers/char/generic_nvram.c @@ -48,7 +48,7 @@ static ssize_t read_nvram(struct file *file, char __user *buf, if (*ppos >= nvram_len) return 0; for (i = *ppos; count > 0 && i < nvram_len; ++i, ++p, --count)
if (__put_user(nvram_read_byte(i), p))
if (__put_user(arch_nvram_ops.read_byte(i), p))
Instead of modifying all drivers (in this patch and previous ones related to other arches), wouldn't it be better to add helpers like the following in nvram.h:
Static inline unsigned char nvram_read_byte(int addr) { return arch_nvram_ops.read_byte(addr); }
Christophe
return -EFAULT;*ppos = i; return p - buf; @@ -68,7 +68,7 @@ static ssize_t write_nvram(struct file *file, const char __user *buf, for (i = *ppos; count > 0 && i < nvram_len; ++i, ++p, --count) { if (__get_user(c, p)) return -EFAULT;
nvram_write_byte(c, i);
arch_nvram_ops.write_byte(c, i);diff --git a/drivers/video/fbdev/controlfb.c b/drivers/video/fbdev/controlfb.c index 9cb0ef7ac29e..27ff33ccafcb 100644 --- a/drivers/video/fbdev/controlfb.c +++ b/drivers/video/fbdev/controlfb.c @@ -413,7 +413,7 @@ static int __init init_control(struct fb_info_control *p) /* Try to pick a video mode out of NVRAM if we have one. */ #ifdef CONFIG_NVRAM if (default_cmode == CMODE_NVRAM) {
cmode = nvram_read_byte(NV_CMODE);
cmode = arch_nvram_ops.read_byte(NV_CMODE);@@ -421,7 +421,7 @@ static int __init init_control(struct fb_info_control *p) cmode=default_cmode; #ifdef CONFIG_NVRAM if (default_vmode == VMODE_NVRAM) {
vmode = nvram_read_byte(NV_VMODE);
vmode = arch_nvram_ops.read_byte(NV_VMODE);diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c index 8d231591ff0e..e9f3b8914145 100644 --- a/drivers/video/fbdev/imsttfb.c +++ b/drivers/video/fbdev/imsttfb.c @@ -1393,12 +1393,12 @@ static void init_imstt(struct fb_info *info) int vmode = init_vmode, cmode = init_cmode;
if (vmode == -1) {
vmode = nvram_read_byte(NV_VMODE);
vmode = arch_nvram_ops.read_byte(NV_VMODE); if (vmode <= 0 || vmode > VMODE_MAX) vmode = VMODE_640_480_67;
cmode = nvram_read_byte(NV_CMODE);
cmode = arch_nvram_ops.read_byte(NV_CMODE); if (cmode < CMODE_8 || cmode > CMODE_32) cmode = CMODE_8;diff --git a/drivers/video/fbdev/matrox/matroxfb_base.c b/drivers/video/fbdev/matrox/matroxfb_base.c index cac5865d461c..3fecc628752c 100644 --- a/drivers/video/fbdev/matrox/matroxfb_base.c +++ b/drivers/video/fbdev/matrox/matroxfb_base.c @@ -1876,7 +1876,7 @@ static int initMatrox2(struct matrox_fb_info *minfo, struct board *b) default_vmode = VMODE_640_480_60; #if defined(CONFIG_PPC32) && defined(CONFIG_NVRAM) if (default_cmode == CMODE_NVRAM)
default_cmode = nvram_read_byte(NV_CMODE);
default_cmode = arch_nvram_ops.read_byte(NV_CMODE);#endif if (default_cmode < CMODE_8 || default_cmode > CMODE_32) default_cmode = CMODE_8; diff --git a/drivers/video/fbdev/platinumfb.c b/drivers/video/fbdev/platinumfb.c index bf6b7fb83cf4..3efceaf38f98 100644 --- a/drivers/video/fbdev/platinumfb.c +++ b/drivers/video/fbdev/platinumfb.c @@ -347,7 +347,7 @@ static int platinum_init_fb(struct fb_info *info) printk(KERN_INFO "platinumfb: Monitor sense value = 0x%x, ", sense); if (default_vmode == VMODE_NVRAM) { #ifdef CONFIG_NVRAM
default_vmode = nvram_read_byte(NV_VMODE);
default_vmode = arch_nvram_ops.read_byte(NV_VMODE);#endif @@ -360,7 +360,7 @@ static int platinum_init_fb(struct fb_info *info) default_vmode = VMODE_640_480_60; #ifdef CONFIG_NVRAM if (default_cmode == CMODE_NVRAM)
default_cmode = nvram_read_byte(NV_CMODE);
default_cmode = arch_nvram_ops.read_byte(NV_CMODE);#endif if (default_cmode < CMODE_8 || default_cmode > CMODE_32) default_cmode = CMODE_8; diff --git a/drivers/video/fbdev/valkyriefb.c b/drivers/video/fbdev/valkyriefb.c index 8022316ee9c9..81f66d69d9dd 100644 --- a/drivers/video/fbdev/valkyriefb.c +++ b/drivers/video/fbdev/valkyriefb.c @@ -283,7 +283,7 @@ static void __init valkyrie_choose_mode(struct fb_info_valkyrie *p) /* Try to pick a video mode out of NVRAM if we have one. */ #if defined(CONFIG_PPC_PMAC) && defined(CONFIG_NVRAM) if (default_vmode == VMODE_NVRAM) {
default_vmode = nvram_read_byte(NV_VMODE);
default_vmode = arch_nvram_ops.read_byte(NV_VMODE);@@ -296,7 +296,7 @@ static void __init valkyrie_choose_mode(struct fb_info_valkyrie *p) default_vmode = VMODE_640_480_67; #if defined(CONFIG_PPC_PMAC) && defined(CONFIG_NVRAM) if (default_cmode == CMODE_NVRAM)
default_cmode = nvram_read_byte(NV_CMODE);
default_cmode = arch_nvram_ops.read_byte(NV_CMODE);#endif
/*
2.19.2
On Sat, 29 Dec 2018, LEROY Christophe wrote:
Finn Thain fthain@telegraphics.com.au a ?crit?:
Make use of arch_nvram_ops in device drivers so that the nvram_* function exports can be removed.
Since they are no longer global symbols, rename the PPC32 nvram_* functions appropriately.
Signed-off-by: Finn Thain fthain@telegraphics.com.au
arch/powerpc/kernel/setup_32.c | 8 ++++---- drivers/char/generic_nvram.c | 4 ++-- drivers/video/fbdev/controlfb.c | 4 ++-- drivers/video/fbdev/imsttfb.c | 4 ++-- drivers/video/fbdev/matrox/matroxfb_base.c | 2 +- drivers/video/fbdev/platinumfb.c | 4 ++-- drivers/video/fbdev/valkyriefb.c | 4 ++-- 7 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c index e0d045677472..bdbe6acbef11 100644 --- a/arch/powerpc/kernel/setup_32.c +++ b/arch/powerpc/kernel/setup_32.c @@ -152,20 +152,18 @@ __setup("l3cr=", ppc_setup_l3cr);
#ifdef CONFIG_GENERIC_NVRAM
-unsigned char nvram_read_byte(int addr) +static unsigned char ppc_nvram_read_byte(int addr) { if (ppc_md.nvram_read_val) return ppc_md.nvram_read_val(addr); return 0xff; } -EXPORT_SYMBOL(nvram_read_byte);
-void nvram_write_byte(unsigned char val, int addr) +static void ppc_nvram_write_byte(unsigned char val, int addr) { if (ppc_md.nvram_write_val) ppc_md.nvram_write_val(addr, val); } -EXPORT_SYMBOL(nvram_write_byte);
static ssize_t ppc_nvram_get_size(void) { @@ -182,6 +180,8 @@ static long ppc_nvram_sync(void) }
const struct nvram_ops arch_nvram_ops = {
- .read_byte = ppc_nvram_read_byte,
- .write_byte = ppc_nvram_write_byte, .get_size = ppc_nvram_get_size, .sync = ppc_nvram_sync,
}; diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c index f32d5663de95..41b76bf9614e 100644 --- a/drivers/char/generic_nvram.c +++ b/drivers/char/generic_nvram.c @@ -48,7 +48,7 @@ static ssize_t read_nvram(struct file *file, char __user *buf, if (*ppos >= nvram_len) return 0; for (i = *ppos; count > 0 && i < nvram_len; ++i, ++p, --count)
if (__put_user(nvram_read_byte(i), p))
if (__put_user(arch_nvram_ops.read_byte(i), p))Instead of modifying all drivers (in this patch and previous ones related to other arches), wouldn't it be better to add helpers like the following in nvram.h:
Static inline unsigned char nvram_read_byte(int addr) { return arch_nvram_ops.read_byte(addr); }
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.
On Sun, Dec 30, 2018 at 12:43 AM Finn Thain fthain@telegraphics.com.au 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
On Mon, 31 Dec 2018, Arnd Bergmann wrote:
On Sun, Dec 30, 2018 at 12:43 AM Finn Thain fthain@telegraphics.com.au 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.
--
On Sun, 30 Dec 2018, I wrote:
On Sat, 29 Dec 2018, LEROY Christophe wrote:
Finn Thain fthain@telegraphics.com.au a ?crit?:
Make use of arch_nvram_ops in device drivers so that the nvram_* function exports can be removed.
Since they are no longer global symbols, rename the PPC32 nvram_* functions appropriately.
Signed-off-by: Finn Thain fthain@telegraphics.com.au
arch/powerpc/kernel/setup_32.c | 8 ++++---- drivers/char/generic_nvram.c | 4 ++-- drivers/video/fbdev/controlfb.c | 4 ++-- drivers/video/fbdev/imsttfb.c | 4 ++-- drivers/video/fbdev/matrox/matroxfb_base.c | 2 +- drivers/video/fbdev/platinumfb.c | 4 ++-- drivers/video/fbdev/valkyriefb.c | 4 ++-- 7 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c index e0d045677472..bdbe6acbef11 100644 --- a/arch/powerpc/kernel/setup_32.c +++ b/arch/powerpc/kernel/setup_32.c @@ -152,20 +152,18 @@ __setup("l3cr=", ppc_setup_l3cr);
#ifdef CONFIG_GENERIC_NVRAM
-unsigned char nvram_read_byte(int addr) +static unsigned char ppc_nvram_read_byte(int addr) { if (ppc_md.nvram_read_val) return ppc_md.nvram_read_val(addr); return 0xff; } -EXPORT_SYMBOL(nvram_read_byte);
-void nvram_write_byte(unsigned char val, int addr) +static void ppc_nvram_write_byte(unsigned char val, int addr) { if (ppc_md.nvram_write_val) ppc_md.nvram_write_val(addr, val); } -EXPORT_SYMBOL(nvram_write_byte);
static ssize_t ppc_nvram_get_size(void) { @@ -182,6 +180,8 @@ static long ppc_nvram_sync(void) }
const struct nvram_ops arch_nvram_ops = {
- .read_byte = ppc_nvram_read_byte,
- .write_byte = ppc_nvram_write_byte, .get_size = ppc_nvram_get_size, .sync = ppc_nvram_sync,
}; diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c index f32d5663de95..41b76bf9614e 100644 --- a/drivers/char/generic_nvram.c +++ b/drivers/char/generic_nvram.c @@ -48,7 +48,7 @@ static ssize_t read_nvram(struct file *file, char __user *buf, if (*ppos >= nvram_len) return 0; for (i = *ppos; count > 0 && i < nvram_len; ++i, ++p, --count)
if (__put_user(nvram_read_byte(i), p))
if (__put_user(arch_nvram_ops.read_byte(i), p))Instead of modifying all drivers (in this patch and previous ones related to other arches), wouldn't it be better to add helpers like the following in nvram.h:
Static inline unsigned char nvram_read_byte(int addr) { return arch_nvram_ops.read_byte(addr); }
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.
Contrary to what I said above, this kind of double indirection could be useful if it allows us to avoid the kind of double indirection which Arnd objected to (which arises when an arch_nvram_ops method invokes a ppc_md method). For example,
static inline unsigned char nvram_read_byte(int addr) { #ifdef CONFIG_PPC return ppc_md.nvram_read_byte(addr); #else return arch_nvram_ops.read_byte(addr); #endif }
I'll try this approach for v9 if there are no objections. It may be the least invasive approach. Also, arch_nvram_ops can remain const.
--
dri-devel@lists.freedesktop.org
-
Arnd Bergmann -
Finn Thain -
LEROY Christophe