[U-Boot] [PATCH] power: LP8720 regulator support
Przemyslaw Marczak
p.marczak at samsung.com
Tue Nov 4 18:46:23 CET 2014
Hello Paul,
On 11/01/2014 12:01 PM, Paul Kocialkowski wrote:
> Le mardi 28 octobre 2014 à 18:32 +0100, Paul Kocialkowski a écrit :
>> This adds support for the LP8720 i2c regulator, as found in e.g. the LG
>> Optimus Black (P970), codename sniper. This code supports setting up and
>> enabling one of the 5 LDOs that the IC provides.
>> Other more advanced features are unsupported.
>>
>> Signed-off-by: Paul Kocialkowski <contact at paulk.fr>
>> ---
>> drivers/power/Makefile | 1 +
>> drivers/power/lp8720.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++
>> include/lp8720.h | 93 +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 201 insertions(+)
>> create mode 100644 drivers/power/lp8720.c
>> create mode 100644 include/lp8720.h
>>
>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
>> index dc64e4d..65be5a0 100644
>> --- a/drivers/power/Makefile
>> +++ b/drivers/power/Makefile
>> @@ -9,6 +9,7 @@ obj-$(CONFIG_AXP152_POWER) += axp152.o
>> obj-$(CONFIG_AXP209_POWER) += axp209.o
>> obj-$(CONFIG_EXYNOS_TMU) += exynos-tmu.o
>> obj-$(CONFIG_FTPMU010_POWER) += ftpmu010.o
>> +obj-$(CONFIG_LP8720_POWER) += lp8720.o
>> obj-$(CONFIG_TPS6586X_POWER) += tps6586x.o
>> obj-$(CONFIG_TWL4030_POWER) += twl4030.o
>> obj-$(CONFIG_TWL6030_POWER) += twl6030.o
>> diff --git a/drivers/power/lp8720.c b/drivers/power/lp8720.c
>> new file mode 100644
>> index 0000000..ac7fc11
>> --- /dev/null
>> +++ b/drivers/power/lp8720.c
>> @@ -0,0 +1,107 @@
>> +/*
>> + * Copyright (C) 2014 Paul Kocialkowski <contact at paulk.fr>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <i2c.h>
>> +#include <asm/gpio.h>
>> +#include <lp8720.h>
>> +
>> +static struct lp8720_info lp8720_info;
>> +
>> +static int lp8720_write(u8 reg, u8 val)
>> +{
>> + return i2c_write(lp8720_info.chip_idsel, reg, 1, &val, 1);
>> +}
>> +
>> +static int lp8720_read(u8 reg, u8 *val)
>> +{
>> + return i2c_read(lp8720_info.chip_idsel, reg, 1, val, 1);
>> +}
At present we have API for the PMIC drivers.
It's located here: drivers/power/
and for i2c operations, you can use this: drivers/power/power_i2c.c
Please look at some pmic drivers, some simple is:
drivers/power/pmic/pmic_pfuze100.c and some more complicated with
device-tree: drivers/power/pmic/pmic_max77686.c
We have also some drivers without the pmic framework support - but we
are going to keep clean code, rather than introducing api for each driver.
>
> Should I ifdef for I2C and GPIO support? It seems that GPIO support only
> has board-sepcific config options, so this may be hard. There is
> CONFIG_DM_GPIO for driver model, but it is apparently not always used,
> especially not on SPL.
>
>> +int lp8720_init(int enable_gpio, int chip_idsel)
>> +{
>> + int ret;
>> +
>> + if (enable_gpio) {
>
> I'm guessing that in the case of a provided negative GPIO, this will
> return -1, which is not appropriate behavior (the regulator can still be
> used without an enable GPIO). I should probbaly directly go with
> gpio_is_valid.
>
>> + if (!gpio_is_valid(enable_gpio))
>> + return -1;
>> +
>> + ret = gpio_request(enable_gpio, "lp8720_en");
>> + if (ret)
>> + return ret;
>> +
>> + ret = gpio_direction_output(enable_gpio, 0);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + lp8720_info.enable_gpio = enable_gpio;
>> + lp8720_info.chip_idsel = chip_idsel;
>> +
>> + return 0;
>> +}
>> +
>> +int lp8720_enable(void)
>> +{
>> + int ret;
>> +
>> + if (lp8720_info.enable_gpio) {
>> + ret = gpio_set_value(lp8720_info.enable_gpio, 1);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int lp8720_is_enabled(void)
>> +{
>> + if (lp8720_info.enable_gpio)
>> + return gpio_get_value(lp8720_info.enable_gpio);
>> +
>> + /* Assume LP8720 enabled when there is no enable GPIO */
>> + return 1;
>> +}
>> +
This function is quite unclean. The argument and the function name
suggests that you enable one ldo, but you modify all ldo bits.
Please modify this code with getting "ldo" argument as chosen LDO number.
int lp8720_ldo_enable(int ldo)
>> +int lp8720_ldo_enable(u8 ldo_enable)
>> +{
>> + u8 val;
>> + int ret;
>> +
>> + ret = lp8720_read(LP8720_ENABLE_BITS, &val);
>> + if (ret)
>> + return ret;
>> +
>> + val |= ldo_enable;
what about:
val |= (1 << (ldo - 1)); (for ldo number starting from "1")
Here you should also add LP8720_ENABLE_BITS_MASK
>> +
>> + ret = lp8720_write(LP8720_ENABLE_BITS, val);
>> + if (ret)
>> + return ret;
>> +
>> + /* Enable the IC */
>> + if (!lp8720_is_enabled())
>> + lp8720_enable();
>> +
>> + return 0;
>> +}
>> +
Please use u32 for register as pmic api uses.
int lp8720_ldo_voltage(u32 ldo_reg, u32 voltage)
>> +int lp8720_ldo_voltage(u8 ldo_reg, u8 voltage, u8 delay)
Setting the delay is a different operation than setting the voltage, and
it not fits to the pmic api.
So you should add additional function like:
int lp8720_ldo_startup_delay(u32 ldo_reg, u32 delay)
>> +{
>> + u8 val;
>> + int ret;
>> +
>> + /* Register V field */
>> + val = voltage & 0x1F;
>> +
>> + /* Register T field */
>> + val |= (delay & 0x07) << 5;
>> +
>> + ret = lp8720_write(ldo_reg, val);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> diff --git a/include/lp8720.h b/include/lp8720.h
>> new file mode 100644
>> index 0000000..033f2c4
>> --- /dev/null
>> +++ b/include/lp8720.h
>> @@ -0,0 +1,93 @@
>> +/*
>> + * Copyright (C) 2014 Paul Kocialkowski <contact at paulk.fr>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#ifndef LP8720_H
>> +#define LP8720_H
>> +
>> +#include <common.h>
>> +
>> +/*
>> + * Chip ID selection
>> + */
>> +
>> +#define LP8720_CHIP_IDSEL_VBATT 0x7F
>> +#define LP8720_CHIP_IDSEL_HI_Z 0x7C
>> +#define LP8720_CHIP_IDSEL_GND 0x7D
>> +
>> +/*
>> + * Registers
>> + */
>> +
>> +#define LP8720_GENERAL_SETTINGS 0x00
>> +#define LP8720_LDO1_SETTINGS 0x01
>> +#define LP8720_LDO2_SETTINGS 0x02
>> +#define LP8720_LDO3_SETTINGS 0x03
>> +#define LP8720_LDO4_SETTINGS 0x04
>> +#define LP8720_LDO5_SETTINGS 0x05
>> +#define LP8720_BUCK_SETTINGS1 0x06
>> +#define LP8720_BUCK_SETTINGS2 0x07
>> +#define LP8720_ENABLE_BITS 0x08
>> +#define LP8720_PULLDOWN_BITS 0x09
>> +#define LP8720_STATUS_BITS 0x0A
>> +#define LP8720_INTERRUPT_BITS 0x0B
>> +#define LP8720_INTERRUPT_MASK 0x0C
>> +
>> +/*
>> + * Values
>> + */
>> +
>> +/* LP8720_GENERAL_SETTINGS */
>> +#define LP8720_25_US_TIME_STEP (1 << 0)
>> +#define LP8720_50_US_TIME_STEP (0 << 0)
>> +
>> +/* LP8720_LDO*_SETTINGS */
>> +#define LP8720_LDO1235_V_12 0x00
>> +#define LP8720_LDO1235_V_18 0x0C
>> +#define LP8720_LDO1235_V_30 0x1D
>> +#define LP8720_LDO1235_V_33 0x1F
>> +
>> +#define LP8720_LDO4_V_12 0x00
>> +#define LP8720_LDO4_V_14 0x09
>> +#define LP8720_LDO4_V_18 0x11
>> +#define LP8720_LDO4_V_28 0x1E
>> +
>> +#define LP8720_DELAY_0 0
>> +#define LP8720_DELAY_1_TS 1
>> +#define LP8720_DELAY_2_TS 2
>> +#define LP8720_DELAY_3_TS 3
>> +#define LP8720_DELAY_4_TS 4
>> +#define LP8720_DELAY_5_TS 5
>> +#define LP8720_DELAY_6_TS 6
>> +#define LP8720_DELAY_NO_START 7
>> +
>> +/* LP8720_ENABLE_BITS */
>> +#define LP8720_LDO1_EN (1 << 0)
>> +#define LP8720_LDO2_EN (1 << 1)
>> +#define LP8720_LDO3_EN (1 << 2)
>> +#define LP8720_LDO4_EN (1 << 3)
>> +#define LP8720_LDO5_EN (1 << 4)
>> +#define LP8720_BUCK_EN (1 << 5)
>> +
>> +/*
>> + * Driver info
>> + */
>> +
>> +struct lp8720_info {
>> + int enable_gpio;
>> + int chip_idsel;
>> +};
>> +
>> +/*
>> + * Declarations
>> + */
>> +
>> +int lp8720_init(int enable_gpio, int chip_idsel);
>> +int lp8720_enable(void);
>> +int lp8720_is_enabled(void);
>> +int lp8720_ldo_enable(u8 ldo_enable);
>> +int lp8720_ldo_voltage(u8 ldo_reg, u8 voltage, u8 delay);
>> +
>> +#endif
>
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Maybe the new pmic framework will be ready before you add the new board,
and it should be more flexible. It allows setting custom modes for each
ldo - like ldo startup delay in your driver.
But if you will add the new board before the new pmic framework, then
this code should be generally ok. Just please use the present pmic
framework.
Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com
More information about the U-Boot
mailing list