2018-12-11 10:10:21

by Andrei.Stefanescu

[permalink] [raw]
Subject: [RESEND PATCH v3 0/3] add support for MCP16502 PMIC

The resend is because I forgot to add Mark.

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.

v3:
- use CONFIG_SUSPEND and CONFIG_PM to fix compile errors for some configs

v2:
- use lpm-gpios instead of lpm-gpio in devicetree bindings documentation
- describe the regulators present on the PMIC in the devicetree bindings
documentation
- add SPDX license inside a C++ comment
- prefix macro
- remove mcp16502_update_regulator and mcp16502_read
- replace ?: with if-else
- change some if-else with switch statements for legibility
- use regmap helpers for regultor settings during runtime
- make mcp16502_get_status read the status from the PMIC STS registers
- use module_i2c_driver
- use the PMIC's Hibernate registers for suspend-to-mem, the PMIC's
Low-power registers for standby and the PMIC's Active registers for
normal runtime

Note about mcp16502_suspend:
- mcp16502_gpio_set_mode(mcp, MCP16502_OPMODE_HIB) has now been changed to
mcp16502_gpio_set_mode(mcp, MCP16502_OPMODE_LPM) for legibility.

Note that the function call only sets the LPM pin of the PMIC to high.
This puts the PMIC in Low-power operating mode. Hibernate operating mode
is reached when the MPU sets the PWRHLD line to zero (typically when
entering suspend-to-ram).

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 | 143 ++++++
MAINTAINERS | 7 +
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/mcp16502.c | 555 +++++++++++++++++++++
5 files changed, 715 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/mcp16502-regulator.txt
create mode 100644 drivers/regulator/mcp16502.c

--
2.7.4



2018-12-11 10:11:36

by Andrei.Stefanescu

[permalink] [raw]
Subject: [RESEND PATCH v3 1/3] regulator: dt-bindings: add MCP16502 regulator bindings

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

Signed-off-by: Andrei Stefanescu <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../bindings/regulator/mcp16502-regulator.txt | 143 +++++++++++++++++++++
1 file changed, 143 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..b8f843f
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/mcp16502-regulator.txt
@@ -0,0 +1,143 @@
+MCP16502 PMIC
+
+Required properties:
+- compatible: "microchip,mcp16502"
+- reg: I2C slave address
+- lpm-gpios: 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.
+
+Regualtors of MCP16502 PMIC:
+1) VDD_IO - Buck (1.2 - 3.7 V)
+2) VDD_DDR - Buck (0.6 - 1.85 V)
+3) VDD_CORE - Buck (0.6 - 1.85 V)
+4) VDD_OTHER - BUCK (0.6 - 1.85 V)
+5) LDO1 - LDO (1.2 - 3.7 V)
+6) LDO2 - LDO (1.2 - 3.7 V)
+
+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-standby {
+ regulator-on-in-suspend;
+ regulator-mode = <4>;
+ };
+
+ 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-standby {
+ regulator-on-in-suspend;
+ regulator-mode = <4>;
+ };
+
+ 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-standby {
+ regulator-on-in-suspend;
+ regulator-mode = <4>;
+ };
+
+ 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-standby {
+ regulator-on-in-suspend;
+ regulator-mode = <4>;
+ };
+
+ 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-standby {
+ regulator-on-in-suspend;
+ };
+
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
+ };
+
+ LDO2 {
+ regulator-name = "LDO2";
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <3700000>;
+ regulator-always-on;
+
+ regulator-state-standby {
+ regulator-on-in-suspend;
+ };
+
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
+ };
+
+ };
+};
--
2.7.4


2018-12-11 10:12:01

by Andrei.Stefanescu

[permalink] [raw]
Subject: [RESEND PATCH v3 3/3] regulator: mcp16502: add regulator driver for MCP16502

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 <[email protected]>
---
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/mcp16502.c | 555 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 565 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..54db3df
--- /dev/null
+++ b/drivers/regulator/mcp16502.c
@@ -0,0 +1,555 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * MCP16502 PMIC driver
+ *
+ * Copyright (C) 2018 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Andrei Stefanescu <[email protected]>
+ *
+ * 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>
+#include <linux/suspend.h>
+
+#define VDD_LOW_SEL 0x0D
+#define VDD_HIGH_SEL 0x3F
+
+#define MCP16502_FLT BIT(7)
+#define MCP16502_ENS BIT(0)
+
+/*
+ *
+ * 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 function is useful for iterating over all regulators and accessing their
+ * registers in a generic way or accessing a regulator device by its id.
+ */
+#define MCP16502_BASE(i) (((i) + 1) << 4)
+#define MCP16502_STAT_BASE(i) ((i) + 5)
+
+#define MCP16502_OFFSET_MODE_A 0
+#define MCP16502_OFFSET_MODE_LPM 1
+#define MCP16502_OFFSET_MODE_HIB 2
+
+#define MCP16502_OPMODE_ACTIVE REGULATOR_MODE_NORMAL
+#define MCP16502_OPMODE_LPM REGULATOR_MODE_IDLE
+#define MCP16502_OPMODE_HIB REGULATOR_MODE_STANDBY
+
+#define MCP16502_MODE_AUTO_PFM 0
+#define MCP16502_MODE_FPWM BIT(6)
+
+#define MCP16502_VSEL 0x3F
+#define MCP16502_EN BIT(7)
+#define MCP16502_MODE BIT(6)
+
+#define MCP16502_MIN_REG 0x0
+#define MCP16502_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, \
+ .vsel_reg = (((_id) + 1) << 4), \
+ .vsel_mask = MCP16502_VSEL, \
+ .enable_reg = (((_id) + 1) << 4), \
+ .enable_mask = MCP16502_EN, \
+ }
+
+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_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)
+{
+ switch (mode) {
+ case MCP16502_OPMODE_ACTIVE:
+ gpiod_set_value(mcp->lpm, 0);
+ break;
+ case MCP16502_OPMODE_LPM:
+ case MCP16502_OPMODE_HIB:
+ gpiod_set_value(mcp->lpm, 1);
+ break;
+ default:
+ pr_err("%s: %d invalid\n", __func__, mode);
+ }
+}
+
+/*
+ * mcp16502_get_reg() - get the PMIC's configuration register for opmode
+ *
+ * @rdev: the regulator whose register we are searching
+ * @opmode: the PMIC's operating mode ACTIVE, Low-power, Hibernate
+ */
+static int mcp16502_get_reg(struct regulator_dev *rdev, int opmode)
+{
+ int reg = MCP16502_BASE(rdev_get_id(rdev));
+
+ switch (opmode) {
+ case MCP16502_OPMODE_ACTIVE:
+ return reg + MCP16502_OFFSET_MODE_A;
+ case MCP16502_OPMODE_LPM:
+ return reg + MCP16502_OFFSET_MODE_LPM;
+ case MCP16502_OPMODE_HIB:
+ return reg + MCP16502_OFFSET_MODE_HIB;
+ default:
+ return -EINVAL;
+ }
+}
+
+/*
+ * 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)
+{
+ unsigned int val;
+ int ret, reg;
+ struct mcp16502 *mcp = rdev_get_drvdata(rdev);
+
+ reg = mcp16502_get_reg(rdev, MCP16502_OPMODE_ACTIVE);
+ if (reg < 0)
+ return reg;
+
+ ret = regmap_read(mcp->rmap, reg, &val);
+ if (ret)
+ return ret;
+
+ switch (val & MCP16502_MODE) {
+ case MCP16502_MODE_FPWM:
+ return REGULATOR_MODE_NORMAL;
+ case MCP16502_MODE_AUTO_PFM:
+ return REGULATOR_MODE_IDLE;
+ default:
+ return REGULATOR_MODE_INVALID;
+ }
+}
+
+/*
+ * _mcp16502_set_mode() - helper for set_mode and set_suspend_mode
+ *
+ * @rdev: the regulator for which we are setting the mode
+ * @mode: the regulator's mode (the one from MODE bit)
+ * @opmode: the PMIC's operating mode: Active/Low-power/Hibernate
+ */
+static int _mcp16502_set_mode(struct regulator_dev *rdev, unsigned int mode,
+ unsigned int op_mode)
+{
+ int val;
+ int reg;
+ struct mcp16502 *mcp = rdev_get_drvdata(rdev);
+
+ reg = mcp16502_get_reg(rdev, op_mode);
+ if (reg < 0)
+ return reg;
+
+ switch (mode) {
+ case REGULATOR_MODE_NORMAL:
+ val = MCP16502_MODE_FPWM;
+ break;
+ case REGULATOR_MODE_IDLE:
+ val = MCP16502_MODE_AUTO_PFM;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ reg = regmap_update_bits(mcp->rmap, reg, MCP16502_MODE, val);
+ return reg;
+}
+
+/*
+ * mcp16502_set_mode() - regulator_ops set_mode
+ */
+static int mcp16502_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+ return _mcp16502_set_mode(rdev, mode, MCP16502_OPMODE_ACTIVE);
+}
+
+/*
+ * mcp16502_get_status() - regulator_ops get_status
+ */
+static int mcp16502_get_status(struct regulator_dev *rdev)
+{
+ int ret;
+ unsigned int val;
+ struct mcp16502 *mcp = rdev_get_drvdata(rdev);
+
+ ret = regmap_read(mcp->rmap, MCP16502_STAT_BASE(rdev_get_id(rdev)),
+ &val);
+ if (ret)
+ return ret;
+
+ if (val & MCP16502_FLT)
+ return REGULATOR_STATUS_ERROR;
+ else if (val & MCP16502_ENS)
+ return REGULATOR_STATUS_ON;
+ else if (!(val & MCP16502_ENS))
+ return REGULATOR_STATUS_OFF;
+
+ return REGULATOR_STATUS_UNDEFINED;
+}
+
+#ifdef CONFIG_SUSPEND
+/*
+ * mcp16502_suspend_get_target_reg() - get the reg of the target suspend PMIC
+ * mode
+ */
+static int mcp16502_suspend_get_target_reg(struct regulator_dev *rdev)
+{
+ switch (pm_suspend_target_state) {
+ case PM_SUSPEND_STANDBY:
+ return mcp16502_get_reg(rdev, MCP16502_OPMODE_LPM);
+ case PM_SUSPEND_ON:
+ case PM_SUSPEND_MEM:
+ return mcp16502_get_reg(rdev, MCP16502_OPMODE_HIB);
+ default:
+ dev_err(&rdev->dev, "invalid suspend target: %d\n",
+ pm_suspend_target_state);
+ }
+
+ return -EINVAL;
+}
+
+/*
+ * mcp16502_set_suspend_voltage() - regulator_ops set_suspend_voltage
+ */
+static int mcp16502_set_suspend_voltage(struct regulator_dev *rdev, int uV)
+{
+ struct mcp16502 *mcp = rdev_get_drvdata(rdev);
+ int sel = regulator_map_voltage_linear_range(rdev, uV, uV);
+ int reg = mcp16502_suspend_get_target_reg(rdev);
+
+ if (sel < 0)
+ return sel;
+
+ if (reg < 0)
+ return reg;
+
+ return regmap_update_bits(mcp->rmap, reg, MCP16502_VSEL, sel);
+}
+
+/*
+ * mcp16502_set_suspend_mode() - regulator_ops set_suspend_mode
+ */
+static int mcp16502_set_suspend_mode(struct regulator_dev *rdev,
+ unsigned int mode)
+{
+ switch (pm_suspend_target_state) {
+ case PM_SUSPEND_STANDBY:
+ return _mcp16502_set_mode(rdev, mode, MCP16502_OPMODE_LPM);
+ case PM_SUSPEND_ON:
+ case PM_SUSPEND_MEM:
+ return _mcp16502_set_mode(rdev, mode, MCP16502_OPMODE_HIB);
+ default:
+ dev_err(&rdev->dev, "invalid suspend target: %d\n",
+ pm_suspend_target_state);
+ }
+
+ return -EINVAL;
+}
+
+/*
+ * mcp16502_set_suspend_enable() - regulator_ops set_suspend_enable
+ */
+static int mcp16502_set_suspend_enable(struct regulator_dev *rdev)
+{
+ struct mcp16502 *mcp = rdev_get_drvdata(rdev);
+ int reg = mcp16502_suspend_get_target_reg(rdev);
+
+ if (reg < 0)
+ return reg;
+
+ return regmap_update_bits(mcp->rmap, reg, MCP16502_EN, MCP16502_EN);
+}
+
+/*
+ * mcp16502_set_suspend_disable() - regulator_ops set_suspend_disable
+ */
+static int mcp16502_set_suspend_disable(struct regulator_dev *rdev)
+{
+ struct mcp16502 *mcp = rdev_get_drvdata(rdev);
+ int reg = mcp16502_suspend_get_target_reg(rdev);
+
+ if (reg < 0)
+ return reg;
+
+ return regmap_update_bits(mcp->rmap, reg, MCP16502_EN, 0);
+}
+#endif /* CONFIG_SUSPEND */
+
+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 = regulator_get_voltage_sel_regmap,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .get_status = mcp16502_get_status,
+
+ .set_mode = mcp16502_set_mode,
+ .get_mode = mcp16502_get_mode,
+
+#ifdef CONFIG_SUSPEND
+ .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,
+#endif /* CONFIG_SUSPEND */
+};
+
+/*
+ * 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 = regulator_get_voltage_sel_regmap,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .get_status = mcp16502_get_status,
+
+#ifdef CONFIG_SUSPEND
+ .set_suspend_voltage = mcp16502_set_suspend_voltage,
+ .set_suspend_enable = mcp16502_set_suspend_enable,
+ .set_suspend_disable = mcp16502_set_suspend_disable,
+#endif /* CONFIG_SUSPEND */
+};
+
+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(MCP16502_MIN_REG, MCP16502_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 = MCP16502_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_OPMODE_ACTIVE);
+
+ return 0;
+}
+
+#ifdef CONFIG_SUSPEND
+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_OPMODE_LPM);
+
+ 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_OPMODE_ACTIVE);
+
+ return 0;
+}
+#else /* !CONFIG_SUSPEND */
+#define mcp16502_suspend NULL
+#define mcp16502_resume NULL
+#endif /* !CONFIG_SUSPEND */
+
+#ifdef CONFIG_PM
+static const struct dev_pm_ops mcp16502_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(mcp16502_suspend, mcp16502_resume)
+};
+#endif
+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",
+ .of_match_table = of_match_ptr(mcp16502_ids),
+#ifdef CONFIG_PM
+ .pm = &mcp16502_pm_ops,
+#endif
+ },
+ .id_table = mcp16502_i2c_id,
+};
+
+module_i2c_driver(mcp16502_drv);
+
+MODULE_VERSION("1.0");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MCP16502 PMIC driver");
+MODULE_AUTHOR("Andrei Stefanescu [email protected]");
--
2.7.4


2018-12-11 10:28:04

by Andrei.Stefanescu

[permalink] [raw]
Subject: [RESEND PATCH v3 2/3] MAINTAINERS: add maintainer for MCP16502 PMIC driver

This patch adds a maintainer for the MCP16502 PMIC
driver.

Signed-off-by: Andrei Stefanescu <[email protected]>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b755a89..6a74a65 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9730,6 +9730,13 @@ M: Ludovic Desroches <[email protected]>
S: Maintained
F: drivers/mmc/host/atmel-mci.c

+MICROCHIP MCP16502 PMIC DRIVER
+M: Andrei Stefanescu <[email protected]>
+L: [email protected] (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 <[email protected]>
M: Kent Gustavsson <[email protected]>
--
2.7.4


2018-12-11 17:20:43

by Mark Brown

[permalink] [raw]
Subject: Re: [RESEND PATCH v3 3/3] regulator: mcp16502: add regulator driver for MCP16502

On Tue, Dec 11, 2018 at 10:09:15AM +0000, [email protected] wrote:
> This patch adds a regulator driver for the MCP16502 PMIC.
> This drivers supports basic operations through the
> regulator interface such as:

Overall this looks really good, just a couple of comments:

> +++ b/drivers/regulator/mcp16502.c
> @@ -0,0 +1,555 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * MCP16502 PMIC driver

SPDX headers need to be C++ comments - please make the entire comment
block a C++ one so it looks more intentional.

> +#ifdef CONFIG_SUSPEND
> +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_OPMODE_LPM);
> +
> + return 0;
> +}

This puts the device into low power mode when the suspend function gets
called but this might not be safe - devices using the regulator may not
be suspended yet so could still need full regulation. Normally a GPIO
triggered transition like this would be being done by hardware as part
of the process of suspending the SoC. Is there some reason to do this
manually?


Attachments:
(No filename) (1.17 kB)
signature.asc (499.00 B)
Download all attachments

2018-12-12 08:02:32

by Andrei.Stefanescu

[permalink] [raw]
Subject: Re: [RESEND PATCH v3 3/3] regulator: mcp16502: add regulator driver for MCP16502

Hi Mark,

Thank you for the review.

> SPDX headers need to be C++ comments - please make the entire comment
> block a C++ one so it looks more intentional.
I sent a new patch (v4) with the modified comment.
>> +#ifdef CONFIG_SUSPEND
>> +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_OPMODE_LPM);
>> +
>> + return 0;
>> +}
> This puts the device into low power mode when the suspend function gets
> called but this might not be safe - devices using the regulator may not
> be suspended yet so could still need full regulation. Normally a GPIO
> triggered transition like this would be being done by hardware as part
> of the process of suspending the SoC. Is there some reason to do this
> manually?
There is a line from the MPU (SHDN) which goes low only when the MPU
turns off. That line is already connected to the PMIC and it differentiates
between suspend-to-mem and standby. To switch to low-power, the PMIC must
be controlled by the GPIO pin LPM.

The suspend sequence is:
- LPM pin goes high (PMIC enters Low-Power <-> Linux standby)
- SHDN goes low (if target suspend state is mem) and then PMIC enters
HIBERNATE


Best regards,
Andrei

2018-12-12 15:57:11

by Mark Brown

[permalink] [raw]
Subject: Re: [RESEND PATCH v3 3/3] regulator: mcp16502: add regulator driver for MCP16502

On Wed, Dec 12, 2018 at 08:01:07AM +0000, [email protected] wrote:

> > This puts the device into low power mode when the suspend function gets
> > called but this might not be safe - devices using the regulator may not
> > be suspended yet so could still need full regulation. Normally a GPIO
> > triggered transition like this would be being done by hardware as part
> > of the process of suspending the SoC. Is there some reason to do this
> > manually?

> There is a line from the MPU (SHDN) which goes low only when the MPU
> turns off. That line is already connected to the PMIC and it differentiates
> between suspend-to-mem and standby. To switch to low-power, the PMIC must
> be controlled by the GPIO pin LPM.

> The suspend sequence is:
> - LPM pin goes high (PMIC enters Low-Power <-> Linux standby)
> - SHDN goes low (if target suspend state is mem) and then PMIC enters
> HIBERNATE

This feels like it should be being controlled somewhere else, if it's
actually causing a change in the PMIC state it seems like it wants to be
done as late as possible in suspend to minimize the risks. At the very
least suspend_late() for the driver seems appropriate.

Could you submit a version with this feature at least split out into a
separate patch please so we can apply the rest of the code while this is
discussed?


Attachments:
(No filename) (1.33 kB)
signature.asc (499.00 B)
Download all attachments

2018-12-13 12:20:43

by Andrei.Stefanescu

[permalink] [raw]
Subject: Re: [RESEND PATCH v3 3/3] regulator: mcp16502: add regulator driver for MCP16502

Hi Mark,

On 12.12.2018 17:55, Mark Brown wrote:
> On Wed, Dec 12, 2018 at 08:01:07AM +0000, [email protected] wrote:
>
>>> This puts the device into low power mode when the suspend function gets
>>> called but this might not be safe - devices using the regulator may not
>>> be suspended yet so could still need full regulation. Normally a GPIO
>>> triggered transition like this would be being done by hardware as part
>>> of the process of suspending the SoC. Is there some reason to do this
>>> manually?
>> There is a line from the MPU (SHDN) which goes low only when the MPU
>> turns off. That line is already connected to the PMIC and it differentiates
>> between suspend-to-mem and standby. To switch to low-power, the PMIC must
>> be controlled by the GPIO pin LPM.
>> The suspend sequence is:
>> - LPM pin goes high (PMIC enters Low-Power <-> Linux standby)
>> - SHDN goes low (if target suspend state is mem) and then PMIC enters
>> HIBERNATE
> This feels like it should be being controlled somewhere else, if it's
> actually causing a change in the PMIC state it seems like it wants to be
> done as late as possible in suspend to minimize the risks. At the very
> least suspend_late() for the driver seems appropriate.
I tried with both suspend_late/resume_early and suspend_noirq/resume_noirq.
Everything seems to work ok.

Which one do you think is more appropriate? I am guessing that during
suspend_late some devices may still need to be turned on and that it would
be the safest during suspend_noirq.

Note: the GPIO pin used for LPM is a special one (it maintains its
voltage during
suspend-to-mem). It is part of the SoC and its methods for setting the
values
do not sleep.

>
> Could you submit a version with this feature at least split out into a
> separate patch please so we can apply the rest of the code while this is
> discussed?
Sent one patch series and another patch with the current suspend/resume
implementation.

Best regards,
Andrei

2018-12-13 16:24:24

by Mark Brown

[permalink] [raw]
Subject: Re: [RESEND PATCH v3 3/3] regulator: mcp16502: add regulator driver for MCP16502

On Thu, Dec 13, 2018 at 12:19:16PM +0000, [email protected] wrote:

> I tried with both suspend_late/resume_early and suspend_noirq/resume_noirq.
> Everything seems to work ok.

> Which one do you think is more appropriate? I am guessing that during
> suspend_late some devices may still need to be turned on and that it would
> be the safest during suspend_noirq.

I agree, noirq is going to be the safest option. Can you respin with
that please?


Attachments:
(No filename) (474.00 B)
signature.asc (499.00 B)
Download all attachments