[U-Boot] [PATCH v2 10/12] odroid: board: add support to dm pmic api

[Simon Glass]

Hi Przemyslaw,

On 3 March 2015 at 09:24, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
> This commit change the old pmic framework calls with the new ones.
>
> Signed-off-by: Przemyslaw Marczak <p.marczak at samsung.com>
> ---
> Changes v2:
> - remove board_init_i2c() call
> - update regulator calls
> - update headers
> - samsung/misc.c: include required header
> ---
>  board/samsung/common/misc.c   |  1 +
>  board/samsung/odroid/odroid.c | 52 ++++++++++++++++++++++++++-----------------
>  2 files changed, 33 insertions(+), 20 deletions(-)
>
> diff --git a/board/samsung/common/misc.c b/board/samsung/common/misc.c
> index 4538ac7..18d71e8 100644
> --- a/board/samsung/common/misc.c
> +++ b/board/samsung/common/misc.c
> @@ -16,6 +16,7 @@
>  #include <asm/arch/cpu.h>
>  #include <asm/gpio.h>
>  #include <linux/input.h>
> +#include <dm.h>
>  #include <power/pmic.h>
>  #include <mmc.h>
>
> diff --git a/board/samsung/odroid/odroid.c b/board/samsung/odroid/odroid.c
> index bff6ac9..2448cde 100644
> --- a/board/samsung/odroid/odroid.c
> +++ b/board/samsung/odroid/odroid.c
> @@ -12,7 +12,9 @@
>  #include <asm/arch/gpio.h>
>  #include <asm/gpio.h>
>  #include <asm/arch/cpu.h>
> +#include <dm.h>
>  #include <power/pmic.h>
> +#include <power/regulator.h>
>  #include <power/max77686_pmic.h>
>  #include <errno.h>
>  #include <usb.h>
> @@ -402,15 +404,23 @@ static void board_gpio_init(void)
>
>  static int pmic_init_max77686(void)
>  {
> -       struct pmic *p = pmic_get("MAX77686_PMIC");
> +       struct udevice *reg;
> +       int type;
>
> -       if (pmic_probe(p))
> +       if (regulator_get("max77686", &reg)) {
> +               error("Regulator get error\n");
>                 return -ENODEV;
> +       }
>
>         /* Set LDO Voltage */
> -       max77686_set_ldo_voltage(p, 20, 1800000);       /* LDO20 eMMC */
> -       max77686_set_ldo_voltage(p, 21, 2800000);       /* LDO21 SD */
> -       max77686_set_ldo_voltage(p, 22, 2800000);       /* LDO22 eMMC */
> +       type = REGULATOR_TYPE_LDO;
> +       regulator_set_value(reg, type, 20, 1800000);    /* LDO20 eMMC */
> +       regulator_set_value(reg, type, 21, 2800000);    /* LDO21 SD */
> +       regulator_set_value(reg, type, 22, 2800000);    /* LDO22 eMMC */
> +
> +       regulator_set_mode(reg, type, 20, OPMODE_ON);
> +       regulator_set_mode(reg, type, 21, OPMODE_ON);
> +       regulator_set_mode(reg, type, 22, OPMODE_ON);

These 20, 21, 22 values seem bad to me. Do we not have names? Also I
see in the device tree that these regulators have the same min and max
voltage, so perhaps you can use that voltage automatically?

I think when you change the LDOs to devices you might end up with:

struct udevice *ldo;

ret = regulator_get("VCCQ_MMC2_2.8V", &ldo);   // use the regulator
device name which comes from regulator-name property.
if (ret) {
   debug(...)
   goto err;
}
ret = regulator_set_voltage(ldo, 1800000);
...error check
ret = regulator_set_mode(ldo, OPMODE_ON);
...error check

>
>         return 0;
>  }
> @@ -435,7 +445,6 @@ int exynos_init(void)
>
>  int exynos_power_init(void)
>  {
> -       pmic_init(0);
>         pmic_init_max77686();
>
>         return 0;
> @@ -444,19 +453,21 @@ int exynos_power_init(void)
>  #ifdef CONFIG_USB_GADGET
>  static int s5pc210_phy_control(int on)
>  {
> -       struct pmic *p_pmic;
> +       struct udevice *reg;
> +       int type;
>
> -       p_pmic = pmic_get("MAX77686_PMIC");
> -       if (!p_pmic)
> +       if (regulator_get("max77686", &reg)) {
> +               error("Regulator get error\n");
>                 return -ENODEV;
> +       }
>
> -       if (pmic_probe(p_pmic))
> -               return -1;
> +       type = REGULATOR_TYPE_LDO;
>
>         if (on)
> -               return max77686_set_ldo_mode(p_pmic, 12, OPMODE_ON);
> +               return regulator_set_mode(reg, type, 12, OPMODE_ON);
>         else
> -               return max77686_set_ldo_mode(p_pmic, 12, OPMODE_LPM);
> +               return regulator_set_mode(reg, type, 12, OPMODE_LPM);
> +
>  }
>
>  struct s3c_plat_otg_data s5pc210_otg_data = {
> @@ -473,7 +484,8 @@ struct s3c_plat_otg_data s5pc210_otg_data = {
>  int board_usb_init(int index, enum usb_init_type init)
>  {
>  #ifdef CONFIG_CMD_USB
> -       struct pmic *p_pmic;
> +       struct udevice *reg;
> +       int type, ret;
>
>         /* Set Ref freq 0 => 24MHz, 1 => 26MHz*/
>         /* Odroid Us have it at 24MHz, Odroid Xs at 26MHz */
> @@ -491,14 +503,14 @@ int board_usb_init(int index, enum usb_init_type init)
>         /* Power off and on BUCK8 for LAN9730 */
>         debug("LAN9730 - Turning power buck 8 OFF and ON.\n");
>
> -       p_pmic = pmic_get("MAX77686_PMIC");
> -       if (p_pmic && !pmic_probe(p_pmic)) {
> -               max77686_set_buck_voltage(p_pmic, 8, 750000);
> -               max77686_set_buck_voltage(p_pmic, 8, 3300000);
> +       if (regulator_get("max77686", &reg)) {
> +               type = REGULATOR_TYPE_BUCK;
> +               ret = regulator_set_value(reg, type, 8, 750000);
> +               ret |= regulator_set_value(reg, type, 8, 3300000);
> +               if (ret)
> +                       error("Can't set regulator\n");

We should return a proper error number. Also print it here as it might
provide clues.

>         }
> -
>  #endif
> -
>         debug("USB_udc_probe\n");
>         return s3c_udc_probe(&s5pc210_otg_data);
>  }
> --
> 1.9.1
>

Regards,
Simon