Re: [PATCH] MIPS: BCM47XX: Move NVRAM driver to the drivers/misc/

[Hauke Mehrtens <hauke@hauke-m.de>]

On 11/23/2014 10:45 AM, Rafał Miłecki wrote:
> On 12 November 2014 at 22:16, Hauke Mehrtens <hauke@hauke-m.de> wrote:
>> On 11/11/2014 08:45 PM, Rafał Miłecki wrote:
>>> After Broadcom switched from MIPS to ARM for their home routers we need
>>> to have NVRAM driver in some common place (not arch/mips/).
>>> We were thinking about putting it in bus directory, however there are
>>> two possible buses for MIPS: drivers/ssb/ and drivers/bcma/. So this
>>> won't fit there neither.
>>> This is why I would like to move this driver to the drivers/misc/
>>
>> I will do a more detailed review when you send a patch with -M
>>
>>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>>> ---
>>>  arch/mips/Kconfig                                  |   1 +
>>>  arch/mips/bcm47xx/Makefile                         |   2 +-
>>>  arch/mips/bcm47xx/board.c                          |   2 +-
>>>  arch/mips/bcm47xx/nvram.c                          | 228 --------------------
>>>  arch/mips/bcm47xx/setup.c                          |   1 -
>>>  arch/mips/bcm47xx/sprom.c                          |   1 -
>>>  arch/mips/bcm47xx/time.c                           |   1 -
>>>  arch/mips/include/asm/mach-bcm47xx/bcm47xx.h       |   1 +
>>>  arch/mips/include/asm/mach-bcm47xx/bcm47xx_nvram.h |  21 --
>>>  drivers/bcma/driver_mips.c                         |   2 +-
>>>  drivers/misc/Kconfig                               |   9 +
>>>  drivers/misc/Makefile                              |   1 +
>>>  drivers/misc/bcm47xx_nvram.c                       | 230 +++++++++++++++++++++
>>>  drivers/net/ethernet/broadcom/b44.c                |   2 +-
>>>  drivers/net/ethernet/broadcom/bgmac.c              |   2 +-
>>>  drivers/ssb/driver_chipcommon_pmu.c                |   2 +-
>>>  drivers/ssb/driver_mipscore.c                      |   2 +-
>>>  include/linux/bcm47xx_nvram.h                      |  18 ++
>>>  18 files changed, 267 insertions(+), 259 deletions(-)
>>>  delete mode 100644 arch/mips/bcm47xx/nvram.c
>>>  delete mode 100644 arch/mips/include/asm/mach-bcm47xx/bcm47xx_nvram.h
>>>  create mode 100644 drivers/misc/bcm47xx_nvram.c
>>>  create mode 100644 include/linux/bcm47xx_nvram.h
>>
>> ....
>>
>>> diff --git a/include/linux/bcm47xx_nvram.h b/include/linux/bcm47xx_nvram.h
>>> new file mode 100644
>>> index 0000000..5ed6917
>>> --- /dev/null
>>> +++ b/include/linux/bcm47xx_nvram.h
>>> @@ -0,0 +1,18 @@
>>> +/*
>>> + *  This program is free software; you can redistribute  it and/or modify it
>>> + *  under  the terms of  the GNU General  Public License as published by the
>>> + *  Free Software Foundation;  either version 2 of the  License, or (at your
>>> + *  option) any later version.
>>> + */
>>> +
>>> +#ifndef __BCM47XX_NVRAM_H
>>> +#define __BCM47XX_NVRAM_H
>>> +
>>> +#include <linux/types.h>
>>> +#include <linux/kernel.h>
>>> +
>>> +int bcm47xx_nvram_init_from_mem(u32 base, u32 lim);
>>> +int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len);
>>> +int bcm47xx_nvram_gpio_pin(const char *name);
>>
>> Could you change this to something like this:
>>
>> #ifdef CONFIG_BCM47XX_NVRAM
>> int bcm47xx_nvram_init_from_mem(u32 base, u32 lim);
>> int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len);
>> int bcm47xx_nvram_gpio_pin(const char *name);
>> #else
>> static inline int bcm47xx_nvram_init_from_mem(u32 base, u32 lim) {return
>> -1;};
>> static inline int bcm47xx_nvram_getenv(const char *name, char *val,
>> size_t val_len) {return -1;};
>> static inline int bcm47xx_nvram_gpio_pin(const char *name) {return -1;};
>> #endif
>>
>> and use something better than -1.
>>
>> This way we can get rid of these  all other the code.
>> #ifdef CONFIG_BCM47XX
>> ..
>> #endif
> 
> How many drivers using
> #ifdef CONFIG_BCM47XX
> bcm47xx_nvram_foo(...)
> #endif
> do we have?
> 
> I think right now it is done in
> drivers/bcma/driver_mips.c
> but should be dropped anyway. We should make BCMA_DRIVER_MIPS depend on BCM47XX.
> 
> And second usage is in:
> drivers/net/ethernet/broadcom/b44.c
> 
> Do you think it's worth doing just for b44.c?
> 

There is also a call in
drivers/ssb/driver_chipcommon_pmu.c

In addition
drivers/net/ethernet/broadcom/bgmac.c:
calls bcm47xx_nvram_foo(...) without any ifdef and depends on this.

I think it would be best to separate these two things and and try to
remove the ifdefs in a separate patch.

Hauke