[U-Boot] [PATCH v2] power: pmic/regulator: Add basic support for TPS65910
Simon Glass
sjg at chromium.org
Wed Nov 29 13:08:36 UTC 2017
On 28 November 2017 at 07:16, Felix Brack <fb at 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 at 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 at 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
More information about the U-Boot
mailing list