[PATCH v8 15/25] powerpc: Clean up nvram includes

[PATCH v8 15/25] powerpc: Clean up nvram includes

[Finn Thain]

The nvram_read_byte() and nvram_write_byte() definitions in asm/nvram.h
duplicate those in linux/nvram.h. Get rid of the former to prepare for
adoption of struct arch_nvram_ops (which is defined in linux/nvram.h for
general use).

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Stan Johnson <userm57@yahoo.com>
---
 arch/powerpc/include/asm/nvram.h           | 3 ---
 arch/powerpc/kernel/setup_32.c             | 1 +
 drivers/char/generic_nvram.c               | 1 +
 drivers/video/fbdev/matrox/matroxfb_base.c | 2 +-
 4 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/nvram.h b/arch/powerpc/include/asm/nvram.h
index 09a518bb7c03..1443b40f44b9 100644
--- a/arch/powerpc/include/asm/nvram.h
+++ b/arch/powerpc/include/asm/nvram.h
@@ -101,7 +101,4 @@ extern int nvram_write_os_partition(struct nvram_os_partition *part,
 /* Determine NVRAM size */
 extern ssize_t nvram_get_size(void);
 
-/* Normal access to NVRAM */
-extern unsigned char nvram_read_byte(int i);
-extern void nvram_write_byte(unsigned char c, int i);
 #endif /* _ASM_POWERPC_NVRAM_H */
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
index 81909600013a..04c0315cfe46 100644
--- a/arch/powerpc/kernel/setup_32.c
+++ b/arch/powerpc/kernel/setup_32.c
@@ -17,6 +17,7 @@
 #include <linux/console.h>
 #include <linux/memblock.h>
 #include <linux/export.h>
+#include <linux/nvram.h>
 
 #include <asm/io.h>
 #include <asm/prom.h>
diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c
index 14e728fbb8a0..0381af638fe3 100644
--- a/drivers/char/generic_nvram.c
+++ b/drivers/char/generic_nvram.c
@@ -20,6 +20,7 @@
 #include <linux/fcntl.h>
 #include <linux/init.h>
 #include <linux/mutex.h>
+#include <linux/nvram.h>
 #include <linux/pagemap.h>
 #include <linux/uaccess.h>
 #include <asm/nvram.h>
diff --git a/drivers/video/fbdev/matrox/matroxfb_base.c b/drivers/video/fbdev/matrox/matroxfb_base.c
index 838869c6490c..0a4e5bad33f4 100644
--- a/drivers/video/fbdev/matrox/matroxfb_base.c
+++ b/drivers/video/fbdev/matrox/matroxfb_base.c
@@ -111,12 +111,12 @@
 #include "matroxfb_g450.h"
 #include <linux/matroxfb.h>
 #include <linux/interrupt.h>
+#include <linux/nvram.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 
 #ifdef CONFIG_PPC_PMAC
 #include <asm/machdep.h>
-unsigned char nvram_read_byte(int);
 static int default_vmode = VMODE_NVRAM;
 static int default_cmode = CMODE_NVRAM;
 #endif
-- 
2.19.2

[PATCH v8 19/25] powerpc, fbdev: Use NV_CMODE and NV_VMODE only when CONFIG_PPC32 && CONFIG_PPC_PMAC

[Finn Thain]

This patch addresses inconsistencies in Mac framebuffer drivers and their
use of Kconfig symbols relating to NVRAM, so PPC64 can use CONFIG_NVRAM.

Macintosh framebuffer drivers use default settings for color mode and
video mode that are found in NVRAM. On PCI Macs, MacOS stores display
settings in the Name Registry (NR) partition in NVRAM*. On NuBus Macs,
there is no NR partition and MacOS stores display mode settings in PRAM**.

Early-model Macs are the ones most likely to benefit from these settings,
since they are more likely to have a fixed-frequency monitor connected to
the built-in framebuffer device. Moreover, a single NV_CMODE value and
a single NV_VMODE value provide for only one display.

The NV_CMODE and NV_VMODE constants are apparently offsets into the NR
partition for Old World machines. This also suggests that these defaults
are not useful on later models. The NR partition seems to be optional on
New World machines. CONFIG_NVRAM cannot be enabled on PPC64 at present.

It is safe to say that NVRAM support in PowerMac fbdev drivers is only
applicable to CONFIG_PPC32 so make this condition explicit. This means
matroxfb driver won't crash on PPC64 when CONFIG_NVRAM becomes available
there.

For imsttfb, add the missing CONFIG_NVRAM test to prevent a build failure,
since PPC64 does not implement nvram_read_byte(). Also add a missing
machine_is(powermac) check. Change the inconsistent dependency on
CONFIG_PPC and the matching #ifdef tests to CONFIG_PPC_PMAC.

For valkyriefb, to improve clarity and consistency with the other PowerMac
fbdev drivers, test for CONFIG_PPC_PMAC instead of !CONFIG_MAC. Remove a
bogus comment regarding PRAM.

* See GetPreferredConfiguration and SavePreferredConfiguration in
"Designing PCI Cards and Drivers for Power Macintosh Computers".

** See SetDefaultMode and GetDefaultMode in "Designing Cards and Drivers
for the Macintosh Family".

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/video/fbdev/Kconfig                |  2 +-
 drivers/video/fbdev/imsttfb.c              | 12 +++++-------
 drivers/video/fbdev/matrox/matroxfb_base.c |  2 +-
 drivers/video/fbdev/valkyriefb.c           | 14 ++++++--------
 4 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index e413f54208f4..52453ce3040a 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -536,7 +536,7 @@ config FB_IMSTT
 	bool "IMS Twin Turbo display support"
 	depends on (FB = y) && PCI
 	select FB_CFB_IMAGEBLIT
-	select FB_MACMODES if PPC
+	select FB_MACMODES if PPC_PMAC
 	help
 	  The IMS Twin Turbo is a PCI-based frame buffer card bundled with
 	  many Macintosh and compatible computers.
diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
index 901ca4ed10e9..8d231591ff0e 100644
--- a/drivers/video/fbdev/imsttfb.c
+++ b/drivers/video/fbdev/imsttfb.c
@@ -30,9 +30,8 @@
 #include <asm/io.h>
 #include <linux/uaccess.h>
 
-#if defined(CONFIG_PPC)
+#if defined(CONFIG_PPC_PMAC)
 #include <linux/nvram.h>
-#include <asm/prom.h>
 #include "macmodes.h"
 #endif
 
@@ -327,14 +326,13 @@ enum {
 	TVP = 1
 };
 
-#define USE_NV_MODES		1
 #define INIT_BPP		8
 #define INIT_XRES		640
 #define INIT_YRES		480
 
 static int inverse = 0;
 static char fontname[40] __initdata = { 0 };
-#if defined(CONFIG_PPC)
+#if defined(CONFIG_PPC_PMAC)
 static signed char init_vmode = -1, init_cmode = -1;
 #endif
 
@@ -1390,8 +1388,8 @@ static void init_imstt(struct fb_info *info)
 		}
 	}
 
-#if USE_NV_MODES && defined(CONFIG_PPC32)
-	{
+#if defined(CONFIG_PPC_PMAC) && defined(CONFIG_PPC32) && defined(CONFIG_NVRAM)
+	if (machine_is(powermac)) {
 		int vmode = init_vmode, cmode = init_cmode;
 
 		if (vmode = -1) {
@@ -1565,7 +1563,7 @@ imsttfb_setup(char *options)
 			inverse = 1;
 			fb_invert_cmaps();
 		}
-#if defined(CONFIG_PPC)
+#if defined(CONFIG_PPC_PMAC)
 		else if (!strncmp(this_opt, "vmode:", 6)) {
 			int vmode = simple_strtoul(this_opt+6, NULL, 0);
 			if (vmode > 0 && vmode <= VMODE_MAX)
diff --git a/drivers/video/fbdev/matrox/matroxfb_base.c b/drivers/video/fbdev/matrox/matroxfb_base.c
index 0a4e5bad33f4..cac5865d461c 100644
--- a/drivers/video/fbdev/matrox/matroxfb_base.c
+++ b/drivers/video/fbdev/matrox/matroxfb_base.c
@@ -1874,7 +1874,7 @@ static int initMatrox2(struct matrox_fb_info *minfo, struct board *b)
 		struct fb_var_screeninfo var;
 		if (default_vmode <= 0 || default_vmode > VMODE_MAX)
 			default_vmode = VMODE_640_480_60;
-#ifdef CONFIG_NVRAM
+#if defined(CONFIG_PPC32) && defined(CONFIG_NVRAM)
 		if (default_cmode = CMODE_NVRAM)
 			default_cmode = nvram_read_byte(NV_CMODE);
 #endif
diff --git a/drivers/video/fbdev/valkyriefb.c b/drivers/video/fbdev/valkyriefb.c
index d51c3a8009cb..8022316ee9c9 100644
--- a/drivers/video/fbdev/valkyriefb.c
+++ b/drivers/video/fbdev/valkyriefb.c
@@ -63,14 +63,12 @@
 #include "macmodes.h"
 #include "valkyriefb.h"
 
-#ifdef CONFIG_MAC
-/* We don't yet have functions to read the PRAM... perhaps we can
-   adapt them from the PPC code? */
-static int default_vmode = VMODE_CHOOSE;
-static int default_cmode = CMODE_8;
-#else
+#ifdef CONFIG_PPC_PMAC
 static int default_vmode = VMODE_NVRAM;
 static int default_cmode = CMODE_NVRAM;
+#else
+static int default_vmode = VMODE_CHOOSE;
+static int default_cmode = CMODE_8;
 #endif
 
 struct fb_par_valkyrie {
@@ -283,7 +281,7 @@ static void __init valkyrie_choose_mode(struct fb_info_valkyrie *p)
 	printk(KERN_INFO "Monitor sense value = 0x%x\n", p->sense);
 
 	/* Try to pick a video mode out of NVRAM if we have one. */
-#if !defined(CONFIG_MAC) && defined(CONFIG_NVRAM)
+#if defined(CONFIG_PPC_PMAC) && defined(CONFIG_NVRAM)
 	if (default_vmode = VMODE_NVRAM) {
 		default_vmode = nvram_read_byte(NV_VMODE);
 		if (default_vmode <= 0
@@ -296,7 +294,7 @@ static void __init valkyrie_choose_mode(struct fb_info_valkyrie *p)
 		default_vmode = mac_map_monitor_sense(p->sense);
 	if (!valkyrie_reg_init[default_vmode - 1])
 		default_vmode = VMODE_640_480_67;
-#if !defined(CONFIG_MAC) && defined(CONFIG_NVRAM)
+#if defined(CONFIG_PPC_PMAC) && defined(CONFIG_NVRAM)
 	if (default_cmode = CMODE_NVRAM)
 		default_cmode = nvram_read_byte(NV_CMODE);
 #endif
-- 
2.19.2

[PATCH v8 20/25] powerpc, fbdev: Use arch_nvram_ops methods instead of nvram_read_byte() and nvram_w

[Finn Thain]

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Þfault_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
 
 	/*
-- 
2.19.2

Re: [PATCH v8 20/25] powerpc, fbdev: Use arch_nvram_ops methods instead of nvram_read_byte() and nvr

[LEROY Christophe]

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);
>  	}
>  	*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Þfault_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
>
>  	/*
> --
> 2.19.2

Re: [PATCH v8 20/25] powerpc, fbdev: Use arch_nvram_ops methods instead of nvram_read_byte() and nvr

[Finn Thain]

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.

-- 

> Christophe
> 

Re: [PATCH v8 20/25] powerpc, fbdev: Use arch_nvram_ops methods instead of nvram_read_byte() and nvr

[Arnd Bergmann]

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

Re: [PATCH v8 20/25] powerpc, fbdev: Use arch_nvram_ops methods instead of nvram_read_byte() and nvr

[Finn Thain]

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.

-- 

Re: [PATCH v8 20/25] powerpc, fbdev: Use arch_nvram_ops methods instead of nvram_read_byte() and nvr

[Finn Thain]

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.

--