* [U-Boot] [PATCH] power: pmic/regulator: Add basic support for TPS65910
@ 2017-11-08 11:04 Felix Brack
2017-11-20 15:38 ` Simon Glass
0 siblings, 1 reply; 10+ messages in thread
From: Felix Brack @ 2017-11-08 11:04 UTC (permalink / raw)
To: u-boot
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 <fb@ltec.ch>
---
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
diff --git a/drivers/power/pmic/Kconfig b/drivers/power/pmic/Kconfig
index e3f9e4d..5d49c93 100644
--- a/drivers/power/pmic/Kconfig
+++ b/drivers/power/pmic/Kconfig
@@ -201,3 +201,11 @@ config POWER_MC34VR500
The MC34VR500 is used in conjunction with the FSL T1 and LS1 series
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.
diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile
index f7bdfa5..7d6c583 100644
--- a/drivers/power/pmic/Makefile
+++ b/drivers/power/pmic/Makefile
@@ -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
diff --git a/drivers/power/pmic/pmic_tps65910_dm.c b/drivers/power/pmic/pmic_tps65910_dm.c
new file mode 100644
index 0000000..1410657
--- /dev/null
+++ b/drivers/power/pmic/pmic_tps65910_dm.c
@@ -0,0 +1,138 @@
+/*
+ * Copyright (C) EETS GmbH, 2017, Felix Brack <f.brack@eets.ch>
+ *
+ * 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)
+{
+ 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;
+ }
+
+ return 0;
+}
+
+static int pmic_tps65910_read(struct udevice *dev, uint reg, u8 *buffer,
+ int len)
+{
+ if (dm_i2c_read(dev, reg, buffer, len)) {
+ error("%s read error on register %02x\n", dev->name, reg);
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int pmic_tps65910_bind(struct udevice *dev)
+{
+ ofnode regulators_node;
+ int children;
+
+ regulators_node = dev_read_subnode(dev, "regulators");
+ if (!ofnode_valid(regulators_node)) {
+ debug("%s regulators subnode not found\n", dev->name);
+ return -ENXIO;
+ }
+
+ children = pmic_bind_children(dev, regulators_node, pmic_children_info);
+ if (!children)
+ debug("%s has no children (regulators)\n", dev->name);
+
+ return 0;
+}
+
+static int pmic_tps65910_probe(struct udevice *dev)
+{
+ /* use I2C control interface instead of I2C smartreflex interface to
+ * access smartrefelex registers VDD1_OP_REG, VDD1_SR_REG, VDD2_OP_REG
+ * and VDD2_SR_REG
+ */
+ return pmic_clrsetbits(dev, TPS65910_REG_DEVICE_CTRL, 0,
+ TPS65910_I2C_SEL_MASK);
+}
+
+static int pmic_tps65910_ofdata_to_platdata(struct udevice *dev)
+{
+ struct tps65910_pdata *pdata = dev->platdata;
+ int i, voltage;
+ const char *supply_name;
+ struct udevice *supply;
+
+ /* for each PMIC regulator get its supply voltage */
+ for (i = TPS65910_SUPPLY_VCCIO; i < TPS65910_NUM_SUPPLIES; i++) {
+ supply_name = *(supply_names + i);
+ debug("Looking up supply power %s\n", supply_name);
+ if (device_get_supply_regulator(dev, supply_name, &supply)) {
+ debug(" missing supply power %s\n", supply_name);
+ continue;
+ }
+ voltage = regulator_get_value(supply);
+ if (voltage < 0) {
+ debug(" invalid supply voltage for regulator %s\n",
+ supply->name);
+ continue;
+ }
+ *(pdata->supply + i) = voltage;
+ debug(" set to %duV by %s\n", *(pdata->supply + i),
+ supply->name);
+ }
+
+ return 0;
+}
+
+static struct dm_pmic_ops pmic_tps65910_ops = {
+ .reg_count = pmic_tps65910_reg_count,
+ .read = pmic_tps65910_read,
+ .write = pmic_tps65910_write,
+};
+
+static const struct udevice_id pmic_tps65910_match[] = {
+ { .compatible = "ti,tps65910" },
+ { /* sentinel */ }
+};
+
+U_BOOT_DRIVER(pmic_tps65910) = {
+ .name = "pmic_tps65910",
+ .id = UCLASS_PMIC,
+ .of_match = pmic_tps65910_match,
+ .bind = pmic_tps65910_bind,
+ .probe = pmic_tps65910_probe,
+ .ops = &pmic_tps65910_ops,
+ .platdata_auto_alloc_size = sizeof(struct tps65910_pdata),
+ .ofdata_to_platdata = pmic_tps65910_ofdata_to_platdata,
+};
diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
index c82a936..2d6a150 100644
--- a/drivers/power/regulator/Kconfig
+++ b/drivers/power/regulator/Kconfig
@@ -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.
+
+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.
diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
index 18fb870..3eef297 100644
--- a/drivers/power/regulator/Makefile
+++ b/drivers/power/regulator/Makefile
@@ -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
diff --git a/drivers/power/regulator/tps65910_regulator.c b/drivers/power/regulator/tps65910_regulator.c
new file mode 100644
index 0000000..d212b70
--- /dev/null
+++ b/drivers/power/regulator/tps65910_regulator.c
@@ -0,0 +1,493 @@
+/*
+ * Copyright (C) EETS GmbH, 2017, Felix Brack <f.brack@eets.ch>
+ *
+ * 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;
+ case TPS65910_UNIT_VAUX2:
+ return TPS65910_REG_VAUX2;
+ case TPS65910_UNIT_VAUX33:
+ return TPS65910_REG_VAUX33;
+ case TPS65910_UNIT_VMMC:
+ return TPS65910_REG_VMMC;
+ }
+
+ return -ENXIO;
+}
+
+static int simple_regulator_get_value(struct udevice *dev,
+ const struct regulator_props *rgp)
+{
+ 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;
+
+ if (enable) {
+ clr = 0x02;
+ set = 0x01;
+ } else {
+ clr = 0x03;
+ set = 0x00;
+ }
+ 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;
+ }
+
+ /* check setpoint is within limits */
+ ret = ofnode_read_u32(dev->node, "regulator-min-microvolt", &limit);
+ 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);
+ 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))
+ /* neither do we change the gain, nor do we allow shutdown or
+ * 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),
+};
diff --git a/include/power/tps65910_pmic.h b/include/power/tps65910_pmic.h
new file mode 100644
index 0000000..23e031e
--- /dev/null
+++ b/include/power/tps65910_pmic.h
@@ -0,0 +1,130 @@
+/*
+ * Copyright (C) EETS GmbH, 2017, Felix Brack <f.brack@eets.ch>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#ifndef __TPS65910_PMIC_H_
+#define __TPS65910_PMIC_H_
+
+#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)
+
+/* 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,
+};
+
+/* platform data */
+struct tps65910_pdata {
+ u32 supply[TPS65910_NUM_SUPPLIES]; /* regulator supply voltage in uV */
+};
+
+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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] power: pmic/regulator: Add basic support for TPS65910
2017-11-08 11:04 [U-Boot] [PATCH] power: pmic/regulator: Add basic support for TPS65910 Felix Brack
@ 2017-11-20 15:38 ` Simon Glass
2017-11-22 10:39 ` Felix Brack
0 siblings, 1 reply; 10+ messages in thread
From: Simon Glass @ 2017-11-20 15:38 UTC (permalink / raw)
To: u-boot
Hi Felix,
On 8 November 2017 at 04:04, Felix Brack <fb@ltec.ch> wrote:
> 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 <fb@ltec.ch>
> ---
>
> 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
>
> diff --git a/drivers/power/pmic/Kconfig b/drivers/power/pmic/Kconfig
> index e3f9e4d..5d49c93 100644
> --- a/drivers/power/pmic/Kconfig
> +++ b/drivers/power/pmic/Kconfig
> @@ -201,3 +201,11 @@ config POWER_MC34VR500
> The MC34VR500 is used in conjunction with the FSL T1 and LS1 series
> 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!
> diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile
> index f7bdfa5..7d6c583 100644
> --- a/drivers/power/pmic/Makefile
> +++ b/drivers/power/pmic/Makefile
> @@ -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
> diff --git a/drivers/power/pmic/pmic_tps65910_dm.c b/drivers/power/pmic/pmic_tps65910_dm.c
> new file mode 100644
> index 0000000..1410657
> --- /dev/null
> +++ b/drivers/power/pmic/pmic_tps65910_dm.c
> @@ -0,0 +1,138 @@
> +/*
> + * Copyright (C) EETS GmbH, 2017, Felix Brack <f.brack@eets.ch>
> + *
> + * 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)
> +{
> + 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.
[...]
> diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
> index c82a936..2d6a150 100644
> --- a/drivers/power/regulator/Kconfig
> +++ b/drivers/power/regulator/Kconfig
> @@ -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.
> +
> +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.
> diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
> index 18fb870..3eef297 100644
> --- a/drivers/power/regulator/Makefile
> +++ b/drivers/power/regulator/Makefile
> @@ -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
> diff --git a/drivers/power/regulator/tps65910_regulator.c b/drivers/power/regulator/tps65910_regulator.c
> new file mode 100644
> index 0000000..d212b70
> --- /dev/null
> +++ b/drivers/power/regulator/tps65910_regulator.c
> @@ -0,0 +1,493 @@
> +/*
> + * Copyright (C) EETS GmbH, 2017, Felix Brack <f.brack@eets.ch>
> + *
> + * 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;
> + 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?
> + }
> +
> + 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.
> +{
> + 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;
> +
> + if (enable) {
> + clr = 0x02;
> + set = 0x01;
> + } else {
> + clr = 0x03;
> + set = 0x00;
Do you not have defines / enums for these values?
> + }
> + 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;
> + }
> +
> + /* 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().
> + 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);
> + 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.
> + /* neither do we change the gain, nor do we allow shutdown or
/*
* Neither do we...
* ...
*/
> + * 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),
> +};
> diff --git a/include/power/tps65910_pmic.h b/include/power/tps65910_pmic.h
> new file mode 100644
> index 0000000..23e031e
> --- /dev/null
> +++ b/include/power/tps65910_pmic.h
> @@ -0,0 +1,130 @@
> +/*
> + * Copyright (C) EETS GmbH, 2017, Felix Brack <f.brack@eets.ch>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#ifndef __TPS65910_PMIC_H_
> +#define __TPS65910_PMIC_H_
> +
> +#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.
> +
> +/* 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,
> +};
> +
> +/* 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.
> +
> +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
>
Regards,
Simon
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] power: pmic/regulator: Add basic support for TPS65910
2017-11-20 15:38 ` Simon Glass
@ 2017-11-22 10:39 ` Felix Brack
2017-11-23 15:36 ` Felix Brack
2017-11-26 11:39 ` Simon Glass
0 siblings, 2 replies; 10+ messages in thread
From: Felix Brack @ 2017-11-22 10:39 UTC (permalink / raw)
To: u-boot
Hello Simon,
Many thanks for taking the time to review my patch.
On 20.11.2017 16:38, Simon Glass wrote:
> Hi Felix,
>
> On 8 November 2017 at 04:04, Felix Brack <fb@ltec.ch> wrote:
>> 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 <fb@ltec.ch>
>> ---
>>
>> 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
>>
>> diff --git a/drivers/power/pmic/Kconfig b/drivers/power/pmic/Kconfig
>> index e3f9e4d..5d49c93 100644
>> --- a/drivers/power/pmic/Kconfig
>> +++ b/drivers/power/pmic/Kconfig
>> @@ -201,3 +201,11 @@ config POWER_MC34VR500
>> The MC34VR500 is used in conjunction with the FSL T1 and LS1 series
>> 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!
>
>> diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile
>> index f7bdfa5..7d6c583 100644
>> --- a/drivers/power/pmic/Makefile
>> +++ b/drivers/power/pmic/Makefile
>> @@ -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
>> diff --git a/drivers/power/pmic/pmic_tps65910_dm.c b/drivers/power/pmic/pmic_tps65910_dm.c
>> new file mode 100644
>> index 0000000..1410657
>> --- /dev/null
>> +++ b/drivers/power/pmic/pmic_tps65910_dm.c
>> @@ -0,0 +1,138 @@
>> +/*
>> + * Copyright (C) EETS GmbH, 2017, Felix Brack <f.brack@eets.ch>
>> + *
>> + * 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)
>> +{
>> + 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.
>
> [...]
>
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?
>> diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
>> index c82a936..2d6a150 100644
>> --- a/drivers/power/regulator/Kconfig
>> +++ b/drivers/power/regulator/Kconfig
>> @@ -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.
>> +
>> +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.
>
Okay
>> diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
>> index 18fb870..3eef297 100644
>> --- a/drivers/power/regulator/Makefile
>> +++ b/drivers/power/regulator/Makefile
>> @@ -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
>> diff --git a/drivers/power/regulator/tps65910_regulator.c b/drivers/power/regulator/tps65910_regulator.c
>> new file mode 100644
>> index 0000000..d212b70
>> --- /dev/null
>> +++ b/drivers/power/regulator/tps65910_regulator.c
>> @@ -0,0 +1,493 @@
>> +/*
>> + * Copyright (C) EETS GmbH, 2017, Felix Brack <f.brack@eets.ch>
>> + *
>> + * 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;
>> + 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 ...
>> + }
>> +
>> + 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.
>
Okay
>> +{
>> + 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;
>> +
>> + if (enable) {
>> + clr = 0x02;
>> + set = 0x01;
>> + } else {
>> + clr = 0x03;
>> + set = 0x00;
>
> Do you not have defines / enums for these values?
>
Not yet, I'll try to invent some.
>> + }
>> + 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;
>> + }
>> +
>> + /* 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().
>
Okay. I will use regulator's ofdata_to_platdata() then.
>> + 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);
>> + 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.
>
Okay
>> + /* neither do we change the gain, nor do we allow shutdown or
>
> /*
> * Neither do we...
> * ...
> */
>
Okay
>> + * 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),
>> +};
>> diff --git a/include/power/tps65910_pmic.h b/include/power/tps65910_pmic.h
>> new file mode 100644
>> index 0000000..23e031e
>> --- /dev/null
>> +++ b/include/power/tps65910_pmic.h
>> @@ -0,0 +1,130 @@
>> +/*
>> + * Copyright (C) EETS GmbH, 2017, Felix Brack <f.brack@eets.ch>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#ifndef __TPS65910_PMIC_H_
>> +#define __TPS65910_PMIC_H_
>> +
>> +#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.
>
Sure, okay.
>> +
>> +/* 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,
>> +};
>> +
>> +/* 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.
>> +
>> +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
>>
>
> Regards,
> Simon
>
Regards, Felix
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] power: pmic/regulator: Add basic support for TPS65910
2017-11-22 10:39 ` Felix Brack
@ 2017-11-23 15:36 ` Felix Brack
2017-11-26 11:38 ` Simon Glass
2017-11-26 11:39 ` Simon Glass
1 sibling, 1 reply; 10+ messages in thread
From: Felix Brack @ 2017-11-23 15:36 UTC (permalink / raw)
To: u-boot
On 22.11.2017 11:39, Felix Brack wrote:
> Hello Simon,
>
> Many thanks for taking the time to review my patch.
>
> On 20.11.2017 16:38, Simon Glass wrote:
>> Hi Felix,
>>
>>> +
>>> +/* 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.
>
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.
Regards Felix
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] power: pmic/regulator: Add basic support for TPS65910
2017-11-23 15:36 ` Felix Brack
@ 2017-11-26 11:38 ` Simon Glass
2017-11-27 13:51 ` Felix Brack
0 siblings, 1 reply; 10+ messages in thread
From: Simon Glass @ 2017-11-26 11:38 UTC (permalink / raw)
To: u-boot
Hi Felix,
On 23 November 2017 at 08:36, Felix Brack <fb@ltec.ch> wrote:
>
>
> On 22.11.2017 11:39, Felix Brack wrote:
>> Hello Simon,
>>
>> Many thanks for taking the time to review my patch.
>>
>> On 20.11.2017 16:38, Simon Glass wrote:
>>> Hi Felix,
>>>
>>>> +
>>>> +/* 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.
>>
> 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.
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).
Regards,
Simon
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] power: pmic/regulator: Add basic support for TPS65910
2017-11-22 10:39 ` Felix Brack
2017-11-23 15:36 ` Felix Brack
@ 2017-11-26 11:39 ` Simon Glass
2017-11-27 10:26 ` Felix Brack
1 sibling, 1 reply; 10+ messages in thread
From: Simon Glass @ 2017-11-26 11:39 UTC (permalink / raw)
To: u-boot
Hi Felix,
On 22 November 2017 at 03:39, Felix Brack <fb@ltec.ch> wrote:
> Hello Simon,
>
> Many thanks for taking the time to review my patch.
>
> On 20.11.2017 16:38, Simon Glass wrote:
>> Hi Felix,
>>
>> On 8 November 2017 at 04:04, Felix Brack <fb@ltec.ch> wrote:
>>> 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 <fb@ltec.ch>
>>> ---
>>>
>>> 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
>>>
[..]
>>> +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.
>>
>> [...]
>>
> 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.
[..]
>>> +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;
>>> + 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 ...
I think it is better to use const array in this case.
[...]
Regards,
Simon
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] power: pmic/regulator: Add basic support for TPS65910
2017-11-26 11:39 ` Simon Glass
@ 2017-11-27 10:26 ` Felix Brack
0 siblings, 0 replies; 10+ messages in thread
From: Felix Brack @ 2017-11-27 10:26 UTC (permalink / raw)
To: u-boot
Hello Simon
On 26.11.2017 12:39, Simon Glass wrote:
> Hi Felix,
>
> On 22 November 2017 at 03:39, Felix Brack <fb@ltec.ch> wrote:
>> Hello Simon,
>>
>> Many thanks for taking the time to review my patch.
>>
>> On 20.11.2017 16:38, Simon Glass wrote:
>>> Hi Felix,
>>>
>>> On 8 November 2017 at 04:04, Felix Brack <fb@ltec.ch> wrote:
>>>> 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 <fb@ltec.ch>
>>>> ---
>>>>
>>>> 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
>>>>
>
> [..]
>
>>>> +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.
>>>
>>> [...]
>>>
>> 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.
>
> [..]
>
queued for V2
>>>> +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;
>>>> + 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 ...
>
> I think it is better to use const array in this case.
>
> [...]
>
queued for V2
> Regards,
> Simon
>
regards, Felix
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] power: pmic/regulator: Add basic support for TPS65910
2017-11-26 11:38 ` Simon Glass
@ 2017-11-27 13:51 ` Felix Brack
2017-11-27 17:16 ` Simon Glass
0 siblings, 1 reply; 10+ messages in thread
From: Felix Brack @ 2017-11-27 13:51 UTC (permalink / raw)
To: u-boot
Hello Simon,
On 26.11.2017 12:38, Simon Glass wrote:
> Hi Felix,
>
> On 23 November 2017 at 08:36, Felix Brack <fb@ltec.ch> wrote:
>>
>>
>> On 22.11.2017 11:39, Felix Brack wrote:
>>> Hello Simon,
>>>
>>> Many thanks for taking the time to review my patch.
>>>
>>> On 20.11.2017 16:38, Simon Glass wrote:
>>>> Hi Felix,
>>>>
>>>>> +
>>>>> +/* 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.
>>>
>> 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.
>
> OK, so the problem is that the regulator has to look up the supply
> voltage in the PMIC?
>
Yes
> 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.
> 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?
regards, Felix
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] power: pmic/regulator: Add basic support for TPS65910
2017-11-27 13:51 ` Felix Brack
@ 2017-11-27 17:16 ` Simon Glass
2017-11-28 14:08 ` Felix Brack
0 siblings, 1 reply; 10+ messages in thread
From: Simon Glass @ 2017-11-27 17:16 UTC (permalink / raw)
To: u-boot
Hi Felix,
On 27 November 2017 at 06:51, Felix Brack <fb@ltec.ch> wrote:
> Hello Simon,
>
> On 26.11.2017 12:38, Simon Glass wrote:
>> Hi Felix,
>>
>> On 23 November 2017 at 08:36, Felix Brack <fb@ltec.ch> wrote:
>>>
>>>
>>> On 22.11.2017 11:39, Felix Brack wrote:
>>>> Hello Simon,
>>>>
>>>> Many thanks for taking the time to review my patch.
>>>>
>>>> On 20.11.2017 16:38, Simon Glass wrote:
>>>>> Hi Felix,
>>>>>
>>>>>> +
>>>>>> +/* 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.
>>>>
>>> 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.
>>
>> OK, so the problem is that the regulator has to look up the supply
>> voltage in the PMIC?
>>
> Yes
>
>> 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.
OK.
>
>> 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?
Yes I think that is best from what I understand.
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.
Regards,
Simon
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] power: pmic/regulator: Add basic support for TPS65910
2017-11-27 17:16 ` Simon Glass
@ 2017-11-28 14:08 ` Felix Brack
0 siblings, 0 replies; 10+ messages in thread
From: Felix Brack @ 2017-11-28 14:08 UTC (permalink / raw)
To: u-boot
Hello Simon,
On 27.11.2017 18:16, Simon Glass wrote:
> 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.
>
In fact - after finishing v2 of this patch which I will post shortly -
it turns out that keeping PMIC and regulator strictly separated greatly
simplifies the code.
Even if power supplies are modeled as PMIC properties in the DT their
sole purpose is to define the supply for one or multiple regulators.
Storing them within the PMIC driver hence makes no sense. The PMIC
driver of v2 patch therefore doesn't even need a ofdata_to_platdata()
function.
many thanks, Felix
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-11-28 14:08 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-08 11:04 [U-Boot] [PATCH] power: pmic/regulator: Add basic support for TPS65910 Felix Brack
2017-11-20 15:38 ` Simon Glass
2017-11-22 10:39 ` Felix Brack
2017-11-23 15:36 ` Felix Brack
2017-11-26 11:38 ` Simon Glass
2017-11-27 13:51 ` Felix Brack
2017-11-27 17:16 ` Simon Glass
2017-11-28 14:08 ` Felix Brack
2017-11-26 11:39 ` Simon Glass
2017-11-27 10:26 ` Felix Brack
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.