Hi Simon, On Sun, Aug 02, 2015 at 04:30:52PM -0600, Simon Glass wrote: >Hi Peng, > >On 28 July 2015 at 08:48, Peng Fan <peng....@freescale.com> wrote: >> 1. Add new regulator driver pfuze100. >> * Introduce struct pfuze100_regulator_desc for mataining info for >> regulator. >> 2. Add new Kconfig entry DM_REGULATOR_PFUZE100 for pfuze100. >> 3. This driver intends to support PF100, PF200 and PF3000. >> 4. Add related macro definition in pfuze header file. >> >> Signed-off-by: Peng Fan <peng....@freescale.com> >> Cc: Przemyslaw Marczak <p.marc...@samsung.com> >> Cc: Simon Glass <s...@chromium.org> > >It looks correct but I have code style comments - see below. They all >apply globally.
Ok. Will fix them in V2. > >> --- >> drivers/power/regulator/Kconfig | 8 + >> drivers/power/regulator/Makefile | 1 + >> drivers/power/regulator/pfuze100.c | 596 >> +++++++++++++++++++++++++++++++++++++ >> include/power/pfuze100_pmic.h | 24 +- >> 4 files changed, 625 insertions(+), 4 deletions(-) >> create mode 100644 drivers/power/regulator/pfuze100.c >> >> diff --git a/drivers/power/regulator/Kconfig >> b/drivers/power/regulator/Kconfig >> index 6289b83..b854773 100644 >> --- a/drivers/power/regulator/Kconfig >> +++ b/drivers/power/regulator/Kconfig >> @@ -16,6 +16,14 @@ config DM_REGULATOR >> for this purpose if PMIC I/O driver is implemented or >> dm_scan_fdt_node() >> otherwise. Detailed information can be found in the header file. >> >> +config DM_REGULATOR_PFUZE100 >> + bool "Enable Driver Model for REGULATOR PFUZE100" >> + depends on DM_REGULATOR && DM_PMIC_PFUZE100 >> + ---help--- >> + This config enables implementation of driver-model regulator uclass >> + features for REGULATOR PFUZE100. The driver implements get/set api >> for: >> + value, enable and mode. >> + >> 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 96aa624..9f8f17b 100644 >> --- a/drivers/power/regulator/Makefile >> +++ b/drivers/power/regulator/Makefile >> @@ -7,5 +7,6 @@ >> >> obj-$(CONFIG_DM_REGULATOR) += regulator-uclass.o >> obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o >> +obj-$(CONFIG_DM_REGULATOR_PFUZE100) += pfuze100.o >> obj-$(CONFIG_DM_REGULATOR_FIXED) += fixed.o >> obj-$(CONFIG_DM_REGULATOR_SANDBOX) += sandbox.o >> diff --git a/drivers/power/regulator/pfuze100.c >> b/drivers/power/regulator/pfuze100.c >> new file mode 100644 >> index 0000000..6c513e9 >> --- /dev/null >> +++ b/drivers/power/regulator/pfuze100.c >> @@ -0,0 +1,596 @@ >> +#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/pfuze100_pmic.h> >> + >> +/** >> + * struct pfuze100_regulator_desc - regulator descriptor >> + * >> + * @name: Identify name for the regulator. >> + * @type: Indicates the regulator type. >> + * @uV_step: Voltage increase for each selector. >> + * @vsel_reg: Register for adjust regulator voltage for normal. >> + * @vsel_mask: Mask bit for setting regulator voltage for normal. >> + * @stby_reg: Register for adjust regulator voltage for standby. >> + * @stby_mask: Mask bit for setting regulator voltage for standby. >> + * @volt_table: Voltage mapping table (if table based mapping). >> + * @voltage: Current voltage for REGULATOR_TYPE_FIXED type regulator. >> + */ >> +struct pfuze100_regulator_desc { >> + char *name; >> + enum regulator_type type; >> + unsigned int uV_step; >> + unsigned int vsel_reg; >> + unsigned int vsel_mask; >> + unsigned int stby_reg; >> + unsigned int stby_mask; >> + unsigned int *volt_table; >> + unsigned int voltage; >> +}; >> + >> +#define PFUZE100_FIXED_REG(_name, base, vol) \ >> + { \ >> + .name = #_name, \ >> + .type = REGULATOR_TYPE_FIXED, \ >> + .voltage = (vol), \ >> + } >> + >> +#define PFUZE100_SW_REG(_name, base, step) \ >> + { \ >> + .name = #_name, \ >> + .type = REGULATOR_TYPE_BUCK, \ >> + .uV_step = (step), \ >> + .vsel_reg = (base) + PFUZE100_VOL_OFFSET, \ >> + .vsel_mask = 0x3F, \ >> + .stby_reg = (base) + PFUZE100_STBY_OFFSET, \ >> + .stby_mask = 0x3F, \ >> + } >> + >> +#define PFUZE100_SWB_REG(_name, base, mask, step, voltages) \ >> + { \ >> + .name = #_name, \ >> + .type = REGULATOR_TYPE_BUCK, \ >> + .uV_step = (step), \ >> + .vsel_reg = (base), \ >> + .vsel_mask = (mask), \ >> + .volt_table = (voltages), \ >> + } >> + >> +#define PFUZE100_SNVS_REG(_name, base, mask, voltages) \ >> + { \ >> + .name = #_name, \ >> + .type = REGULATOR_TYPE_OTHER, \ >> + .vsel_reg = (base), \ >> + .vsel_mask = (mask), \ >> + .volt_table = (voltages), \ >> + } >> + >> +#define PFUZE100_VGEN_REG(_name, base, step) \ >> + { \ >> + .name = #_name, \ >> + .type = REGULATOR_TYPE_LDO, \ >> + .uV_step = (step), \ >> + .vsel_reg = (base), \ >> + .vsel_mask = 0xF, \ >> + .stby_reg = (base), \ >> + .stby_mask = 0x20, \ >> + } >> + >> +#define PFUZE3000_VCC_REG(_name, base, step) \ >> + { \ >> + .name = #_name, \ >> + .type = REGULATOR_TYPE_LDO, \ >> + .uV_step = (step), \ >> + .vsel_reg = (base), \ >> + .vsel_mask = 0x3, \ >> + .stby_reg = (base), \ >> + .stby_mask = 0x20, \ >> +} >> + >> +#define PFUZE3000_SW1_REG(_name, base, step) \ >> + { \ >> + .name = #_name, \ >> + .type = REGULATOR_TYPE_BUCK, \ >> + .uV_step = (step), \ >> + .vsel_reg = (base) + PFUZE100_VOL_OFFSET, \ >> + .vsel_mask = 0x1F, \ >> + .stby_reg = (base) + PFUZE100_STBY_OFFSET, \ >> + .stby_mask = 0x1F, \ >> + } >> + >> +#define PFUZE3000_SW2_REG(_name, base, step) \ >> + { \ >> + .name = #_name, \ >> + .type = REGULATOR_TYPE_BUCK, \ >> + .uV_step = (step), \ >> + .vsel_reg = (base) + PFUZE100_VOL_OFFSET, \ >> + .vsel_mask = 0x7, \ >> + .stby_reg = (base) + PFUZE100_STBY_OFFSET, \ >> + .stby_mask = 0x7, \ >> + } >> + >> +#define PFUZE3000_SW3_REG(_name, base, step) \ >> + { \ >> + .name = #_name, \ >> + .type = REGULATOR_TYPE_BUCK, \ >> + .uV_step = (step), \ >> + .vsel_reg = (base) + PFUZE100_VOL_OFFSET, \ >> + .vsel_mask = 0xF, \ >> + .stby_reg = (base) + PFUZE100_STBY_OFFSET, \ >> + .stby_mask = 0xF, \ >> + } >> + >> +static unsigned int pfuze100_swbst[] = { >> + 5000000, 5050000, 5100000, 5150000 >> +}; >> + >> +static unsigned int pfuze100_vsnvs[] = { >> + 1000000, 1100000, 1200000, 1300000, 1500000, 1800000, 3000000, -1 >> +}; >> + >> +static unsigned int pfuze3000_vsnvs[] = { >> + -1, -1, -1, -1, -1, -1, 3000000, -1 >> +}; >> + >> +static unsigned int pfuze3000_sw2lo[] = { >> + 1500000, 1550000, 1600000, 1650000, 1700000, 1750000, 1800000, >> 1850000 >> +}; >> + >> +/* PFUZE100 */ >> +static struct pfuze100_regulator_desc pfuze100_regulators[] = { >> + PFUZE100_SW_REG(sw1ab, PFUZE100_SW1ABVOL, 25000), >> + PFUZE100_SW_REG(sw1c, PFUZE100_SW1CVOL, 25000), >> + PFUZE100_SW_REG(sw2, PFUZE100_SW2VOL, 25000), >> + PFUZE100_SW_REG(sw3a, PFUZE100_SW3AVOL, 25000), >> + PFUZE100_SW_REG(sw3b, PFUZE100_SW3BVOL, 25000), >> + PFUZE100_SW_REG(sw4, PFUZE100_SW4VOL, 25000), >> + PFUZE100_SWB_REG(swbst, PFUZE100_SWBSTCON1, 0x3, 50000, >> pfuze100_swbst), >> + PFUZE100_SNVS_REG(vsnvs, PFUZE100_VSNVSVOL, 0x7, pfuze100_vsnvs), >> + PFUZE100_FIXED_REG(vrefddr, PFUZE100_VREFDDRCON, 750000), >> + PFUZE100_VGEN_REG(vgen1, PFUZE100_VGEN1VOL, 50000), >> + PFUZE100_VGEN_REG(vgen2, PFUZE100_VGEN2VOL, 50000), >> + PFUZE100_VGEN_REG(vgen3, PFUZE100_VGEN3VOL, 100000), >> + PFUZE100_VGEN_REG(vgen4, PFUZE100_VGEN4VOL, 100000), >> + PFUZE100_VGEN_REG(vgen5, PFUZE100_VGEN5VOL, 100000), >> + PFUZE100_VGEN_REG(vgen6, PFUZE100_VGEN6VOL, 100000), >> +}; >> + >> +/* PFUZE200 */ >> +static struct pfuze100_regulator_desc pfuze200_regulators[] = { >> + PFUZE100_SW_REG(sw1ab, PFUZE100_SW1ABVOL, 25000), >> + PFUZE100_SW_REG(sw2, PFUZE100_SW2VOL, 25000), >> + PFUZE100_SW_REG(sw3a, PFUZE100_SW3AVOL, 25000), >> + PFUZE100_SW_REG(sw3b, PFUZE100_SW3BVOL, 25000), >> + PFUZE100_SWB_REG(swbst, PFUZE100_SWBSTCON1, 0x3, 50000, >> pfuze100_swbst), >> + PFUZE100_SNVS_REG(vsnvs, PFUZE100_VSNVSVOL, 0x7, pfuze100_vsnvs), >> + PFUZE100_FIXED_REG(vrefddr, PFUZE100_VREFDDRCON, 750000), >> + PFUZE100_VGEN_REG(vgen1, PFUZE100_VGEN1VOL, 50000), >> + PFUZE100_VGEN_REG(vgen2, PFUZE100_VGEN2VOL, 50000), >> + PFUZE100_VGEN_REG(vgen3, PFUZE100_VGEN3VOL, 100000), >> + PFUZE100_VGEN_REG(vgen4, PFUZE100_VGEN4VOL, 100000), >> + PFUZE100_VGEN_REG(vgen5, PFUZE100_VGEN5VOL, 100000), >> + PFUZE100_VGEN_REG(vgen6, PFUZE100_VGEN6VOL, 100000), >> +}; >> + >> +/* PFUZE3000 */ >> +static struct pfuze100_regulator_desc pfuze3000_regulators[] = { >> + PFUZE3000_SW1_REG(sw1a, PFUZE100_SW1ABVOL, 25000), >> + PFUZE3000_SW1_REG(sw1b, PFUZE100_SW1CVOL, 25000), >> + PFUZE100_SWB_REG(sw2, PFUZE100_SW2VOL, 0x7, 50000, pfuze3000_sw2lo), >> + PFUZE3000_SW3_REG(sw3, PFUZE100_SW3AVOL, 50000), >> + PFUZE100_SWB_REG(swbst, PFUZE100_SWBSTCON1, 0x3, 50000, >> pfuze100_swbst), >> + PFUZE100_SNVS_REG(vsnvs, PFUZE100_VSNVSVOL, 0x7, pfuze3000_vsnvs), >> + PFUZE100_FIXED_REG(vrefddr, PFUZE100_VREFDDRCON, 750000), >> + PFUZE100_VGEN_REG(vldo1, PFUZE100_VGEN1VOL, 100000), >> + PFUZE100_VGEN_REG(vldo2, PFUZE100_VGEN2VOL, 50000), >> + PFUZE3000_VCC_REG(vccsd, PFUZE100_VGEN3VOL, 150000), >> + PFUZE3000_VCC_REG(v33, PFUZE100_VGEN4VOL, 150000), >> + PFUZE100_VGEN_REG(vldo3, PFUZE100_VGEN5VOL, 100000), >> + PFUZE100_VGEN_REG(vldo4, PFUZE100_VGEN6VOL, 100000), >> +}; >> + >> +#define MODE(_id, _val, _name) { \ >> + .id = _id, \ >> + .register_value = _val, \ >> + .name = _name, \ >> +} >> + >> +/* SWx Buck regulator mode */ >> +static struct dm_regulator_mode pfuze_sw_modes[] = { >> + MODE(OFF_OFF, OFF_OFF, "OFF_OFF"), >> + MODE(PWM_OFF, PWM_OFF, "PWM_OFF"), >> + MODE(PFM_OFF, PFM_OFF, "PFM_OFF"), >> + MODE(APS_OFF, APS_OFF, "APS_OFF"), >> + MODE(PWM_PWM, PWM_PWM, "PWM_PWM"), >> + MODE(PWM_APS, PWM_APS, "PWM_APS"), >> + MODE(APS_APS, APS_APS, "APS_APS"), >> + MODE(APS_PFM, APS_PFM, "APS_PFM"), >> + MODE(PWM_PFM, PWM_PFM, "PWM_PFM"), >> +}; >> + >> +/* Boost Buck regulator mode for normal operation */ >> +static struct dm_regulator_mode pfuze_swbst_modes[] = { >> + MODE(SWBST_MODE_OFF, SWBST_MODE_OFF , "SWBST_MODE_OFF"), >> + MODE(SWBST_MODE_PFM, SWBST_MODE_PFM, "SWBST_MODE_PFM"), >> + MODE(SWBST_MODE_AUTO, SWBST_MODE_AUTO, "SWBST_MODE_AUTO"), >> + MODE(SWBST_MODE_APS, SWBST_MODE_APS, "SWBST_MODE_APS"), >> +}; >> + >> +/* VGENx LDO regulator mode for normal operation */ >> +static struct dm_regulator_mode pfuze_ldo_modes[] = { >> + MODE(LDO_MODE_OFF, LDO_MODE_OFF, "LDO_MODE_OFF"), >> + MODE(LDO_MODE_ON, LDO_MODE_ON, "LDO_MODE_ON"), >> +}; >> + >> +static struct pfuze100_regulator_desc *se_desc(struct >> pfuze100_regulator_desc *desc, >> + int size, >> + const char *name) >> +{ >> + int i; > >blank line between declarations and rest - please fix global Will fix in V2. > >> + for (i = 0; i < size; desc++) { >> + if (!strcmp(desc->name, name)) >> + return desc; >> + continue; >> + } >> + >> + return NULL; >> +} >> + >> +static int pfuze100_regulator_probe(struct udevice *dev) >> +{ >> + struct dm_regulator_uclass_platdata *uc_pdata; >> + struct pfuze100_regulator_desc **desc = NULL; > >Can't this just be a single pointer? hmm. My bad, will fix this. > >> + >> + desc = dev_get_platdata(dev); >> + >> + switch (dev_get_driver_data(dev_get_parent(dev))) { >> + case PFUZE100: >> + *desc = se_desc(pfuze100_regulators, >> + ARRAY_SIZE(pfuze100_regulators), >> + dev->name); >> + break; >> + case PFUZE200: >> + *desc = se_desc(pfuze200_regulators, >> + ARRAY_SIZE(pfuze200_regulators), >> + dev->name); >> + break; >> + case PFUZE3000: >> + *desc = se_desc(pfuze3000_regulators, >> + ARRAY_SIZE(pfuze3000_regulators), >> + dev->name); >> + break; >> + } >> + if (!*desc) { >> + debug("Do not support regulator %s\n", dev->name); >> + return -EINVAL; >> + } >> + >> + uc_pdata = dev_get_uclass_platdata(dev); >> + >> + uc_pdata->type = (*desc)->type; >> + if (uc_pdata->type == REGULATOR_TYPE_BUCK) { >> + if (!strcmp(dev->name, "swbst")) { >> + uc_pdata->mode = pfuze_swbst_modes; >> + uc_pdata->mode_count = ARRAY_SIZE(pfuze_swbst_modes); >> + } else { >> + uc_pdata->mode = pfuze_sw_modes; >> + uc_pdata->mode_count = ARRAY_SIZE(pfuze_sw_modes); >> + } >> + } else if (uc_pdata->type == REGULATOR_TYPE_LDO) { >> + uc_pdata->mode = pfuze_ldo_modes; >> + uc_pdata->mode_count = ARRAY_SIZE(pfuze_ldo_modes); >> + } else { >> + uc_pdata->mode = NULL; >> + uc_pdata->mode_count = 0; >> + } >> + >> + return 0; >> +} >> + >> +static int pfuze100_regulator_mode(struct udevice *dev, int op, int *opmode) >> +{ >> + unsigned char val; >> + int ret; >> + struct pfuze100_regulator_desc *desc = >> + *(struct pfuze100_regulator_desc **)dev_get_platdata(dev); > >You don't actually need the cast. Will discard the cast. > >> + >> + if (op == PMIC_OP_GET) { >> + if (desc->type == REGULATOR_TYPE_BUCK) { >> + if (!strcmp(dev->name, "swbst")) { >> + ret = pmic_read(dev->parent, desc->vsel_reg, >> + &val, 1); >> + if (ret) >> + return ret; >> + >> + val &= SWBST_MODE_MASK; >> + val >>= SWBST_MODE_SHIFT; >> + *opmode = val; >> + >> + return 0; >> + } >> + ret = pmic_read(dev->parent, >> + desc->vsel_reg + >> PFUZE100_MODE_OFFSET, >> + &val, 1); > >pmic_reg_read() > >> + if (ret) >> + return ret; >> + >> + val &= SW_MODE_MASK; >> + val >>= SW_MODE_SHIFT; >> + *opmode = val; >> + >> + return 0; >> + >> + } else if (desc->type == REGULATOR_TYPE_LDO) { >> + ret = pmic_read(dev->parent, desc->vsel_reg, &val, >> 1); >> + if (ret) >> + return ret; >> + >> + val &= LDO_MODE_MASK; >> + val >>= LDO_MODE_SHIFT; >> + *opmode = val; >> + >> + return 0; >> + } else { >> + return -EINVAL; >> + } >> + } >> + >> + if (desc->type == REGULATOR_TYPE_BUCK) { >> + if (!strcmp(dev->name, "swbst")) { >> + ret = pmic_read(dev->parent, desc->vsel_reg, &val, >> 1); >> + if (ret) >> + return ret; >> + >> + val &= ~SWBST_MODE_MASK; >> + val |= *opmode << SWBST_MODE_SHIFT; >> + >> + ret = pmic_write(dev->parent, desc->vsel_reg, &val, >> 1); > >pmic_reg_write Ok. > >> + if (ret) >> + return ret; >> + >> + return 0; >> + } >> + ret = pmic_read(dev->parent, >> + desc->vsel_reg + PFUZE100_MODE_OFFSET, >> + &val, 1); >> + if (ret) >> + return ret; >> + >> + val &= ~SW_MODE_MASK; >> + val |= *opmode << SW_MODE_SHIFT; >> + >> + ret = pmic_write(dev->parent, >> + desc->vsel_reg + PFUZE100_MODE_OFFSET, >> + &val, 1); > >pmic_clrsetbits() can replace all of this ok. Will use this new way. [...] Thanks, Peng. -- _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot