Re: [PATCH v3 3/7] power: regulator: max77663: add regulator support

Hi Svyatoslav,

On Sun, 23 Jul 2023 at 06:28, Svyatoslav Ryhel clamor95@gmail.com wrote:

The driver provides regulator set/get voltage enable/disable functions for MAXIM MAX77663 PMICs.

Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com

drivers/power/regulator/Kconfig | 8 + drivers/power/regulator/Makefile | 1 + drivers/power/regulator/max77663_regulator.c | 358 +++++++++++++++++++ 3 files changed, 367 insertions(+) create mode 100644 drivers/power/regulator/max77663_regulator.c

Reviewed-by: Simon Glass sjg@chromium.org

nits below

diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig index eb5aa38c1c..571debd54e 100644 --- a/drivers/power/regulator/Kconfig +++ b/drivers/power/regulator/Kconfig @@ -141,6 +141,14 @@ config SPL_REGULATOR_PWM This config enables implementation of driver-model regulator uclass features for PWM regulators in SPL.

+config DM_REGULATOR_MAX77663

•   bool "Enable Driver Model for REGULATOR MAX77663"
  

•   depends on DM_REGULATOR && DM_PMIC_MAX77663
  

•   ---help---
  

•   This config enables implementation of driver-model regulator uclass
  

•   features for REGULATOR MAX77663. The driver implements get/set api for:
  

•   value and enable.
  

any thoughts on the feature set of this regulator?

config DM_REGULATOR_MAX77686 bool "Enable Driver Model for REGULATOR MAX77686" depends on DM_REGULATOR && DM_PMIC_MAX77686 diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile index d9e0cd5949..8d73169b50 100644 --- a/drivers/power/regulator/Makefile +++ b/drivers/power/regulator/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_REGULATOR_AS3722) += as3722_regulator.o obj-$(CONFIG_$(SPL_)REGULATOR_AXP) += axp_regulator.o obj-$(CONFIG_$(SPL_)REGULATOR_AXP_USB_POWER) += axp_usb_power.o obj-$(CONFIG_$(SPL_)DM_REGULATOR_DA9063) += da9063.o +obj-$(CONFIG_$(SPL_)DM_REGULATOR_MAX77663) += max77663_regulator.o obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o obj-$(CONFIG_DM_REGULATOR_NPCM8XX) += npcm8xx_regulator.o obj-$(CONFIG_$(SPL_)DM_PMIC_PFUZE100) += pfuze100.o diff --git a/drivers/power/regulator/max77663_regulator.c b/drivers/power/regulator/max77663_regulator.c new file mode 100644 index 0000000000..cbc275b30b --- /dev/null +++ b/drivers/power/regulator/max77663_regulator.c @@ -0,0 +1,358 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright(C) 2023 Svyatoslav Ryhel clamor95@gmail.com
• */

+#include <common.h> +#include <fdtdec.h> +#include <errno.h> +#include <dm.h> +#include <power/pmic.h> +#include <power/regulator.h> +#include <power/max77663.h>

+/* fist row is control registers, second is voltage registers */ +static const char max77663_sd_reg[][MAX77663_SD_NUM] = {

•   { 0x1d, 0x1e, 0x1f, 0x20, 0x21 },
  

•   { 0x16, 0x17, 0x18, 0x19, 0x2a },
  

+};

+static const char max77663_ldo_reg[MAX77663_LDO_NUM] = {

•   0x23, 0x25, 0x27, 0x29, 0x2b, 0x2d, 0x2f, 0x31, 0x33
  

+};

+static int max77663_sd_enable(struct udevice *dev, int op, bool *enable) +{

•   struct dm_regulator_uclass_plat *uc_pdata =
  

•                                   dev_get_uclass_plat(dev);
  

•   u32 adr = uc_pdata->ctrl_reg;
  

•   int ret;
  

•   ret = pmic_reg_read(dev->parent, adr);
  

•   if (ret < 0)
  

•           return ret;
  

•   if (op == PMIC_OP_GET) {
  

•           if (ret & MAX77663_SD_STATUS_MASK)
  

•                   *enable = true;
  

•           else
  

•                   *enable = false;
  

•           return 0;
  

•   } else if (op == PMIC_OP_SET) {
  

•           ret &= ~(MAX77663_SD_STATUS_MASK);
  

If that macro is set up correct you should not need the ()

•           if (*enable)
  

•                   ret |= MAX77663_SD_STATUS_MASK;
  

•           ret = pmic_reg_write(dev->parent, adr, ret);
  

•           if (ret)
  

•                   return ret;
  

•   }
  

•   return 0;
  

+}

+static int max77663_sd_volt2hex(int sd, int uV) +{

•   switch (sd) {
  

•   case 0:
  

•           /* SD0 has max voltage 1.4V */
  

•           if (uV > MAX77663_SD0_VOLT_MAX)
  

•                   return -EINVAL;
  

•           break;
  

•   case 1:
  

•           /* SD1 has max voltage 1.55V */
  

•           if (uV > MAX77663_SD1_VOLT_MAX)
  

•                   return -EINVAL;
  

•           break;
  

•   default:
  

•           /* SD2 and SD3 have max voltage 3.79V */
  

•           if (uV > MAX77663_SD_VOLT_MAX)
  

•                   return -EINVAL;
  

•           break;
  

•   };
  

•   if (uV < MAX77663_SD_VOLT_MIN)
  

•           uV = MAX77663_SD_VOLT_MIN;
  

•   return (uV - MAX77663_SD_VOLT_BASE) / 12500;
  

If the header file with these defines is only included by this driver, you can drop the MAX77663_ prefix and make everything shorter.

+}

+static int max77663_sd_hex2volt(int sd, int hex)

What is sd? What is hex? Please add a function comment.

+{

•   switch (sd) {
  

•   case 0:
  

•           /* SD0 has max voltage 1.4V */
  

•           if (hex > MAX77663_SD0_VOLT_MAX_HEX)
  

•                   return -EINVAL;
  

•           break;
  

•   case 1:
  

•           /* SD1 has max voltage 1.55V */
  

•           if (hex > MAX77663_SD1_VOLT_MAX_HEX)
  

•                   return -EINVAL;
  

•           break;
  

•   default:
  

•           /* SD2 and SD3 have max voltage 3.79V */
  

•           if (hex > MAX77663_SD_VOLT_MAX_HEX)
  

•                   return -EINVAL;
  

•           break;
  

•   };
  

•   if (hex < MAX77663_SD_VOLT_MIN_HEX)
  

•           hex = MAX77663_SD_VOLT_MIN_HEX;
  

•   return MAX77663_SD_VOLT_BASE + hex * 12500;
  

+}

+static int max77663_sd_val(struct udevice *dev, int op, int *uV) +{

•   struct dm_regulator_uclass_plat *uc_pdata =
  

•                                   dev_get_uclass_plat(dev);
  

•   u32 adr = uc_pdata->volt_reg;
  

•   int id = dev->driver_data;
  

•   int ret;
  

•   if (op == PMIC_OP_GET) {
  

•           ret = pmic_reg_read(dev->parent, adr);
  

•           if (ret < 0)
  

•                   return ret;
  

•           *uV = 0;
  

•           ret = max77663_sd_hex2volt(id, ret);
  

•           if (ret < 0)
  

•                   return ret;
  

•           *uV = ret;
  

•           return 0;
  

•   }
  

•   /* SD regulators use entire register for voltage */
  

•   ret = max77663_sd_volt2hex(id, *uV);
  

•   if (ret < 0)
  

•           return ret;
  

•   return pmic_reg_write(dev->parent, adr, ret);
  

+}

+static int max77663_sd_probe(struct udevice *dev) +{

•   struct dm_regulator_uclass_plat *uc_pdata =
  

•                                   dev_get_uclass_plat(dev);
  

•   uc_pdata->type = REGULATOR_TYPE_BUCK;
  

•   uc_pdata->ctrl_reg = max77663_sd_reg[0][dev->driver_data];
  

•   uc_pdata->volt_reg = max77663_sd_reg[1][dev->driver_data];
  

•   return 0;
  

+}

+static int sd_get_value(struct udevice *dev) +{

•   int uV;
  

•   int ret;
  

•   ret = max77663_sd_val(dev, PMIC_OP_GET, &uV);
  

•   if (ret)
  

•           return ret;
  

•   return uV;
  

+}

+static int sd_set_value(struct udevice *dev, int uV) +{

•   return max77663_sd_val(dev, PMIC_OP_SET, &uV);
  

+}

+static int sd_get_enable(struct udevice *dev) +{

•   bool enable = false;
  

•   int ret;
  

•   ret = max77663_sd_enable(dev, PMIC_OP_GET, &enable);
  

•   if (ret)
  

•           return ret;
  

•   return enable;
  

+}

+static int sd_set_enable(struct udevice *dev, bool enable) +{

•   return max77663_sd_enable(dev, PMIC_OP_SET, &enable);
  

+}

+static const struct dm_regulator_ops max77663_sd_ops = {

•   .get_value  = sd_get_value,
  

•   .set_value  = sd_set_value,
  

•   .get_enable = sd_get_enable,
  

•   .set_enable = sd_set_enable,
  

+};

+U_BOOT_DRIVER(max77663_sd) = {

•   .name = MAX77663_SD_DRIVER,
  

•   .id = UCLASS_REGULATOR,
  

•   .ops = &max77663_sd_ops,
  

•   .probe = max77663_sd_probe,
  

+};

+static int max77663_ldo_enable(struct udevice *dev, int op, bool *enable) +{

•   struct dm_regulator_uclass_plat *uc_pdata =
  

•                                   dev_get_uclass_plat(dev);
  

•   u32 adr = uc_pdata->ctrl_reg;
  

•   int ret;
  

•   ret = pmic_reg_read(dev->parent, adr);
  

•   if (ret < 0)
  

•           return ret;
  

•   if (op == PMIC_OP_GET) {
  

•           if (ret & MAX77663_LDO_STATUS_MASK)
  

•                   *enable = true;
  

•           else
  

•                   *enable = false;
  

•           return 0;
  

•   } else if (op == PMIC_OP_SET) {
  

•           ret &= ~(MAX77663_LDO_STATUS_MASK);
  

•           if (*enable)
  

•                   ret |= MAX77663_LDO_STATUS_MASK;
  

•           ret = pmic_reg_write(dev->parent, adr, ret);
  

•           if (ret)
  

•                   return ret;
  

•   }
  

•   return 0;
  

+}

+static int max77663_ldo_volt2hex(int ldo, int uV) +{

•   switch (ldo) {
  

•   case 0:
  

•   case 1:
  

•           if (uV > MAX77663_LDO01_VOLT_MAX)
  

•                   return -EINVAL;
  

•           return (uV - MAX77663_LDO_VOLT_BASE) / 25000;
  

•   case 4:
  

•           if (uV > MAX77663_LDO4_VOLT_MAX)
  

•                   return -EINVAL;
  

•           return (uV - MAX77663_LDO_VOLT_BASE) / 12500;
  

•   default:
  

•           if (uV > MAX77663_LDO_VOLT_MAX)
  

•                   return -EINVAL;
  

•           return (uV - MAX77663_LDO_VOLT_BASE) / 50000;
  

•   };
  

+}

+static int max77663_ldo_hex2volt(int ldo, int hex) +{

•   if (hex > MAX77663_LDO_VOLT_MAX_HEX)
  

•           return -EINVAL;
  

•   switch (ldo) {
  

•   case 0:
  

•   case 1:
  

•           return (hex * 25000) + MAX77663_LDO_VOLT_BASE;
  

•   case 4:
  

•           return (hex * 12500) + MAX77663_LDO_VOLT_BASE;
  

•   default:
  

•           return (hex * 50000) + MAX77663_LDO_VOLT_BASE;
  

•   };
  

+}

+static int max77663_ldo_val(struct udevice *dev, int op, int *uV)

We tend to add a 'p' suffix to return values and use snake case, i.e int *uvp

+{

•   struct dm_regulator_uclass_plat *uc_pdata =
  

•                                   dev_get_uclass_plat(dev);
  

•   u32 adr = uc_pdata->ctrl_reg;
  

•   int id = dev->driver_data;
  

•   int hex, ret;
  

hex is a strange varname. Does it have an actual meaning? Perhaps it is the code for the microvolts?

•   ret = pmic_reg_read(dev->parent, adr);
  

•   if (ret < 0)
  

•           return ret;
  

•   if (op == PMIC_OP_GET) {
  

•           *uV = 0;
  

•           ret = max77663_ldo_hex2volt(id, ret & MAX77663_LDO_VOLT_MASK);
  

•           if (ret < 0)
  

•                   return ret;
  

•           *uV = ret;
  

•           return 0;
  

•   }
  

•   hex = max77663_ldo_volt2hex(id, *uV);
  

•   if (hex < 0)
  

•           return hex;
  

•   ret &= ~(MAX77663_LDO_VOLT_MASK);
  

•   return pmic_reg_write(dev->parent, adr, ret | hex);
  

+}

+static int max77663_ldo_probe(struct udevice *dev) +{

•   struct dm_regulator_uclass_plat *uc_pdata =
  

•                                   dev_get_uclass_plat(dev);
  

•   uc_pdata->type = REGULATOR_TYPE_LDO;
  

•   uc_pdata->ctrl_reg = max77663_ldo_reg[dev->driver_data];
  

•   return 0;
  

+}

+static int ldo_get_value(struct udevice *dev) +{

•   int uV;
  

•   int ret;
  

•   ret = max77663_ldo_val(dev, PMIC_OP_GET, &uV);
  

•   if (ret)
  

•           return ret;
  

•   return uV;
  

+}

+static int ldo_set_value(struct udevice *dev, int uV) +{

•   return max77663_ldo_val(dev, PMIC_OP_SET, &uV);
  

+}

+static int ldo_get_enable(struct udevice *dev) +{

•   bool enable = false;
  

•   int ret;
  

•   ret = max77663_ldo_enable(dev, PMIC_OP_GET, &enable);
  

•   if (ret)
  

•           return ret;
  

•   return enable;
  

+}

+static int ldo_set_enable(struct udevice *dev, bool enable) +{

•   return max77663_ldo_enable(dev, PMIC_OP_SET, &enable);
  

+}

+static const struct dm_regulator_ops max77663_ldo_ops = {

•   .get_value  = ldo_get_value,
  

•   .set_value  = ldo_set_value,
  

•   .get_enable = ldo_get_enable,
  

•   .set_enable = ldo_set_enable,
  

+};

+U_BOOT_DRIVER(max77663_ldo) = {

•   .name = MAX77663_LDO_DRIVER,
  

•   .id = UCLASS_REGULATOR,
  

•   .ops = &max77663_ldo_ops,
  

•   .probe = max77663_ldo_probe,
  

+};

2.39.2

Regards, Simon