On Tue, Aug 04, 2015 at 06:46:09AM -0600, Simon Glass wrote:
>Hi Peng,
>
>On 3 August 2015 at 20:08, Peng Fan <b51...@freescale.com> wrote:
>> On Mon, Aug 03, 2015 at 08:38:49AM +0800, Peng Fan wrote:
>>>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?
>>
>> After a thought, a single pointer can not make it work.
>>
>> struct pfuze100_regulator_desc is for per regulator.
>> If use a single pointer, then "desc = dev_get_platdata(dev);" can not
>> change the platdata for each regulator. I need to use "struct xx **" to
>> reflect the change into platdata for each regulator.
>>
>> [...]
>>
>>
>
>I see that you a searching your table based the device name. Normally
>we name the regulators LDO1, LDO2 and use the number (1, 2) to find
>the right device. Can you please point me to your device tree
>contents?
I replied in v2 patch set. Still paste url here,
http://lxr.free-electrons.com/source/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
line from 208 to 306.
>
>You could make the platdata be something like
>
>struct pfuze_platdata {
> struct pfuze100_regulator_desc *desc;
>};
>
>I think the code would be clearer that way. You can assign to the desc
>member instead of a double pointer indirect.
Thanks. Will consider to use this way.
Regards,
Peng.
--
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
