[U-Boot] [PATCH 2/2] power: regulator: lp873x: Add regulator support

[Simon Glass]

Hi,

On 26 September 2016 at 00:00, Keerthy <j-keerthy at ti.com> wrote:
> The driver provides regulator set/get voltage
> enable/disable functions for lp873x family of PMICs.
>
> Signed-off-by: Keerthy <j-keerthy at ti.com>
> ---
>  drivers/power/regulator/Kconfig            |   8 +
>  drivers/power/regulator/Makefile           |   1 +
>  drivers/power/regulator/lp873x_regulator.c | 361 +++++++++++++++++++++++++++++
>  3 files changed, 370 insertions(+)
>  create mode 100644 drivers/power/regulator/lp873x_regulator.c
>
> diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
> index adb710a..84cf914 100644
> --- a/drivers/power/regulator/Kconfig
> +++ b/drivers/power/regulator/Kconfig
> @@ -123,3 +123,11 @@ config DM_REGULATOR_PALMAS
>         This enables implementation of driver-model regulator uclass
>         features for REGULATOR PALMAS and the family of PALMAS PMICs.
>         The driver implements get/set api for: value and enable.
> +
> +config DM_REGULATOR_LP873X
> +       bool "Enable driver for LP873X PMIC regulators"
> +        depends on PMIC_LP873X
> +       ---help---
> +       This enables implementation of driver-model regulator uclass
> +       features for REGULATOR LP873X and the family of LP873X PMICs.
> +       The driver implements get/set api for: value and enable.
> diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
> index 75080d4..2093048 100644
> --- a/drivers/power/regulator/Makefile
> +++ b/drivers/power/regulator/Makefile
> @@ -15,3 +15,4 @@ obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
>  obj-$(CONFIG_DM_REGULATOR_SANDBOX) += sandbox.o
>  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
> diff --git a/drivers/power/regulator/lp873x_regulator.c b/drivers/power/regulator/lp873x_regulator.c
> new file mode 100644
> index 0000000..7675173
> --- /dev/null
> +++ b/drivers/power/regulator/lp873x_regulator.c
> @@ -0,0 +1,361 @@
> +/*
> + * (C) Copyright 2016
> + * Texas Instruments Incorporated, <www.ti.com>
> + *
> + * Keerthy <j-keerthy at ti.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <fdtdec.h>
> +#include <errno.h>
> +#include <dm.h>
> +#include <i2c.h>
> +#include <power/pmic.h>
> +#include <power/regulator.h>
> +#include <power/lp873x.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static const char lp873x_buck_ctrl[LP873X_BUCK_NUM] = {0x2, 0x4};
> +static const char lp873x_buck_volt[LP873X_BUCK_NUM] = {0x6, 0x7};
> +static const char lp873x_ldo_ctrl[LP873X_LDO_NUM] = {0x8, 0x9};
> +static const char lp873x_ldo_volt[LP873X_LDO_NUM] = {0xA, 0xB};
> +
> +static int lp873x_buck_enable(struct udevice *dev, int op, bool *enable)
> +{
> +       int ret;
> +       uint8_t val;
> +       unsigned int adr;
> +       struct dm_regulator_uclass_platdata *uc_pdata;
> +
> +       uc_pdata = dev_get_uclass_platdata(dev);
> +       adr = uc_pdata->ctrl_reg;
> +
> +       ret = dm_i2c_u8_read(dev->parent, adr, &val);
> +       if (ret < 0)
> +               return ret;

You can use

val = dm_i2c_reg_reg(dev->parent, adr);
if (val < 0)
   return val;

What is the benefit of using extra u8 variable?

But in this case, you should use pmic_reg_read(). We should avoid i2c
access in drivers. There is no need for it, and the PMIC may come in a
SPI version one day.

> +
> +       if (op == PMIC_OP_GET) {
> +               val &= LP873X_BUCK_MODE_MASK;
> +
> +               if (val)
> +                       *enable = true;
> +               else
> +                       *enable = false;
> +
> +               return 0;
> +       } else if (op == PMIC_OP_SET) {
> +               if (*enable)
> +                       val |= LP873X_BUCK_MODE_MASK;
> +               else
> +                       val &= ~(LP873X_BUCK_MODE_MASK);
> +               ret = dm_i2c_u8_write(dev->parent, adr, &val);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}

Regards,
Simon