Re: [PATCH v6 05/15] mfd: bd71828: Support ROHM BD71828 PMIC - core

["gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>]

On Tue, Dec 17, 2019 at 01:54:30PM +0000, Lee Jones wrote:
> On Tue, 17 Dec 2019, Vaittinen, Matti wrote:
> 
> > Hello Lee,
> > 
> > On Mon, 2019-12-16 at 16:46 +0000, Lee Jones wrote:
> > > On Wed, 11 Dec 2019, Matti Vaittinen wrote:
> > > 
> > > > BD71828GW is a single-chip power management IC for battery-powered
> > > > portable
> > > > devices. The IC integrates 7 buck converters, 7 LDOs, and a 1500 mA
> > > > single-cell linear charger. Also included is a Coulomb counter, a
> > > > real-time
> > > > clock (RTC), 3 GPO/regulator control pins, HALL input and a 32.768
> > > > kHz
> > > > clock gate.
> > > > 
> > > > Add MFD core driver providing interrupt controller facilities and
> > > > i2c
> > > > access to sub device drivers.
> > > > 
> > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > > ---
> > > > 
> > > > Changes since v5:
> > > > - No changes
> > > > 
> > > >  drivers/mfd/Kconfig              |  15 ++
> > > >  drivers/mfd/Makefile             |   2 +-
> > > >  drivers/mfd/rohm-bd71828.c       | 319 +++++++++++++++++++++++
> > > >  include/linux/mfd/rohm-bd71828.h | 425
> > > > +++++++++++++++++++++++++++++++
> > > >  include/linux/mfd/rohm-generic.h |   1 +
> > > >  5 files changed, 761 insertions(+), 1 deletion(-)
> > > >  create mode 100644 drivers/mfd/rohm-bd71828.c
> > > >  create mode 100644 include/linux/mfd/rohm-bd71828.h
> > > 
> > > Couple of small nits.  Once fixed, please apply my:
> > > 
> > > For my own reference:
> > >   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> > > 
> > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > index 420900852166..c3c9432ef51c 100644
> > > > --- a/drivers/mfd/Kconfig
> > > > +++ b/drivers/mfd/Kconfig
> > > > @@ -1906,6 +1906,21 @@ config MFD_ROHM_BD70528
> > > >  	  10 bits SAR ADC for battery temperature monitor and 1S
> > > > battery
> > > >  	  charger.
> > > >  
> > > > +config MFD_ROHM_BD71828
> > > > +	tristate "ROHM BD71828 Power Management IC"
> > > > +	depends on I2C=y
> > > > +	depends on OF
> > > > +	select REGMAP_I2C
> > > > +	select REGMAP_IRQ
> > > > +	select MFD_CORE
> > > > +	help
> > > > +	  Select this option to get support for the ROHM BD71828 Power
> > > > +	  Management IC. BD71828GW is a single-chip power management IC
> > > > for
> > > > +	  battery-powered portable devices. The IC integrates 7 buck
> > > > +	  converters, 7 LDOs, and a 1500 mA single-cell linear charger.
> > > > +	  Also included is a Coulomb counter, a real-time clock (RTC),
> > > > and
> > > > +	  a 32.768 kHz clock gate.
> > > > +
> > > >  config MFD_STM32_LPTIMER
> > > >  	tristate "Support for STM32 Low-Power Timer"
> > > >  	depends on (ARCH_STM32 && OF) || COMPILE_TEST
> > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > > index aed99f08739f..ca2d55c679c5 100644
> > > > --- a/drivers/mfd/Makefile
> > > > +++ b/drivers/mfd/Makefile
> > > > @@ -252,6 +252,6 @@ obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
> > > >  obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
> > > >  obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
> > > >  obj-$(CONFIG_MFD_ROHM_BD70528)	+= rohm-bd70528.o
> > > > +obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
> > > >  obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
> > > >  obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
> > > > -
> > > 
> > > Nit: This is an unrelated change and should not really be in this
> > > patch.
> > 
> > Ok. Will get rid of it.
> > 
> > > 
> > > > diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-
> > > > bd71828.c
> > > > new file mode 100644
> > > > index 000000000000..7f445d699fd9
> > > > --- /dev/null
> > > > +++ b/drivers/mfd/rohm-bd71828.c
> > > > @@ -0,0 +1,319 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +//
> > > > +// Copyright (C) 2019 ROHM Semiconductors
> > > > +//
> > > > +// ROHM BD71828 PMIC driver
> > > > +
> > 
> > //snip
> > 
> > > > +
> > > > +static struct i2c_driver bd71828_drv = {
> > > > +	.driver = {
> > > > +		.name = "rohm-bd71828",
> > > > +		.of_match_table = bd71828_of_match,
> > > > +	},
> > > > +	.probe_new = &bd71828_i2c_probe,
> > > > +};
> > > > +
> > > 
> > > Nit: You can remove this line.
> > 
> > Will do.
> > 
> > > 
> > > > +module_i2c_driver(bd71828_drv);
> > > > +
> > > > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > > ");
> > > > +MODULE_DESCRIPTION("ROHM BD71828 Power Management IC driver");
> > > > +MODULE_LICENSE("GPL");
> > > 
> > > This does not match the header.
> > 
> > How is that? This is what is stated in module.h for the 
> > MODULE_LICENSE:
> > 
> > /*
> >  * The following license idents are currently accepted as indicating
> > free
> >  * software modules
> >  *
> >  *	"GPL"				[GNU Public License v2]
> >  *	"GPL v2"			[GNU Public License v2]
> >  *	"GPL and additional rights"	[GNU Public License v2 rights
> > and more]
> >  *	"Dual BSD/GPL"			[GNU Public License v2
> >  *					 or BSD license choice]
> >  *	"Dual MIT/GPL"			[GNU Public License v2
> >  *					 or MIT license choice]
> >  *	"Dual MPL/GPL"			[GNU Public License v2
> >  *					 or Mozilla license choice]
> >  *
> >  * The following other idents are available
> >  *
> >  *	"Proprietary"			[Non free products]
> >  *
> >  * Both "GPL v2" and "GPL" (the latter also in dual licensed strings)
> > are
> >  * merely stating that the module is licensed under the GPL v2, but are
> > not
> >  * telling whether "GPL v2 only" or "GPL v2 or later". The reason why
> > there
> >  * are two variants is a historic and failed attempt to convey more
> >  * information in the MODULE_LICENSE string. For module loading the
> >  * "only/or later" distinction is completely irrelevant and does
> > neither
> >  * replace the proper license identifiers in the corresponding source
> > file
> >  * nor amends them in any way. The sole purpose is to make the
> >  * 'Proprietary' flagging work and to refuse to bind symbols which are
> >  * exported with EXPORT_SYMBOL_GPL when a non free module is loaded.
> >  *
> >  * In the same way "BSD" is not a clear license information. It merely
> >  * states, that the module is licensed under one of the compatible BSD
> >  * license variants. The detailed and correct license information is
> > again
> >  * to be found in the corresponding source files.
> >  *
> >  * There are dual licensed components, but when running with Linux it
> > is the
> >  * GPL that is relevant so this is a non issue. Similarly LGPL linked
> > with GPL
> >  * is a GPL combined work.
> >  *
> >  * This exists for several reasons
> >  * 1.	So modinfo can show license info for users wanting to vet their
> > setup
> >  *	is free
> >  * 2.	So the community can ignore bug reports including proprietary
> > modules
> >  * 3.	So vendors can do likewise based on their own policies
> >  */
> > #define MODULE_LICENSE(_license) MODULE_INFO(license, _license)
> > 
> > I have no objections on changing the license if needed but can you
> > please tell me what is Ok combos then - I am having hard time when
> > trying to select licenses which are acceptable for all.
> 
> If you have this in your header:
> 
>   GPL-2.0-only
> 
> Your MODULE tags should read:
> 
> MODULE_LICENSE("GPL v2");

Nope, as per module.h, which is quoted here, either:
	MODULE_LICENSE("GPL");
or:
	MODULE_LICENSE("GPL v2");
mean the exact same thing.

thanks,

greg k-h