[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