update at v3
1. Removed independant bugfixes from the patch set.
2. Power Supply API supported.
3. Removed regulators that became useless by introducing Power Supply API.

update at v2
1. Seperated features
2. Added style fixes

v1: "[PATCH] MFD MAX8998/LP3974: Add Features: hibernation, charger,
and other misc."

MyungJoo Ham (4):
  MFD MAX8998/LP3974: Support Hibernation
  MFD MAX8998/LP3974: Support LP3974 RTC
  regulator MAX8998/LP3974: Support DVS-GPIO.
  MFD MAX8998/LP3974: Support Charger

 drivers/mfd/max8998-irq.c           |    7 +
 drivers/mfd/max8998.c               |  155 ++++++++++++++++++++-
 drivers/power/Kconfig               |    7 +
 drivers/power/Makefile              |    1 +
 drivers/power/max8998.c             |  265 +++++++++++++++++++++++++++++++++++
 drivers/regulator/max8998.c         |  223 ++++++++++++++++++++++++++---
 drivers/rtc/rtc-max8998.c           |   55 +++++++-
 include/linux/mfd/max8998-private.h |   15 ++-
 include/linux/mfd/max8998.h         |   42 +++++-
 9 files changed, 732 insertions(+), 38 deletions(-)
 create mode 100644 drivers/power/max8998.c

--

[ message continues ]

The first releases of LP3974 have a large delay in RTC registers,
which requires 2 seconds of delay after writing to a rtc register
(recommended by National Semiconductor's engineers)
before reading it. If the device name is "lp3974-regerr", the rtc driver
assumes that such delays are required. If the device name is "lp3974",
the rtc driver does not. Although we have not seen LP3974s without
requiring such delays, we assume that such LP3974s will be released
soon (or they have done so already) and they are supported by "lp3974".

This patch adds delays with msleep when writing values to RTC registers
if the device name is "lp3974-regerr".

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/mfd/max8998.c               |   41 ++++++++++++++++++++++++--
 drivers/regulator/max8998.c         |    7 ++++
 drivers/rtc/rtc-max8998.c           |   55 +++++++++++++++++++++++++++++++---
 include/linux/mfd/max8998-private.h |    1 +
 4 files changed, 96 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/max8998.c b/drivers/mfd/max8998.c
index 5ce00ad..8b9eed1 100644
--- a/drivers/mfd/max8998.c
+++ b/drivers/mfd/max8998.c
@@ -42,6 +42,22 @@ static struct mfd_cell max8998_devs[] = {
 	},
 };
 
+static struct mfd_cell lp3974_devs[] = {
+	{
+		.name = "lp3974-pmic",
+	}, {
+		.name = "lp3974-rtc",
+	},
+};
+
+static struct mfd_cell lp3974_regerr_devs[] = {
+	{
+		.name = "lp3974-pmic",
+	}, {
+		.name = "lp3974-rtc-regerr",
+	},
+};
+
 int max8998_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
 {
 	struct max8998_dev *max8998 = i2c_get_clientdata(i2c);
@@ -146,11 +162,29 @@ static int max8998_i2c_probe(struct i2c_client *i2c,
 
 	max8998_irq_init(max8998);
 
-	ret = mfd_add_devices(max8998->dev, -1,
-			      max8998_devs, ARRAY_SIZE(max8998_devs),
-			      NULL, 0);
 	pm_runtime_set_active(max8998->dev);
 
+	switch (id->driver_data) {
+	case TYPE_LP3974_REGERR:
+		ret = ...
[ message continues ]

Hi MyungJoo,

As Mark pointed out, I'd prefer to add a regerr flag to the driver platform
data and decide which cell to add from there.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--

[ message continues ]

update at v4
1. Removed "MFD MAX8998/LP3974: Support Hibernation" as this patch
is already applied.
2. "MFD MAX8998/LP3974: Support LP3974 RTC": RTC register delay chip bug
information is included at platform data, not at the device id.
Note that the other two patches remain the same and the ACK from the
previous patch release is attached.

update at v3
1. Removed independant bugfixes from the patch set.
2. Power Supply API supported.
3. Removed regulators that became useless by introducing Power Supply API.

update at v2
1. Seperated features
2. Added style fixes

v1: "[PATCH] MFD MAX8998/LP3974: Add Features: hibernation, charger,
and other misc."

MyungJoo Ham (3):
  MFD MAX8998/LP3974: Support LP3974 RTC
  regulator MAX8998/LP3974: Support DVS-GPIO.
  MFD MAX8998/LP3974: Support Charger

 drivers/mfd/max8998.c               |   30 ++++-
 drivers/power/Kconfig               |    7 +
 drivers/power/Makefile              |    1 +
 drivers/power/max8998.c             |  265 +++++++++++++++++++++++++++++++++++
 drivers/regulator/max8998.c         |  223 ++++++++++++++++++++++++++---
 drivers/rtc/rtc-max8998.c           |   54 +++++++-
 include/linux/mfd/max8998-private.h |   12 ++-
 include/linux/mfd/max8998.h         |   45 +++++-
 8 files changed, 599 insertions(+), 38 deletions(-)
 create mode 100644 drivers/power/max8998.c

--

[ message continues ]

With the new regulator, "CHARGER", users can control charging
current and turn on and off the charger. Note that the charger
specification of LP3974 is different from that of MAX8998.

driver/power/max8998.c supports power supply APIs for

1. "ONLINE" monitors the charger status, which can be
different from the status "CHARGER"; e.g., users allowed the charger
to charge, but the MAX8998 chip decided not to do so.

2. "PRESENT" monitors the battery status (the existence of the
battery).

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/mfd/max8998.c               |    4 +
 drivers/power/Kconfig               |    7 +
 drivers/power/Makefile              |    1 +
 drivers/power/max8998.c             |  265 +++++++++++++++++++++++++++++++++++
 drivers/regulator/max8998.c         |  129 +++++++++++++++++-
 include/linux/mfd/max8998-private.h |   12 ++-
 include/linux/mfd/max8998.h         |   15 ++
 7 files changed, 429 insertions(+), 4 deletions(-)
 create mode 100644 drivers/power/max8998.c

diff --git a/drivers/mfd/max8998.c b/drivers/mfd/max8998.c
index bbfe867..4716003 100644
--- a/drivers/mfd/max8998.c
+++ b/drivers/mfd/max8998.c
@@ -39,6 +39,8 @@ static struct mfd_cell max8998_devs[] = {
 		.name = "max8998-pmic",
 	}, {
 		.name = "max8998-rtc",
+	}, {
+		.name = "max8998-battery",
 	},
 };
 
@@ -47,6 +49,8 @@ static struct mfd_cell lp3974_devs[] = {
 		.name = "lp3974-pmic",
 	}, {
 		.name = "lp3974-rtc",
+	}, {
+		.name = "lp3974-battery",
 	},
 };
 
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 60d83d9..06b720c 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -185,4 +185,11 @@ config CHARGER_TWL4030
 	help
 	  Say Y here to enable support for TWL4030 Battery Charge Interface.
 
+config CHARGERCTRL_MAX8998
+	tristate "Battery charger driver for MAX8998/LP3974 PMIC"
+	depends on MFD_MAX8998 && REGULATOR_MAX8998
+	help
+	  Say Y to ...
[ message continues ]

It's probably better to split the power supply driver into a separate
patch as there should be no build time dependency between the two.  I've
added Anton to the CCs as he is the drivers/power maintainer.

One thing that jumps out here is that there's no regulator API usage at
all in the power driver - the power driver just jumps straight in
and updates registers when it needs to do anything.  Are you sure that
the regulator API driver is needed at all, if it is I'd expect to see


This should be be driver data rather than a global for neatness (though
in reality the chances of more than one of these chargers in a system

Ah, this is exported from the regulator driver...  That's slightly odd.
For voltages we've an enumeration API for the supported settings, we

dev_dbg() or remove it entirely please, otherwise it might get a bit

That's really confusing...  why?
--

[ message continues ]

The previous driver did not support BUCK1-DVS3, BUCK1-DVS4, and
BUCK2-DVS2 modes. This patch adds such modes and an option to block
setting buck1/2 voltages out of the preset values.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/regulator/max8998.c |   87 +++++++++++++++++++++++++++++++++----------
 include/linux/mfd/max8998.h |   26 ++++++++++---
 2 files changed, 87 insertions(+), 26 deletions(-)

diff --git a/drivers/regulator/max8998.c b/drivers/regulator/max8998.c
index d183756..9946488 100644
--- a/drivers/regulator/max8998.c
+++ b/drivers/regulator/max8998.c
@@ -425,6 +425,9 @@ static int max8998_set_voltage_buck(struct regulator_dev *rdev,
 				}
 			}
 
+			if (pdata->buck_voltage_lock)
+				return -EINVAL;
+
 			/* no predefine regulator found */
 			max8998->buck1_idx = (buck1_last_val % 2) + 2;
 			dev_dbg(max8998->dev, "max8998->buck1_idx:%d\n",
@@ -452,18 +455,26 @@ buck1_exit:
 			"BUCK2, i:%d buck2_vol1:%d, buck2_vol2:%d\n"
 			, i, max8998->buck2_vol[0], max8998->buck2_vol[1]);
 		if (gpio_is_valid(pdata->buck2_set3)) {
-			if (max8998->buck2_vol[0] == i) {
-				max8998->buck2_idx = 0;
-				buck2_gpio_set(pdata->buck2_set3, 0);
-			} else {
-				max8998->buck2_idx = 1;
-				ret = max8998_get_voltage_register(rdev, &reg,
-								   &shift,
-								   &mask);
-				ret = max8998_write_reg(i2c, reg, i);
-				max8998->buck2_vol[1] = i;
-				buck2_gpio_set(pdata->buck2_set3, 1);
+
+			/* check if requested voltage */
+			/* value is already defined */
+			for (j = 0; j < ARRAY_SIZE(max8998->buck2_vol); j++) {
+				if (max8998->buck2_vol[j] == i) {
+					max8998->buck2_idx = j;
+					buck2_gpio_set(pdata->buck2_set3, j);
+					goto buck2_exit;
+				}
 			}
+
+			if (pdata->buck_voltage_lock)
+				return -EINVAL;
+
+			max8998_get_voltage_register(rdev,
+					&reg, &shift, &mask);
+			ret = ...
[ message continues ]

On Tue, 04 Jan 2011 14:17:40 +0900
MyungJoo Ham <myungjoo.ham@samsung.com> wrote:

Hi all,

I've posted some comments below:

gpio_get_value(pdata->buck2_set3));


Is it desirable to define all four for BUCK1 and two for BUCK2 DVS
voltages in platform code? 

Why the "general purpose" slots approach for user changeable/definable
voltages (as it was done previously) have been replaced? Is the current
approach faster?


-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center
Platform Group
--

[ message continues ]

In case a system with 4 buck1 preset voltages and 2 buck2 preset

As long as "buck_voltage_lock" is not set, user may use voltages not
defined as a preset (buckx_voltagex).

If buck_voltage_lock is true, any voltage not predefined is
rejected.However, if not, undefined voltages are accepted and from the
point where an undefined voltage is submitted, presets are not

Thanks.

-- 
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
--

[ message continues ]

I suspect Lukasz's question is more why this would be useful; I can
imagine that the main reason would be performance and in any case I
don't see it hurting to have the option.
--

[ message continues ]

The first releases of LP3974 have a large delay in RTC registers,
which requires 2 seconds of delay after writing to a rtc register
(recommended by National Semiconductor's engineers)
before reading it.

If "rtc_delay" field of the platform data is true, the rtc driver
assumes that such delays are required. Although we have not seen
LP3974s without requiring such delays, we assume that such LP3974s
will be released soon (or they have done so already) and they are
supported by "lp3974" without setting "rtc_delay" at the platform
data.

This patch adds delays with msleep when writing values to RTC registers
if the platform data has rtc_delay set.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/mfd/max8998.c       |   26 ++++++++++++++++++--
 drivers/regulator/max8998.c |    7 +++++
 drivers/rtc/rtc-max8998.c   |   54 +++++++++++++++++++++++++++++++++++++++----
 include/linux/mfd/max8998.h |    4 +++
 4 files changed, 83 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/max8998.c b/drivers/mfd/max8998.c
index 5ce00ad..bbfe867 100644
--- a/drivers/mfd/max8998.c
+++ b/drivers/mfd/max8998.c
@@ -42,6 +42,14 @@ static struct mfd_cell max8998_devs[] = {
 	},
 };
 
+static struct mfd_cell lp3974_devs[] = {
+	{
+		.name = "lp3974-pmic",
+	}, {
+		.name = "lp3974-rtc",
+	},
+};
+
 int max8998_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
 {
 	struct max8998_dev *max8998 = i2c_get_clientdata(i2c);
@@ -146,11 +154,23 @@ static int max8998_i2c_probe(struct i2c_client *i2c,
 
 	max8998_irq_init(max8998);
 
-	ret = mfd_add_devices(max8998->dev, -1,
-			      max8998_devs, ARRAY_SIZE(max8998_devs),
-			      NULL, 0);
 	pm_runtime_set_active(max8998->dev);
 
+	switch (id->driver_data) {
+	case TYPE_LP3974:
+		ret = mfd_add_devices(max8998->dev, -1,
+				lp3974_devs, ARRAY_SIZE(lp3974_devs),
+				NULL, 0);
+		break;
+	case TYPE_MAX8998:
+		ret = mfd_add_devices(max8998->dev, ...
[ message continues ]

Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
--

[ message continues ]