power: pmic/regulator: Add basic support for TPS65910

Texas Instrument's TPS65910 PMIC contains 3 buck DC-DC converts, one boost DC-DC converter and 8 LDOs. This patch implements driver model support for the TPS65910 PMIC and its regulators making the get/set API for regulator value/enable available.
This patch depends on the patch "am33xx: Add a function to query MPU voltage in uV" to build correctly. For boards relying on the DT include file tps65910.dtsi the v2 patch "power: extend prefix match to regulator-name property" and an appropriate regulator naming is also required.

Signed-off-by Felix Brack <[email protected]>

---

 drivers/power/pmic/Kconfig                   |   8 +
 drivers/power/pmic/Makefile                  |   1 +
 drivers/power/pmic/pmic_tps65910_dm.c        | 138 ++++++++
 drivers/power/regulator/Kconfig              |   7 +
 drivers/power/regulator/Makefile             |   1 +
 drivers/power/regulator/tps65910_regulator.c | 493 +++++++++++++++++++++++++++
 include/power/tps65910_pmic.h                | 130 +++++++
 7 files changed, 778 insertions(+)
 create mode 100644 drivers/power/pmic/pmic_tps65910_dm.c
 create mode 100644 drivers/power/regulator/tps65910_regulator.c
 create mode 100644 include/power/tps65910_pmic.h

drivers/power/pmic/Kconfig +0 -0

index e3f9e4d..5d49c93 100644
@@ -201,3 +201,11 @@ config POWER_MC34VR500
        The MC34VR500 is used in conjunction with the FSL T1 and LS1 series

drivers/power/pmic/Makefile +1 -0

index f7bdfa5..7d6c583 100644
@@ -19,6 +19,7 @@ obj-$(CONFIG_PMIC_RK8XX) += rk8xx.o
 obj-$(CONFIG_PMIC_RN5T567) += rn5t567.o
 obj-$(CONFIG_PMIC_TPS65090) += tps65090.o
 obj-$(CONFIG_PMIC_S5M8767) += s5m8767.o
+obj-$(CONFIG_DM_PMIC_TPS65910) += pmic_tps65910_dm.o
 obj-$(CONFIG_$(SPL_)PMIC_PALMAS) += palmas.o
 obj-$(CONFIG_$(SPL_)PMIC_LP873X) += lp873x.o
 obj-$(CONFIG_$(SPL_)PMIC_LP87565) += lp87565.o

drivers/power/pmic/pmic_tps65910_dm.c new file +34 -0

new file mode 100644
index 0000000..1410657
@@ -0,0 +1,138 @@
+/*
+ * Copyright (C) EETS GmbH, 2017, Felix Brack <[email protected]>
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <i2c.h>
+#include <power/pmic.h>
+#include <power/regulator.h>
+#include <power/tps65910_pmic.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static const struct pmic_child_info pmic_children_info[] = {
+       { .prefix = "ldo_", .driver = TPS65910_LDO_DRIVER },
+       { .prefix = "buck_", .driver = TPS65910_BUCK_DRIVER },
+       { .prefix = "boost_", .driver = TPS65910_BOOST_DRIVER },
+       { },
+};
+
+static const char * const supply_names[] = {
+       "vccio-supply",
+       "vcc1-supply",
+       "vcc2-supply",
+       "vcc3-supply",
+       "vcc4-supply",
+       "vcc5-supply",
+       "vcc6-supply",
+       "vcc7-supply",
+};
+
+static int pmic_tps65910_reg_count(struct udevice *dev)

drivers/power/regulator/Kconfig +0 -0

index c82a936..2d6a150 100644

drivers/power/regulator/Makefile +1 -0

index 18fb870..3eef297 100644
@@ -20,3 +20,4 @@ obj-$(CONFIG_REGULATOR_TPS65090) += tps65090_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_PALMAS) += palmas_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_LP873X) += lp873x_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_LP87565) += lp87565_regulator.o
+obj-$(CONFIG_DM_REGULATOR_TPS65910) += tps65910_regulator.o

drivers/power/regulator/tps65910_regulator.c new file +105 -0

new file mode 100644
index 0000000..d212b70
@@ -0,0 +1,493 @@
+/*
+ * Copyright (C) EETS GmbH, 2017, Felix Brack <[email protected]>
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <power/pmic.h>
+#include <power/regulator.h>
+#include <power/tps65910_pmic.h>
+
+#define VOUT_CHOICE_COUNT 4
+
+/*
+ * struct regulator_props - Properties of a LDO and VIO SMPS regulator
+ *
+ * All of these regulators allow setting one out of four output voltages.
+ * These output voltages are only achievable when supplying the regulator
+ * with a minimum input voltage.
+ *
+ * @vin_min[]: minimum supply input voltage in uV required to achieve the
+ *             corresponding vout[] voltage
+ * @vout[]:    regulator output voltage in uV
+ * @reg:       I2C register used to set regulator voltage
+ */
+struct regulator_props {
+       int vin_min[VOUT_CHOICE_COUNT];
+       int vout[VOUT_CHOICE_COUNT];
+       int reg;
+};
+
+static const struct regulator_props ldo_props_vdig1 = {
+       .vin_min = { 1700000, 2100000, 2700000, 3200000 },
+       .vout = { 1200000, 1500000, 1800000, 2700000 },
+       .reg = TPS65910_REG_VDIG1
+};
+
+static const struct regulator_props ldo_props_vdig2 = {
+       .vin_min = { 1700000, 1700000, 1700000, 2700000 },
+       .vout = { 1000000, 1100000, 1200000, 1800000 },
+       .reg = TPS65910_REG_VDIG2
+};
+
+static const struct regulator_props ldo_props_vpll = {
+       .vin_min = { 2700000, 2700000, 2700000, 3000000 },
+       .vout = { 1000000, 1100000, 1800000, 2500000 },
+       .reg = TPS65910_REG_VPLL
+};
+
+static const struct regulator_props ldo_props_vdac = {
+       .vin_min = { 2700000, 3000000, 3200000, 3200000 },
+       .vout = { 1800000, 2600000, 2800000, 2850000 },
+       .reg = TPS65910_REG_VDAC
+};
+
+static const struct regulator_props ldo_props_vaux1 = {
+       .vin_min = { 2700000, 3200000, 3200000, 3200000 },
+       .vout = { 1800000, 2500000, 2800000, 2850000 },
+       .reg = TPS65910_REG_VAUX1
+};
+
+static const struct regulator_props ldo_props_vaux2 = {
+       .vin_min = { 2700000, 3200000, 3200000, 3600000 },
+       .vout = { 1800000, 2800000, 2900000, 3300000 },
+       .reg = TPS65910_REG_VAUX2
+};
+
+static const struct regulator_props ldo_props_vaux33 = {
+       .vin_min = { 2700000, 2700000, 3200000, 3600000 },
+       .vout = { 1800000, 2000000, 2800000, 3300000 },
+       .reg = TPS65910_REG_VAUX33
+};
+
+static const struct regulator_props ldo_props_vmmc = {
+       .vin_min = { 2700000, 3200000, 3200000, 3600000 },
+       .vout = { 1800000, 2800000, 3000000, 3300000 },
+       .reg = TPS65910_REG_VMMC
+};
+
+static const struct regulator_props smps_props_vio = {
+       .vin_min = { 3200000, 3200000, 4000000, 4400000 },
+       .vout = { 1500000, 1800000, 2500000, 3300000 },
+       .reg = TPS65910_REG_VIO
+};
+
+static int get_ctrl_reg_from_unit_addr(const int unit_addr)
+{
+       switch (unit_addr) {
+       case TPS65910_UNIT_VRTC:
+               return TPS65910_REG_VRTC;
+       case TPS65910_UNIT_VIO:
+               return TPS65910_REG_VIO;
+       case TPS65910_UNIT_VDD1:
+               return TPS65910_REG_VDD1;
+       case TPS65910_UNIT_VDD2:
+               return TPS65910_REG_VDD2;
+       case TPS65910_UNIT_VDD3:
+               return TPS65910_REG_VDD3;
+       case TPS65910_UNIT_VDIG1:
+               return TPS65910_REG_VDIG1;
+       case TPS65910_UNIT_VDIG2:
+               return TPS65910_REG_VDIG2;
+       case TPS65910_UNIT_VPLL:
+               return TPS65910_REG_VPLL;

drivers/power/regulator/tps65910_regulator.c new file +127 -0

+{
+       int sel;
+       u8 val;
+       int vout = 0;
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+       int vin = pdata->supply;
+
+       val = pmic_reg_read(dev->parent, rgp->reg);
+       if (val < 0)
+               return val;
+       sel = (val & TPS65910_SEL_MASK) >> 2;
+       vout = (vin >= *(rgp->vin_min + sel)) ? *(rgp->vout + sel) : 0;
+       vout = ((val & TPS65910_SUPPLY_STATE_MASK) == 1) ? vout : 0;
+
+       return vout;
+}
+
+static int tps65910_ldo_get_value(struct udevice *dev)
+{
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+       int vin = pdata->supply;
+
+       switch (dev->driver_data) {
+       case TPS65910_UNIT_VRTC:
+               /* VRTC is fixed and can't be turned off */
+               return (vin >= 2500000) ? 1830000 : 0;
+       case TPS65910_UNIT_VDIG1:
+               return simple_regulator_get_value(dev, &ldo_props_vdig1);
+       case TPS65910_UNIT_VDIG2:
+               return simple_regulator_get_value(dev, &ldo_props_vdig2);
+       case TPS65910_UNIT_VPLL:
+               return simple_regulator_get_value(dev, &ldo_props_vpll);
+       case TPS65910_UNIT_VDAC:
+               return simple_regulator_get_value(dev, &ldo_props_vdac);
+       case TPS65910_UNIT_VAUX1:
+               return simple_regulator_get_value(dev, &ldo_props_vaux1);
+       case TPS65910_UNIT_VAUX2:
+               return simple_regulator_get_value(dev, &ldo_props_vaux2);
+       case TPS65910_UNIT_VAUX33:
+               return simple_regulator_get_value(dev, &ldo_props_vaux33);
+       case TPS65910_UNIT_VMMC:
+               return simple_regulator_get_value(dev, &ldo_props_vmmc);
+       }
+
+       return 0;
+}
+
+static int simple_regulator_set_value(struct udevice *dev,
+                                     const struct regulator_props *ldo,
+                                     int uV)
+{
+       u8 val;
+       int sel = 0;
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+
+       do {
+               /* we only allow exact voltage matches */
+               if (uV == *(ldo->vout + sel))
+                       break;
+       } while (++sel < VOUT_CHOICE_COUNT);
+       if (sel == VOUT_CHOICE_COUNT)
+               return -EINVAL;
+       if (pdata->supply < *(ldo->vin_min + sel))
+               return -EINVAL;
+
+       val = pmic_reg_read(dev->parent, ldo->reg);
+       if (val < 0)
+               return val;
+       val &= ~TPS65910_SEL_MASK;
+       val |= sel << 2;
+       return pmic_reg_write(dev->parent, ldo->reg, val);
+}
+
+static int tps65910_ldo_set_value(struct udevice *dev, int uV)
+{
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+       int vin = pdata->supply;
+
+       switch (dev->driver_data) {
+       case TPS65910_UNIT_VRTC:
+               /* VRTC is fixed to 1.83V and can't be turned off */
+               if (vin < 2500000)
+                       return -EINVAL;
+               return 0;
+       case TPS65910_UNIT_VDIG1:
+               return simple_regulator_set_value(dev, &ldo_props_vdig1, uV);
+       case TPS65910_UNIT_VDIG2:
+               return simple_regulator_set_value(dev, &ldo_props_vdig2, uV);
+       case TPS65910_UNIT_VPLL:
+               return simple_regulator_set_value(dev, &ldo_props_vpll, uV);
+       case TPS65910_UNIT_VDAC:
+               return simple_regulator_set_value(dev, &ldo_props_vdac, uV);
+       case TPS65910_UNIT_VAUX1:
+               return simple_regulator_set_value(dev, &ldo_props_vaux1, uV);
+       case TPS65910_UNIT_VAUX2:
+               return simple_regulator_set_value(dev, &ldo_props_vaux2, uV);
+       case TPS65910_UNIT_VAUX33:
+               return simple_regulator_set_value(dev, &ldo_props_vaux33, uV);
+       case TPS65910_UNIT_VMMC:
+               return simple_regulator_set_value(dev, &ldo_props_vmmc, uV);
+       }
+
+       return 0;
+}
+
+static int tps65910_get_enable(struct udevice *dev)
+{
+       int reg, ret;
+       u8 val;
+
+       reg = get_ctrl_reg_from_unit_addr(dev->driver_data);
+       if (reg < 0)
+               return reg;
+
+       ret = pmic_read(dev->parent, reg, &val, 1);
+       if (ret)
+               return ret;
+
+       /* bits 2:0 of regulator control register define state */
+       return ((val & TPS65910_SUPPLY_STATE_MASK) == 1);
+}
+
+static int tps65910_set_enable(struct udevice *dev, bool enable)
+{
+       int reg;
+       uint clr, set;
+

drivers/power/regulator/tps65910_regulator.c new file +86 -0

+       }
+       return pmic_clrsetbits(dev->parent, reg, clr, set);
+}
+
+static int tps65910_ldo_probe(struct udevice *dev)
+{
+       int ret = 0;
+       int unit = dev->driver_data;
+       struct tps65910_pdata *pmic_pdata = dev->parent->platdata;
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+
+       switch (unit) {
+       case TPS65910_UNIT_VRTC:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC7);
+               break;
+       case TPS65910_UNIT_VDIG1:
+       case TPS65910_UNIT_VDIG2:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC6);
+               break;
+       case TPS65910_UNIT_VPLL:
+       case TPS65910_UNIT_VDAC:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC5);
+               break;
+       case TPS65910_UNIT_VAUX1:
+       case TPS65910_UNIT_VAUX2:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC4);
+               break;
+       case TPS65910_UNIT_VAUX33:
+       case TPS65910_UNIT_VMMC:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC3);
+               break;
+       default:
+               ret = -ENXIO;
+       }
+
+       return ret;
+}
+
+static int buck_get_vdd1_vdd2_value(struct udevice *dev, int reg_vdd)
+{
+       int gain;
+       u8 val = pmic_reg_read(dev, reg_vdd);
+
+       if (val < 0)
+               return val;
+       gain = (val & TPS65910_GAIN_SEL_MASK) >> 6;
+       gain = (gain == 0) ? 1 : gain;
+       val = pmic_reg_read(dev, reg_vdd + 1);
+       if (val < 0)
+               return val;
+       if (val & TPS65910_VDD_SR_MASK)
+               /* use smart reflex value instead */
+               val = pmic_reg_read(dev, reg_vdd + 2);
+       if (val < 0)
+               return val;
+       return (562500 + (val & TPS65910_VDD_SEL_MASK) * 12500) * gain;
+}
+
+static int tps65910_buck_get_value(struct udevice *dev)
+{
+       int vout = 0;
+
+       switch (dev->driver_data) {
+       case TPS65910_UNIT_VIO:
+               vout = simple_regulator_get_value(dev, &smps_props_vio);
+               break;
+       case TPS65910_UNIT_VDD1:
+               vout = buck_get_vdd1_vdd2_value(dev->parent, TPS65910_REG_VDD1);
+               break;
+       case TPS65910_UNIT_VDD2:
+               vout = buck_get_vdd1_vdd2_value(dev->parent, TPS65910_REG_VDD2);
+               break;
+       }
+
+       return vout;
+}
+
+static int buck_set_vdd1_vdd2_value(struct udevice *dev, int uV)
+{
+       int ret, reg_vdd, gain;
+       u32 limit;
+       int val;
+
+       switch (dev->driver_data) {
+       case TPS65910_UNIT_VDD1:
+               reg_vdd = TPS65910_REG_VDD1;

drivers/power/regulator/tps65910_regulator.c new file +21 -0

+       if (ret) {
+               /* too dangerous without limit */
+               error("missing regulator-min-microvolt property for %s\n",
+                     dev->name);
+               return ret;
+       }
+       if (uV < limit) {
+               error("voltage %duV for %s too low\n",
+                     limit, dev->name);
+               return -EINVAL;
+       }
+       ret = ofnode_read_u32(dev->node, "regulator-max-microvolt", &limit);
+       if (ret) {
+               /* too dangerous without limit */
+               error("missing regulator-max-microvolt property for %s\n",
+                     dev->name);
+               return ret;
+       }
+       if (uV > limit) {
+               error("voltage %duV for %s too high\n",
+                     limit, dev->name);

drivers/power/regulator/tps65910_regulator.c new file +106 -0

+                *  any approximate value (for now)
+                */
+               return -EPERM;
+       val &= TPS65910_VDD_SEL_MASK;
+       ret = pmic_reg_write(dev->parent, reg_vdd + 1, val);
+       if (ret)
+               return ret;
+       return 0;
+}
+
+static int tps65910_buck_set_value(struct udevice *dev, int uV)
+{
+       if (dev->driver_data == TPS65910_UNIT_VIO)
+               return simple_regulator_set_value(dev, &smps_props_vio, uV);
+
+       return buck_set_vdd1_vdd2_value(dev, uV);
+}
+
+static int tps65910_buck_probe(struct udevice *dev)
+{
+       int ret = 0;
+       int unit = dev->driver_data;
+       struct tps65910_pdata *pmic_pdata = dev->parent->platdata;
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+
+       switch (unit) {
+       case TPS65910_UNIT_VIO:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCCIO);
+               break;
+       case TPS65910_UNIT_VDD1:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC1);
+               break;
+       case TPS65910_UNIT_VDD2:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC2);
+               break;
+       default:
+               ret = -ENXIO;
+       }
+       return ret;
+}
+
+static int tps65910_boost_get_value(struct udevice *dev)
+{
+       int vout;
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+
+       vout = (pdata->supply >= 3000000) ? 5000000 : 0;
+       return vout;
+}
+
+static int tps65910_boost_probe(struct udevice *dev)
+{
+       int unit = dev->driver_data;
+       struct tps65910_pdata *pmic_pdata = dev->parent->platdata;
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+
+       if (unit != TPS65910_UNIT_VDD3)
+               return -ENXIO;
+
+       pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC7);
+       return 0;
+}
+
+static const struct dm_regulator_ops tps65910_boost_ops = {
+       .get_value  = tps65910_boost_get_value,
+       .get_enable = tps65910_get_enable,
+       .set_enable = tps65910_set_enable,
+};
+
+U_BOOT_DRIVER(tps65910_boost) = {
+       .name = TPS65910_BOOST_DRIVER,
+       .id = UCLASS_REGULATOR,
+       .ops = &tps65910_boost_ops,
+       .probe = tps65910_boost_probe,
+       .platdata_auto_alloc_size = sizeof(struct tps65910_regulator_pdata),
+};
+
+static const struct dm_regulator_ops tps65910_buck_ops = {
+       .get_value  = tps65910_buck_get_value,
+       .set_value  = tps65910_buck_set_value,
+       .get_enable = tps65910_get_enable,
+       .set_enable = tps65910_set_enable,
+};
+
+U_BOOT_DRIVER(tps65910_buck) = {
+       .name = TPS65910_BUCK_DRIVER,
+       .id = UCLASS_REGULATOR,
+       .ops = &tps65910_buck_ops,
+       .probe = tps65910_buck_probe,
+       .platdata_auto_alloc_size = sizeof(struct tps65910_regulator_pdata),
+};
+
+static const struct dm_regulator_ops tps65910_ldo_ops = {
+       .get_value  = tps65910_ldo_get_value,
+       .set_value  = tps65910_ldo_set_value,
+       .get_enable = tps65910_get_enable,
+       .set_enable = tps65910_set_enable,
+};
+
+U_BOOT_DRIVER(tps65910_ldo) = {
+       .name = TPS65910_LDO_DRIVER,
+       .id = UCLASS_REGULATOR,
+       .ops = &tps65910_ldo_ops,
+       .probe = tps65910_ldo_probe,
+       .platdata_auto_alloc_size = sizeof(struct tps65910_regulator_pdata),
+};

include/power/tps65910_pmic.h new file +5 -0

new file mode 100644
index 0000000..23e031e
@@ -0,0 +1,130 @@
+/*
+ * Copyright (C) EETS GmbH, 2017, Felix Brack <[email protected]>
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */

include/power/tps65910_pmic.h new file +94 -0

+
+/* i2c registers */
+enum {
+       TPS65910_REG_RTC_SEC                    = 0x00,
+       TPS65910_REG_RTC_MIN,
+       TPS65910_REG_RTC_HOUR,
+       TPS65910_REG_RTC_DAY,
+       TPS65910_REG_RTC_MONTH,
+       TPS65910_REG_RTC_YEAR,
+       TPS65910_REG_RTC_WEEK,
+       TPS65910_REG_RTC_ALARM_SEC              = 0x08,
+       TPS65910_REG_RTC_ALARM_MIN,
+       TPS65910_REG_RTC_ALARM_HOUR,
+       TPS65910_REG_RTC_ALARM_DAY,
+       TPS65910_REG_RTC_ALARM_MONTH,
+       TPS65910_REG_RTC_ALARM_YEAR,
+       TPS65910_REG_RTC_CTRL                   = 0x10,
+       TPS65910_REG_RTC_STAT,
+       TPS65910_REG_RTC_INT,
+       TPS65910_REG_RTC_COMP_LSB,
+       TPS65910_REG_RTC_COMP_MSB,
+       TPS65910_REG_RTC_RESISTOR_PRG,
+       TPS65910_REG_RTC_RESET_STAT,
+       TPS65910_REG_BACKUP1,
+       TPS65910_REG_BACKUP2,
+       TPS65910_REG_BACKUP3,
+       TPS65910_REG_BACKUP4,
+       TPS65910_REG_BACKUP5,
+       TPS65910_REG_PUADEN,
+       TPS65910_REG_REF,
+       TPS65910_REG_VRTC,
+       TPS65910_REG_VIO                        = 0x20,
+       TPS65910_REG_VDD1,
+       TPS65910_REG_VDD1_VAL,
+       TPS65910_REG_VDD1_VAL_SR,
+       TPS65910_REG_VDD2,
+       TPS65910_REG_VDD2_VAL,
+       TPS65910_REG_VDD2_VAL_SR,
+       TPS65910_REG_VDD3,
+       TPS65910_REG_VDIG1                      = 0x30,
+       TPS65910_REG_VDIG2,
+       TPS65910_REG_VAUX1,
+       TPS65910_REG_VAUX2,
+       TPS65910_REG_VAUX33,
+       TPS65910_REG_VMMC,
+       TPS65910_REG_VPLL,
+       TPS65910_REG_VDAC,
+       TPS65910_REG_THERM,
+       TPS65910_REG_BATTERY_BACKUP_CHARGE,
+       TPS65910_REG_DCDC_CTRL                  = 0x3e,
+       TPS65910_REG_DEVICE_CTRL,
+       TPS65910_REG_DEVICE_CTRL2,
+       TPS65910_REG_SLEEP_KEEP_LDO_ON,
+       TPS65910_REG_SLEEP_KEEP_RES_ON,
+       TPS65910_REG_SLEEP_SET_LDO_OFF,
+       TPS65910_REG_SLEEP_SET_RES_OFF,
+       TPS65910_REG_EN1_LDO_ASS,
+       TPS65910_REG_EM1_SMPS_ASS,
+       TPS65910_REG_EN2_LDO_ASS,
+       TPS65910_REG_EM2_SMPS_ASS,
+       TPS65910_REG_INT_STAT                   = 0x50,
+       TPS65910_REG_INT_MASK,
+       TPS65910_REG_INT_STAT2,
+       TPS65910_REG_INT_MASK2,
+       TPS65910_REG_GPIO                       = 0x60,
+       TPS65910_REG_JTAGREVNUM                 = 0x80,
+       TPS65910_NUM_REGS
+};
+
+/* chip supplies */
+enum {
+       TPS65910_SUPPLY_VCCIO   = 0x00,
+       TPS65910_SUPPLY_VCC1,
+       TPS65910_SUPPLY_VCC2,
+       TPS65910_SUPPLY_VCC3,
+       TPS65910_SUPPLY_VCC4,
+       TPS65910_SUPPLY_VCC5,
+       TPS65910_SUPPLY_VCC6,
+       TPS65910_SUPPLY_VCC7,
+       TPS65910_NUM_SUPPLIES
+};
+
+/* regulator unit numbers */
+enum {
+       TPS65910_UNIT_VRTC = 0x00,
+       TPS65910_UNIT_VIO,
+       TPS65910_UNIT_VDD1,
+       TPS65910_UNIT_VDD2,
+       TPS65910_UNIT_VDD3,
+       TPS65910_UNIT_VDIG1,
+       TPS65910_UNIT_VDIG2,
+       TPS65910_UNIT_VPLL,
+       TPS65910_UNIT_VDAC,
+       TPS65910_UNIT_VAUX1,

include/power/tps65910_pmic.h new file +11 -1

+
+struct tps65910_regulator_pdata {
+       u32 supply;    /* regulator supply voltage in uV */
+};
+
+/* driver names */
+#define TPS65910_BUCK_DRIVER "tps65910_buck"
+#define TPS65910_BOOST_DRIVER "tps65910_boost"
+#define TPS65910_LDO_DRIVER "tps65910_ldo"
+
+ #endif /* __TPS65910_PMIC_H_ */
--

2.7.4

Hi Felix,

Quoted 31 lines · Felix Brack

Texas Instrument's TPS65910 PMIC contains 3 buck DC-DC converts, one boost DC-DC converter and 8 LDOs. This patch implements driver model support for the TPS65910 PMIC and its regulators making the get/set API for regulator value/enable available.
This patch depends on the patch "am33xx: Add a function to query MPU voltage in uV" to build correctly. For boards relying on the DT include file tps65910.dtsi the v2 patch "power: extend prefix match to regulator-name property" and an appropriate regulator naming is also required.

Signed-off-by Felix Brack <[email protected]>

---

 drivers/power/pmic/Kconfig                   |   8 +
 drivers/power/pmic/Makefile                  |   1 +
 drivers/power/pmic/pmic_tps65910_dm.c        | 138 ++++++++
 drivers/power/regulator/Kconfig              |   7 +
 drivers/power/regulator/Makefile             |   1 +
 drivers/power/regulator/tps65910_regulator.c | 493 +++++++++++++++++++++++++++
 include/power/tps65910_pmic.h                | 130 +++++++
 7 files changed, 778 insertions(+)
 create mode 100644 drivers/power/pmic/pmic_tps65910_dm.c
 create mode 100644 drivers/power/regulator/tps65910_regulator.c
 create mode 100644 include/power/tps65910_pmic.h

drivers/power/pmic/Kconfig +0 -0

index e3f9e4d..5d49c93 100644
@@ -201,3 +201,11 @@ config POWER_MC34VR500
        The MC34VR500 is used in conjunction with the FSL T1 and LS1 series

drivers/power/pmic/Kconfig config POWER_MC34VR500 · line 211

Quoted 10 lines

drivers/power/pmic/Kconfig +8 -0

        SoC. It provides 4 buck DC-DC convertors and 5 LDOs, and it is accessed
        via an I2C interface.
+
+config DM_PMIC_TPS65910
+       bool "Enable driver for Texas Instruments TPS65910 PMIC"
+       depends on DM_PMIC
+       ---help---
+       The TPS65910 is a PMIC containing 3 buck DC-DC converters, one boost
+       DC-DC converter, 8 LDOs and a RTC. This driver binds the SMPS and LDO
+       pmic children.

Great!

Quoted 52 lines

drivers/power/pmic/Makefile +1 -0

index f7bdfa5..7d6c583 100644
@@ -19,6 +19,7 @@ obj-$(CONFIG_PMIC_RK8XX) += rk8xx.o
 obj-$(CONFIG_PMIC_RN5T567) += rn5t567.o
 obj-$(CONFIG_PMIC_TPS65090) += tps65090.o
 obj-$(CONFIG_PMIC_S5M8767) += s5m8767.o
+obj-$(CONFIG_DM_PMIC_TPS65910) += pmic_tps65910_dm.o
 obj-$(CONFIG_$(SPL_)PMIC_PALMAS) += palmas.o
 obj-$(CONFIG_$(SPL_)PMIC_LP873X) += lp873x.o
 obj-$(CONFIG_$(SPL_)PMIC_LP87565) += lp87565.o

drivers/power/pmic/pmic_tps65910_dm.c new file +34 -0

new file mode 100644
index 0000000..1410657
@@ -0,0 +1,138 @@
+/*
+ * Copyright (C) EETS GmbH, 2017, Felix Brack <[email protected]>
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <i2c.h>
+#include <power/pmic.h>
+#include <power/regulator.h>
+#include <power/tps65910_pmic.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static const struct pmic_child_info pmic_children_info[] = {
+       { .prefix = "ldo_", .driver = TPS65910_LDO_DRIVER },
+       { .prefix = "buck_", .driver = TPS65910_BUCK_DRIVER },
+       { .prefix = "boost_", .driver = TPS65910_BOOST_DRIVER },
+       { },
+};
+
+static const char * const supply_names[] = {
+       "vccio-supply",
+       "vcc1-supply",
+       "vcc2-supply",
+       "vcc3-supply",
+       "vcc4-supply",
+       "vcc5-supply",
+       "vcc6-supply",
+       "vcc7-supply",
+};
+
+static int pmic_tps65910_reg_count(struct udevice *dev)

drivers/power/pmic/pmic_tps65910_dm.c line 44

Quoted 10 lines

drivers/power/pmic/pmic_tps65910_dm.c new file +10 -0

+{
+       return TPS65910_NUM_REGS;
+}
+
+static int pmic_tps65910_write(struct udevice *dev, uint reg, const u8 *buffer,
+                              int len)
+{
+       if (dm_i2c_write(dev, reg, buffer, len)) {
+               error("%s write error on register %02x\n", dev->name, reg);
+               return -EIO;

Can you return ret here instead (and in cases below)? I does not seem necessary to obscure the original error.

[...]

Quoted 9 lines

drivers/power/regulator/Kconfig +1 -0

index c82a936..2d6a150 100644
@@ -168,3 +168,10 @@ config DM_REGULATOR_LP87565
        LP87565 series of PMICs have 4 single phase BUCKs that can also
        be configured in multi phase modes. The driver implements
        get/set api for value and enable.
+

drivers/power/regulator/Kconfig config DM_REGULATOR_LP87565 · line 177

Quoted 6 lines

drivers/power/regulator/Kconfig +6 -0

+config DM_REGULATOR_TPS65910
+       bool "Enable driver for TPS65910 PMIC regulators"
+       depends on DM_PMIC_TPS65910
+       ---help---
+       The TPS65910 PMIC provides 4 SMPSs and 8 LDOs. This driver implements
+       the get/set api for value and enable for these regulators.

I think that last sentence should be split into two.

Quoted 124 lines

drivers/power/regulator/Makefile +1 -0

index 18fb870..3eef297 100644
@@ -20,3 +20,4 @@ obj-$(CONFIG_REGULATOR_TPS65090) += tps65090_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_PALMAS) += palmas_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_LP873X) += lp873x_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_LP87565) += lp87565_regulator.o
+obj-$(CONFIG_DM_REGULATOR_TPS65910) += tps65910_regulator.o

drivers/power/regulator/tps65910_regulator.c new file +109 -0

new file mode 100644
index 0000000..d212b70
@@ -0,0 +1,493 @@
+/*
+ * Copyright (C) EETS GmbH, 2017, Felix Brack <[email protected]>
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <power/pmic.h>
+#include <power/regulator.h>
+#include <power/tps65910_pmic.h>
+
+#define VOUT_CHOICE_COUNT 4
+
+/*
+ * struct regulator_props - Properties of a LDO and VIO SMPS regulator
+ *
+ * All of these regulators allow setting one out of four output voltages.
+ * These output voltages are only achievable when supplying the regulator
+ * with a minimum input voltage.
+ *
+ * @vin_min[]: minimum supply input voltage in uV required to achieve the
+ *             corresponding vout[] voltage
+ * @vout[]:    regulator output voltage in uV
+ * @reg:       I2C register used to set regulator voltage
+ */
+struct regulator_props {
+       int vin_min[VOUT_CHOICE_COUNT];
+       int vout[VOUT_CHOICE_COUNT];
+       int reg;
+};
+
+static const struct regulator_props ldo_props_vdig1 = {
+       .vin_min = { 1700000, 2100000, 2700000, 3200000 },
+       .vout = { 1200000, 1500000, 1800000, 2700000 },
+       .reg = TPS65910_REG_VDIG1
+};
+
+static const struct regulator_props ldo_props_vdig2 = {
+       .vin_min = { 1700000, 1700000, 1700000, 2700000 },
+       .vout = { 1000000, 1100000, 1200000, 1800000 },
+       .reg = TPS65910_REG_VDIG2
+};
+
+static const struct regulator_props ldo_props_vpll = {
+       .vin_min = { 2700000, 2700000, 2700000, 3000000 },
+       .vout = { 1000000, 1100000, 1800000, 2500000 },
+       .reg = TPS65910_REG_VPLL
+};
+
+static const struct regulator_props ldo_props_vdac = {
+       .vin_min = { 2700000, 3000000, 3200000, 3200000 },
+       .vout = { 1800000, 2600000, 2800000, 2850000 },
+       .reg = TPS65910_REG_VDAC
+};
+
+static const struct regulator_props ldo_props_vaux1 = {
+       .vin_min = { 2700000, 3200000, 3200000, 3200000 },
+       .vout = { 1800000, 2500000, 2800000, 2850000 },
+       .reg = TPS65910_REG_VAUX1
+};
+
+static const struct regulator_props ldo_props_vaux2 = {
+       .vin_min = { 2700000, 3200000, 3200000, 3600000 },
+       .vout = { 1800000, 2800000, 2900000, 3300000 },
+       .reg = TPS65910_REG_VAUX2
+};
+
+static const struct regulator_props ldo_props_vaux33 = {
+       .vin_min = { 2700000, 2700000, 3200000, 3600000 },
+       .vout = { 1800000, 2000000, 2800000, 3300000 },
+       .reg = TPS65910_REG_VAUX33
+};
+
+static const struct regulator_props ldo_props_vmmc = {
+       .vin_min = { 2700000, 3200000, 3200000, 3600000 },
+       .vout = { 1800000, 2800000, 3000000, 3300000 },
+       .reg = TPS65910_REG_VMMC
+};
+
+static const struct regulator_props smps_props_vio = {
+       .vin_min = { 3200000, 3200000, 4000000, 4400000 },
+       .vout = { 1500000, 1800000, 2500000, 3300000 },
+       .reg = TPS65910_REG_VIO
+};
+
+static int get_ctrl_reg_from_unit_addr(const int unit_addr)
+{
+       switch (unit_addr) {
+       case TPS65910_UNIT_VRTC:
+               return TPS65910_REG_VRTC;
+       case TPS65910_UNIT_VIO:
+               return TPS65910_REG_VIO;
+       case TPS65910_UNIT_VDD1:
+               return TPS65910_REG_VDD1;
+       case TPS65910_UNIT_VDD2:
+               return TPS65910_REG_VDD2;
+       case TPS65910_UNIT_VDD3:
+               return TPS65910_REG_VDD3;
+       case TPS65910_UNIT_VDIG1:
+               return TPS65910_REG_VDIG1;
+       case TPS65910_UNIT_VDIG2:
+               return TPS65910_REG_VDIG2;
+       case TPS65910_UNIT_VPLL:
+               return TPS65910_REG_VPLL;
+       case TPS65910_UNIT_VDAC:
+               return TPS65910_REG_VDAC;
+       case TPS65910_UNIT_VAUX1:
+               return TPS65910_REG_VAUX1;

drivers/power/regulator/tps65910_regulator.c line 115

Quoted 6 lines

drivers/power/regulator/tps65910_regulator.c new file +6 -0

+       case TPS65910_UNIT_VAUX2:
+               return TPS65910_REG_VAUX2;
+       case TPS65910_UNIT_VAUX33:
+               return TPS65910_REG_VAUX33;
+       case TPS65910_UNIT_VMMC:
+               return TPS65910_REG_VMMC;

I'm guess this cannot be done with an array lookup?

drivers/power/regulator/tps65910_regulator.c line 122

Quoted 7 lines

drivers/power/regulator/tps65910_regulator.c new file +7 -0

+       }
+
+       return -ENXIO;
+}
+
+static int simple_regulator_get_value(struct udevice *dev,
+                                     const struct regulator_props *rgp)

Can you rename this to have the same prefix as the rest of your driver? Otherwise people might think it is a generic function.

Quoted 131 lines

drivers/power/regulator/tps65910_regulator.c new file +131 -0

+{
+       int sel;
+       u8 val;
+       int vout = 0;
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+       int vin = pdata->supply;
+
+       val = pmic_reg_read(dev->parent, rgp->reg);
+       if (val < 0)
+               return val;
+       sel = (val & TPS65910_SEL_MASK) >> 2;
+       vout = (vin >= *(rgp->vin_min + sel)) ? *(rgp->vout + sel) : 0;
+       vout = ((val & TPS65910_SUPPLY_STATE_MASK) == 1) ? vout : 0;
+
+       return vout;
+}
+
+static int tps65910_ldo_get_value(struct udevice *dev)
+{
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+       int vin = pdata->supply;
+
+       switch (dev->driver_data) {
+       case TPS65910_UNIT_VRTC:
+               /* VRTC is fixed and can't be turned off */
+               return (vin >= 2500000) ? 1830000 : 0;
+       case TPS65910_UNIT_VDIG1:
+               return simple_regulator_get_value(dev, &ldo_props_vdig1);
+       case TPS65910_UNIT_VDIG2:
+               return simple_regulator_get_value(dev, &ldo_props_vdig2);
+       case TPS65910_UNIT_VPLL:
+               return simple_regulator_get_value(dev, &ldo_props_vpll);
+       case TPS65910_UNIT_VDAC:
+               return simple_regulator_get_value(dev, &ldo_props_vdac);
+       case TPS65910_UNIT_VAUX1:
+               return simple_regulator_get_value(dev, &ldo_props_vaux1);
+       case TPS65910_UNIT_VAUX2:
+               return simple_regulator_get_value(dev, &ldo_props_vaux2);
+       case TPS65910_UNIT_VAUX33:
+               return simple_regulator_get_value(dev, &ldo_props_vaux33);
+       case TPS65910_UNIT_VMMC:
+               return simple_regulator_get_value(dev, &ldo_props_vmmc);
+       }
+
+       return 0;
+}
+
+static int simple_regulator_set_value(struct udevice *dev,
+                                     const struct regulator_props *ldo,
+                                     int uV)
+{
+       u8 val;
+       int sel = 0;
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+
+       do {
+               /* we only allow exact voltage matches */
+               if (uV == *(ldo->vout + sel))
+                       break;
+       } while (++sel < VOUT_CHOICE_COUNT);
+       if (sel == VOUT_CHOICE_COUNT)
+               return -EINVAL;
+       if (pdata->supply < *(ldo->vin_min + sel))
+               return -EINVAL;
+
+       val = pmic_reg_read(dev->parent, ldo->reg);
+       if (val < 0)
+               return val;
+       val &= ~TPS65910_SEL_MASK;
+       val |= sel << 2;
+       return pmic_reg_write(dev->parent, ldo->reg, val);
+}
+
+static int tps65910_ldo_set_value(struct udevice *dev, int uV)
+{
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+       int vin = pdata->supply;
+
+       switch (dev->driver_data) {
+       case TPS65910_UNIT_VRTC:
+               /* VRTC is fixed to 1.83V and can't be turned off */
+               if (vin < 2500000)
+                       return -EINVAL;
+               return 0;
+       case TPS65910_UNIT_VDIG1:
+               return simple_regulator_set_value(dev, &ldo_props_vdig1, uV);
+       case TPS65910_UNIT_VDIG2:
+               return simple_regulator_set_value(dev, &ldo_props_vdig2, uV);
+       case TPS65910_UNIT_VPLL:
+               return simple_regulator_set_value(dev, &ldo_props_vpll, uV);
+       case TPS65910_UNIT_VDAC:
+               return simple_regulator_set_value(dev, &ldo_props_vdac, uV);
+       case TPS65910_UNIT_VAUX1:
+               return simple_regulator_set_value(dev, &ldo_props_vaux1, uV);
+       case TPS65910_UNIT_VAUX2:
+               return simple_regulator_set_value(dev, &ldo_props_vaux2, uV);
+       case TPS65910_UNIT_VAUX33:
+               return simple_regulator_set_value(dev, &ldo_props_vaux33, uV);
+       case TPS65910_UNIT_VMMC:
+               return simple_regulator_set_value(dev, &ldo_props_vmmc, uV);
+       }
+
+       return 0;
+}
+
+static int tps65910_get_enable(struct udevice *dev)
+{
+       int reg, ret;
+       u8 val;
+
+       reg = get_ctrl_reg_from_unit_addr(dev->driver_data);
+       if (reg < 0)
+               return reg;
+
+       ret = pmic_read(dev->parent, reg, &val, 1);
+       if (ret)
+               return ret;
+
+       /* bits 2:0 of regulator control register define state */
+       return ((val & TPS65910_SUPPLY_STATE_MASK) == 1);
+}
+
+static int tps65910_set_enable(struct udevice *dev, bool enable)
+{
+       int reg;
+       uint clr, set;
+
+       reg = get_ctrl_reg_from_unit_addr(dev->driver_data);
+       if (reg < 0)
+               return reg;
+

drivers/power/regulator/tps65910_regulator.c line 259

Quoted 6 lines

drivers/power/regulator/tps65910_regulator.c new file +6 -0

+       if (enable) {
+               clr = 0x02;
+               set = 0x01;
+       } else {
+               clr = 0x03;
+               set = 0x00;

Do you not have defines / enums for these values?

Quoted 92 lines

drivers/power/regulator/tps65910_regulator.c new file +92 -0

+       }
+       return pmic_clrsetbits(dev->parent, reg, clr, set);
+}
+
+static int tps65910_ldo_probe(struct udevice *dev)
+{
+       int ret = 0;
+       int unit = dev->driver_data;
+       struct tps65910_pdata *pmic_pdata = dev->parent->platdata;
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+
+       switch (unit) {
+       case TPS65910_UNIT_VRTC:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC7);
+               break;
+       case TPS65910_UNIT_VDIG1:
+       case TPS65910_UNIT_VDIG2:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC6);
+               break;
+       case TPS65910_UNIT_VPLL:
+       case TPS65910_UNIT_VDAC:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC5);
+               break;
+       case TPS65910_UNIT_VAUX1:
+       case TPS65910_UNIT_VAUX2:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC4);
+               break;
+       case TPS65910_UNIT_VAUX33:
+       case TPS65910_UNIT_VMMC:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC3);
+               break;
+       default:
+               ret = -ENXIO;
+       }
+
+       return ret;
+}
+
+static int buck_get_vdd1_vdd2_value(struct udevice *dev, int reg_vdd)
+{
+       int gain;
+       u8 val = pmic_reg_read(dev, reg_vdd);
+
+       if (val < 0)
+               return val;
+       gain = (val & TPS65910_GAIN_SEL_MASK) >> 6;
+       gain = (gain == 0) ? 1 : gain;
+       val = pmic_reg_read(dev, reg_vdd + 1);
+       if (val < 0)
+               return val;
+       if (val & TPS65910_VDD_SR_MASK)
+               /* use smart reflex value instead */
+               val = pmic_reg_read(dev, reg_vdd + 2);
+       if (val < 0)
+               return val;
+       return (562500 + (val & TPS65910_VDD_SEL_MASK) * 12500) * gain;
+}
+
+static int tps65910_buck_get_value(struct udevice *dev)
+{
+       int vout = 0;
+
+       switch (dev->driver_data) {
+       case TPS65910_UNIT_VIO:
+               vout = simple_regulator_get_value(dev, &smps_props_vio);
+               break;
+       case TPS65910_UNIT_VDD1:
+               vout = buck_get_vdd1_vdd2_value(dev->parent, TPS65910_REG_VDD1);
+               break;
+       case TPS65910_UNIT_VDD2:
+               vout = buck_get_vdd1_vdd2_value(dev->parent, TPS65910_REG_VDD2);
+               break;
+       }
+
+       return vout;
+}
+
+static int buck_set_vdd1_vdd2_value(struct udevice *dev, int uV)
+{
+       int ret, reg_vdd, gain;
+       u32 limit;
+       int val;
+
+       switch (dev->driver_data) {
+       case TPS65910_UNIT_VDD1:
+               reg_vdd = TPS65910_REG_VDD1;
+               break;
+       case TPS65910_UNIT_VDD2:
+               reg_vdd = TPS65910_REG_VDD2;
+               break;
+       default:
+               return -EINVAL;

drivers/power/regulator/tps65910_regulator.c line 355

Quoted 4 lines

drivers/power/regulator/tps65910_regulator.c new file +4 -0

+       }
+
+       /* check setpoint is within limits */
+       ret = ofnode_read_u32(dev->node, "regulator-min-microvolt", &limit);

This should be read at the start and stored somewhere. In general you should not be checking the device outside of ofdata_to_platdata() / probe().

Quoted 21 lines

drivers/power/regulator/tps65910_regulator.c new file +21 -0

+       if (ret) {
+               /* too dangerous without limit */
+               error("missing regulator-min-microvolt property for %s\n",
+                     dev->name);
+               return ret;
+       }
+       if (uV < limit) {
+               error("voltage %duV for %s too low\n",
+                     limit, dev->name);
+               return -EINVAL;
+       }
+       ret = ofnode_read_u32(dev->node, "regulator-max-microvolt", &limit);
+       if (ret) {
+               /* too dangerous without limit */
+               error("missing regulator-max-microvolt property for %s\n",
+                     dev->name);
+               return ret;
+       }
+       if (uV > limit) {
+               error("voltage %duV for %s too high\n",
+                     limit, dev->name);

drivers/power/regulator/tps65910_regulator.c line 386

Quoted 10 lines

drivers/power/regulator/tps65910_regulator.c new file +10 -0

+               return -EINVAL;
+       }
+
+       val = pmic_reg_read(dev->parent, reg_vdd);
+       if (val < 0)
+               return val;
+       gain = (val & TPS65910_GAIN_SEL_MASK) >> 6;
+       gain = (gain == 0) ? 1 : gain;
+       val = ((uV / gain) - 562500) / 12500;
+       if ((val < 3) || (val > 75))

You don't need all the brackets on this line.

drivers/power/regulator/tps65910_regulator.c line 387

Quoted 1 line

drivers/power/regulator/tps65910_regulator.c new file +1 -0

+               /* neither do we change the gain, nor do we allow shutdown or

/*
* Neither do we...
* ...
*/

Quoted 121 lines

drivers/power/regulator/tps65910_regulator.c new file +106 -0

+                *  any approximate value (for now)
+                */
+               return -EPERM;
+       val &= TPS65910_VDD_SEL_MASK;
+       ret = pmic_reg_write(dev->parent, reg_vdd + 1, val);
+       if (ret)
+               return ret;
+       return 0;
+}
+
+static int tps65910_buck_set_value(struct udevice *dev, int uV)
+{
+       if (dev->driver_data == TPS65910_UNIT_VIO)
+               return simple_regulator_set_value(dev, &smps_props_vio, uV);
+
+       return buck_set_vdd1_vdd2_value(dev, uV);
+}
+
+static int tps65910_buck_probe(struct udevice *dev)
+{
+       int ret = 0;
+       int unit = dev->driver_data;
+       struct tps65910_pdata *pmic_pdata = dev->parent->platdata;
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+
+       switch (unit) {
+       case TPS65910_UNIT_VIO:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCCIO);
+               break;
+       case TPS65910_UNIT_VDD1:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC1);
+               break;
+       case TPS65910_UNIT_VDD2:
+               pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC2);
+               break;
+       default:
+               ret = -ENXIO;
+       }
+       return ret;
+}
+
+static int tps65910_boost_get_value(struct udevice *dev)
+{
+       int vout;
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+
+       vout = (pdata->supply >= 3000000) ? 5000000 : 0;
+       return vout;
+}
+
+static int tps65910_boost_probe(struct udevice *dev)
+{
+       int unit = dev->driver_data;
+       struct tps65910_pdata *pmic_pdata = dev->parent->platdata;
+       struct tps65910_regulator_pdata *pdata = dev->platdata;
+
+       if (unit != TPS65910_UNIT_VDD3)
+               return -ENXIO;
+
+       pdata->supply = *(pmic_pdata->supply + TPS65910_SUPPLY_VCC7);
+       return 0;
+}
+
+static const struct dm_regulator_ops tps65910_boost_ops = {
+       .get_value  = tps65910_boost_get_value,
+       .get_enable = tps65910_get_enable,
+       .set_enable = tps65910_set_enable,
+};
+
+U_BOOT_DRIVER(tps65910_boost) = {
+       .name = TPS65910_BOOST_DRIVER,
+       .id = UCLASS_REGULATOR,
+       .ops = &tps65910_boost_ops,
+       .probe = tps65910_boost_probe,
+       .platdata_auto_alloc_size = sizeof(struct tps65910_regulator_pdata),
+};
+
+static const struct dm_regulator_ops tps65910_buck_ops = {
+       .get_value  = tps65910_buck_get_value,
+       .set_value  = tps65910_buck_set_value,
+       .get_enable = tps65910_get_enable,
+       .set_enable = tps65910_set_enable,
+};
+
+U_BOOT_DRIVER(tps65910_buck) = {
+       .name = TPS65910_BUCK_DRIVER,
+       .id = UCLASS_REGULATOR,
+       .ops = &tps65910_buck_ops,
+       .probe = tps65910_buck_probe,
+       .platdata_auto_alloc_size = sizeof(struct tps65910_regulator_pdata),
+};
+
+static const struct dm_regulator_ops tps65910_ldo_ops = {
+       .get_value  = tps65910_ldo_get_value,
+       .set_value  = tps65910_ldo_set_value,
+       .get_enable = tps65910_get_enable,
+       .set_enable = tps65910_set_enable,
+};
+
+U_BOOT_DRIVER(tps65910_ldo) = {
+       .name = TPS65910_LDO_DRIVER,
+       .id = UCLASS_REGULATOR,
+       .ops = &tps65910_ldo_ops,
+       .probe = tps65910_ldo_probe,
+       .platdata_auto_alloc_size = sizeof(struct tps65910_regulator_pdata),
+};

include/power/tps65910_pmic.h new file +9 -0

new file mode 100644
index 0000000..23e031e
@@ -0,0 +1,130 @@
+/*
+ * Copyright (C) EETS GmbH, 2017, Felix Brack <[email protected]>
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#ifndef __TPS65910_PMIC_H_
+#define __TPS65910_PMIC_H_
+

include/power/tps65910_pmic.h line 15

Quoted 6 lines

include/power/tps65910_pmic.h new file +6 -0

+#define TPS65910_I2C_SEL_MASK (0x1 << 4)
+#define TPS65910_VDD_SR_MASK (0x1 << 7)
+#define TPS65910_GAIN_SEL_MASK (0x3 << 6)
+#define TPS65910_VDD_SEL_MASK (0x7f)
+#define TPS65910_SEL_MASK (0x3 << 2)
+#define TPS65910_SUPPLY_STATE_MASK (0x3)

Might be easier to read if you tab out the values.

Quoted 99 lines

include/power/tps65910_pmic.h new file +99 -0

+
+/* i2c registers */
+enum {
+       TPS65910_REG_RTC_SEC                    = 0x00,
+       TPS65910_REG_RTC_MIN,
+       TPS65910_REG_RTC_HOUR,
+       TPS65910_REG_RTC_DAY,
+       TPS65910_REG_RTC_MONTH,
+       TPS65910_REG_RTC_YEAR,
+       TPS65910_REG_RTC_WEEK,
+       TPS65910_REG_RTC_ALARM_SEC              = 0x08,
+       TPS65910_REG_RTC_ALARM_MIN,
+       TPS65910_REG_RTC_ALARM_HOUR,
+       TPS65910_REG_RTC_ALARM_DAY,
+       TPS65910_REG_RTC_ALARM_MONTH,
+       TPS65910_REG_RTC_ALARM_YEAR,
+       TPS65910_REG_RTC_CTRL                   = 0x10,
+       TPS65910_REG_RTC_STAT,
+       TPS65910_REG_RTC_INT,
+       TPS65910_REG_RTC_COMP_LSB,
+       TPS65910_REG_RTC_COMP_MSB,
+       TPS65910_REG_RTC_RESISTOR_PRG,
+       TPS65910_REG_RTC_RESET_STAT,
+       TPS65910_REG_BACKUP1,
+       TPS65910_REG_BACKUP2,
+       TPS65910_REG_BACKUP3,
+       TPS65910_REG_BACKUP4,
+       TPS65910_REG_BACKUP5,
+       TPS65910_REG_PUADEN,
+       TPS65910_REG_REF,
+       TPS65910_REG_VRTC,
+       TPS65910_REG_VIO                        = 0x20,
+       TPS65910_REG_VDD1,
+       TPS65910_REG_VDD1_VAL,
+       TPS65910_REG_VDD1_VAL_SR,
+       TPS65910_REG_VDD2,
+       TPS65910_REG_VDD2_VAL,
+       TPS65910_REG_VDD2_VAL_SR,
+       TPS65910_REG_VDD3,
+       TPS65910_REG_VDIG1                      = 0x30,
+       TPS65910_REG_VDIG2,
+       TPS65910_REG_VAUX1,
+       TPS65910_REG_VAUX2,
+       TPS65910_REG_VAUX33,
+       TPS65910_REG_VMMC,
+       TPS65910_REG_VPLL,
+       TPS65910_REG_VDAC,
+       TPS65910_REG_THERM,
+       TPS65910_REG_BATTERY_BACKUP_CHARGE,
+       TPS65910_REG_DCDC_CTRL                  = 0x3e,
+       TPS65910_REG_DEVICE_CTRL,
+       TPS65910_REG_DEVICE_CTRL2,
+       TPS65910_REG_SLEEP_KEEP_LDO_ON,
+       TPS65910_REG_SLEEP_KEEP_RES_ON,
+       TPS65910_REG_SLEEP_SET_LDO_OFF,
+       TPS65910_REG_SLEEP_SET_RES_OFF,
+       TPS65910_REG_EN1_LDO_ASS,
+       TPS65910_REG_EM1_SMPS_ASS,
+       TPS65910_REG_EN2_LDO_ASS,
+       TPS65910_REG_EM2_SMPS_ASS,
+       TPS65910_REG_INT_STAT                   = 0x50,
+       TPS65910_REG_INT_MASK,
+       TPS65910_REG_INT_STAT2,
+       TPS65910_REG_INT_MASK2,
+       TPS65910_REG_GPIO                       = 0x60,
+       TPS65910_REG_JTAGREVNUM                 = 0x80,
+       TPS65910_NUM_REGS
+};
+
+/* chip supplies */
+enum {
+       TPS65910_SUPPLY_VCCIO   = 0x00,
+       TPS65910_SUPPLY_VCC1,
+       TPS65910_SUPPLY_VCC2,
+       TPS65910_SUPPLY_VCC3,
+       TPS65910_SUPPLY_VCC4,
+       TPS65910_SUPPLY_VCC5,
+       TPS65910_SUPPLY_VCC6,
+       TPS65910_SUPPLY_VCC7,
+       TPS65910_NUM_SUPPLIES
+};
+
+/* regulator unit numbers */
+enum {
+       TPS65910_UNIT_VRTC = 0x00,
+       TPS65910_UNIT_VIO,
+       TPS65910_UNIT_VDD1,
+       TPS65910_UNIT_VDD2,
+       TPS65910_UNIT_VDD3,
+       TPS65910_UNIT_VDIG1,
+       TPS65910_UNIT_VDIG2,
+       TPS65910_UNIT_VPLL,
+       TPS65910_UNIT_VDAC,
+       TPS65910_UNIT_VAUX1,
+       TPS65910_UNIT_VAUX2,
+       TPS65910_UNIT_VAUX33,
+       TPS65910_UNIT_VMMC,
+       TPS65910_UNIT_VBB,
+};

include/power/tps65910_pmic.h line 119

Quoted 5 lines

include/power/tps65910_pmic.h new file +5 -0

+
+/* platform data */
+struct tps65910_pdata {
+       u32 supply[TPS65910_NUM_SUPPLIES]; /* regulator supply voltage in uV */
+};

Is this used outside the driver? Do you need one driver to access another's platform data? That seems dodgy.

Quoted 9 lines

include/power/tps65910_pmic.h new file +9 -0

+
+struct tps65910_regulator_pdata {
+       u32 supply;    /* regulator supply voltage in uV */
+};
+
+/* driver names */
+#define TPS65910_BUCK_DRIVER "tps65910_buck"
+#define TPS65910_BOOST_DRIVER "tps65910_boost"
+#define TPS65910_LDO_DRIVER "tps65910_ldo"

Quoted 5 lines

include/power/tps65910_pmic.h new file +2 -1

+
+ #endif /* __TPS65910_PMIC_H_ */
--

2.7.4

Regards,
Simon

Hello Simon,

Many thanks for taking the time to review my patch.

Quoted 11 lines · Simon Glass

Hi Felix,

include/power/tps65910_pmic.h line 119

Quoted 5 lines

include/power/tps65910_pmic.h new file +5 -0

+
+/* platform data */
+struct tps65910_pdata {
+       u32 supply[TPS65910_NUM_SUPPLIES]; /* regulator supply voltage in uV */
+};

Is this used outside the driver? Do you need one driver to access another's platform data? That seems dodgy.

No. You are right: no need to place it in the header file. I will move it to pmic_tps65910_dm.c file.

Quoted 16 lines · Felix Brack

Hello Simon,

Many thanks for taking the time to review my patch.

Quoted 11 lines · Simon Glass

Hi Felix,

include/power/tps65910_pmic.h line 119

Quoted 5 lines

include/power/tps65910_pmic.h new file +5 -0

+
+/* platform data */
+struct tps65910_pdata {
+       u32 supply[TPS65910_NUM_SUPPLIES]; /* regulator supply voltage in uV */
+};

Is this used outside the driver? Do you need one driver to access another's platform data? That seems dodgy.

Quoted 3 lines

No. You are right: no need to place it in the header file. I will move it to pmic_tps65910_dm.c file.

Sorry, this is not correct. The regulators of the PMIC need access to this. Let me try to explain:

The PMIC has 8 different supply inputs. These are therefore properties of the PMIC. Each of the 12 regulators uses one of these 8 supply voltages as its input. Hence the regulators of the TPS65910 need read access to the PMIC's information about the supplies. In other words: a regulator's supply voltage has to be queried from the corresponding PMIC.

I think I followed the "rules" when implementing the PMIC and the regulator code in separate files.

Hi Felix,

Quoted 31 lines · Felix Brack

Quoted 16 lines · Felix Brack

Hello Simon,

Many thanks for taking the time to review my patch.

Quoted 11 lines · Simon Glass

Hi Felix,

include/power/tps65910_pmic.h line 119

Quoted 5 lines

include/power/tps65910_pmic.h new file +5 -0

+
+/* platform data */
+struct tps65910_pdata {
+       u32 supply[TPS65910_NUM_SUPPLIES]; /* regulator supply voltage in uV */
+};

Is this used outside the driver? Do you need one driver to access another's platform data? That seems dodgy.

Quoted 3 lines

No. You are right: no need to place it in the header file. I will move it to pmic_tps65910_dm.c file.

Sorry, this is not correct. The regulators of the PMIC need access to this. Let me try to explain:

The PMIC has 8 different supply inputs. These are therefore properties of the PMIC. Each of the 12 regulators uses one of these 8 supply voltages as its input. Hence the regulators of the TPS65910 need read access to the PMIC's information about the supplies. In other words: a regulator's supply voltage has to be queried from the corresponding PMIC.

Quoted 2 lines

I think I followed the "rules" when implementing the PMIC and the regulator code in separate files.

OK, so the problem is that the regulator has to look up the supply voltage in the PMIC?

Should this be platform data (accessible before the device is probed) or private data (which exists only when the device is active)?

Also why does the PMIC driver itself know about supply voltages? I think that info should be in the regulator driver. The PMIC is really just a parent driver providing access to the various subsystems of the PMIC (regulator, GPIO, battery, etc).

Hello Simon,

Quoted 34 lines · Simon Glass

Hi Felix,

Quoted 31 lines · Felix Brack

Quoted 16 lines · Felix Brack

Hello Simon,

Many thanks for taking the time to review my patch.

Quoted 11 lines · Simon Glass

Hi Felix,

include/power/tps65910_pmic.h line 119

Quoted 5 lines

include/power/tps65910_pmic.h new file +5 -0

+
+/* platform data */
+struct tps65910_pdata {
+       u32 supply[TPS65910_NUM_SUPPLIES]; /* regulator supply voltage in uV */
+};

Is this used outside the driver? Do you need one driver to access another's platform data? That seems dodgy.

Quoted 3 lines

No. You are right: no need to place it in the header file. I will move it to pmic_tps65910_dm.c file.

Sorry, this is not correct. The regulators of the PMIC need access to this. Let me try to explain:

The PMIC has 8 different supply inputs. These are therefore properties of the PMIC. Each of the 12 regulators uses one of these 8 supply voltages as its input. Hence the regulators of the TPS65910 need read access to the PMIC's information about the supplies. In other words: a regulator's supply voltage has to be queried from the corresponding PMIC.

Quoted 6 lines

Quoted 2 lines

I think I followed the "rules" when implementing the PMIC and the regulator code in separate files.

OK, so the problem is that the regulator has to look up the supply voltage in the PMIC?

Yes

Quoted 3 lines

Should this be platform data (accessible before the device is probed) or private data (which exists only when the device is active)?

The regulator supply voltages are defined in the DT. Hence they should be read by ofdata_to_platdata() and store as platform data, I think.

Quoted 6 lines

Also why does the PMIC driver itself know about supply voltages? I think that info should be in the regulator driver. The PMIC is really just a parent driver providing access to the various subsystems of the PMIC (regulator, GPIO, battery, etc).

I agree - the PMIC driver does not require the knowledge about the supply voltage of a distinct regulator. It is just that the PMIC is modeled like this in the DT, i.e. the supplies are proerties of the PMIC and not the regulators (which are subnodes on the same level as the supply nodes).
Having said that, I think the proper solution is as follows:
Use the regulator driver's ofdata_to_platdata() to query the supply voltage of that specific regulator by device_get_supply_regulator() using dev->parent (the PMIC) as device parameter.
The supply voltage will then be "part" of the regulator instead of the PMIC. Do you agree to this solution?

However if you find that you really do need to blur the line between the regulator and the pmic, and share the pdata structure, it is not the end of the world. It seems wrong to me, so I'd like to avoid it if possible. We should have a good reason. The DT node structure is certainly a reason, but it does not seem a strong enough reason to me.

Hello Simon,

Many thanks for taking the time to review my patch.

Quoted 28 lines · Simon Glass

Hi Felix,

Quoted 25 lines · Felix Brack

Texas Instrument's TPS65910 PMIC contains 3 buck DC-DC converts, one boost DC-DC converter and 8 LDOs. This patch implements driver model support for the TPS65910 PMIC and its regulators making the get/set API for regulator value/enable available.
This patch depends on the patch "am33xx: Add a function to query MPU voltage in uV" to build correctly. For boards relying on the DT include file tps65910.dtsi the v2 patch "power: extend prefix match to regulator-name property" and an appropriate regulator naming is also required.

Signed-off-by Felix Brack <[email protected]>

---

 drivers/power/pmic/Kconfig                   |   8 +
 drivers/power/pmic/Makefile                  |   1 +
 drivers/power/pmic/pmic_tps65910_dm.c        | 138 ++++++++
 drivers/power/regulator/Kconfig              |   7 +
 drivers/power/regulator/Makefile             |   1 +
 drivers/power/regulator/tps65910_regulator.c | 493 +++++++++++++++++++++++++++
 include/power/tps65910_pmic.h                | 130 +++++++
 7 files changed, 778 insertions(+)
 create mode 100644 drivers/power/pmic/pmic_tps65910_dm.c
 create mode 100644 drivers/power/regulator/tps65910_regulator.c
 create mode 100644 include/power/tps65910_pmic.h

Quoted 10 lines

drivers/power/pmic/pmic_tps65910_dm.c line 44

Quoted 6 lines

drivers/power/pmic/pmic_tps65910_dm.c new file +6 -0

+static int pmic_tps65910_write(struct udevice *dev, uint reg, const u8 *buffer,
+                              int len)
+{
+       if (dm_i2c_write(dev, reg, buffer, len)) {
+               error("%s write error on register %02x\n", dev->name, reg);
+               return -EIO;

Can you return ret here instead (and in cases below)? I does not seem necessary to obscure the original error.

Quoted 2 lines

[...]

Yes for the calls to dm_i2c_write() and dm_i2c_read(). How about the call to ofnode_valid()? Is it okay to return -ENXIO instead of NULL in case of failure?

Quoted 23 lines

Quoted 23 lines

drivers/power/regulator/tps65910_regulator.c new file +23 -0

+static int get_ctrl_reg_from_unit_addr(const int unit_addr)
+{
+       switch (unit_addr) {
+       case TPS65910_UNIT_VRTC:
+               return TPS65910_REG_VRTC;
+       case TPS65910_UNIT_VIO:
+               return TPS65910_REG_VIO;
+       case TPS65910_UNIT_VDD1:
+               return TPS65910_REG_VDD1;
+       case TPS65910_UNIT_VDD2:
+               return TPS65910_REG_VDD2;
+       case TPS65910_UNIT_VDD3:
+               return TPS65910_REG_VDD3;
+       case TPS65910_UNIT_VDIG1:
+               return TPS65910_REG_VDIG1;
+       case TPS65910_UNIT_VDIG2:
+               return TPS65910_REG_VDIG2;
+       case TPS65910_UNIT_VPLL:
+               return TPS65910_REG_VPLL;
+       case TPS65910_UNIT_VDAC:
+               return TPS65910_REG_VDAC;
+       case TPS65910_UNIT_VAUX1:
+               return TPS65910_REG_VAUX1;

Quoted 9 lines

drivers/power/regulator/tps65910_regulator.c line 115

Quoted 6 lines

drivers/power/regulator/tps65910_regulator.c new file +6 -0

+       case TPS65910_UNIT_VAUX2:
+               return TPS65910_REG_VAUX2;
+       case TPS65910_UNIT_VAUX33:
+               return TPS65910_REG_VAUX33;
+       case TPS65910_UNIT_VMMC:
+               return TPS65910_REG_VMMC;

I'm guess this cannot be done with an array lookup?

It could be done, as the unit numbers are consecutive (for now) and starting at 0. I don't like that piece of code either. Should I replace it with a static array of constants? I'm just not sure ...

Hi Felix,

Quoted 33 lines · Felix Brack

Hello Simon,

Many thanks for taking the time to review my patch.

Quoted 28 lines · Simon Glass

Hi Felix,

Quoted 25 lines · Felix Brack

Texas Instrument's TPS65910 PMIC contains 3 buck DC-DC converts, one boost DC-DC converter and 8 LDOs. This patch implements driver model support for the TPS65910 PMIC and its regulators making the get/set API for regulator value/enable available.
This patch depends on the patch "am33xx: Add a function to query MPU voltage in uV" to build correctly. For boards relying on the DT include file tps65910.dtsi the v2 patch "power: extend prefix match to regulator-name property" and an appropriate regulator naming is also required.

Signed-off-by Felix Brack <[email protected]>

---

 drivers/power/pmic/Kconfig                   |   8 +
 drivers/power/pmic/Makefile                  |   1 +
 drivers/power/pmic/pmic_tps65910_dm.c        | 138 ++++++++
 drivers/power/regulator/Kconfig              |   7 +
 drivers/power/regulator/Makefile             |   1 +
 drivers/power/regulator/tps65910_regulator.c | 493 +++++++++++++++++++++++++++
 include/power/tps65910_pmic.h                | 130 +++++++
 7 files changed, 778 insertions(+)
 create mode 100644 drivers/power/pmic/pmic_tps65910_dm.c
 create mode 100644 drivers/power/regulator/tps65910_regulator.c
 create mode 100644 include/power/tps65910_pmic.h

[..]

Quoted 10 lines

Quoted 10 lines

drivers/power/pmic/pmic_tps65910_dm.c line 44

Quoted 6 lines

drivers/power/pmic/pmic_tps65910_dm.c new file +6 -0

+static int pmic_tps65910_write(struct udevice *dev, uint reg, const u8 *buffer,
+                              int len)
+{
+       if (dm_i2c_write(dev, reg, buffer, len)) {
+               error("%s write error on register %02x\n", dev->name, reg);
+               return -EIO;

Can you return ret here instead (and in cases below)? I does not seem necessary to obscure the original error.

Quoted 5 lines

Quoted 2 lines

[...]

Yes for the calls to dm_i2c_write() and dm_i2c_read(). How about the call to ofnode_valid()? Is it okay to return -ENXIO instead of NULL in case of failure?

Yes, all I was referring to was that we should not change the error number when passing it through, unless there is a valid reason.

For ofnode_valid() you have an invalid device tree node I think, so -EINVAL is better.

[..]

Quoted 32 lines

Quoted 32 lines

Quoted 19 lines

drivers/power/regulator/tps65910_regulator.c new file +19 -0

+static int get_ctrl_reg_from_unit_addr(const int unit_addr)
+{
+       switch (unit_addr) {
+       case TPS65910_UNIT_VRTC:
+               return TPS65910_REG_VRTC;
+       case TPS65910_UNIT_VIO:
+               return TPS65910_REG_VIO;
+       case TPS65910_UNIT_VDD1:
+               return TPS65910_REG_VDD1;
+       case TPS65910_UNIT_VDD2:
+               return TPS65910_REG_VDD2;
+       case TPS65910_UNIT_VDD3:
+               return TPS65910_REG_VDD3;
+       case TPS65910_UNIT_VDIG1:
+               return TPS65910_REG_VDIG1;
+       case TPS65910_UNIT_VDIG2:
+               return TPS65910_REG_VDIG2;
+       case TPS65910_UNIT_VPLL:
+               return TPS65910_REG_VPLL;

drivers/power/regulator/tps65910_regulator.c line 115

Quoted 10 lines

drivers/power/regulator/tps65910_regulator.c new file +10 -0

+       case TPS65910_UNIT_VDAC:
+               return TPS65910_REG_VDAC;
+       case TPS65910_UNIT_VAUX1:
+               return TPS65910_REG_VAUX1;
+       case TPS65910_UNIT_VAUX2:
+               return TPS65910_REG_VAUX2;
+       case TPS65910_UNIT_VAUX33:
+               return TPS65910_REG_VAUX33;
+       case TPS65910_UNIT_VMMC:
+               return TPS65910_REG_VMMC;

I'm guess this cannot be done with an array lookup?

Quoted 3 lines

It could be done, as the unit numbers are consecutive (for now) and starting at 0. I don't like that piece of code either. Should I replace it with a static array of constants? I'm just not sure ...

I think it is better to use const array in this case.

[...]

Regards,
Simon