Skip to contentSkip to conversation

power: pmic/regulator: Add basic support for TPS65910

PATCH Superseded Felix Brack 2 participants 2 messages Nov 28, 2017 – Nov 29, 2017

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 power: pmic/regulator: Add basic support for TPS65910

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

---

Changes in v2:
- move information about regulator supply power from the PMIC to the regulator driver which greatly simplifies and streamlines the code of both drivers (thanks Simon!)
- use dev_get_uclass_platdata() to get the regulator constraints instead of reinventing the wheel and reading them from the DT
- make pmic_tps65910_write() and pmic_tps65910_read() return the results of the i2c operation instead of obscuring it
- generally review and eventually fix return values
- modify regulator description in Kconfig
- get_ctrl_reg_from_unit_addr() uses a lookup array instead of a switch statement
- rename some local functions to prevent confusion with generic functions
- reformatting code for better readability

 drivers/power/pmic/Kconfig                   |   8 +
 drivers/power/pmic/Makefile                  |   1 +
 drivers/power/pmic/pmic_tps65910_dm.c        |  98 ++++++
 drivers/power/regulator/Kconfig              |   8 +
 drivers/power/regulator/Makefile             |   1 +
 drivers/power/regulator/tps65910_regulator.c | 456 +++++++++++++++++++++++++++
 include/power/tps65910_pmic.h                | 129 ++++++++
 7 files changed, 701 insertions(+)
 create mode 100644 drivers/power/pmic/pmic_tps65910_dm.c
 create mode 100644 drivers/power/regulator/tps65910_regulator.c
 create mode 100644 include/power/tps65910_pmic.h
drivers/power/pmic/Kconfig +8 -0
index e3f9e4d..5d49c93 100644
@@ -201,3 +201,11 @@ config POWER_MC34VR500
 	The MC34VR500 is used in conjunction with the FSL T1 and LS1 series
 	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.
drivers/power/pmic/Makefile +1 -0
index f7bdfa5..7d6c583 100644
@@ -19,6 +19,7 @@ obj-$(CONFIG_PMIC_RK8XX) += rk8xx.o
 obj-$(CONFIG_PMIC_RN5T567) += rn5t567.o
 obj-$(CONFIG_PMIC_TPS65090) += tps65090.o
 obj-$(CONFIG_PMIC_S5M8767) += s5m8767.o
+obj-$(CONFIG_DM_PMIC_TPS65910) += pmic_tps65910_dm.o
 obj-$(CONFIG_$(SPL_)PMIC_PALMAS) += palmas.o
 obj-$(CONFIG_$(SPL_)PMIC_LP873X) += lp873x.o
 obj-$(CONFIG_$(SPL_)PMIC_LP87565) += lp87565.o
drivers/power/pmic/pmic_tps65910_dm.c new file +98 -0

98 lines not on this page · raw message

drivers/power/regulator/Kconfig +8 -0
index c82a936..fb6a357 100644
@@ -168,3 +168,11 @@ 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 supports all
+	regulator types of the TPS65910 (BUCK, BOOST and LDO). It implements
+	the get/set api for value and enable.
drivers/power/regulator/Makefile +1 -0
index 18fb870..3eef297 100644
@@ -20,3 +20,4 @@ obj-$(CONFIG_REGULATOR_TPS65090) += tps65090_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_PALMAS) += palmas_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_LP873X) += lp873x_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_LP87565) += lp87565_regulator.o
+obj-$(CONFIG_DM_REGULATOR_TPS65910) += tps65910_regulator.o
drivers/power/regulator/tps65910_regulator.c new file +456 -0

456 lines not on this page · raw message

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

129 lines not on this page · raw message

Signature · 1 line
2.7.4
#1 lore raw

PATCH v2 1/1 power: pmic/regulator: Add basic support for TPS65910

Quoted 29 lines · Felix Brack

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

---

Changes in v2:
- move information about regulator supply power from the PMIC to the regulator driver which greatly simplifies and streamlines the code of both drivers (thanks Simon!)
- use dev_get_uclass_platdata() to get the regulator constraints instead of reinventing the wheel and reading them from the DT
- make pmic_tps65910_write() and pmic_tps65910_read() return the results of the i2c operation instead of obscuring it
- generally review and eventually fix return values
- modify regulator description in Kconfig
- get_ctrl_reg_from_unit_addr() uses a lookup array instead of a switch statement
- rename some local functions to prevent confusion with generic functions
- reformatting code for better readability

Quoted 11 lines
 drivers/power/pmic/Kconfig                   |   8 +
 drivers/power/pmic/Makefile                  |   1 +
 drivers/power/pmic/pmic_tps65910_dm.c        |  98 ++++++
 drivers/power/regulator/Kconfig              |   8 +
 drivers/power/regulator/Makefile             |   1 +
 drivers/power/regulator/tps65910_regulator.c | 456 +++++++++++++++++++++++++++
 include/power/tps65910_pmic.h                | 129 ++++++++
 7 files changed, 701 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

This looks good to me. I just have a few more nits.

[..]

Quoted 27 lines
drivers/power/regulator/Makefile +1 -0
index 18fb870..3eef297 100644
@@ -20,3 +20,4 @@ obj-$(CONFIG_REGULATOR_TPS65090) += tps65090_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_PALMAS) += palmas_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_LP873X) += lp873x_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_LP87565) += lp87565_regulator.o
+obj-$(CONFIG_DM_REGULATOR_TPS65910) += tps65910_regulator.o
drivers/power/regulator/tps65910_regulator.c new file +12 -0
new file mode 100644
index 0000000..8e29b53
@@ -0,0 +1,456 @@
+/*
+ * Copyright (C) EETS GmbH, 2017, Felix Brack <[email protected]>
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <power/pmic.h>
+#include <power/regulator.h>
+#include <power/tps65910_pmic.h>
+

[..]

drivers/power/regulator/tps65910_regulator.c line 135

Quoted 3 lines
drivers/power/regulator/tps65910_regulator.c new file +3 -0
+static int get_ctrl_reg_from_unit_addr(const int unit_addr)
+{
+       if (unit_addr >= 0 && unit_addr < ARRAY_SIZE(ctrl_regs))

You could make that parameter uint if you like and avoid the first check.

drivers/power/regulator/tps65910_regulator.c line 144

Quoted 9 lines
drivers/power/regulator/tps65910_regulator.c new file +9 -0
+               return ctrl_regs[unit_addr];
+       return -ENXIO;
+}
+
+static int tps65910_regulator_get_value(struct udevice *dev,
+                                       const struct regulator_props *rgp)
+{
+       int sel;
+       u8 val;

int val

(particularly as you check for val < 0 below)

Please fix in rest of file also. In general these small-size variables just force the compiler to mask values and don't add any benefit IMO. We may as well use int.

Quoted 7 lines
drivers/power/regulator/tps65910_regulator.c new file +7 -0
+       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;

drivers/power/regulator/tps65910_regulator.c line 161

Quoted 10 lines
drivers/power/regulator/tps65910_regulator.c new file +10 -0
+       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;

dev_get_platdata(dev)

Quoted 27 lines
drivers/power/regulator/tps65910_regulator.c new file +27 -0
+       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 tps65910_regulator_get_value(dev, &ldo_props_vdig1);
+       case TPS65910_UNIT_VDIG2:
+               return tps65910_regulator_get_value(dev, &ldo_props_vdig2);
+       case TPS65910_UNIT_VPLL:
+               return tps65910_regulator_get_value(dev, &ldo_props_vpll);
+       case TPS65910_UNIT_VDAC:
+               return tps65910_regulator_get_value(dev, &ldo_props_vdac);
+       case TPS65910_UNIT_VAUX1:
+               return tps65910_regulator_get_value(dev, &ldo_props_vaux1);
+       case TPS65910_UNIT_VAUX2:
+               return tps65910_regulator_get_value(dev, &ldo_props_vaux2);
+       case TPS65910_UNIT_VAUX33:
+               return tps65910_regulator_get_value(dev, &ldo_props_vaux33);
+       case TPS65910_UNIT_VMMC:
+               return tps65910_regulator_get_value(dev, &ldo_props_vmmc);
+       }
+
+       return 0;
+}
+

[..]

drivers/power/regulator/tps65910_regulator.c line 256

Quoted 10 lines
drivers/power/regulator/tps65910_regulator.c new file +10 -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);

Does pmic_reg_read() work here? Then you don't need the u8 val.

Quoted 7 lines
drivers/power/regulator/tps65910_regulator.c new file +7 -0
+       if (ret)
+               return ret;
+
+       /* bits 1:0 of regulator control register define state */
+       return ((val & TPS65910_SUPPLY_STATE_MASK) == 1);
+}
+

[..]

drivers/power/regulator/tps65910_regulator.c line 307

Quoted 5 lines
drivers/power/regulator/tps65910_regulator.c new file +5 -0
+static int tps65910_buck_get_value(struct udevice *dev)
+{
+       int vout = 0;
+
+       switch (dev->driver_data) {

dev_get_driver_data(dev)

But please can you store this in your priv/platdata structure so you set it up in probe() or ofdata_to_pdata()? Ideally this should be an enum field. Similar below.

Quoted 4 lines
drivers/power/regulator/tps65910_regulator.c new file +4 -0
+       case TPS65910_UNIT_VIO:
+               vout = tps65910_regulator_get_value(dev, &smps_props_vio);
+               break;
+       case TPS65910_UNIT_VDD1:

drivers/power/regulator/tps65910_regulator.c line 321

Quoted 10 lines
drivers/power/regulator/tps65910_regulator.c new file +10 -0
+               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;
+}
+

[..]

Regards,
Simon