lkml.org 
[lkml]   [2019]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v8 20/25] powerpc, fbdev: Use arch_nvram_ops methods instead of nvram_read_byte() and nvram_write_byte()
    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.

    --

    \
     
     \ /
      Last update: 2019-01-06 00:06    [W:4.208 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site