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 <[email protected]> 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 <[email protected]> >> --- >> >> drivers/power/pmic/Kconfig | 8 + >> drivers/power/pmic/Makefile | 1 + >> drivers/power/pmic/pmic_tps65910_dm.c | 138 ++++++++ >> drivers/power/regulator/Kconfig | 7 + >> drivers/power/regulator/Makefile | 1 + >> drivers/power/regulator/tps65910_regulator.c | 493 >> +++++++++++++++++++++++++++ >> include/power/tps65910_pmic.h | 130 +++++++ >> 7 files changed, 778 insertions(+) >> create mode 100644 drivers/power/pmic/pmic_tps65910_dm.c >> create mode 100644 drivers/power/regulator/tps65910_regulator.c >> create mode 100644 include/power/tps65910_pmic.h >> >> 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 <[email protected]> >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include <common.h> >> +#include <dm.h> >> +#include <i2c.h> >> +#include <power/pmic.h> >> +#include <power/regulator.h> >> +#include <power/tps65910_pmic.h> >> + >> +DECLARE_GLOBAL_DATA_PTR; >> + >> +static const struct pmic_child_info pmic_children_info[] = { >> + { .prefix = "ldo_", .driver = TPS65910_LDO_DRIVER }, >> + { .prefix = "buck_", .driver = TPS65910_BUCK_DRIVER }, >> + { .prefix = "boost_", .driver = TPS65910_BOOST_DRIVER }, >> + { }, >> +}; >> + >> +static const char * const supply_names[] = { >> + "vccio-supply", >> + "vcc1-supply", >> + "vcc2-supply", >> + "vcc3-supply", >> + "vcc4-supply", >> + "vcc5-supply", >> + "vcc6-supply", >> + "vcc7-supply", >> +}; >> + >> +static int pmic_tps65910_reg_count(struct udevice *dev) >> +{ >> + 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 <[email protected]> >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include <common.h> >> +#include <dm.h> >> +#include <power/pmic.h> >> +#include <power/regulator.h> >> +#include <power/tps65910_pmic.h> >> + >> +#define VOUT_CHOICE_COUNT 4 >> + >> +/* >> + * struct regulator_props - Properties of a LDO and VIO SMPS regulator >> + * >> + * All of these regulators allow setting one out of four output voltages. >> + * These output voltages are only achievable when supplying the regulator >> + * with a minimum input voltage. >> + * >> + * @vin_min[]: minimum supply input voltage in uV required to achieve the >> + * corresponding vout[] voltage >> + * @vout[]: regulator output voltage in uV >> + * @reg: I2C register used to set regulator voltage >> + */ >> +struct regulator_props { >> + int vin_min[VOUT_CHOICE_COUNT]; >> + int vout[VOUT_CHOICE_COUNT]; >> + int reg; >> +}; >> + >> +static const struct regulator_props ldo_props_vdig1 = { >> + .vin_min = { 1700000, 2100000, 2700000, 3200000 }, >> + .vout = { 1200000, 1500000, 1800000, 2700000 }, >> + .reg = TPS65910_REG_VDIG1 >> +}; >> + >> +static const struct regulator_props ldo_props_vdig2 = { >> + .vin_min = { 1700000, 1700000, 1700000, 2700000 }, >> + .vout = { 1000000, 1100000, 1200000, 1800000 }, >> + .reg = TPS65910_REG_VDIG2 >> +}; >> + >> +static const struct regulator_props ldo_props_vpll = { >> + .vin_min = { 2700000, 2700000, 2700000, 3000000 }, >> + .vout = { 1000000, 1100000, 1800000, 2500000 }, >> + .reg = TPS65910_REG_VPLL >> +}; >> + >> +static const struct regulator_props ldo_props_vdac = { >> + .vin_min = { 2700000, 3000000, 3200000, 3200000 }, >> + .vout = { 1800000, 2600000, 2800000, 2850000 }, >> + .reg = TPS65910_REG_VDAC >> +}; >> + >> +static const struct regulator_props ldo_props_vaux1 = { >> + .vin_min = { 2700000, 3200000, 3200000, 3200000 }, >> + .vout = { 1800000, 2500000, 2800000, 2850000 }, >> + .reg = TPS65910_REG_VAUX1 >> +}; >> + >> +static const struct regulator_props ldo_props_vaux2 = { >> + .vin_min = { 2700000, 3200000, 3200000, 3600000 }, >> + .vout = { 1800000, 2800000, 2900000, 3300000 }, >> + .reg = TPS65910_REG_VAUX2 >> +}; >> + >> +static const struct regulator_props ldo_props_vaux33 = { >> + .vin_min = { 2700000, 2700000, 3200000, 3600000 }, >> + .vout = { 1800000, 2000000, 2800000, 3300000 }, >> + .reg = TPS65910_REG_VAUX33 >> +}; >> + >> +static const struct regulator_props ldo_props_vmmc = { >> + .vin_min = { 2700000, 3200000, 3200000, 3600000 }, >> + .vout = { 1800000, 2800000, 3000000, 3300000 }, >> + .reg = TPS65910_REG_VMMC >> +}; >> + >> +static const struct regulator_props smps_props_vio = { >> + .vin_min = { 3200000, 3200000, 4000000, 4400000 }, >> + .vout = { 1500000, 1800000, 2500000, 3300000 }, >> + .reg = TPS65910_REG_VIO >> +}; >> + >> +static int get_ctrl_reg_from_unit_addr(const int unit_addr) >> +{ >> + switch (unit_addr) { >> + case TPS65910_UNIT_VRTC: >> + return TPS65910_REG_VRTC; >> + case TPS65910_UNIT_VIO: >> + return TPS65910_REG_VIO; >> + case TPS65910_UNIT_VDD1: >> + return TPS65910_REG_VDD1; >> + case TPS65910_UNIT_VDD2: >> + return TPS65910_REG_VDD2; >> + case TPS65910_UNIT_VDD3: >> + return TPS65910_REG_VDD3; >> + case TPS65910_UNIT_VDIG1: >> + return TPS65910_REG_VDIG1; >> + case TPS65910_UNIT_VDIG2: >> + return TPS65910_REG_VDIG2; >> + case TPS65910_UNIT_VPLL: >> + return TPS65910_REG_VPLL; >> + case TPS65910_UNIT_VDAC: >> + return TPS65910_REG_VDAC; >> + case TPS65910_UNIT_VAUX1: >> + return TPS65910_REG_VAUX1; >> + 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 <[email protected]> >> + * >> + * 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
