On Tue, Dec 17, 2019 at 01:54:30PM +0000, Lee Jones wrote:

On Tue, 17 Dec 2019, Vaittinen, Matti wrote:

quoted

Hello Lee,

On Mon, 2019-12-16 at 16:46 +0000, Lee Jones wrote:

quoted

On Wed, 11 Dec 2019, Matti Vaittinen wrote:

quoted

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

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 [off-list ref]

quoted

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.

quoted

quoted

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

quoted

quoted

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

quoted

quoted

+module_i2c_driver(bd71828_drv);
+
+MODULE_AUTHOR("Matti Vaittinen [off-list ref]
");
+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