[PATCH v2 0/2] Add Richtek RTQ2208 SubPMIC support

[PATCH v2 0/2] Add Richtek RTQ2208 SubPMIC support

[alina_yu]

From: alinayu <alina_yu@richtek.com>

This driver adds support for RTQ2208 SubPMIC regulators.        
The RTQ2208 is a multi-phase, programmable power management IC that
integrate with dual multi-configurable, synchronous buck converters
and two ldos. The bucks features wide output voltage range from 0.4V to 2.05V
and the capability to configure the corresponding power stages.


alinayu (2):
  regulator: dt-bindings: rtq2208: Add Richtek RTQ2208 SubPMIC
  regulator: rtq2208: Add Richtek RTQ2208 SubPMIC driver

 .../regulator/richtek,rtq2208-regulator.yaml       | 228 +++++++++
 drivers/regulator/Kconfig                          |  12 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/rtq2208-regulator.c              | 550 +++++++++++++++++++++
 4 files changed, 791 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rtq2208-regulator.yaml
 create mode 100644 drivers/regulator/rtq2208-regulator.c

-- 
2.7.4

[PATCH v2 1/2] regulator: dt-bindings: rtq2208: Add Richtek RTQ2208 SubPMIC

[alina_yu]

From: alinayu <alina_yu@richtek.com>

Add bindings for Richtek RTQ2208 IC controlled SubPMIC

Signed-off-by: alinayu <alina_yu@richtek.com>
---
 .../regulator/richtek,rtq2208-regulator.yaml       | 228 +++++++++++++++++++++
 1 file changed, 228 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rtq2208-regulator.yaml

diff --git a/Documentation/devicetree/bindings/regulator/richtek,rtq2208-regulator.yaml b/Documentation/devicetree/bindings/regulator/richtek,rtq2208-regulator.yaml
new file mode 100644
index 0000000..2a060ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/richtek,rtq2208-regulator.yaml
@@ -0,0 +1,228 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/richtek,rtq2208-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RTQ2208 SubPMIC Regulator
+
+maintainers:
+  - Alina Yu <alina_yu@richtek.com>
+
+description: |
+  The RTQ2208 is a highly integrated multi-configurable power converter that
+  offers functional safety embedded dual multi-configurable synchronous buck
+  converters and two LDOs.
+
+  Bucks support "regulator-allowed-modes" and "regulator-mode". The former defines the permitted
+  swiching operation in normal mode; the latter defines the operation in suspend to RAM mode.
+
+  No matter the RTQ2208 is configured in normal or suspend to RAM mode, there are two switching
+  operation modes for all buck rails, automic power saving mode (Auto mode) and forced continious
+  conduction mode (FCCM).
+
+  The meaning of modes is defined in the datasheet which is avaliabe in below link
+  and their meaning is::
+    0 - Auto mode for power saving, which reducing the switching frequency at light load condition
+    to maintain high frequency.
+    1 - FCCM to meet the strict voltage regulation accuracy, which keeping constant switching frequency.
+
+  Datasheet will be available soon at
+  https://www.richtek.com/assets/Products
+
+  The standard "regulator-mode" property can only be used for bucks that
+  changing their mode to suspend to RAM mode. Also, it only takes effect if the
+  regulator has been enabled for the given suspend state using "regulator-on-in-suspend".
+
+properties:
+  compatible:
+    enum:
+      - richtek,rtq2208
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+    
+  richtek,mtp-sel:
+    description: |
+      Buck and ldo vout selection is based on this value.
+      There are two independently programmable voltage settings named as mtp-sel0 and
+      mtp-sel1 for RTQ2208 bucks vout voltage. 0 which means this property isn't present
+      and 1 which means this property is present corresponds to different adjustable registers.
+
+      0 - DVS0 registers to adjust buck vout and BUCK_[A-H]_EN_NR_MTP_SEL0 register to en/disable vout.
+      1 - DVS1 registers to adjust buck vout and BUCK_[A-H]_EN_NR_MTP_SEL1 register to en/disable vout.
+
+    type: boolean
+
+  regulators:
+    type: object
+
+    patternProperties:
+      "^buck_[A-H]$":
+        type: object
+        $ref: regulator.yaml#
+        description: |
+          regulator description for buck[A-H].
+
+        properties:
+          regulator-compatible:
+            pattern: "^BUCK_[A-H]$"
+
+          regulator-allowed-modes:
+            description: |
+              describe buck permitted modes.
+
+      "^ldo[1-2]$":
+        type: object
+        $ref: regulator.yaml#
+        description: |
+          regulator description for ldo[1-2].
+
+        properties:
+          regulator-compatible:
+            pattern: "^LDO[1-2]$"
+
+          richtek,fixed_uV:
+            $ref: "/schemas/types.yaml#/definitions/uint32"
+            enum: [ 900000, 1200000, 1800000, 3300000 ]
+            description: |
+              the fixed voltage in microvolt which is descided at the factory.
+
+      regulator-state-(mem):
+        type: object
+        additionalProperties: true
+        properties:
+          regulator-on-in-suspend: false
+          regulator-mode: false
+
+required:
+  - compatible
+  - reg
+  - regulators
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      rtq2208@10 {
+        compatible = "richtek,rtq2208";
+        reg = <0x10>;
+        interrupts-extended = <&gpio26 0 IRQ_TYPE_LEVEL_LOW>;
+        richtek,mtp-sel;
+
+        regulators {
+         BUCK_A:buck_A {
+            regulator-compatible = "BUCK_A";
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         BUCK_B:buck_B {
+            regulator-compatible = "BUCK_B";
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         BUCK_C:buck_C {
+            regulator-compatible = "BUCK_C";
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         BUCK_D:buck_D {
+            regulator-compatible = "BUCK_D";
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         BUCK_E:buck_E {
+            regulator-compatible = "BUCK_E";
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         BUCK_F:buck_F {
+            regulator-compatible = "BUCK_F";
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         BUCK_G:buck_G {
+            regulator-compatible = "BUCK_G";
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         BUCK_H:buck_H {
+            regulator-compatible = "BUCK_H";
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         LDO1:ldo1 {
+            regulator-compatible = "LDO1";
+            richtek,fixed_uV = <1200000>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+            };
+          };
+         LDO2:ldo2 {
+            regulator-compatible = "LDO2";
+            regulator-always-on;
+            richtek,fixed_uV = <3300000>;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+            };
+          };
+        };
+      };
+    };
-- 
2.7.4

[PATCH v2 2/2] regulator: rtq2208: Add Richtek RTQ2208 SubPMIC driver

[alina_yu]

From: alinayu <alina_yu@richtek.com>

Add support for the RTQ2208 SubPMIC
This ic integrates with configurable, synchrnous buck converters and two ldos.

Signed-off-by: alinayu <alina_yu@richtek.com>
---
 drivers/regulator/Kconfig             |  12 +
 drivers/regulator/Makefile            |   1 +
 drivers/regulator/rtq2208-regulator.c | 550 ++++++++++++++++++++++++++++++++++
 3 files changed, 563 insertions(+)
 create mode 100644 drivers/regulator/rtq2208-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index e5f3613..e754034 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1213,6 +1213,18 @@ config REGULATOR_RTQ6752
 	  synchronous boost converters for PAVDD, and one synchronous NAVDD
 	  buck-boost. This device is suitable for automotive TFT-LCD panel.
 
+config REGULATOR_RTQ2208
+	tristate "Richtek RTQ2208 SubPMIC Regulator"
+	depends on I2C
+	select REGMAP_I2C
+	select RTGMAP_IRQ
+	help
+	  This driver adds support for RTQ2208 SubPMIC regulators.
+	  The RTQ2208 is a multi-phase, programmable power management IC that
+	  integrate with dual multi-configurable, synchronous buck converters
+	  and two ldos. It features wide output voltage range from 0.4V to 2.05V
+	  and the capability to configure the corresponding power stages.
+
 config REGULATOR_S2MPA01
 	tristate "Samsung S2MPA01 voltage regulator"
 	depends on MFD_SEC_CORE || COMPILE_TEST
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 58dfe01..04cbad17 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -143,6 +143,7 @@ obj-$(CONFIG_REGULATOR_RT6245)	+= rt6245-regulator.o
 obj-$(CONFIG_REGULATOR_RTMV20)	+= rtmv20-regulator.o
 obj-$(CONFIG_REGULATOR_RTQ2134) += rtq2134-regulator.o
 obj-$(CONFIG_REGULATOR_RTQ6752)	+= rtq6752-regulator.o
+obj-$(CONFIG_REGULATOR_RTQ2208) += rtq2208-regulator.o
 obj-$(CONFIG_REGULATOR_S2MPA01) += s2mpa01.o
 obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o
 obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
diff --git a/drivers/regulator/rtq2208-regulator.c b/drivers/regulator/rtq2208-regulator.c
new file mode 100644
index 0000000..22b6d90
--- /dev/null
+++ b/drivers/regulator/rtq2208-regulator.c
@@ -0,0 +1,550 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/bitops.h>
+#include <linux/util_macros.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+
+/* Register */
+#define RTQ2208_REG_GLOBAL_INT1			0x12
+#define RTQ2208_REG_FLT_RECORDBUCK_CB		0x18
+#define RTQ2208_REG_GLOBAL_INT1_MASK		0x1D
+#define RTQ2208_REG_FLT_MASKBUCK_CB		0x1F
+#define RTQ2208_REG_BUCK_C_CFG0			0x32
+#define RTQ2208_REG_BUCK_B_CFG0			0x42
+#define RTQ2208_REG_BUCK_A_CFG0			0x52
+#define RTQ2208_REG_BUCK_D_CFG0			0x62
+#define RTQ2208_REG_BUCK_G_CFG0			0x72
+#define RTQ2208_REG_BUCK_F_CFG0			0x82
+#define RTQ2208_REG_BUCK_E_CFG0			0x92
+#define RTQ2208_REG_BUCK_H_CFG0			0xA2
+#define RTQ2208_REG_LDO1_CFG			0xB1
+#define RTQ2208_REG_LDO2_CFG			0xC1
+
+/* Mask */
+#define RTQ2208_BUCK_NR_MTP_SEL_MASK		GENMASK(7, 0)
+#define RTQ2208_BUCK_EN_NR_MTP_SEL0_MASK	BIT(0)
+#define RTQ2208_BUCK_EN_NR_MTP_SEL1_MASK	BIT(1)
+#define RTQ2208_BUCK_RSPUP_MASK			GENMASK(6, 4)
+#define RTQ2208_BUCK_RSPDN_MASK			GENMASK(2, 0)
+#define RTQ2208_BUCK_NRMODE_MASK		BIT(5)
+#define RTQ2208_BUCK_STRMODE_MASK		BIT(5)
+#define RTQ2208_BUCK_EN_STR_MASK		BIT(0)
+#define RTQ2208_LDO_EN_STR_MASK			BIT(7)
+#define RTQ2208_EN_DIS_MASK			BIT(0)
+#define RTQ2208_RAMP_MASK			GENMASK(2, 0)
+
+/* Size */
+#define RTQ2208_VOUT_MAXNUM			256
+#define RTQ2208_BUCK_NUM_IRQ_REGS		5
+#define RTQ2208_STS_NUM_IRQ_REGS		2
+
+/* Value */
+#define RTQ2208_RAMP_VALUE_MIN			500
+#define RTQ2208_RAMP_VALUE_MAX			64000
+
+enum {
+	RTQ2208_BUCK_B = 0,
+	RTQ2208_BUCK_C,
+	RTQ2208_BUCK_D,
+	RTQ2208_BUCK_A,
+	RTQ2208_BUCK_F,
+	RTQ2208_BUCK_G,
+	RTQ2208_BUCK_H,
+	RTQ2208_BUCK_E,
+	RTQ2208_LDO2,
+	RTQ2208_LDO1,
+	RTQ2208_LDO_MAX,
+};
+
+enum {
+	RTQ2208_AUTO_MODE = 0,
+	RTQ2208_FCCM,
+};
+
+struct rtq2208_regulator_desc {
+	struct regulator_desc desc;
+	unsigned int mtp_sel_reg;
+	unsigned int mtp_sel_mask;
+	unsigned int mode_reg;
+	unsigned int mode_mask;
+	unsigned int suspend_config_reg;
+	unsigned int suspend_enable_mask;
+	unsigned int suspend_mode_mask;
+	unsigned int fixed_uV;
+};
+
+struct rtq2208_rdev_map {
+	struct regulator_dev *rdev[RTQ2208_LDO_MAX];
+	struct regmap *regmap;
+	struct device *dev;
+};
+
+/* set Normal Auto/FCCM mode */
+static int rtq2208_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+	struct rtq2208_regulator_desc *rdesc =
+		(struct rtq2208_regulator_desc *)rdev->desc;
+	unsigned int val, shift;
+
+	switch (mode) {
+	case REGULATOR_MODE_NORMAL:
+		val = RTQ2208_AUTO_MODE;
+		break;
+	case REGULATOR_MODE_FAST:
+		val = RTQ2208_FCCM;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	shift = ffs(rdesc->mode_mask) - 1;
+	return regmap_update_bits(rdev->regmap, rdesc->mode_reg,
+				  rdesc->mode_mask, val << shift);
+}
+
+static unsigned int rtq2208_get_mode(struct regulator_dev *rdev)
+{
+	struct rtq2208_regulator_desc *rdesc =
+		(struct rtq2208_regulator_desc *)rdev->desc;
+	unsigned int mode_val;
+	int ret;
+
+	ret = regmap_read(rdev->regmap, rdesc->mode_reg, &mode_val);
+	if (ret)
+		return REGULATOR_MODE_INVALID;
+
+	return (mode_val & rdesc->mode_mask) ? REGULATOR_MODE_FAST : REGULATOR_MODE_NORMAL;
+}
+
+static int rtq2208_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
+{
+	const struct regulator_desc *rdesc = rdev->desc;
+	unsigned int sel = 0, val;
+
+	ramp_delay = max(ramp_delay, RTQ2208_RAMP_VALUE_MIN);
+	ramp_delay = min(ramp_delay, RTQ2208_RAMP_VALUE_MAX);
+
+	ramp_delay /= RTQ2208_RAMP_VALUE_MIN;
+	sel = RTQ2208_RAMP_MASK - (fls(ramp_delay) - 1);
+
+	val = FIELD_PREP(RTQ2208_BUCK_RSPUP_MASK, sel) | FIELD_PREP(RTQ2208_BUCK_RSPDN_MASK, sel);
+
+	return regmap_update_bits(rdev->regmap, rdesc->ramp_reg,
+				  RTQ2208_BUCK_RSPUP_MASK | RTQ2208_BUCK_RSPDN_MASK, val);
+}
+
+static int rtq2208_set_suspend_enable(struct regulator_dev *rdev)
+{
+	struct rtq2208_regulator_desc *rdesc =
+		(struct rtq2208_regulator_desc *)rdev->desc;
+
+	return regmap_set_bits(rdev->regmap, rdesc->suspend_config_reg, rdesc->suspend_enable_mask);
+}
+
+static int rtq2208_set_suspend_disable(struct regulator_dev *rdev)
+{
+	struct rtq2208_regulator_desc *rdesc =
+		(struct rtq2208_regulator_desc *)rdev->desc;
+
+	return regmap_update_bits(rdev->regmap, rdesc->suspend_config_reg, rdesc->suspend_enable_mask, 0);
+}
+
+static int rtq2208_set_suspend_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+	struct rtq2208_regulator_desc *rdesc =
+		(struct rtq2208_regulator_desc *)rdev->desc;
+	unsigned int val, shift;
+
+	switch (mode) {
+	case REGULATOR_MODE_NORMAL:
+		val = RTQ2208_AUTO_MODE;
+		break;
+	case REGULATOR_MODE_FAST:
+		val = RTQ2208_FCCM;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	shift = ffs(rdesc->suspend_mode_mask) - 1;
+
+	return regmap_update_bits(rdev->regmap, rdesc->suspend_config_reg,
+			rdesc->suspend_mode_mask, val << shift);
+}
+
+static int rtq2208_ldo_get_voltage(struct regulator_dev *rdev)
+{
+	struct rtq2208_regulator_desc *rdesc =
+		(struct rtq2208_regulator_desc *)rdev->desc;
+
+	return rdesc->fixed_uV;
+}
+
+static const struct regulator_ops rtq2208_regulator_buck_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+	.list_voltage = regulator_list_voltage_linear_range,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_mode = rtq2208_set_mode,
+	.get_mode = rtq2208_get_mode,
+	.set_ramp_delay = rtq2208_set_ramp_delay,
+	.set_active_discharge = regulator_set_active_discharge_regmap,
+	.set_suspend_enable = rtq2208_set_suspend_enable,
+	.set_suspend_disable = rtq2208_set_suspend_disable,
+	.set_suspend_mode = rtq2208_set_suspend_mode,
+};
+
+static const struct regulator_ops rtq2208_regulator_ldo_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+	.set_active_discharge = regulator_set_active_discharge_regmap,
+	.set_suspend_enable = rtq2208_set_suspend_enable,
+	.set_suspend_disable = rtq2208_set_suspend_disable,
+	.get_voltage = rtq2208_ldo_get_voltage,
+};
+
+static int rtq2208_of_parse_cb(struct device_node *np,
+				const struct regulator_desc *desc,
+				struct regulator_config *config)
+{
+	struct rtq2208_regulator_desc *rdesc =
+		(struct rtq2208_regulator_desc *)desc;
+
+	if (desc->id != RTQ2208_LDO1 && desc->id != RTQ2208_LDO2)
+		return 0;
+
+	/* ldo voltage depends on factory setting */
+	return of_property_read_u32(np, "richtek,fixed_uV", &rdesc->fixed_uV);
+}
+
+static unsigned int rtq2208_of_map_mode(unsigned int mode)
+{
+	switch (mode) {
+	case RTQ2208_AUTO_MODE:
+		return REGULATOR_MODE_NORMAL;
+	case RTQ2208_FCCM:
+		return REGULATOR_MODE_FAST;
+	default:
+		return REGULATOR_MODE_INVALID;
+	}
+}
+
+static int rtq2208_init_alert_mask(struct rtq2208_rdev_map *rdev_map)
+{
+	struct regulator_dev *rdev;
+	const struct regulator_desc *desc;
+	unsigned int reg, mask, uv_irq, ov_irq;
+	unsigned char clr_mask[] = {0x33, 0x33, 0x33, 0x33, 0x33};
+	int i, ret;
+
+	/* write clear all irq once */
+	ret = regmap_bulk_write(rdev_map->regmap, RTQ2208_REG_FLT_RECORDBUCK_CB,
+				clr_mask, ARRAY_SIZE(clr_mask));
+	if (ret)
+		return dev_err_probe(rdev_map->dev, ret, "Failed to clr buck irqs\n");
+
+	/* write clear sts hd */
+	ret = regmap_update_bits(rdev_map->regmap, 0x1E, 0x01, 0x01);
+	if (ret)
+		return dev_err_probe(rdev_map->dev, ret, "Failed to clr sts hd irq\n");
+
+	/* unmask buck ov/uv irq */
+	for (i = 0; i < RTQ2208_LDO_MAX; i++) {
+		rdev = rdev_map->rdev[i];
+		if (!rdev)
+			continue;
+
+		desc = rdev[i].desc;
+		uv_irq = 4 * (desc->id);
+		ov_irq = 4 * (desc->id) + 1;
+		reg = RTQ2208_REG_FLT_MASKBUCK_CB + uv_irq / 8;
+		mask = uv_irq % 8 | ov_irq % 8;
+
+		ret = regmap_update_bits(rdev_map->regmap, reg, mask, 0x00);
+		if (ret)
+			return dev_err_probe(rdev_map->dev, ret, "Failed to unmask 0x%02x\n", reg);
+	}
+
+	/* unmask sts hd irq */
+	return regmap_update_bits(rdev_map->regmap, 0x1E, 0x01, 0x00);
+}
+
+static irqreturn_t rtq2208_buck_irq_handler(int irqno, void *devid)
+{
+	unsigned char buck_flags[RTQ2208_BUCK_NUM_IRQ_REGS], buck_masks[RTQ2208_BUCK_NUM_IRQ_REGS],
+			sts_flags[RTQ2208_STS_NUM_IRQ_REGS], sts_masks[RTQ2208_STS_NUM_IRQ_REGS];
+	int ret = 0, i, j, irq, mask, idx;
+	struct rtq2208_rdev_map *rdev_map = devid;
+	struct regulator_dev *rdev;
+
+	if (!rdev_map)
+		return IRQ_NONE;
+
+	ret = regmap_bulk_read(rdev_map->regmap, RTQ2208_REG_FLT_RECORDBUCK_CB,
+				buck_flags, ARRAY_SIZE(buck_flags));
+	if (ret)
+		return IRQ_NONE;
+
+	ret = regmap_bulk_read(rdev_map->regmap, RTQ2208_REG_GLOBAL_INT1,
+				sts_flags, ARRAY_SIZE(sts_flags));
+	if (ret)
+		goto out;
+
+	ret = regmap_bulk_read(rdev_map->regmap, RTQ2208_REG_FLT_MASKBUCK_CB,
+				buck_masks, ARRAY_SIZE(buck_masks));
+	if (ret)
+		goto out;
+
+	ret = regmap_bulk_read(rdev_map->regmap, RTQ2208_REG_GLOBAL_INT1_MASK,
+				sts_masks, ARRAY_SIZE(sts_masks));
+	if (ret)
+		goto out;
+
+	for (i = 0; i < RTQ2208_BUCK_NUM_IRQ_REGS; i++) {
+		buck_flags[i] &= buck_masks[i];
+		if (!buck_flags[i])
+			continue;
+
+		for (j = 0; j < 8; j++) {
+			irq = i * 8 + j;
+			mask = 1 << j;
+			idx = irq / 4;
+
+			rdev = rdev_map->rdev[idx];
+
+			if (mask & buck_flags[i] && rdev) {
+				/* uv irq */
+				if (irq % 4 == 0)
+					regulator_notifier_call_chain(rdev,
+							REGULATOR_EVENT_UNDER_VOLTAGE, NULL);
+				/* ov irq */
+				else if (irq % 4 == 1)
+					regulator_notifier_call_chain(rdev,
+							REGULATOR_EVENT_REGULATION_OUT, NULL);
+			}
+		}
+	}
+
+	for (i = 0; i < RTQ2208_STS_NUM_IRQ_REGS; i++) {
+		sts_flags[i] &= sts_masks[i];
+
+		/* sts hd_irq */
+		if (i == 1 && (sts_flags[i] & BIT(0))) {
+			for (j = 0; j < RTQ2208_LDO_MAX; j++) {
+				rdev = rdev_map->rdev[j];
+				if (!rdev)
+					continue;
+				regulator_notifier_call_chain(rdev, REGULATOR_EVENT_OVER_TEMP, NULL);
+			}
+		}
+	}
+
+out:
+	/* write clear irqs */
+	regmap_bulk_write(rdev_map->regmap, RTQ2208_REG_FLT_RECORDBUCK_CB,
+				buck_flags, ARRAY_SIZE(buck_flags));
+
+	regmap_bulk_write(rdev_map->regmap, RTQ2208_REG_GLOBAL_INT1,
+				sts_flags, ARRAY_SIZE(sts_flags));
+
+	return ret ? IRQ_NONE : IRQ_HANDLED;
+}
+
+#define RTQ2208_REGULATOR_INFO(_name, _base) \
+{ \
+	.name = #_name, \
+	.base = _base, \
+}
+
+#define BUCK_RG_BASE(_id)	RTQ2208_REG_BUCK_##_id##_CFG0
+#define BUCK_RG_SHIFT(_base, _shift)	(_base + _shift)
+#define LDO_RG_BASE(_id)	RTQ2208_REG_LDO##_id##_CFG
+#define LDO_RG_SHIFT(_base, _shift)	(_base + _shift)
+#define	VSEL_SHIFT(_sel)	(_sel ? 3 : 1)
+#define MTP_SEL_MASK(_sel)	RTQ2208_BUCK_EN_NR_MTP_SEL##_sel##_MASK
+
+static const struct linear_range rtq2208_vout_range[] = {
+	REGULATOR_LINEAR_RANGE(400000, 0, 180, 5000),
+	REGULATOR_LINEAR_RANGE(1310000, 181, 255, 10000),
+};
+
+static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, int mtp_sel, int idx)
+{
+	struct regulator_desc *desc;
+	static const struct {
+		char *name;
+		int base;
+	} regulator_info[] = {
+		RTQ2208_REGULATOR_INFO(BUCK_B, BUCK_RG_BASE(B)),
+		RTQ2208_REGULATOR_INFO(BUCK_C, BUCK_RG_BASE(C)),
+		RTQ2208_REGULATOR_INFO(BUCK_D, BUCK_RG_BASE(D)),
+		RTQ2208_REGULATOR_INFO(BUCK_A, BUCK_RG_BASE(A)),
+		RTQ2208_REGULATOR_INFO(BUCK_F, BUCK_RG_BASE(F)),
+		RTQ2208_REGULATOR_INFO(BUCK_G, BUCK_RG_BASE(G)),
+		RTQ2208_REGULATOR_INFO(BUCK_H, BUCK_RG_BASE(H)),
+		RTQ2208_REGULATOR_INFO(BUCK_E, BUCK_RG_BASE(E)),
+		RTQ2208_REGULATOR_INFO(LDO2, LDO_RG_BASE(2)),
+		RTQ2208_REGULATOR_INFO(LDO1, LDO_RG_BASE(1)),
+	}, *curr_info;
+
+	curr_info = regulator_info + idx;
+	desc = &rdesc->desc;
+	desc->name = curr_info->name;
+	desc->of_match = of_match_ptr(curr_info->name);
+	desc->id = idx;
+	desc->owner = THIS_MODULE;
+	desc->type = REGULATOR_VOLTAGE;
+	desc->enable_mask = mtp_sel ? MTP_SEL_MASK(1) : MTP_SEL_MASK(0);
+	desc->active_discharge_on = RTQ2208_EN_DIS_MASK;
+	desc->active_discharge_off = 0;
+	desc->active_discharge_mask = RTQ2208_EN_DIS_MASK;
+
+	rdesc->mode_mask = RTQ2208_BUCK_NRMODE_MASK;
+
+	if (idx >= RTQ2208_BUCK_B && idx <= RTQ2208_BUCK_E) {
+		/* init buck desc */
+		desc->enable_reg = BUCK_RG_SHIFT(curr_info->base, 2);
+		desc->ops = &rtq2208_regulator_buck_ops;
+		desc->vsel_reg = curr_info->base + VSEL_SHIFT(mtp_sel);
+		desc->vsel_mask = RTQ2208_BUCK_NR_MTP_SEL_MASK;
+		desc->n_voltages = RTQ2208_VOUT_MAXNUM;
+		desc->linear_ranges = rtq2208_vout_range;
+		desc->n_linear_ranges = ARRAY_SIZE(rtq2208_vout_range);
+		desc->ramp_reg = BUCK_RG_SHIFT(curr_info->base, 5);
+		desc->active_discharge_reg = curr_info->base;
+		desc->of_map_mode = rtq2208_of_map_mode;
+
+		rdesc->mode_reg = BUCK_RG_SHIFT(curr_info->base, 2);
+		rdesc->suspend_config_reg = BUCK_RG_SHIFT(curr_info->base, 4);
+		rdesc->suspend_enable_mask = RTQ2208_BUCK_EN_STR_MASK;
+		rdesc->suspend_mode_mask = RTQ2208_BUCK_STRMODE_MASK;
+	} else {
+		/* init ldo desc */
+		desc->enable_reg = curr_info->base;
+		desc->ops = &rtq2208_regulator_ldo_ops;
+		desc->n_voltages = 1;
+		desc->active_discharge_reg = LDO_RG_SHIFT(curr_info->base, 2);
+		desc->of_parse_cb = rtq2208_of_parse_cb;
+
+		rdesc->suspend_config_reg = curr_info->base;
+		rdesc->suspend_enable_mask = RTQ2208_LDO_EN_STR_MASK;
+	}
+}
+
+/** different slave address corresponds different used bucks
+ * slave address 0x10: BUCK[BCA FGE]
+ * slave address 0x20: BUCK[BC FGHE]
+ * slave address 0x40: BUCK[C G]
+ */
+static int rtq2208_regulator_check(int slave_addr, int *num, int *regulator_idx_table)
+{
+	static bool rtq2208_used_table[3][RTQ2208_LDO_MAX] = {
+		/* BUCK[BCA FGE], LDO[12] */
+		{1, 1, 0, 1, 1, 1, 0, 1, 1, 1},
+		/* BUCK[BC FGHE], LDO[12]*/
+		{1, 1, 0, 0, 1, 1, 1, 1, 1, 1},
+		/* BUCK[C G], LDO[12] */
+		{0, 1, 0, 0, 0, 1, 0, 0, 1, 1},
+	};
+	int i, idx = ffs(slave_addr >> 4) - 1;
+
+	for (i = 0; i < RTQ2208_LDO_MAX; i++) {
+		if (!rtq2208_used_table[idx][i])
+			continue;
+
+		regulator_idx_table[(*num)++] = i;
+	}
+
+	return 0;
+}
+
+static const struct regmap_config rtq2208_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0xEF,
+};
+
+static int rtq2208_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct regmap *regmap;
+	struct rtq2208_regulator_desc *rdesc;
+	struct regulator_dev *rdev;
+	struct regulator_config cfg;
+	struct rtq2208_rdev_map *rdev_map;
+	int i, ret = 0, idx, mtp_sel, n_regulator = 0, regulator_idx_table[RTQ2208_LDO_MAX];
+
+	rdev_map = devm_kzalloc(dev, sizeof(struct rtq2208_rdev_map), GFP_KERNEL);
+	if (!rdev_map)
+		return -ENOMEM;
+
+	/* get mtp_sel0 or mtp_sel1 */
+	mtp_sel = device_property_read_bool(dev, "richtek,mtp-sel");
+
+	regmap = devm_regmap_init_i2c(i2c, &rtq2208_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap), "Failed to allocate regmap\n");
+
+	/* get needed regulator */
+	ret = rtq2208_regulator_check(i2c->addr, &n_regulator, regulator_idx_table);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to check used regulators\n");
+
+	rdev_map->regmap = regmap;
+	rdev_map->dev = dev;
+
+	cfg.dev = dev;
+	for (i = 0; i < n_regulator; i++) {
+		idx = regulator_idx_table[i];
+
+		rdesc = devm_kcalloc(dev, 1, sizeof(struct rtq2208_regulator_desc), GFP_KERNEL);
+		if (!rdesc)
+			return -ENOMEM;
+
+		/* init regulator desc */
+		rtq2208_init_regulator_desc(rdesc, mtp_sel, idx);
+
+		/* regiser regulator */
+		rdev = devm_regulator_register(dev, &rdesc->desc, &cfg);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+
+		rdev_map->rdev[idx] = rdev;
+	}
+
+	/* register interrupt */
+	ret = devm_request_threaded_irq(dev, i2c->irq, NULL, rtq2208_buck_irq_handler,
+					IRQF_ONESHOT, rdev_get_name(rdev), rdev_map);
+	if (ret)
+		return ret;
+
+	/* init interrupt mask */
+	ret = rtq2208_init_alert_mask(rdev_map);
+
+	return ret;
+}
+
+static const struct of_device_id __maybe_unused rtq2208_device_tables[] = {
+	{ .compatible = "richtek,rtq2208", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rtq2208_device_tables);
+
+static struct i2c_driver rtq2208_driver = {
+	.driver = {
+		.name = "rtq2208",
+		.of_match_table = rtq2208_device_tables,
+	},
+	.probe_new = rtq2208_probe,
+};
+module_i2c_driver(rtq2208_driver);
+
+MODULE_AUTHOR("Alina Yu <alina_yu@richtek.com>");
+MODULE_DESCRIPTION("Richtek RTQ2208 Regulator Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v2 1/2] regulator: dt-bindings: rtq2208: Add Richtek RTQ2208 SubPMIC

[Krzysztof Kozlowski]

On 05/07/2023 17:27, alina_yu@richtek.com wrote:
> From: alinayu <alina_yu@richtek.com>
> 
> Add bindings for Richtek RTQ2208 IC controlled SubPMIC

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least DT list (maybe more), so this won't be tested by our
tools. Performing review on untested code might be a waste of time, thus
I will skip this patch entirely till you follow the process allowing the
patch to be tested.

Please kindly resend and include all necessary To/Cc entries.

Limited review follows.

> 
> Signed-off-by: alinayu <alina_yu@richtek.com>
> ---
>  .../regulator/richtek,rtq2208-regulator.yaml       | 228 +++++++++++++++++++++
>  1 file changed, 228 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rtq2208-regulator.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/richtek,rtq2208-regulator.yaml b/Documentation/devicetree/bindings/regulator/richtek,rtq2208-regulator.yaml
> new file mode 100644
> index 0000000..2a060ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/richtek,rtq2208-regulator.yaml

Filename like compatible.

> @@ -0,0 +1,228 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/richtek,rtq2208-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Richtek RTQ2208 SubPMIC Regulator
> +
> +maintainers:
> +  - Alina Yu <alina_yu@richtek.com>
> +
> +description: |
> +  The RTQ2208 is a highly integrated multi-configurable power converter that
> +  offers functional safety embedded dual multi-configurable synchronous buck
> +  converters and two LDOs.
> +
> +  Bucks support "regulator-allowed-modes" and "regulator-mode". The former defines the permitted
> +  swiching operation in normal mode; the latter defines the operation in suspend to RAM mode.
> +
> +  No matter the RTQ2208 is configured in normal or suspend to RAM mode, there are two switching
> +  operation modes for all buck rails, automic power saving mode (Auto mode) and forced continious
> +  conduction mode (FCCM).
> +
> +  The meaning of modes is defined in the datasheet which is avaliabe in below link

You have several typos here and before. Please run spellcheck.

> +  and their meaning is::
> +    0 - Auto mode for power saving, which reducing the switching frequency at light load condition
> +    to maintain high frequency.
> +    1 - FCCM to meet the strict voltage regulation accuracy, which keeping constant switching frequency.
> +
> +  Datasheet will be available soon at
> +  https://www.richtek.com/assets/Products
> +
> +  The standard "regulator-mode" property can only be used for bucks that
> +  changing their mode to suspend to RAM mode. Also, it only takes effect if the
> +  regulator has been enabled for the given suspend state using "regulator-on-in-suspend".
> +
> +properties:
> +  compatible:
> +    enum:
> +      - richtek,rtq2208
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +    
> +  richtek,mtp-sel:
> +    description: |
> +      Buck and ldo vout selection is based on this value.
> +      There are two independently programmable voltage settings named as mtp-sel0 and
> +      mtp-sel1 for RTQ2208 bucks vout voltage. 0 which means this property isn't present
> +      and 1 which means this property is present corresponds to different adjustable registers.
> +
> +      0 - DVS0 registers to adjust buck vout and BUCK_[A-H]_EN_NR_MTP_SEL0 register to en/disable vout.
> +      1 - DVS1 registers to adjust buck vout and BUCK_[A-H]_EN_NR_MTP_SEL1 register to en/disable vout.

I read it three times and still don't understand. This is bool, not 0/1,
so are these "0" refer to DVS0 or to presence of the property? Maybe
Mark will understand it, I don't get it.

> +

No blank line. Just put type before description.

> +    type: boolean
> +
> +  regulators:
> +    type: object
> +
> +    patternProperties:
> +      "^buck_[A-H]$":

No underscores in node names. Lowercase names.

> +        type: object
> +        $ref: regulator.yaml#

Missing unevaluatedProperties: false

> +        description: |
> +          regulator description for buck[A-H].
> +
> +        properties:
> +          regulator-compatible:
> +            pattern: "^BUCK_[A-H]$"

Drop the property.

> +
> +          regulator-allowed-modes:
> +            description: |

Do not need '|' unless you need to preserve formatting.

> +              describe buck permitted modes.

Exactly: describe them

> +
> +      "^ldo[1-2]$":
> +        type: object
> +        $ref: regulator.yaml#
> +        description: |
> +          regulator description for ldo[1-2].
> +
> +        properties:
> +          regulator-compatible:
> +            pattern: "^LDO[1-2]$"
> +
> +          richtek,fixed_uV:
> +            $ref: "/schemas/types.yaml#/definitions/uint32"
> +            enum: [ 900000, 1200000, 1800000, 3300000 ]
> +            description: |
> +              the fixed voltage in microvolt which is descided at the factory.
> +
> +      regulator-state-(mem):

That's not a pattern.

> +        type: object
> +        additionalProperties: true

Why?

> +        properties:
> +          regulator-on-in-suspend: false
> +          regulator-mode: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - regulators
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      rtq2208@10 {

Node names should be generic. See also explanation and list of examples
in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +        compatible = "richtek,rtq2208";
> +        reg = <0x10>;
> +        interrupts-extended = <&gpio26 0 IRQ_TYPE_LEVEL_LOW>;
> +        richtek,mtp-sel;
> +
> +        regulators {
> +         BUCK_A:buck_A {

Drop labels.


Best regards,
Krzysztof

Re: [PATCH v2 1/2] regulator: dt-bindings: rtq2208: Add Richtek RTQ2208 SubPMIC

[Alina Yu]

On Wed, Jul 05, 2023 at 07:58:53PM +0200, Krzysztof Kozlowski wrote:
> On 05/07/2023 17:27, alina_yu@richtek.com wrote:
> > From: alinayu <alina_yu@richtek.com>
> > 
> > Add bindings for Richtek RTQ2208 IC controlled SubPMIC
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC.  It might happen, that command when run on an older
> kernel, gives you outdated entries.  Therefore please be sure you base
> your patches on recent Linux kernel.
> 
> You missed at least DT list (maybe more), so this won't be tested by our
> tools. Performing review on untested code might be a waste of time, thus
> I will skip this patch entirely till you follow the process allowing the
> patch to be tested.
> 
> Please kindly resend and include all necessary To/Cc entries.
> 
> Limited review follows.
> 

Sorry, I will add all necessary people to CC list to v3.

...

> > +      Buck and ldo vout selection is based on this value.
> > +      There are two independently programmable voltage settings named as mtp-sel0 and
> > +      mtp-sel1 for RTQ2208 bucks vout voltage. 0 which means this property isn't present
> > +      and 1 which means this property is present corresponds to different adjustable registers.
> > +
> > +      0 - DVS0 registers to adjust buck vout and BUCK_[A-H]_EN_NR_MTP_SEL0 register to en/disable vout.
> > +      1 - DVS1 registers to adjust buck vout and BUCK_[A-H]_EN_NR_MTP_SEL1 register to en/disable vout.
> 
> I read it three times and still don't understand. This is bool, not 0/1,
> so are these "0" refer to DVS0 or to presence of the property? Maybe
> Mark will understand it, I don't get it.
> 

Yes, "0" refers to DVS0 registers, and "1" refers to DVS1.
and there is only DVS0 and DVS1, so I use boolean to check which one is used.

Is it more understandable if I modify that to enum ? And description will be like this

richtek,mtp-sel:
  enum: [0, 1]
    description: |
      vout register selection besed on this value.
      0 - Using DVS0 register setting to adjust vout
      1 - Using DVS1 register setting to adjust vout

...

> > +
> > +      regulator-state-(mem):
> 
> That's not a pattern.
>

Should I revise that like this ?

patternProperties:
  "^regulator-state-mem$":


> > +        type: object
> > +        additionalProperties: true
> 
> Why?

Does "additionalProperties: true" mean I need to define my own property ?
If yes, I misunderstood additionalProperties as properties like "regulator-on-in-suspend" or "regulator-mode".

> 
> > +        properties:
> > +          regulator-on-in-suspend: false
> > +          regulator-mode: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - regulators
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      rtq2208@10 {
> 
> Node names should be generic. See also explanation and list of examples
> in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 

I'll modify the node name to 

regulator@10

in v3

> > +        compatible = "richtek,rtq2208";
> > +        reg = <0x10>;
> > +        interrupts-extended = <&gpio26 0 IRQ_TYPE_LEVEL_LOW>;
> > +        richtek,mtp-sel;
> > +
> > +        regulators {
> > +         BUCK_A:buck_A {
> 
> Drop labels.
> 
> 

BR,
Alina

Re: [PATCH v2 1/2] regulator: dt-bindings: rtq2208: Add Richtek RTQ2208 SubPMIC

[Krzysztof Kozlowski]

On 06/07/2023 12:30, Alina Yu wrote:
>>> +
>>> +      regulator-state-(mem):
>>
>> That's not a pattern.
>>
> 
> Should I revise that like this ?
> 
> patternProperties:
>   "^regulator-state-mem$":

It's still not a pattern, but a regular property, so keep it in properties.

I don't even understand what you want to say with this. You put it
outside of any regulator. I don't think this was tested at all. :(

> 
> 
>>> +        type: object
>>> +        additionalProperties: true
>>
>> Why?
> 
> Does "additionalProperties: true" mean I need to define my own property ?

No.

> If yes, I misunderstood additionalProperties as properties like "regulator-on-in-suspend" or "regulator-mode".

Please open recent regulator bindings and use them as example.

> 
>>
>>> +        properties:
>>> +          regulator-on-in-suspend: false
>>> +          regulator-mode: false
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - regulators
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>> +    i2c {
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +
>>> +      rtq2208@10 {
>>
>> Node names should be generic. See also explanation and list of examples
>> in DT specification:
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>>
> 
> I'll modify the node name to 
> 
> regulator@10


If this is PMIC, then "pmic" as name.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] regulator: dt-bindings: rtq2208: Add Richtek RTQ2208 SubPMIC

[Alina Yu]

Hi, Krzystof

On Thu, Jul 06, 2023 at 06:30:40PM +0800, Alina Yu wrote:
> On Wed, Jul 05, 2023 at 07:58:53PM +0200, Krzysztof Kozlowski wrote:
> > On 05/07/2023 17:27, alina_yu@richtek.com wrote:
> > > From: alinayu <alina_yu@richtek.com>
> > > 
> > > Add bindings for Richtek RTQ2208 IC controlled SubPMIC
> > 
> 
> ...
> 
> > > +      Buck and ldo vout selection is based on this value.
> > > +      There are two independently programmable voltage settings named as mtp-sel0 and
> > > +      mtp-sel1 for RTQ2208 bucks vout voltage. 0 which means this property isn't present
> > > +      and 1 which means this property is present corresponds to different adjustable registers.
> > > +
> > > +      0 - DVS0 registers to adjust buck vout and BUCK_[A-H]_EN_NR_MTP_SEL0 register to en/disable vout.
> > > +      1 - DVS1 registers to adjust buck vout and BUCK_[A-H]_EN_NR_MTP_SEL1 register to en/disable vout.
> > 
> > I read it three times and still don't understand. This is bool, not 0/1,
> > so are these "0" refer to DVS0 or to presence of the property? Maybe
> > Mark will understand it, I don't get it.
> > 
> 
> Yes, "0" refers to DVS0 registers, and "1" refers to DVS1.
> and there is only DVS0 and DVS1, so I use boolean to check which one is used.
> 
> Is it more understandable if I modify that to enum ? And description will be like this
> 
> richtek,mtp-sel:
>   enum: [0, 1]
>     description: |
>       vout register selection besed on this value.
>       0 - Using DVS0 register setting to adjust vout
>       1 - Using DVS1 register setting to adjust vout
> 

May I ask one more question ?
If I modify the name into "richtek,mtp-sel-high", is that more understandable ?
It will be like this,

richtek,mtp-sel-high:
  type: boolean
  description:
  vout register selection besed on this value.
  0 - Using DVS0 register setting to adjust vout
  1 - Using DVS1 register setting to adjust vout

> 
> > 
> > 
> 
> BR,
> Alina

Re: [PATCH v2 1/2] regulator: dt-bindings: rtq2208: Add Richtek RTQ2208 SubPMIC

[Krzysztof Kozlowski]

On 10/07/2023 05:08, Alina Yu wrote:
>> Yes, "0" refers to DVS0 registers, and "1" refers to DVS1.
>> and there is only DVS0 and DVS1, so I use boolean to check which one is used.
>>
>> Is it more understandable if I modify that to enum ? And description will be like this
>>
>> richtek,mtp-sel:
>>   enum: [0, 1]
>>     description: |
>>       vout register selection besed on this value.
>>       0 - Using DVS0 register setting to adjust vout
>>       1 - Using DVS1 register setting to adjust vout
>>
> 
> May I ask one more question ?
> If I modify the name into "richtek,mtp-sel-high", is that more understandable ?
> It will be like this,
> 
> richtek,mtp-sel-high:
>   type: boolean
>   description:
>   vout register selection besed on this value.
>   0 - Using DVS0 register setting to adjust vout
>   1 - Using DVS1 register setting to adjust vout

You don't have 0 or 1 values in such case. The property can be bool, but
description is not good.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] regulator: dt-bindings: rtq2208: Add Richtek RTQ2208 SubPMIC

[Alina Yu]

On Mon, Jul 10, 2023 at 08:02:39AM +0200, Krzysztof Kozlowski wrote:
> On 10/07/2023 05:08, Alina Yu wrote:
> >> Yes, "0" refers to DVS0 registers, and "1" refers to DVS1.
> >> and there is only DVS0 and DVS1, so I use boolean to check which one is used.
> >>
> >> Is it more understandable if I modify that to enum ? And description will be like this
> >>
> >> richtek,mtp-sel:
> >>   enum: [0, 1]
> >>     description: |
> >>       vout register selection besed on this value.
> >>       0 - Using DVS0 register setting to adjust vout
> >>       1 - Using DVS1 register setting to adjust vout
> >>
> > 
> > May I ask one more question ?
> > If I modify the name into "richtek,mtp-sel-high", is that more understandable ?
> > It will be like this,
> > 
> > richtek,mtp-sel-high:
> >   type: boolean
> >   description:
> >   vout register selection besed on this value.
> >   0 - Using DVS0 register setting to adjust vout
> >   1 - Using DVS1 register setting to adjust vout
> 
> You don't have 0 or 1 values in such case. The property can be bool, but
> description is not good.
> 

May I modify the description like this ?

richtek,mtp-sel-high:
  type: boolean
  description:
  vout register selection besed on this boolean value.
  false - Using DVS0 register setting to adjust vout
  true - Using DVS1 register setting to adjust vout


BR,
Alina.

Re: [PATCH v2 2/2] regulator: rtq2208: Add Richtek RTQ2208 SubPMIC driver

[Alina Yu]

On Wed, Jul 05, 2023 at 11:27:09PM +0800, alina_yu@richtek.com wrote:
> From: alinayu <alina_yu@richtek.com>
> 
> Add support for the RTQ2208 SubPMIC
> This ic integrates with configurable, synchrnous buck converters and two ldos.
> 
> Signed-off-by: alinayu <alina_yu@richtek.com>
> ---
>  drivers/regulator/Kconfig             |  12 +
>  drivers/regulator/Makefile            |   1 +
>  drivers/regulator/rtq2208-regulator.c | 550 ++++++++++++++++++++++++++++++++++
>  3 files changed, 563 insertions(+)
>  create mode 100644 drivers/regulator/rtq2208-regulator.c
>
...
Sorry, not intend to ping, just want to check the current review status.
Is there any comment about rtq2208-regulator.c


Thanks, Alina

Re: [PATCH v2 2/2] regulator: rtq2208: Add Richtek RTQ2208 SubPMIC driver

[Mark Brown]

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

On Tue, Jul 18, 2023 at 04:40:28PM +0800, Alina Yu wrote:

> Sorry, not intend to ping, just want to check the current review status.
> Is there any comment about rtq2208-regulator.c

It's not in my queue any more so probably no but I really don't
remember, I guess there was some issue with the DT binding that I was
expecting a respin for.

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

Re: [PATCH v2 2/2] regulator: rtq2208: Add Richtek RTQ2208 SubPMIC driver

[Alina Yu]

Hi,
Mark

On Tue, Jul 18, 2023 at 03:16:54PM +0100, Mark Brown wrote:
> On Tue, Jul 18, 2023 at 04:40:28PM +0800, Alina Yu wrote:
> 
> > Sorry, not intend to ping, just want to check the current review status.
> > Is there any comment about rtq2208-regulator.c
> 
> It's not in my queue any more so probably no but I really don't
> remember, I guess there was some issue with the DT binding that I was
> expecting a respin for.


Thanks you for your reply,
I send v3 which including modification for dt-binging.


Best regards,
Alina