[PATCH v2 8/9] power: pmic: tps65910: add TPS65911 PMIC support

Svyatoslav Ryhel clamor95 at gmail.com
Fri Jul 21 08:19:27 CEST 2023


чт, 20 лип. 2023 р. о 22:43 Simon Glass <sjg at chromium.org> пише:
>
> On Thu, 20 Jul 2023 at 02:48, Svyatoslav Ryhel <clamor95 at gmail.com> wrote:
> >
> > Add support to bind the regulators/child nodes with the pmic.
> > Also adds the pmic i2c based read/write functions to access pmic
> > registers.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95 at gmail.com>
> > ---
> >  doc/device-tree-bindings/pmic/tps65911.txt | 78 ++++++++++++++++++++++
> >  drivers/power/pmic/pmic_tps65910_dm.c      | 49 +++++++++++++-
> >  include/power/tps65910_pmic.h              | 52 +++++++++++++++
> >  3 files changed, 176 insertions(+), 3 deletions(-)
> >  create mode 100644 doc/device-tree-bindings/pmic/tps65911.txt
> >
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> nits below
>
> [..]
>
> > diff --git a/drivers/power/pmic/pmic_tps65910_dm.c b/drivers/power/pmic/pmic_tps65910_dm.c
> > index e03ddc98d7..770010d53d 100644
> > --- a/drivers/power/pmic/pmic_tps65910_dm.c
> > +++ b/drivers/power/pmic/pmic_tps65910_dm.c
> > @@ -11,13 +11,19 @@
> >  #include <power/regulator.h>
> >  #include <power/tps65910_pmic.h>
> >
> > -static const struct pmic_child_info pmic_children_info[] = {
> > +static const struct pmic_child_info tps65910_children_info[] = {
> >         { .prefix = "ldo_", .driver = TPS65910_LDO_DRIVER },
> >         { .prefix = "buck_", .driver = TPS65910_BUCK_DRIVER },
> >         { .prefix = "boost_", .driver = TPS65910_BOOST_DRIVER },
> >         { },
> >  };
> >
> > +static const struct pmic_child_info tps65911_children_info[] = {
> > +       { .prefix = "ldo", .driver = TPS65911_LDO_DRIVER },
> > +       { .prefix = "vdd", .driver = TPS65911_VDD_DRIVER },
> > +       { },
> > +};
> > +
> >  static int pmic_tps65910_reg_count(struct udevice *dev)
> >  {
> >         return TPS65910_NUM_REGS;
> > @@ -47,10 +53,29 @@ static int pmic_tps65910_read(struct udevice *dev, uint reg, u8 *buffer,
> >         return ret;
> >  }
> >
> > +static int pmic_tps65910_poweroff(struct udevice *dev)
> > +{
> > +       int ret;
> > +
> > +       ret = pmic_reg_read(dev, TPS65910_REG_DEVICE_CTRL);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       ret |= DEVCTRL_PWR_OFF_MASK;
>
> Please use a separate var for this. We use ret for return values, as
> you have above.
>

I have used ret here because pmic_reg_read returns either value of reg
or -errno, I need to modify the value got from
TPS65910_REG_DEVICE_CTRL and not to introduce new variable and perform
unnecessary reassignments I continued with ret.

> > +
> > +       pmic_reg_write(dev, TPS65910_REG_DEVICE_CTRL, ret);
> > +
> > +       ret |= DEVCTRL_DEV_OFF_MASK;
> > +       ret &= ~DEVCTRL_DEV_ON_MASK;
> > +
> > +       return pmic_reg_write(dev, TPS65910_REG_DEVICE_CTRL, ret);
> > +}
> > +
> >  static int pmic_tps65910_bind(struct udevice *dev)
> >  {
> >         ofnode regulators_node;
> >         int children;
> > +       int type = dev_get_driver_data(dev);
> >
> >         regulators_node = dev_read_subnode(dev, "regulators");
> >         if (!ofnode_valid(regulators_node)) {
> > @@ -58,7 +83,19 @@ static int pmic_tps65910_bind(struct udevice *dev)
> >                 return -EINVAL;
> >         }
> >
> > -       children = pmic_bind_children(dev, regulators_node, pmic_children_info);
> > +       switch (type) {
>
> Why not always binding them?
>
> > +       case TPS65910:
> > +               children = pmic_bind_children(dev, regulators_node,
> > +                                             tps65910_children_info);
> > +               break;
> > +       case TPS65911:
> > +               children = pmic_bind_children(dev, regulators_node,
> > +                                             tps65911_children_info);
> > +               break;
> > +       default:
> > +               log_err("unknown PMIC type\n");
> > +       }
> > +
> >         if (!children)
> >                 debug("%s has no children (regulators)\n", dev->name);
> >
> > @@ -67,6 +104,10 @@ static int pmic_tps65910_bind(struct udevice *dev)
> >
> >  static int pmic_tps65910_probe(struct udevice *dev)
> >  {
> > +       struct uc_pmic_priv *priv = dev_get_uclass_priv(dev);
> > +
> > +       priv->sys_pow_ctrl = dev_read_bool(dev, "ti,system-power-controller");
> > +
> >         /* 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
> > @@ -76,13 +117,15 @@ static int pmic_tps65910_probe(struct udevice *dev)
> >  }
> >
> >  static struct dm_pmic_ops pmic_tps65910_ops = {
> > +       .poweroff = pmic_tps65910_poweroff,
> >         .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" },
> > +       { .compatible = "ti,tps65910", .data = TPS65910 },
> > +       { .compatible = "ti,tps65911", .data = TPS65911 },
> >         { /* sentinel */ }
> >  };
> >
> > diff --git a/include/power/tps65910_pmic.h b/include/power/tps65910_pmic.h
> > index 66214786d3..282863805a 100644
> > --- a/include/power/tps65910_pmic.h
> > +++ b/include/power/tps65910_pmic.h
> > @@ -6,6 +6,9 @@
> >  #ifndef __TPS65910_PMIC_H_
> >  #define __TPS65910_PMIC_H_
> >
> > +#define TPS65910                       1
> > +#define TPS65911                       2
> > +
> >  #define TPS65910_I2C_SEL_MASK          (0x1 << 4)
> >  #define TPS65910_VDD_SR_MASK           (0x1 << 7)
> >  #define TPS65910_GAIN_SEL_MASK         (0x3 << 6)
> > @@ -17,6 +20,11 @@
> >  #define TPS65910_SUPPLY_STATE_OFF      0x0
> >  #define TPS65910_SUPPLY_STATE_ON       0x1
> >
> > +/* TPS65910 DEVICE_CTRL bits */
> > +#define DEVCTRL_PWR_OFF_MASK           BIT(7)
> > +#define DEVCTRL_DEV_ON_MASK            BIT(2)
> > +#define DEVCTRL_DEV_OFF_MASK           BIT(0)
> > +
> >  /* i2c registers */
> >  enum {
> >         TPS65910_REG_RTC_SEC                    = 0x00,
> > @@ -126,4 +134,48 @@ struct tps65910_regulator_pdata {
> >  #define TPS65910_BOOST_DRIVER  "tps65910_boost"
> >  #define TPS65910_LDO_DRIVER    "tps65910_ldo"
> >
> > +/* tps65911 i2c registers */
> > +enum {
> > +       TPS65911_REG_VIO                        = 0x20,
> > +       TPS65911_REG_VDD1,
> > +       TPS65911_REG_VDD1_OP,
> > +       TPS65911_REG_VDD1_SR,
> > +       TPS65911_REG_VDD2,
> > +       TPS65911_REG_VDD2_OP,
> > +       TPS65911_REG_VDD2_SR,
> > +       TPS65911_REG_VDDCTRL,
> > +       TPS65911_REG_VDDCTRL_OP,
> > +       TPS65911_REG_VDDCTRL_SR,
> > +       TPS65911_REG_LDO1                       = 0x30,
> > +       TPS65911_REG_LDO2,
> > +       TPS65911_REG_LDO5,
> > +       TPS65911_REG_LDO8,
> > +       TPS65911_REG_LDO7,
> > +       TPS65911_REG_LDO6,
> > +       TPS65911_REG_LDO4,
> > +       TPS65911_REG_LDO3,
> > +};
> > +
> > +#define TPS65911_VDD_NUM               4
> > +#define TPS65911_LDO_NUM               8
> > +
> > +#define TPS65911_VDD_VOLT_MAX          1500000
> > +#define TPS65911_VDD_VOLT_MIN          600000
> > +#define TPS65911_VDD_VOLT_BASE         562500
> > +
> > +#define TPS65911_LDO_VOLT_MAX          3300000
> > +#define TPS65911_LDO_VOLT_BASE         800000
> > +
> > +#define TPS65911_LDO_SEL_MASK          (0x3f << 2)
> > +
> > +#define TPS65911_LDO124_VOLT_MAX_HEX   0x32
> > +#define TPS65911_LDO358_VOLT_MAX_HEX   0x19
> > +#define TPS65911_LDO358_VOLT_MIN_HEX   0x02
> > +
> > +#define TPS65911_LDO124_VOLT_STEP      50000
> > +#define TPS65911_LDO358_VOLT_STEP      100000
> > +
> > +#define TPS65911_VDD_DRIVER            "tps65911_vdd"
> > +#define TPS65911_LDO_DRIVER            "tps65911_ldo"
>
> Drop the TPS65911 prefixes if you can...this is only included in one
> file i think.
>
> > +
> >  #endif /* __TPS65910_PMIC_H_ */
> > --
> > 2.39.2
> >
>
> Regards,
> Simon


More information about the U-Boot mailing list