On 28 November 2017 at 07:16, 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 v3 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
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
This looks good to me. I just have a few more nits.
[..]
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..8e29b53 --- /dev/null +++ b/drivers/power/regulator/tps65910_regulator.c @@ -0,0 +1,456 @@ +/*
- 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>
[..]
+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.
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.
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;
dev_get_platdata(dev)
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;+}
[..]
+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.
if (ret)return ret;/* bits 1:0 of regulator control register define state */return ((val & TPS65910_SUPPLY_STATE_MASK) == 1);+}
[..]
+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.
case TPS65910_UNIT_VIO:vout = tps65910_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;+}
[..]
Regards, Simon