* [U-Boot] [PATCH] power: LP8720 regulator support @ 2014-10-28 17:32 Paul Kocialkowski 2014-11-01 11:01 ` Paul Kocialkowski 0 siblings, 1 reply; 6+ messages in thread From: Paul Kocialkowski @ 2014-10-28 17:32 UTC (permalink / raw) To: u-boot 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@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@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); +} + +int lp8720_init(int enable_gpio, int chip_idsel) +{ + int ret; + + if (enable_gpio) { + 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; +} + +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; + + ret = lp8720_write(LP8720_ENABLE_BITS, val); + if (ret) + return ret; + + /* Enable the IC */ + if (!lp8720_is_enabled()) + lp8720_enable(); + + return 0; +} + +int lp8720_ldo_voltage(u8 ldo_reg, u8 voltage, u8 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@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 -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] power: LP8720 regulator support 2014-10-28 17:32 [U-Boot] [PATCH] power: LP8720 regulator support Paul Kocialkowski @ 2014-11-01 11:01 ` Paul Kocialkowski 2014-11-04 15:58 ` Tom Rini 2014-11-04 17:46 ` Przemyslaw Marczak 0 siblings, 2 replies; 6+ messages in thread From: Paul Kocialkowski @ 2014-11-01 11:01 UTC (permalink / raw) To: u-boot 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@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@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); > +} 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; > +} > + > +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; > + > + ret = lp8720_write(LP8720_ENABLE_BITS, val); > + if (ret) > + return ret; > + > + /* Enable the IC */ > + if (!lp8720_is_enabled()) > + lp8720_enable(); > + > + return 0; > +} > + > +int lp8720_ldo_voltage(u8 ldo_reg, u8 voltage, u8 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@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 -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: This is a digitally signed message part URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141101/fc4901b9/attachment.pgp> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] power: LP8720 regulator support 2014-11-01 11:01 ` Paul Kocialkowski @ 2014-11-04 15:58 ` Tom Rini 2014-11-04 17:46 ` Przemyslaw Marczak 2014-11-04 17:46 ` Przemyslaw Marczak 1 sibling, 1 reply; 6+ messages in thread From: Tom Rini @ 2014-11-04 15:58 UTC (permalink / raw) To: u-boot On Sat, Nov 01, 2014 at 12:01:37PM +0100, 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@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@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); > > +} > > 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. Don't code for what you can't test. Which way are you using this on the Optimus? And in general, any comments here Przemyslaw? Thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141104/7e48030e/attachment.pgp> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] power: LP8720 regulator support 2014-11-04 15:58 ` Tom Rini @ 2014-11-04 17:46 ` Przemyslaw Marczak 2014-11-05 17:44 ` Paul Kocialkowski 0 siblings, 1 reply; 6+ messages in thread From: Przemyslaw Marczak @ 2014-11-04 17:46 UTC (permalink / raw) To: u-boot Hello, On 11/04/2014 04:58 PM, Tom Rini wrote: > On Sat, Nov 01, 2014 at 12:01:37PM +0100, 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@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@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); >>> +} >> >> 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. > > Don't code for what you can't test. Which way are you using this on the > Optimus? And in general, any comments here Przemyslaw? Thanks! > > > > _______________________________________________ > U-Boot mailing list > U-Boot at lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot > Thanks for cc Tom, I have some comments in a next message. Paul, there are no patches for the board, which you mentioned about. Usually we don't add dead code to U-Boot. If you are going to add new board - just add it with all required drivers in a one patchset - then it is consistent and easy to review. Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] power: LP8720 regulator support 2014-11-04 17:46 ` Przemyslaw Marczak @ 2014-11-05 17:44 ` Paul Kocialkowski 0 siblings, 0 replies; 6+ messages in thread From: Paul Kocialkowski @ 2014-11-05 17:44 UTC (permalink / raw) To: u-boot Le mardi 04 novembre 2014 ? 18:46 +0100, Przemyslaw Marczak a ?crit : > Hello, > > On 11/04/2014 04:58 PM, Tom Rini wrote: > > On Sat, Nov 01, 2014 at 12:01:37PM +0100, 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@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@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); > >>> +} > >> > >> 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. > > > > Don't code for what you can't test. Which way are you using this on the > > Optimus? And in general, any comments here Przemyslaw? Thanks! I am using that code both on SPL and regular U-Boot. I'll just drop the idea of adding ifdef on GPIO and go with it all the way. On the Optimus Black, a GPIO is used to enable the regulators. > Paul, > there are no patches for the board, which you mentioned about. Usually > we don't add dead code to U-Boot. If you are going to add new board - > just add it with all required drivers in a one patchset - then it is > consistent and easy to review. I understand that. I'll keep your previous comments aside and will rework the LP8720 code a bit later. I'll focus on having all the changes I need on already-existing code accepted first and then submit new code (that relies on these new features), including the Optimus Black board, configs and the the LP8720 regulator. Thanks! -- Paul Kocialkowski, Replicant developer Replicant is a fully free Android distribution Website: http://www.replicant.us/ Redmine: http://redmine.replicant.us/ -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: This is a digitally signed message part URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141105/287f9616/attachment.pgp> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] power: LP8720 regulator support 2014-11-01 11:01 ` Paul Kocialkowski 2014-11-04 15:58 ` Tom Rini @ 2014-11-04 17:46 ` Przemyslaw Marczak 1 sibling, 0 replies; 6+ messages in thread From: Przemyslaw Marczak @ 2014-11-04 17:46 UTC (permalink / raw) To: u-boot 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@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@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@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-11-05 17:44 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-28 17:32 [U-Boot] [PATCH] power: LP8720 regulator support Paul Kocialkowski 2014-11-01 11:01 ` Paul Kocialkowski 2014-11-04 15:58 ` Tom Rini 2014-11-04 17:46 ` Przemyslaw Marczak 2014-11-05 17:44 ` Paul Kocialkowski 2014-11-04 17:46 ` Przemyslaw Marczak
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox