[PATCH 0/3] add support for MCP16502 PMIC

[PATCH 0/3] add support for MCP16502 PMIC

[Andrei.Stefanescu]

MCP16502 is a Power Management IC from Microchip.
It has 4 Buck outputs and 2 LDOs. The buck regulators
can be used in two modes: normal(FPWM) and low-power(Auto PFM).

This patch series adds support for the MCP16502 PMIC.

Andrei Stefanescu (3):
  regulator: dt-bindings: add MCP16502 regulator bindings
  MAINTAINERS: add maintainer for MCP16502 PMIC driver
  regulator: mcp16502: add regulator driver for MCP16502

 .../bindings/regulator/mcp16502-regulator.txt      | 107 +++++
 MAINTAINERS                                        |   7 +
 drivers/regulator/Kconfig                          |   9 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/mcp16502.c                       | 524 +++++++++++++++++++++
 5 files changed, 648 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/mcp16502-regulator.txt
 create mode 100644 drivers/regulator/mcp16502.c

-- 
2.7.4

[PATCH 1/3] regulator: dt-bindings: add MCP16502 regulator bindings

[Andrei.Stefanescu]

This patch describes the compatible and the device tree
bindings necessary for the MCP16502 PMIC.

Signed-off-by: Andrei Stefanescu <andrei.stefanescu@microchip.com>
---
 .../bindings/regulator/mcp16502-regulator.txt      | 107 +++++++++++++++++++++
 1 file changed, 107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/mcp16502-regulator.txt

diff --git a/Documentation/devicetree/bindings/regulator/mcp16502-regulator.txt b/Documentation/devicetree/bindings/regulator/mcp16502-regulator.txt
new file mode 100644
index 0000000..bb3d787
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/mcp16502-regulator.txt
@@ -0,0 +1,107 @@
+MCP16502 PMIC
+
+Required properties:
+- compatible: "microchip,mcp16502"
+- reg: I2C slave address
+- lpm-gpio: GPIO for LPM pin. Note that this GPIO *must* remain high during
+	     suspend-to-ram, keeping the PMIC into HIBERNATE mode.
+- regulators: A node that houses a sub-node for each regulator within
+              the device. Each sub-node is identified using the node's
+              name. The content of each sub-node is defined by the
+              standard binding for regulators; see regulator.txt.
+
+Regulator modes:
+2 - FPWM: higher precision, higher consumption
+4 - AutoPFM: lower precision, lower consumption
+
+Each regulator is defined using the standard binding for regulators.
+
+Example:
+
+mcp16502@5b {
+	compatible = "microchip,mcp16502";
+	reg = <0x5b>;
+	status = "okay";
+	lpm-gpios = <&pioBU 7 GPIO_ACTIVE_HIGH>;
+
+	regulators {
+		VDD_IO {
+			regulator-name = "VDD_IO";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <3700000>;
+			regulator-initial-mode = <2>;
+			regulator-allowed-modes = <2>, <4>;
+			regulator-always-on;
+
+			regulator-state-mem {
+				regulator-off-in-suspend;
+				regulator-mode = <4>;
+			};
+		};
+
+		VDD_DDR {
+			regulator-name = "VDD_DDR";
+			regulator-min-microvolt = <600000>;
+			regulator-max-microvolt = <1850000>;
+			regulator-initial-mode = <2>;
+			regulator-allowed-modes = <2>, <4>;
+			regulator-always-on;
+
+			regulator-state-mem {
+				regulator-on-in-suspend;
+				regulator-mode = <4>;
+			};
+		};
+
+		VDD_CORE {
+			regulator-name = "VDD_CORE";
+			regulator-min-microvolt = <600000>;
+			regulator-max-microvolt = <1850000>;
+			regulator-initial-mode = <2>;
+			regulator-allowed-modes = <2>, <4>;
+			regulator-always-on;
+
+			regulator-state-mem {
+				regulator-off-in-suspend;
+				regulator-mode = <4>;
+			};
+		};
+
+		VDD_OTHER {
+			regulator-name = "VDD_OTHER";
+			regulator-min-microvolt = <600000>;
+			regulator-max-microvolt = <1850000>;
+			regulator-initial-mode = <2>;
+			regulator-allowed-modes = <2>, <4>;
+			regulator-always-on;
+
+			regulator-state-mem {
+				regulator-off-in-suspend;
+				regulator-mode = <4>;
+			};
+		};
+
+		LDO1 {
+			regulator-name = "LDO1";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <3700000>;
+			regulator-always-on;
+
+			regulator-state-mem {
+				regulator-off-in-suspend;
+			};
+		};
+
+		LDO2 {
+			regulator-name = "LDO2";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <3700000>;
+			regulator-always-on;
+
+			regulator-state-mem {
+				regulator-off-in-suspend;
+			};
+		};
+
+	};
+};
-- 
2.7.4

[PATCH 2/3] MAINTAINERS: add maintainer for MCP16502 PMIC driver

[Andrei.Stefanescu]

This patch adds a maintainer for the MCP16502 PMIC
driver.

Signed-off-by: Andrei Stefanescu <andrei.stefanescu@microchip.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f485597..edfad02 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9723,6 +9723,13 @@ M:	Ludovic Desroches <ludovic.desroches@microchip.com>
 S:	Maintained
 F:	drivers/mmc/host/atmel-mci.c
 
+MICROCHIP MCP16502 PMIC DRIVER
+M:	Andrei Stefanescu <andrei.stefanescu@microchip.com>
+L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
+S:	Maintained
+F:	Documentation/devicetree/bindings/regulator/mcp16502-regulator.txt
+F:	drivers/regulator/mcp16502.c
+
 MICROCHIP MCP3911 ADC DRIVER
 M:	Marcus Folkesson <marcus.folkesson@gmail.com>
 M:	Kent Gustavsson <kent@minoris.se>
-- 
2.7.4

[PATCH 3/3] regulator: mcp16502: add regulator driver for MCP16502

[Andrei.Stefanescu]

This patch adds a regulator driver for the MCP16502 PMIC.
This drivers supports basic operations through the
regulator interface such as:
- setting/reading voltage
- setting/reading operating mode
- reading current status
- transitioning to/from suspend-to-ram and standby
  power states

Signed-off-by: Andrei Stefanescu <andrei.stefanescu@microchip.com>
---
 drivers/regulator/Kconfig    |   9 +
 drivers/regulator/Makefile   |   1 +
 drivers/regulator/mcp16502.c | 524 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 534 insertions(+)
 create mode 100644 drivers/regulator/mcp16502.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 926cee0..719d9d6 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -567,6 +567,15 @@ config REGULATOR_MC13892
 	  Say y here to support the regulators found on the Freescale MC13892
 	  PMIC.
 
+config REGULATOR_MCP16502
+	tristate "Microchip MCP16502 PMIC"
+	depends on I2C && OF
+	help
+	  Say y here to support the MCP16502 PMIC. This driver supports
+	  basic operations (get/set voltage, get/set operating mode)
+	  through the regulator interface. In addition it enables
+	  suspend-to-ram/standby transition.
+
 config REGULATOR_MT6311
 	tristate "MediaTek MT6311 PMIC"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 72488ef..b12e1c9 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -74,6 +74,7 @@ obj-$(CONFIG_REGULATOR_MAX77802) += max77802-regulator.o
 obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
 obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
 obj-$(CONFIG_REGULATOR_MC13XXX_CORE) +=  mc13xxx-regulator-core.o
+obj-$(CONFIG_REGULATOR_MCP16502) += mcp16502.o
 obj-$(CONFIG_REGULATOR_MT6311) += mt6311-regulator.o
 obj-$(CONFIG_REGULATOR_MT6323)	+= mt6323-regulator.o
 obj-$(CONFIG_REGULATOR_MT6380)	+= mt6380-regulator.o
diff --git a/drivers/regulator/mcp16502.c b/drivers/regulator/mcp16502.c
new file mode 100644
index 0000000..29c72d3
--- /dev/null
+++ b/drivers/regulator/mcp16502.c
@@ -0,0 +1,524 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MCP16502 PMIC driver
+ *
+ * Copyright (C) 2018 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Andrei Stefanescu <andrei.stefanescu@microchip.com>
+ *
+ * Inspired from tps65086-regulator.c
+ */
+
+#include  <linux/gpio.h>
+#include  <linux/i2c.h>
+#include  <linux/init.h>
+#include  <linux/kernel.h>
+#include  <linux/module.h>
+#include  <linux/of.h>
+#include  <linux/regmap.h>
+#include  <linux/regulator/driver.h>
+
+#define VDD_LOW_SEL 0x0D
+#define VDD_HIGH_SEL 0x3F
+
+#define MCP16502_VOL_SEL_MASK 0x3F
+#define MCP16502_EN_MASK BIT(7)
+#define MCP16502_MODE_MASK BIT(6)
+
+/*
+ *
+ * The PMIC has four sets of registers corresponding to four power modes:
+ * Performance, Active, Low-power, Hibernate.
+ *
+ * Registers:
+ * Each regulator has a register for each power mode. To access a register
+ * for a specific regulator and mode BASE_* and OFFSET_* need to be added.
+ *
+ * Operating modes:
+ * In order for the PMIC to transition to operating modes it has to be
+ * controlled via GPIO lines called LPM and HPM.
+ *
+ * The registers are fully configurable such that you can put all regulators in
+ * a low-power state while the PMIC is in Active mode. They are supposed to be
+ * configured at startup and then simply transition to/from a global low-power
+ * state by setting the GPIO lpm pin high/low.
+ *
+ * This driver keeps the PMIC in Active mode, Low-power state is set for the
+ * regulators by enabling/disabling operating mode (FPWM or Auto PFM).
+ *
+ * The PMIC's Low-power and Hibernate modes are used during standby/suspend.
+ * To enter standby/suspend the PMIC will go to Low-power mode. From there, it
+ * will transition to Hibernate when the PWRHLD line is set to low by the MPU.
+ */
+
+/*
+ * This macro is useful for iterating over all regulators and accessing their
+ * registers in a generic way or accessing a regulator device by its id.
+ */
+#define BASE(i) (((i) + 1) << 4)
+
+#define OFFSET_MODE_A 0
+#define OFFSET_MODE_HIB 2
+
+#define MCP16502_MODE_ACTIVE REGULATOR_MODE_NORMAL
+#define MCP16502_MODE_HIB REGULATOR_MODE_STANDBY
+
+#define MCP16502_MODE_AUTO_PFM 0
+#define MCP16502_MODE_FPWM BIT(6)
+
+#define MIN_REG 0x0
+#define MAX_REG 0x65
+
+static unsigned int mcp16502_of_map_mode(unsigned int mode)
+{
+	if (mode == REGULATOR_MODE_NORMAL || mode == REGULATOR_MODE_IDLE)
+		return mode;
+
+	return REGULATOR_MODE_INVALID;
+}
+
+#define MCP16502_REGULATOR(_name, _id, _ranges, _ops)			\
+	[_id] = {							\
+		.name			= _name,			\
+		.regulators_node	= of_match_ptr("regulators"),	\
+		.id			= _id,				\
+		.ops			= &(_ops),			\
+		.type			= REGULATOR_VOLTAGE,		\
+		.owner			= THIS_MODULE,			\
+		.linear_ranges		= _ranges,			\
+		.n_linear_ranges	= ARRAY_SIZE(_ranges),		\
+		.of_match		= of_match_ptr(_name),		\
+		.of_map_mode		= mcp16502_of_map_mode,		\
+	}
+
+enum {
+	BUCK1 = 0,
+	BUCK2,
+	BUCK3,
+	BUCK4,
+	LDO1,
+	LDO2,
+	NUM_REGULATORS
+};
+
+/*
+ * struct mcp16502 - PMIC representation
+ * @rdev: the regulators belonging to this chip
+ * @rmap: regmap to be used for I2C communication
+ * @lpm: LPM GPIO descriptor
+ */
+struct mcp16502 {
+	struct regulator_dev *rdev[NUM_REGULATORS];
+	struct regmap *rmap;
+	struct gpio_desc *lpm;
+};
+
+/*
+ * mcp16502_update_regulator - regmap_update_bits called for registers
+ *			       Active or Low-power depending on hibernate
+ *			       parameter
+ * @hibernate - use the registers for hibernate mode
+ *
+ */
+static int mcp16502_update_regulator(struct regulator_dev *rdev,
+				     bool hibernate,
+				     unsigned int mask, unsigned int val)
+{
+	struct mcp16502 *mcp = rdev_get_drvdata(rdev);
+	unsigned int reg = BASE(rdev_get_id(rdev));
+
+	reg += (hibernate) ? OFFSET_MODE_HIB : OFFSET_MODE_A;
+
+	return regmap_update_bits(mcp->rmap, reg, mask, val);
+}
+
+/*
+ * mcp16502_read_regulator - read from Active or Low-power registers depending
+ *			     on the hibernate parameter
+ *
+ * @hibernate - use the registers for hibernate mode
+ *
+ */
+static int mcp16502_read(struct regulator_dev *rdev, bool hibernate,
+			 unsigned int mask)
+{
+	struct mcp16502 *mcp = rdev_get_drvdata(rdev);
+	unsigned int reg = BASE(rdev_get_id(rdev));
+	int ret, val;
+
+	reg += (hibernate) ? OFFSET_MODE_HIB : OFFSET_MODE_A;
+
+	ret = regmap_read(mcp->rmap, reg, &val);
+	if (ret)
+		return ret;
+
+	return val & mask;
+}
+
+/*
+ * mcp16502_gpio_set_mode - set the GPIO corresponding value
+ *
+ * Used to prepare transitioning into hibernate or resuming from it.
+ */
+static void mcp16502_gpio_set_mode(struct mcp16502 *mcp, int mode)
+{
+	int lpm = (mode == MCP16502_MODE_HIB);
+
+	gpiod_set_value(mcp->lpm, lpm);
+}
+
+/*
+ * mcp16502_get_mode - return the current operating mode of a regulator
+ *
+ * Note: all functions that are not part of entering/exiting standby/suspend
+ *	 use the Active mode registers.
+ *
+ * Note: this is different from the PMIC's operatig mode, it is the
+ *	 MODE bit from the regulator's register.
+ */
+static unsigned int mcp16502_get_mode(struct regulator_dev *rdev)
+{
+	int val;
+
+	val = mcp16502_read(rdev, false, MCP16502_MODE_MASK);
+	if (val < 0)
+		return val;
+
+	if (val == MCP16502_MODE_FPWM)
+		return REGULATOR_MODE_NORMAL;
+	else if (val == MCP16502_MODE_AUTO_PFM)
+		return REGULATOR_MODE_IDLE;
+
+	return REGULATOR_MODE_INVALID;
+}
+
+/*
+ * mcp16502_is_enabled - regulator_ops is_enabled
+ */
+static int mcp16502_is_enabled(struct regulator_dev *rdev)
+{
+	int val;
+
+	val = mcp16502_read(rdev, false, MCP16502_EN_MASK);
+	if (val < 0)
+		return val;
+
+	return !!val;
+}
+
+/*
+ * mcp16502_disable - regulator_ops disable
+ */
+static int mcp16502_disable(struct regulator_dev *rdev)
+{
+	return mcp16502_update_regulator(rdev, false, MCP16502_EN_MASK, 0);
+}
+
+/*
+ * _mcp16502_set_mode - helper for set_mode and set_suspend_mode
+ */
+static int _mcp16502_set_mode(struct regulator_dev *rdev, bool hibernate,
+			      unsigned int mode)
+{
+	int val;
+
+	if (mode != REGULATOR_MODE_NORMAL && mode != REGULATOR_MODE_IDLE)
+		return -EINVAL;
+
+	val = (mode == REGULATOR_MODE_NORMAL) ? MCP16502_MODE_FPWM :
+						MCP16502_MODE_AUTO_PFM;
+
+	return mcp16502_update_regulator(rdev, hibernate, MCP16502_MODE_MASK,
+					 val);
+}
+
+/*
+ * mcp16502_set_mode - regulator_ops set_mode
+ */
+static int mcp16502_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+	return _mcp16502_set_mode(rdev, false, mode);
+}
+
+/*
+ * mcp16502_enable - regulator_ops enable
+ */
+static int mcp16502_enable(struct regulator_dev *rdev)
+{
+	return mcp16502_update_regulator(rdev, false, MCP16502_EN_MASK,
+					 MCP16502_EN_MASK);
+}
+
+/*
+ * mcp16502_get_voltage_sel - regulator_ops get_voltage_sel
+ */
+static int mcp16502_get_voltage_sel(struct regulator_dev *rdev)
+{
+	return mcp16502_read(rdev, false, MCP16502_VOL_SEL_MASK);
+}
+
+/*
+ * mcp16502_set_voltage_sel - regulator_ops set_voltage_sel
+ */
+static int mcp16502_set_voltage_sel(struct regulator_dev *rdev,
+				    unsigned int selector)
+{
+	return mcp16502_update_regulator(rdev, false, MCP16502_VOL_SEL_MASK,
+					 selector);
+}
+
+/*
+ * mcp16502_get_status - regulator_ops get_status
+ */
+static int mcp16502_get_status(struct regulator_dev *rdev)
+{
+	int mode = mcp16502_get_mode(rdev);
+
+	if (!mcp16502_is_enabled(rdev))
+		return REGULATOR_STATUS_OFF;
+	else if (mode == REGULATOR_MODE_NORMAL)
+		return REGULATOR_STATUS_NORMAL;
+	else if (mode == REGULATOR_MODE_IDLE)
+		return REGULATOR_STATUS_IDLE;
+
+	return REGULATOR_STATUS_UNDEFINED;
+}
+
+/*
+ * mcp16502_set_suspend_voltage - regulator_ops set_suspend_voltage
+ */
+int mcp16502_set_suspend_voltage(struct regulator_dev *rdev, int uV)
+{
+	int sel = regulator_map_voltage_linear_range(rdev, uV, uV);
+
+	if (sel < 0)
+		return sel;
+
+	return mcp16502_update_regulator(rdev, true, MCP16502_VOL_SEL_MASK,
+					 sel);
+}
+
+/*
+ * mcp16502_set_suspend_mode - regulator_ops set_suspend_mode
+ */
+int mcp16502_set_suspend_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+	return _mcp16502_set_mode(rdev, true, mode);
+}
+
+/*
+ * mcp16502_set_suspend_enable - regulator_ops set_suspend_enable
+ */
+static int mcp16502_set_suspend_enable(struct regulator_dev *rdev)
+{
+	return mcp16502_update_regulator(rdev, true, MCP16502_EN_MASK,
+					 MCP16502_EN_MASK);
+}
+
+/*
+ * mcp16502_set_suspend_disable - regulator_ops set_suspend_disable
+ */
+static int mcp16502_set_suspend_disable(struct regulator_dev *rdev)
+{
+	return mcp16502_update_regulator(rdev, true, MCP16502_EN_MASK, 0);
+}
+
+static const struct regulator_ops mcp16502_buck_ops = {
+	.list_voltage			= regulator_list_voltage_linear_range,
+	.map_voltage			= regulator_map_voltage_linear_range,
+	.get_voltage_sel		= mcp16502_get_voltage_sel,
+	.set_voltage_sel		= mcp16502_set_voltage_sel,
+	.enable				= mcp16502_enable,
+	.disable			= mcp16502_disable,
+	.is_enabled			= mcp16502_is_enabled,
+	.set_mode			= mcp16502_set_mode,
+	.get_mode			= mcp16502_get_mode,
+	.get_status			= mcp16502_get_status,
+	.set_suspend_voltage		= mcp16502_set_suspend_voltage,
+	.set_suspend_mode		= mcp16502_set_suspend_mode,
+	.set_suspend_enable		= mcp16502_set_suspend_enable,
+	.set_suspend_disable		= mcp16502_set_suspend_disable,
+};
+
+/*
+ * LDOs cannot change operating modes.
+ */
+static const struct regulator_ops mcp16502_ldo_ops = {
+	.list_voltage			= regulator_list_voltage_linear_range,
+	.map_voltage			= regulator_map_voltage_linear_range,
+	.get_voltage_sel		= mcp16502_get_voltage_sel,
+	.set_voltage_sel		= mcp16502_set_voltage_sel,
+	.enable				= mcp16502_enable,
+	.disable			= mcp16502_disable,
+	.is_enabled			= mcp16502_is_enabled,
+	.get_status			= mcp16502_get_status,
+	.set_suspend_voltage		= mcp16502_set_suspend_voltage,
+	.set_suspend_enable		= mcp16502_set_suspend_enable,
+	.set_suspend_disable		= mcp16502_set_suspend_disable,
+};
+
+static const struct of_device_id mcp16502_ids[] = {
+	{ .compatible = "microchip,mcp16502", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mcp16502_ids);
+
+static const struct regulator_linear_range b1l12_ranges[] = {
+	REGULATOR_LINEAR_RANGE(1200000, VDD_LOW_SEL, VDD_HIGH_SEL, 50000),
+};
+
+static const struct regulator_linear_range b234_ranges[] = {
+	REGULATOR_LINEAR_RANGE(600000, VDD_LOW_SEL, VDD_HIGH_SEL, 25000),
+};
+
+static const struct regulator_desc mcp16502_desc[] = {
+	/* MCP16502_REGULATOR(_name, _id, ranges, regulator_ops) */
+	MCP16502_REGULATOR("VDD_IO", BUCK1, b1l12_ranges, mcp16502_buck_ops),
+	MCP16502_REGULATOR("VDD_DDR", BUCK2, b234_ranges, mcp16502_buck_ops),
+	MCP16502_REGULATOR("VDD_CORE", BUCK3, b234_ranges, mcp16502_buck_ops),
+	MCP16502_REGULATOR("VDD_OTHER", BUCK4, b234_ranges, mcp16502_buck_ops),
+	MCP16502_REGULATOR("LDO1", LDO1, b1l12_ranges, mcp16502_ldo_ops),
+	MCP16502_REGULATOR("LDO2", LDO2, b1l12_ranges, mcp16502_ldo_ops)
+};
+
+static const struct regmap_range mcp16502_ranges[] = {
+	regmap_reg_range(MIN_REG, MAX_REG)
+};
+
+static const struct regmap_access_table mcp16502_yes_reg_table = {
+	.yes_ranges = mcp16502_ranges,
+	.n_yes_ranges = ARRAY_SIZE(mcp16502_ranges),
+};
+
+static const struct regmap_config mcp16502_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= MAX_REG,
+	.cache_type	= REGCACHE_NONE,
+	.rd_table	= &mcp16502_yes_reg_table,
+	.wr_table	= &mcp16502_yes_reg_table,
+};
+
+/*
+ * set_up_regulators - initialize all regulators
+ */
+static int setup_regulators(struct mcp16502 *mcp, struct device *dev,
+			    struct regulator_config config)
+{
+	int i;
+
+	for (i = 0; i < NUM_REGULATORS; i++) {
+		mcp->rdev[i] = devm_regulator_register(dev,
+						       &mcp16502_desc[i],
+						       &config);
+		if (IS_ERR(mcp->rdev[i])) {
+			dev_err(dev,
+				"failed to register %s regulator %ld\n",
+				mcp16502_desc[i].name, PTR_ERR(mcp->rdev[i]));
+			return PTR_ERR(mcp->rdev[i]);
+		}
+	}
+
+	return 0;
+}
+
+static int mcp16502_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct regulator_config config = { };
+	struct device *dev;
+	struct mcp16502 *mcp;
+	int ret = 0;
+
+	dev = &client->dev;
+	config.dev = dev;
+
+	mcp = devm_kzalloc(dev, sizeof(*mcp), GFP_KERNEL);
+	if (!mcp)
+		return -ENOMEM;
+
+	mcp->rmap = devm_regmap_init_i2c(client, &mcp16502_regmap_config);
+	if (IS_ERR(mcp->rmap)) {
+		ret = PTR_ERR(mcp->rmap);
+		dev_err(dev, "regmap init failed: %d\n", ret);
+		return ret;
+	}
+
+	i2c_set_clientdata(client, mcp);
+	config.regmap = mcp->rmap;
+	config.driver_data = mcp;
+
+	mcp->lpm = devm_gpiod_get(dev, "lpm", GPIOD_OUT_LOW);
+	if (IS_ERR(mcp->lpm)) {
+		dev_err(dev, "failed to get lpm pin: %ld\n", PTR_ERR(mcp->lpm));
+		return PTR_ERR(mcp->lpm);
+	}
+
+	ret = setup_regulators(mcp, dev, config);
+	if (ret != 0)
+		return ret;
+
+	mcp16502_gpio_set_mode(mcp, MCP16502_MODE_ACTIVE);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int mcp16502_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct mcp16502 *mcp = i2c_get_clientdata(client);
+
+	mcp16502_gpio_set_mode(mcp, MCP16502_MODE_HIB);
+
+	return 0;
+}
+
+static int mcp16502_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct mcp16502 *mcp = i2c_get_clientdata(client);
+
+	mcp16502_gpio_set_mode(mcp, MCP16502_MODE_ACTIVE);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops mcp16502_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(mcp16502_suspend, mcp16502_resume)
+};
+
+static const struct i2c_device_id mcp16502_i2c_id[] = {
+	{ "mcp16502", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, mcp16502_i2c_id);
+
+static struct i2c_driver mcp16502_drv = {
+	.probe		= mcp16502_probe,
+	.driver		= {
+		.name	= "mcp16502-regulator",
+		.owner		= THIS_MODULE,
+		.of_match_table	= of_match_ptr(mcp16502_ids),
+		.pm = &mcp16502_pm_ops,
+	},
+	.id_table	= mcp16502_i2c_id,
+};
+
+static int __init mcp16502_init(void)
+{
+	return i2c_add_driver(&mcp16502_drv);
+}
+
+static void __exit mcp16502_exit(void)
+{
+}
+
+module_init(mcp16502_init);
+module_exit(mcp16502_exit);
+
+MODULE_VERSION("1.0");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MCP16502 PMIC driver");
+MODULE_AUTHOR("Andrei Stefanescu andrei.stefanescu@microchip.com");
-- 
2.7.4

Re: [PATCH 1/3] regulator: dt-bindings: add MCP16502 regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On Tue, Nov 13, 2018 at 11:29:24AM +0000, Andrei.Stefanescu@microchip.com wrote:

> +- lpm-gpio: GPIO for LPM pin. Note that this GPIO *must* remain high during
> +	     suspend-to-ram, keeping the PMIC into HIBERNATE mode.

All GPIO properties should be -gpios even if they can only ever have one
GPIO.

> +- regulators: A node that houses a sub-node for each regulator within
> +              the device. Each sub-node is identified using the node's
> +              name. The content of each sub-node is defined by the
> +              standard binding for regulators; see regulator.txt.

You should describe which regulators exist for the device.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] regulator: mcp16502: add regulator driver for MCP16502

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4075 bytes --]

On Tue, Nov 13, 2018 at 11:29:41AM +0000, Andrei.Stefanescu@microchip.com wrote:

> index 0000000..29c72d3
> --- /dev/null
> +++ b/drivers/regulator/mcp16502.c
> @@ -0,0 +1,524 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MCP16502 PMIC driver

Please make the entire comment a C++ comment so it looks more
intentional.

> +/*
> + * This macro is useful for iterating over all regulators and accessing their
> + * registers in a generic way or accessing a regulator device by its id.
> + */
> +#define BASE(i) (((i) + 1) << 4)

This macro name is likely to run into collisions at some point, a prefix
would be better.

> +static int mcp16502_update_regulator(struct regulator_dev *rdev,
> +				     bool hibernate,
> +				     unsigned int mask, unsigned int val)
> +{
> +	struct mcp16502 *mcp = rdev_get_drvdata(rdev);
> +	unsigned int reg = BASE(rdev_get_id(rdev));
> +
> +	reg += (hibernate) ? OFFSET_MODE_HIB : OFFSET_MODE_A;

Please write this as a normal if statement to improve legibility.

> +static int mcp16502_read(struct regulator_dev *rdev, bool hibernate,
> +			 unsigned int mask)
> +{
> +	struct mcp16502 *mcp = rdev_get_drvdata(rdev);
> +	unsigned int reg = BASE(rdev_get_id(rdev));
> +	int ret, val;
> +
> +	reg += (hibernate) ? OFFSET_MODE_HIB : OFFSET_MODE_A;
> +
> +	ret = regmap_read(mcp->rmap, reg, &val);
> +	if (ret)
> +		return ret;
> +
> +	return val & mask;
> +}

I'm not convinced that these custom read/write functions are adding much
compared to just using the standard regmap helpers for things - they're
adding a bunch of code instead of data.  If you need extra helpers for
suspend mode just add those, I'm sure they'd be useful for other
drivers.

> +static unsigned int mcp16502_get_mode(struct regulator_dev *rdev)
> +{
> +	int val;
> +
> +	val = mcp16502_read(rdev, false, MCP16502_MODE_MASK);
> +	if (val < 0)
> +		return val;
> +
> +	if (val == MCP16502_MODE_FPWM)
> +		return REGULATOR_MODE_NORMAL;
> +	else if (val == MCP16502_MODE_AUTO_PFM)
> +		return REGULATOR_MODE_IDLE;
> +
> +	return REGULATOR_MODE_INVALID;
> +}

Please just write a switch statement to improve legibility.

> +/*
> + * mcp16502_is_enabled - regulator_ops is_enabled
> + */
> +static int mcp16502_is_enabled(struct regulator_dev *rdev)
> +{
> +	int val;
> +
> +	val = mcp16502_read(rdev, false, MCP16502_EN_MASK);
> +	if (val < 0)
> +		return val;
> +
> +	return !!val;
> +}

This can use regulator_is_enabled_regmap().  

> +	if (mode != REGULATOR_MODE_NORMAL && mode != REGULATOR_MODE_IDLE)
> +		return -EINVAL;
> +
> +	val = (mode == REGULATOR_MODE_NORMAL) ? MCP16502_MODE_FPWM :
> +						MCP16502_MODE_AUTO_PFM;
> +
> +	return mcp16502_update_regulator(rdev, hibernate, MCP16502_MODE_MASK,
> +					 val);

Again a switch statement would be clearer.

> +static int mcp16502_get_status(struct regulator_dev *rdev)
> +{
> +	int mode = mcp16502_get_mode(rdev);
> +
> +	if (!mcp16502_is_enabled(rdev))
> +		return REGULATOR_STATUS_OFF;
> +	else if (mode == REGULATOR_MODE_NORMAL)
> +		return REGULATOR_STATUS_NORMAL;
> +	else if (mode == REGULATOR_MODE_IDLE)
> +		return REGULATOR_STATUS_IDLE;
> +
> +	return REGULATOR_STATUS_UNDEFINED;
> +}

The _status() function should only be implemented if there's hardware
support for reading back the actual status from the device, we already
know what we set through software.

> +#ifdef CONFIG_PM_SLEEP
> +static int mcp16502_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct mcp16502 *mcp = i2c_get_clientdata(client);
> +
> +	mcp16502_gpio_set_mode(mcp, MCP16502_MODE_HIB);
> +
> +	return 0;
> +}

This will put the regulators into hibernate mode before the system has
finished suspending which is likely to cause breakage - hibernate mode
in the PMIC is expected to be triggered by the SoC as it shuts down.

> +static int __init mcp16502_init(void)
> +{
> +	return i2c_add_driver(&mcp16502_drv);
> +}

module_i2c_driver.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] regulator: mcp16502: add regulator driver for MCP16502

[Andrei.Stefanescu]

Hello Mark,

Thank you for your review. While working on the next version I realized that
the implementation should change a bit. The mcp16502 PMIC has separate 
registers
for each of its operating modes (Performance, Active, Low-power, Hibernate).
So, it is possible to setup the values for Low-power (Linux standby) and
Hibernate (Linux suspend-to-ram) and these values would not be affected by
changes made during runtime (which are made to the ACTIVE registers).

This means that the calls to suspend_set_* are not necessary to be made each
time the board suspends. My idea is to add three functions to the 
regulator_ops
(setup_suspend_standby/mem/max) which would be called after the regulator is
registered and which would set the values found inside the devicetree
in the regulator-state-standby/mem/disk subnodes.

However, I am not sure whether this is in fact a good idea or which is 
the best
approach.

Other possible suggestions for this setup_suspend* functions:
- Break each function into smaller pieces(set_voltage, set_mode, 
enable/disable)
- Add support for regmap helpers, which would mean adding reg and mask 
members
   to regulator_desc. However, I am not sure which naming scheme you prefer
   (enable_suspend_mem_reg for example seems not to be a great idea). 
Perhaps use
   arrays (e.g. enable_suspend[] and index it via PM_SUSPEND_*)?


Best regards,
Andrei

On 13.11.2018 19:45, Mark Brown wrote:
> On Tue, Nov 13, 2018 at 11:29:41AM +0000, Andrei.Stefanescu@microchip.com wrote:
>
>> index 0000000..29c72d3
>> --- /dev/null
>> +++ b/drivers/regulator/mcp16502.c
>> @@ -0,0 +1,524 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * MCP16502 PMIC driver
> Please make the entire comment a C++ comment so it looks more
> intentional.
>
>> +/*
>> + * This macro is useful for iterating over all regulators and accessing their
>> + * registers in a generic way or accessing a regulator device by its id.
>> + */
>> +#define BASE(i) (((i) + 1) << 4)
> This macro name is likely to run into collisions at some point, a prefix
> would be better.
>
>> +static int mcp16502_update_regulator(struct regulator_dev *rdev,
>> +				     bool hibernate,
>> +				     unsigned int mask, unsigned int val)
>> +{
>> +	struct mcp16502 *mcp = rdev_get_drvdata(rdev);
>> +	unsigned int reg = BASE(rdev_get_id(rdev));
>> +
>> +	reg += (hibernate) ? OFFSET_MODE_HIB : OFFSET_MODE_A;
> Please write this as a normal if statement to improve legibility.
>
>> +static int mcp16502_read(struct regulator_dev *rdev, bool hibernate,
>> +			 unsigned int mask)
>> +{
>> +	struct mcp16502 *mcp = rdev_get_drvdata(rdev);
>> +	unsigned int reg = BASE(rdev_get_id(rdev));
>> +	int ret, val;
>> +
>> +	reg += (hibernate) ? OFFSET_MODE_HIB : OFFSET_MODE_A;
>> +
>> +	ret = regmap_read(mcp->rmap, reg, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return val & mask;
>> +}
> I'm not convinced that these custom read/write functions are adding much
> compared to just using the standard regmap helpers for things - they're
> adding a bunch of code instead of data.  If you need extra helpers for
> suspend mode just add those, I'm sure they'd be useful for other
> drivers.
>
>> +static unsigned int mcp16502_get_mode(struct regulator_dev *rdev)
>> +{
>> +	int val;
>> +
>> +	val = mcp16502_read(rdev, false, MCP16502_MODE_MASK);
>> +	if (val < 0)
>> +		return val;
>> +
>> +	if (val == MCP16502_MODE_FPWM)
>> +		return REGULATOR_MODE_NORMAL;
>> +	else if (val == MCP16502_MODE_AUTO_PFM)
>> +		return REGULATOR_MODE_IDLE;
>> +
>> +	return REGULATOR_MODE_INVALID;
>> +}
> Please just write a switch statement to improve legibility.
>
>> +/*
>> + * mcp16502_is_enabled - regulator_ops is_enabled
>> + */
>> +static int mcp16502_is_enabled(struct regulator_dev *rdev)
>> +{
>> +	int val;
>> +
>> +	val = mcp16502_read(rdev, false, MCP16502_EN_MASK);
>> +	if (val < 0)
>> +		return val;
>> +
>> +	return !!val;
>> +}
> This can use regulator_is_enabled_regmap().
>
>> +	if (mode != REGULATOR_MODE_NORMAL && mode != REGULATOR_MODE_IDLE)
>> +		return -EINVAL;
>> +
>> +	val = (mode == REGULATOR_MODE_NORMAL) ? MCP16502_MODE_FPWM :
>> +						MCP16502_MODE_AUTO_PFM;
>> +
>> +	return mcp16502_update_regulator(rdev, hibernate, MCP16502_MODE_MASK,
>> +					 val);
> Again a switch statement would be clearer.
>
>> +static int mcp16502_get_status(struct regulator_dev *rdev)
>> +{
>> +	int mode = mcp16502_get_mode(rdev);
>> +
>> +	if (!mcp16502_is_enabled(rdev))
>> +		return REGULATOR_STATUS_OFF;
>> +	else if (mode == REGULATOR_MODE_NORMAL)
>> +		return REGULATOR_STATUS_NORMAL;
>> +	else if (mode == REGULATOR_MODE_IDLE)
>> +		return REGULATOR_STATUS_IDLE;
>> +
>> +	return REGULATOR_STATUS_UNDEFINED;
>> +}
> The _status() function should only be implemented if there's hardware
> support for reading back the actual status from the device, we already
> know what we set through software.
>
>> +#ifdef CONFIG_PM_SLEEP
>> +static int mcp16502_suspend(struct device *dev)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct mcp16502 *mcp = i2c_get_clientdata(client);
>> +
>> +	mcp16502_gpio_set_mode(mcp, MCP16502_MODE_HIB);
>> +
>> +	return 0;
>> +}
> This will put the regulators into hibernate mode before the system has
> finished suspending which is likely to cause breakage - hibernate mode
> in the PMIC is expected to be triggered by the SoC as it shuts down.
>
>> +static int __init mcp16502_init(void)
>> +{
>> +	return i2c_add_driver(&mcp16502_drv);
>> +}
> module_i2c_driver.

Re: [PATCH 3/3] regulator: mcp16502: add regulator driver for MCP16502

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

On Wed, Nov 21, 2018 at 03:44:31PM +0000, Andrei.Stefanescu@microchip.com wrote:

> Thank you for your review. While working on the next version I realized that
> the implementation should change a bit. The mcp16502 PMIC has separate 
> registers
> for each of its operating modes (Performance, Active, Low-power, Hibernate).
> So, it is possible to setup the values for Low-power (Linux standby) and
> Hibernate (Linux suspend-to-ram) and these values would not be affected by
> changes made during runtime (which are made to the ACTIVE registers).

This is totally normal for regulators, it's the only way that suspend
modes can function sensibly since suspend mode configuration is
typically not capable off supplying the processor.  If your driver is
currently trying to use the normal operating registers then it's buggy.

> This means that the calls to suspend_set_* are not necessary to be made each
> time the board suspends. My idea is to add three functions to the 
> regulator_ops
> (setup_suspend_standby/mem/max) which would be called after the regulator is
> registered and which would set the values found inside the devicetree
> in the regulator-state-standby/mem/disk subnodes.

There is no need to add new operations, we already have a number of
suspend specific operations covering enable state, modes and voltage.
You should just use those, there's already code to call them.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]