On Sun, 30 Dec 2018, I wrote:

On Sat, 29 Dec 2018, LEROY Christophe wrote:

quoted

Finn Thain [off-list ref] a ?crit?:

quoted

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 <redacted>
---
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.

--