Discussion:
[PATCH 1/6] regulator: lp872x: Add missing of_match in regulators descriptions
(too old to reply)
Paul Kocialkowski
2015-12-23 11:10:01 UTC
Permalink
In order to select the regulators via of_find_regulator_by_node (and thus use
them in devicetree), defining of_match for each regulator is required.

Signed-off-by: Paul Kocialkowski <***@paulk.fr>
---
drivers/regulator/lp872x.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index e5af072..9412353 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -520,6 +520,7 @@ static struct regulator_ops lp8725_buck_ops = {
static struct regulator_desc lp8720_regulator_desc[] = {
{
.name = "ldo1",
+ .of_match = of_match_ptr("ldo1"),
.id = LP8720_ID_LDO1,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -533,6 +534,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo2",
+ .of_match = of_match_ptr("ldo2"),
.id = LP8720_ID_LDO2,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -546,6 +548,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo3",
+ .of_match = of_match_ptr("ldo3"),
.id = LP8720_ID_LDO3,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -559,6 +562,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo4",
+ .of_match = of_match_ptr("ldo4"),
.id = LP8720_ID_LDO4,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp8720_ldo4_vtbl),
@@ -572,6 +576,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo5",
+ .of_match = of_match_ptr("ldo5"),
.id = LP8720_ID_LDO5,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -585,6 +590,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "buck",
+ .of_match = of_match_ptr("buck"),
.id = LP8720_ID_BUCK,
.ops = &lp8720_buck_ops,
.n_voltages = ARRAY_SIZE(lp8720_buck_vtbl),
@@ -599,6 +605,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
static struct regulator_desc lp8725_regulator_desc[] = {
{
.name = "ldo1",
+ .of_match = of_match_ptr("ldo1"),
.id = LP8725_ID_LDO1,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -612,6 +619,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo2",
+ .of_match = of_match_ptr("ldo2"),
.id = LP8725_ID_LDO2,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -625,6 +633,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo3",
+ .of_match = of_match_ptr("ldo3"),
.id = LP8725_ID_LDO3,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -638,6 +647,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo4",
+ .of_match = of_match_ptr("ldo4"),
.id = LP8725_ID_LDO4,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -651,6 +661,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo5",
+ .of_match = of_match_ptr("ldo5"),
.id = LP8725_ID_LDO5,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -664,6 +675,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "lilo1",
+ .of_match = of_match_ptr("lilo1"),
.id = LP8725_ID_LILO1,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl),
@@ -677,6 +689,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "lilo2",
+ .of_match = of_match_ptr("lilo2"),
.id = LP8725_ID_LILO2,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl),
@@ -690,6 +703,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "buck1",
+ .of_match = of_match_ptr("buck1"),
.id = LP8725_ID_BUCK1,
.ops = &lp8725_buck_ops,
.n_voltages = ARRAY_SIZE(lp8725_buck_vtbl),
@@ -701,6 +715,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "buck2",
+ .of_match = of_match_ptr("buck2"),
.id = LP8725_ID_BUCK2,
.ops = &lp8725_buck_ops,
.n_voltages = ARRAY_SIZE(lp8725_buck_vtbl),
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Paul Kocialkowski
2015-12-23 11:10:02 UTC
Permalink
The LG Optimus Black (P970) codename sniper is a smartphone that was designed
and manufactured by LG Electronics (LGE) and released back in 2011.
It is using an OMAP3630 SoC, GP version.

This adds devicetree support for the device, with only a few basic features
supported, such as debug uart, i2c, internal emmc and external mmc.

Signed-off-by: Paul Kocialkowski <***@paulk.fr>
---
arch/arm/boot/dts/Makefile | 1 +
arch/arm/boot/dts/omap3-sniper.dts | 220 +++++++++++++++++++++++++++++++++++++
2 files changed, 221 insertions(+)
create mode 100644 arch/arm/boot/dts/omap3-sniper.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 30bbc37..2c71106 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -447,6 +447,7 @@ dtb-$(CONFIG_ARCH_OMAP3) += \
omap3-sbc-t3517.dtb \
omap3-sbc-t3530.dtb \
omap3-sbc-t3730.dtb \
+ omap3-sniper.dtb \
omap3-thunder.dtb \
omap3-zoom3.dtb
dtb-$(CONFIG_SOC_TI81XX) += \
diff --git a/arch/arm/boot/dts/omap3-sniper.dts b/arch/arm/boot/dts/omap3-sniper.dts
new file mode 100644
index 0000000..6c2d99a
--- /dev/null
+++ b/arch/arm/boot/dts/omap3-sniper.dts
@@ -0,0 +1,220 @@
+/*
+ * Copyright (C) 2015 Paul Kocialkowski <***@paulk.fr>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+/dts-v1/;
+
+#include "omap36xx.dtsi"
+
+/ {
+ model = "LG Optimus Black (P970)";
+ compatible = "lge,omap3-sniper", "ti,omap36xx", "ti,omap3";
+
+ cpus {
+ ***@0 {
+ cpu0-supply = <&vcc>;
+ };
+ };
+
+ memory {
+ device_type = "memory";
+ reg = <0x80000000 0x20000000>; /* 512 MB */
+ };
+};
+
+&omap3_pmx_core {
+ pinctrl-names = "default";
+
+ uart3_pins: pinmux_uart3_pins {
+ pinctrl-single,pins = <
+ 0x16e (PIN_INPUT | MUX_MODE0) /* uart3_rx_irrx */
+ 0x170 (PIN_OUTPUT | MUX_MODE0) /* uart3_tx_irtx */
+ >;
+ };
+
+ i2c1_pins: pinmux_i2c1_pins {
+ pinctrl-single,pins = <
+ 0x18a (PIN_INPUT_PULLUP | MUX_MODE0) /* i2c1_scl */
+ 0x18c (PIN_INPUT_PULLUP | MUX_MODE0) /* i2c1_sda */
+ >;
+ };
+
+ i2c2_pins: pinmux_i2c2_pins {
+ pinctrl-single,pins = <
+ 0x18e (PIN_INPUT | MUX_MODE0) /* i2c2_scl */
+ 0x190 (PIN_INPUT | MUX_MODE0) /* i2c2_sda */
+ >;
+ };
+
+ i2c3_pins: pinmux_i2c3_pins {
+ pinctrl-single,pins = <
+ 0x192 (PIN_INPUT | MUX_MODE0) /* i2c3_scl */
+ 0x194 (PIN_INPUT | MUX_MODE0) /* i2c3_sda */
+ >;
+ };
+
+ lp8720_en_pin: pinmux_lp8720_en_pin {
+ pinctrl-single,pins = <
+ 0x80 (PIN_OUTPUT | MUX_MODE4) /* gpio_37 */
+ >;
+ };
+
+ mmc1_pins: pinmux_mmc1_pins {
+ pinctrl-single,pins = <
+ 0x114 (PIN_INPUT | MUX_MODE0) /* sdmmc1_clk */
+ 0x116 (PIN_INPUT | MUX_MODE0) /* sdmmc1_cmd */
+ 0x118 (PIN_INPUT | MUX_MODE0) /* sdmmc1_dat0 */
+ 0x11a (PIN_INPUT | MUX_MODE0) /* sdmmc1_dat1 */
+ 0x11c (PIN_INPUT | MUX_MODE0) /* sdmmc1_dat2 */
+ 0x11e (PIN_INPUT | MUX_MODE0) /* sdmmc1_dat3 */
+ >;
+ };
+
+ mmc2_pins: pinmux_mmc2_pins {
+ pinctrl-single,pins = <
+ 0x128 (PIN_INPUT | MUX_MODE0) /* sdmmc2_clk */
+ 0x12a (PIN_INPUT | MUX_MODE0) /* sdmmc2_cmd */
+ 0x12c (PIN_INPUT | MUX_MODE0) /* sdmmc2_dat0 */
+ 0x12e (PIN_INPUT | MUX_MODE0) /* sdmmc2_dat1 */
+ 0x130 (PIN_INPUT | MUX_MODE0) /* sdmmc2_dat2 */
+ 0x132 (PIN_INPUT | MUX_MODE0) /* sdmmc2_dat3 */
+ 0x134 (PIN_INPUT | MUX_MODE0) /* sdmmc2_dat4 */
+ 0x136 (PIN_INPUT | MUX_MODE0) /* sdmmc2_dat5 */
+ 0x138 (PIN_INPUT | MUX_MODE0) /* sdmmc2_dat6 */
+ 0x13a (PIN_INPUT | MUX_MODE0) /* sdmmc2_dat7 */
+ >;
+ };
+};
+
+&omap3_pmx_wkup {
+ pinctrl-names = "default";
+
+ mmc1_cd_pin: pinmux_mmc1_cd_pin {
+ pinctrl-single,pins = <
+ 0x1a (PIN_INPUT | MUX_MODE4) /* gpio_10 */
+ >;
+ };
+};
+
+&gpio1 {
+ ti,no-reset-on-init;
+};
+
+&gpio2 {
+ ti,no-reset-on-init;
+};
+
+&gpio3 {
+ ti,no-reset-on-init;
+};
+
+&gpio4 {
+ ti,no-reset-on-init;
+};
+
+&gpio5 {
+ ti,no-reset-on-init;
+};
+
+&gpio6 {
+ ti,no-reset-on-init;
+};
+
+&uart3 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart3_pins>;
+
+ interrupts-extended = <&intc 74 &omap3_pmx_core OMAP3_UART3_RX>;
+};
+
+&i2c1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c1_pins>;
+
+ clock-frequency = <2600000>;
+
+ twl: ***@48 {
+ reg = <0x48>;
+ interrupts = <7>; /* SYS_NIRQ cascaded to intc */
+ interrupt-parent = <&intc>;
+
+ twl_power: power {
+ compatible = "ti,twl4030-power";
+ ti,use_poweroff;
+ };
+ };
+};
+
+#include "twl4030.dtsi"
+#include "twl4030_omap3.dtsi"
+
+&i2c2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c2_pins>;
+
+ clock-frequency = <400000>;
+};
+
+&i2c3 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c3_pins>;
+
+ clock-frequency = <400000>;
+
+ ***@7d {
+ pinctrl-names = "default";
+ pinctrl-0 = <&lp8720_en_pin>;
+
+ compatible = "ti,lp8720";
+ reg = <0x7d>;
+
+ ti,enable-gpio = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* gpio_37 */
+
+ lp8720_ldo1: ldo1 {
+ regulator-min-microvolt = <3000000>;
+ regulator-max-microvolt = <3000000>;
+ };
+ };
+};
+
+&mmc1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&mmc1_pins &mmc1_cd_pin>;
+ vmmc-supply = <&lp8720_ldo1>;
+ cd-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>; /* gpio 10 */
+ bus-width = <4>;
+};
+
+&mmc2 {
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&mmc2_pins>;
+ vmmc-supply = <&vmmc2>;
+ ti,non-removable;
+ bus-width = <8>;
+};
+
+&mmc3 {
+ status = "disabled";
+};
+
+/*
+ * The TWL4030 VAUX2 and VDAC regulators power sensors that are slaves on I2C3.
+ * When not powered, these sensors cause the I2C3 clock to stay low at all times,
+ * making it impossible to reach other devices on I2C3.
+ */
+
+&vaux2 {
+ regulator-min-microvolt = <2800000>;
+ regulator-max-microvolt = <2800000>;
+ regulator-always-on;
+};
+
+&vdac {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-always-on;
+};
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Tony Lindgren
2015-12-23 15:50:01 UTC
Permalink
Hi,
Post by Paul Kocialkowski
The LG Optimus Black (P970) codename sniper is a smartphone that was designed
and manufactured by LG Electronics (LGE) and released back in 2011.
It is using an OMAP3630 SoC, GP version.
This adds devicetree support for the device, with only a few basic features
supported, such as debug uart, i2c, internal emmc and external mmc.
Cool :)
Post by Paul Kocialkowski
+&gpio1 {
+ ti,no-reset-on-init;
+};
+
+&gpio2 {
+ ti,no-reset-on-init;
+};
+
+&gpio3 {
+ ti,no-reset-on-init;
+};
+
+&gpio4 {
+ ti,no-reset-on-init;
+};
+
+&gpio5 {
+ ti,no-reset-on-init;
+};
+
+&gpio6 {
+ ti,no-reset-on-init;
+};
Care to try to narrow down exactly which GPIO(s) need to be preserved?
Chances are this will unnecessarily block deeper idle states in hardware
otherwise.

My guess is that the GPIO pins that need to be preserved if any are in
the GPIO bank 1 as that's always powered..

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Paul Kocialkowski
2015-12-24 19:40:02 UTC
Permalink
Hi,
Post by Tony Lindgren
Post by Paul Kocialkowski
+&gpio1 {
+ ti,no-reset-on-init;
+};
+
+&gpio2 {
+ ti,no-reset-on-init;
+};
+
+&gpio3 {
+ ti,no-reset-on-init;
+};
+
+&gpio4 {
+ ti,no-reset-on-init;
+};
+
+&gpio5 {
+ ti,no-reset-on-init;
+};
+
+&gpio6 {
+ ti,no-reset-on-init;
+};
Care to try to narrow down exactly which GPIO(s) need to be preserved?
Chances are this will unnecessarily block deeper idle states in hardware
otherwise.
My guess is that the GPIO pins that need to be preserved if any are in
the GPIO bank 1 as that's always powered..
Well, I actually need to keep the GPIOs handling backlight control,
buttons LEDs and the micro USB connector muxing (which can be set to
UART or USB). Those are spread accross gpio2, gpio5 and gpio6, so I'll
only enable these in v2.

Thanks for the review!
--
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/
Javier Martinez Canillas
2015-12-23 16:10:01 UTC
Permalink
Hello Paul,

[snip]
Post by Paul Kocialkowski
+
+&omap3_pmx_core {
+ pinctrl-names = "default";
+
+ uart3_pins: pinmux_uart3_pins {
+ pinctrl-single,pins = <
+ 0x16e (PIN_INPUT | MUX_MODE0) /* uart3_rx_irrx */
+ 0x170 (PIN_OUTPUT | MUX_MODE0) /* uart3_tx_irtx */
+ >;
Could you please use the IOPAD mux macros from
include/dt-bindings/pinctrl/omap.h instead?

We just did a massive cleanup on the OMAP DTS to use them instead of
an offset from the padconf registers.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Paul Kocialkowski
2015-12-24 19:40:02 UTC
Permalink
Hi,

Le mercredi 23 décembre 2015 à 13:03 -0300, Javier Martinez Canillas a
Post by Javier Martinez Canillas
Hello Paul,
[snip]
Post by Paul Kocialkowski
+
+&omap3_pmx_core {
+ pinctrl-names = "default";
+
+ uart3_pins: pinmux_uart3_pins {
+ pinctrl-single,pins = <
+ 0x16e (PIN_INPUT | MUX_MODE0) /* uart3_rx_irrx */
+ 0x170 (PIN_OUTPUT | MUX_MODE0) /* uart3_tx_irtx */
+ >;
Could you please use the IOPAD mux macros from
include/dt-bindings/pinctrl/omap.h instead?
We just did a massive cleanup on the OMAP DTS to use them instead of
an offset from the padconf registers.
Sure thing, will do in v2.

Thanks for the review!
--
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/
Paul Kocialkowski
2015-12-23 11:10:02 UTC
Permalink
Some devices don't hook the DVS pin to a GPIO but to ground or VCC.
In those cases, it is not a problem to have no DVS GPIO.

Signed-off-by: Paul Kocialkowski <***@paulk.fr>
---
drivers/regulator/lp872x.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index 19d7584..21c49d8 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -738,10 +738,8 @@ static int lp872x_init_dvs(struct lp872x *lp)
goto set_default_dvs_mode;

gpio = dvs->gpio;
- if (!gpio_is_valid(gpio)) {
- dev_warn(lp->dev, "invalid gpio: %d\n", gpio);
+ if (!gpio_is_valid(gpio))
goto set_default_dvs_mode;
- }

pinstate = dvs->init_state;
ret = devm_gpio_request_one(lp->dev, gpio, pinstate, "LP872X DVS");
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Mark Brown
2015-12-23 11:50:03 UTC
Permalink
Post by Paul Kocialkowski
Some devices don't hook the DVS pin to a GPIO but to ground or VCC.
In those cases, it is not a problem to have no DVS GPIO.
I would expect the driver at least needs to know how the pins or
strapped, or otherwise have configuration for ignoring the input on the
pins. This just deletes the warnings, it doesn't have any handling for
this case that I can see.
Paul Kocialkowski
2015-12-23 12:00:02 UTC
Permalink
Post by Mark Brown
Post by Paul Kocialkowski
Some devices don't hook the DVS pin to a GPIO but to ground or VCC.
In those cases, it is not a problem to have no DVS GPIO.
I would expect the driver at least needs to know how the pins or
strapped, or otherwise have configuration for ignoring the input on the
pins. This just deletes the warnings, it doesn't have any handling for
this case that I can see.
When the DVS GPIO is invalid, lp872x_init_dvs jumps to the
set_default_dvs_mode label, which instructs the chip not to use the DVS
pin at all and do it all in software instead (by clearing the
LP8720_EXT_DVS_M bit in the LP872X_GENERAL_CFG register).

That is reflected later in the code, when setting the bucks (the DVS pin
only applies to the bucks) by checking for the LP8720_EXT_DVS_M bit on
the LP872X_GENERAL_CFG register (in lp872x_select_buck_vout_addr) to
decide whether to use software or hardware DVS selection.

As far as I understood, everything goes well when the GPIO is invalid.
--
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/
Mark Brown
2015-12-23 12:00:02 UTC
Permalink
Post by Paul Kocialkowski
Post by Mark Brown
I would expect the driver at least needs to know how the pins or
strapped, or otherwise have configuration for ignoring the input on the
pins. This just deletes the warnings, it doesn't have any handling for
this case that I can see.
When the DVS GPIO is invalid, lp872x_init_dvs jumps to the
set_default_dvs_mode label, which instructs the chip not to use the DVS
pin at all and do it all in software instead (by clearing the
LP8720_EXT_DVS_M bit in the LP872X_GENERAL_CFG register).
That is reflected later in the code, when setting the bucks (the DVS pin
only applies to the bucks) by checking for the LP8720_EXT_DVS_M bit on
the LP872X_GENERAL_CFG register (in lp872x_select_buck_vout_addr) to
decide whether to use software or hardware DVS selection.
As far as I understood, everything goes well when the GPIO is invalid.
So please put this analysis in the changelog so that the changelog
accurately reflects what the change is doing.
Paul Kocialkowski
2015-12-23 11:10:02 UTC
Permalink
LP872x regulators are made active via the EN pin, which might be hooked to a
GPIO. This adds support for driving the GPIO high when the driver is in use.

Signed-off-by: Paul Kocialkowski <***@paulk.fr>
---
.../devicetree/bindings/regulator/lp872x.txt | 1 +
drivers/regulator/lp872x.c | 33 ++++++++++++++++++++--
include/linux/regulator/lp872x.h | 2 ++
3 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/lp872x.txt b/Documentation/devicetree/bindings/regulator/lp872x.txt
index 7818318..0559c25 100644
--- a/Documentation/devicetree/bindings/regulator/lp872x.txt
+++ b/Documentation/devicetree/bindings/regulator/lp872x.txt
@@ -28,6 +28,7 @@ Optional properties:
- ti,dvs-gpio: GPIO specifier for external DVS pin control of LP872x devices.
- ti,dvs-vsel: DVS selector. 0 = SEL_V1, 1 = SEL_V2.
- ti,dvs-state: initial DVS pin state. 0 = DVS_LOW, 1 = DVS_HIGH.
+ - ti,enable-gpio: GPIO specifier for EN pin control of LP872x devices.

Sub nodes for regulator_init_data
LP8720 has maximum 6 nodes. (child name: ldo1 ~ 5 and buck)
diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index 21c49d8..c8855f3 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -726,6 +726,27 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
};

+static int lp872x_init_enable(struct lp872x *lp)
+{
+ int ret, gpio;
+
+ if (!lp->pdata)
+ return -EINVAL;
+
+ gpio = lp->pdata->enable_gpio;
+ if (!gpio_is_valid(gpio))
+ return 0;
+
+ /* Always set enable GPIO high. */
+ ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN");
+ if (ret) {
+ dev_err(lp->dev, "gpio request err: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
static int lp872x_init_dvs(struct lp872x *lp)
{
int ret, gpio;
@@ -763,14 +784,18 @@ static int lp872x_config(struct lp872x *lp)
int ret;

if (!pdata || !pdata->update_config)
- goto init_dvs;
+ goto init_dvs_enable;

ret = lp872x_write_byte(lp, LP872X_GENERAL_CFG, pdata->general_config);
if (ret)
return ret;

-init_dvs:
- return lp872x_init_dvs(lp);
+init_dvs_enable:
+ ret = lp872x_init_dvs(lp);
+ if (ret)
+ return ret;
+
+ return lp872x_init_enable(lp);
}

static struct regulator_init_data
@@ -875,6 +900,8 @@ static struct lp872x_platform_data
of_property_read_u8(np, "ti,dvs-state", &dvs_state);
pdata->dvs->init_state = dvs_state ? DVS_HIGH : DVS_LOW;

+ pdata->enable_gpio = of_get_named_gpio(np, "ti,enable-gpio", 0);
+
if (of_get_child_count(np) == 0)
goto out;

diff --git a/include/linux/regulator/lp872x.h b/include/linux/regulator/lp872x.h
index 132e05c..44316dd 100644
--- a/include/linux/regulator/lp872x.h
+++ b/include/linux/regulator/lp872x.h
@@ -79,12 +79,14 @@ struct lp872x_regulator_data {
* @update_config : if LP872X_GENERAL_CFG register is updated, set true
* @regulator_data : platform regulator id and init data
* @dvs : dvs data for buck voltage control
+ * @enable_gpio : gpio pin number for enable control
*/
struct lp872x_platform_data {
u8 general_config;
bool update_config;
struct lp872x_regulator_data regulator_data[LP872X_MAX_REGULATORS];
struct lp872x_dvs *dvs;
+ int enable_gpio;
};

#endif
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Mark Brown
2015-12-23 12:00:02 UTC
Permalink
Post by Paul Kocialkowski
+ gpio = lp->pdata->enable_gpio;
+ if (!gpio_is_valid(gpio))
+ return 0;
+
+ /* Always set enable GPIO high. */
+ ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN");
+ if (ret) {
+ dev_err(lp->dev, "gpio request err: %d\n", ret);
+ return ret;
+ }
This isn't really adding support for the enable GPIO as the changelog
suggests, it's requesting but not managing the GPIO. Since there is
core support for manging enable GPIOs this seems especially silly,
please tell the core about the GPIO and then it will work at runtime
too.
Paul Kocialkowski
2015-12-23 13:00:01 UTC
Permalink
Post by Mark Brown
Post by Paul Kocialkowski
+ gpio = lp->pdata->enable_gpio;
+ if (!gpio_is_valid(gpio))
+ return 0;
+
+ /* Always set enable GPIO high. */
+ ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN");
+ if (ret) {
+ dev_err(lp->dev, "gpio request err: %d\n", ret);
+ return ret;
+ }
This isn't really adding support for the enable GPIO as the changelog
suggests, it's requesting but not managing the GPIO. Since there is
core support for manging enable GPIOs this seems especially silly,
please tell the core about the GPIO and then it will work at runtime
too.
You're right, I had clearly overlooked this, thanks for pointing it out.
Will send v2 using GPIO handling from core.
--
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/
Paul Kocialkowski
2015-12-24 18:20:01 UTC
Permalink
Post by Mark Brown
Post by Paul Kocialkowski
+ gpio = lp->pdata->enable_gpio;
+ if (!gpio_is_valid(gpio))
+ return 0;
+
+ /* Always set enable GPIO high. */
+ ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN");
+ if (ret) {
+ dev_err(lp->dev, "gpio request err: %d\n", ret);
+ return ret;
+ }
This isn't really adding support for the enable GPIO as the changelog
suggests, it's requesting but not managing the GPIO. Since there is
core support for manging enable GPIOs this seems especially silly,
please tell the core about the GPIO and then it will work at runtime
too.
It looks like the core bindings for GPIO can only be used instead of the
rdev->desc->ops->enable callback, not jointly, which doesn't fit my use
case, where both the GPIO and register write have to be used to enable
regulators.

I think it would be worth making it possible to use both in core, since
that situation is probably shared with other regulators. I suggest the
following diff (that would be split into a separate patch in v2 of this
series):

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e92f344..dd0674f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1963,7 +1963,6 @@ static int regulator_ena_gpio_ctrl(struct
regulator_dev *rdev, bool enable)
if (pin->enable_count == 0)
gpiod_set_value_cansleep(pin->gpiod,
!pin->ena_gpio_invert);
-
pin->enable_count++;
} else {
if (pin->enable_count > 1) {
@@ -2063,6 +2062,14 @@ static int _regulator_do_enable(struct
regulator_dev *rdev)
}
}

+ if (rdev->desc->ops->enable) {
+ ret = rdev->desc->ops->enable(rdev);
+ if (ret < 0)
+ return ret;
+ } else if (!rdev->ena_pin) {
+ return -EINVAL;
+ }
+
if (rdev->ena_pin) {
if (!rdev->ena_gpio_state) {
ret = regulator_ena_gpio_ctrl(rdev, true);
@@ -2070,10 +2077,6 @@ static int _regulator_do_enable(struct
regulator_dev *rdev)
return ret;
rdev->ena_gpio_state = 1;
}
- } else if (rdev->desc->ops->enable) {
- ret = rdev->desc->ops->enable(rdev);
- if (ret < 0)
- return ret;
} else {
return -EINVAL;
}
Mark Brown
2015-12-24 19:40:02 UTC
Permalink
Post by Paul Kocialkowski
Post by Mark Brown
This isn't really adding support for the enable GPIO as the changelog
suggests, it's requesting but not managing the GPIO. Since there is
core support for manging enable GPIOs this seems especially silly,
please tell the core about the GPIO and then it will work at runtime
too.
It looks like the core bindings for GPIO can only be used instead of the
rdev->desc->ops->enable callback, not jointly, which doesn't fit my use
case, where both the GPIO and register write have to be used to enable
regulators.
I think it would be worth making it possible to use both in core, since
that situation is probably shared with other regulators. I suggest the
following diff (that would be split into a separate patch in v2 of this
No, that's broken - the whole point with using a GPIO for enable control
on a lot of devices is that it is much faster than doing a register
write. What I would expect to happen in this case is that when
initialsing the GPIO we set the register to enabled and then only manage
the GPIO at runtime.
Paul Kocialkowski
2015-12-24 20:10:01 UTC
Permalink
Post by Mark Brown
Post by Paul Kocialkowski
Post by Mark Brown
This isn't really adding support for the enable GPIO as the changelog
suggests, it's requesting but not managing the GPIO. Since there is
core support for manging enable GPIOs this seems especially silly,
please tell the core about the GPIO and then it will work at runtime
too.
It looks like the core bindings for GPIO can only be used instead of the
rdev->desc->ops->enable callback, not jointly, which doesn't fit my use
case, where both the GPIO and register write have to be used to enable
regulators.
I think it would be worth making it possible to use both in core, since
that situation is probably shared with other regulators. I suggest the
following diff (that would be split into a separate patch in v2 of this
No, that's broken - the whole point with using a GPIO for enable control
on a lot of devices is that it is much faster than doing a register
write. What I would expect to happen in this case is that when
initialsing the GPIO we set the register to enabled and then only manage
the GPIO at runtime.
That GPIO is shared with all regulators that the chip provides, so doing
things this way certainly won't work if we don't want all the regulators
powered at all times: in that case, the GPIO has precedence over the
regulator-specific registers.

Doing things the other way round would work and I suppose that chips
that can use a GPIO over registers just wouldn't register any
ops->enable callback in their description.
--
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/
Milo Kim
2015-12-28 02:50:12 UTC
Permalink
Hi Paul,
Post by Mark Brown
Post by Paul Kocialkowski
+ gpio = lp->pdata->enable_gpio;
+ if (!gpio_is_valid(gpio))
+ return 0;
+
+ /* Always set enable GPIO high. */
+ ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN");
+ if (ret) {
+ dev_err(lp->dev, "gpio request err: %d\n", ret);
+ return ret;
+ }
This isn't really adding support for the enable GPIO as the changelog
suggests, it's requesting but not managing the GPIO. Since there is
core support for manging enable GPIOs this seems especially silly,
please tell the core about the GPIO and then it will work at runtime
too.
With reference to my previous mail, external GPIOs for LDO3 and BUCK2 in
LP8725 can be specified through regulator_config.ena_gpio. BUCK2 only
can be controlled by external pin when CONFIG pin is grounded.

Please see the description at page 5 of the datasheet.

http://www.ti.com/lit/ds/symlink/lp8725.pdf

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Paul Kocialkowski
2015-12-28 23:00:03 UTC
Permalink
Hi Milo, thanks for the review,
Post by Milo Kim
Hi Paul,
Post by Mark Brown
Post by Paul Kocialkowski
+ gpio = lp->pdata->enable_gpio;
+ if (!gpio_is_valid(gpio))
+ return 0;
+
+ /* Always set enable GPIO high. */
+ ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN");
+ if (ret) {
+ dev_err(lp->dev, "gpio request err: %d\n", ret);
+ return ret;
+ }
This isn't really adding support for the enable GPIO as the changelog
suggests, it's requesting but not managing the GPIO. Since there is
core support for manging enable GPIOs this seems especially silly,
please tell the core about the GPIO and then it will work at runtime
too.
With reference to my previous mail, external GPIOs for LDO3 and BUCK2 in
LP8725 can be specified through regulator_config.ena_gpio. BUCK2 only
can be controlled by external pin when CONFIG pin is grounded.
Please see the description at page 5 of the datasheet.
http://www.ti.com/lit/ds/symlink/lp8725.pdf
After reading the datasheets thoroughly, it seems to me that for the
lp8720, the EN pin is used to enable the regulators output, which is a
good fit for the core regulator GPIO framework, as there is no reason to
keep it on when no regulator is in use. The serial interface is already
available when EN=0 and regulators can be configured in that state. The
lp8725 seems seems to behave the same when CONFIG=0 (the datasheet
clearly states: "CONFIG=0: EN=1 turns on outputs or standby mode if
EN=0"). On the other hand, it is indeed used as a power-on pin when
CONFIG=1.

Since my intent here is to cover the lp8720 use case, I suggest that we
implement this using the core regulator GPIO framework (I have patches
ready for that) and leave out the case where CONFIG=1, which could be
dealt with later by providing that piece of information via platform
data (and devicetree) and then either use the regulator GPIO framework
(when CONFIG=0 and default) or register the GPIO within the driver and
keep it on at all times (when CONFIG=1).

I could most certainly implement that behaviour, but I'd rather leave it
to someone else (or at least the testing) since I don't have any lp8725
to play with.

What do you think?
--
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/
Milo Kim
2015-12-29 00:50:02 UTC
Permalink
Hi Paul,
Post by Paul Kocialkowski
Hi Milo, thanks for the review,
Post by Milo Kim
Hi Paul,
Post by Mark Brown
Post by Paul Kocialkowski
+ gpio = lp->pdata->enable_gpio;
+ if (!gpio_is_valid(gpio))
+ return 0;
+
+ /* Always set enable GPIO high. */
+ ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN");
+ if (ret) {
+ dev_err(lp->dev, "gpio request err: %d\n", ret);
+ return ret;
+ }
This isn't really adding support for the enable GPIO as the changelog
suggests, it's requesting but not managing the GPIO. Since there is
core support for manging enable GPIOs this seems especially silly,
please tell the core about the GPIO and then it will work at runtime
too.
With reference to my previous mail, external GPIOs for LDO3 and BUCK2 in
LP8725 can be specified through regulator_config.ena_gpio. BUCK2 only
can be controlled by external pin when CONFIG pin is grounded.
Please see the description at page 5 of the datasheet.
http://www.ti.com/lit/ds/symlink/lp8725.pdf
After reading the datasheets thoroughly, it seems to me that for the
lp8720, the EN pin is used to enable the regulators output, which is a
good fit for the core regulator GPIO framework, as there is no reason to
keep it on when no regulator is in use. The serial interface is already
available when EN=0 and regulators can be configured in that state. The
lp8725 seems seems to behave the same when CONFIG=0 (the datasheet
clearly states: "CONFIG=0: EN=1 turns on outputs or standby mode if
EN=0"). On the other hand, it is indeed used as a power-on pin when
CONFIG=1.
I think it's different use case. LP8720/5 are designed for system PMU,
so some regulators are enabled by default after the device is on. EN pin
is used for turning on/off the chip. This pin does not control regulator
outputs directly. It's separate functional block in the silicon.

On the other hand, 'ena_gpio' is used for each regulator control itself.
For example, WM8994 has two LDOs which are controlled by external pins.
LDOs are enabled/disabled through LDO1ENA and LDO2ENA pins. In this
case, 'ena_gpio' is used.

http://www.cirrus.com/en/pubs/proDatasheet/WM8994_v4.4.pdf
(please refer to page 224 and 225)

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Paul Kocialkowski
2015-12-29 11:20:02 UTC
Permalink
Hi Milo,
Post by Milo Kim
Hi Paul,
Post by Paul Kocialkowski
Hi Milo, thanks for the review,
Post by Milo Kim
Hi Paul,
Post by Mark Brown
Post by Paul Kocialkowski
+ gpio = lp->pdata->enable_gpio;
+ if (!gpio_is_valid(gpio))
+ return 0;
+
+ /* Always set enable GPIO high. */
+ ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN");
+ if (ret) {
+ dev_err(lp->dev, "gpio request err: %d\n", ret);
+ return ret;
+ }
This isn't really adding support for the enable GPIO as the changelog
suggests, it's requesting but not managing the GPIO. Since there is
core support for manging enable GPIOs this seems especially silly,
please tell the core about the GPIO and then it will work at runtime
too.
With reference to my previous mail, external GPIOs for LDO3 and BUCK2 in
LP8725 can be specified through regulator_config.ena_gpio. BUCK2 only
can be controlled by external pin when CONFIG pin is grounded.
Please see the description at page 5 of the datasheet.
http://www.ti.com/lit/ds/symlink/lp8725.pdf
After reading the datasheets thoroughly, it seems to me that for the
lp8720, the EN pin is used to enable the regulators output, which is a
good fit for the core regulator GPIO framework, as there is no reason to
keep it on when no regulator is in use. The serial interface is already
available when EN=0 and regulators can be configured in that state. The
lp8725 seems seems to behave the same when CONFIG=0 (the datasheet
clearly states: "CONFIG=0: EN=1 turns on outputs or standby mode if
EN=0"). On the other hand, it is indeed used as a power-on pin when
CONFIG=1.
I think it's different use case. LP8720/5 are designed for system PMU,
so some regulators are enabled by default after the device is on. EN pin
is used for turning on/off the chip. This pin does not control regulator
outputs directly. It's separate functional block in the silicon.
Well, I really don't understand why the EN pin would turn on/off the
chip. All it does it enable the regulators outputs (by entering IDLE
mode), the serial block is already available in STANDBY state.

If we want some regulators enabled at boot, the best thing to do seems
to be to request the GPIO with the GPIOF_INIT_HIGH flag, as done in e.g.
the max8952 regulator driver:

if (pdata->reg_data->constraints.boot_on)
config.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
Post by Milo Kim
On the other hand, 'ena_gpio' is used for each regulator control itself.
For example, WM8994 has two LDOs which are controlled by external pins.
LDOs are enabled/disabled through LDO1ENA and LDO2ENA pins. In this
case, 'ena_gpio' is used.
Of course, but the ena_gpio feature is also a good fit for a global
enable pin, as the GPIO can be shared by multiple regulators of the same
chip, which is what we have here.

In my opinion, using the ena_gpio feature is a good fit, as we don't
need to keep the EN pin high when no regulator is used.
Post by Milo Kim
http://www.cirrus.com/en/pubs/proDatasheet/WM8994_v4.4.pdf
(please refer to page 224 and 225)
Cheers,
--
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/
Milo Kim
2015-12-30 00:30:01 UTC
Permalink
Hi Paul,
Post by Paul Kocialkowski
Hi Milo,
Post by Milo Kim
Hi Paul,
Post by Paul Kocialkowski
Hi Milo, thanks for the review,
Post by Milo Kim
Hi Paul,
Post by Mark Brown
Post by Paul Kocialkowski
+ gpio = lp->pdata->enable_gpio;
+ if (!gpio_is_valid(gpio))
+ return 0;
+
+ /* Always set enable GPIO high. */
+ ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN");
+ if (ret) {
+ dev_err(lp->dev, "gpio request err: %d\n", ret);
+ return ret;
+ }
This isn't really adding support for the enable GPIO as the changelog
suggests, it's requesting but not managing the GPIO. Since there is
core support for manging enable GPIOs this seems especially silly,
please tell the core about the GPIO and then it will work at runtime
too.
With reference to my previous mail, external GPIOs for LDO3 and BUCK2 in
LP8725 can be specified through regulator_config.ena_gpio. BUCK2 only
can be controlled by external pin when CONFIG pin is grounded.
Please see the description at page 5 of the datasheet.
http://www.ti.com/lit/ds/symlink/lp8725.pdf
After reading the datasheets thoroughly, it seems to me that for the
lp8720, the EN pin is used to enable the regulators output, which is a
good fit for the core regulator GPIO framework, as there is no reason to
keep it on when no regulator is in use. The serial interface is already
available when EN=0 and regulators can be configured in that state. The
lp8725 seems seems to behave the same when CONFIG=0 (the datasheet
clearly states: "CONFIG=0: EN=1 turns on outputs or standby mode if
EN=0"). On the other hand, it is indeed used as a power-on pin when
CONFIG=1.
I think it's different use case. LP8720/5 are designed for system PMU,
so some regulators are enabled by default after the device is on. EN pin
is used for turning on/off the chip. This pin does not control regulator
outputs directly. It's separate functional block in the silicon.
Well, I really don't understand why the EN pin would turn on/off the
chip. All it does it enable the regulators outputs (by entering IDLE
mode), the serial block is already available in STANDBY state.
If we want some regulators enabled at boot, the best thing to do seems
to be to request the GPIO with the GPIOF_INIT_HIGH flag, as done in e.g.
if (pdata->reg_data->constraints.boot_on)
config.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
According to MAX8952 datasheet, output regulator is enabled/disabled
using EN pin, so ena_gpio is used correctly.
However, LP8720/5 regulators are enabled/disabled through I2C command.
Only few regulators of LP8725 can be on/off by separate external pins.
(B2_EN and LDO3_EN)
Please note that EN pin in LP8720/5 is not the control pin for
enabling/disabling regulators.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Paul Kocialkowski
2015-12-30 08:40:02 UTC
Permalink
Hi Milo,
Post by Milo Kim
Hi Paul,
Post by Paul Kocialkowski
Hi Milo,
Post by Milo Kim
Hi Paul,
Post by Paul Kocialkowski
Hi Milo, thanks for the review,
Post by Milo Kim
Hi Paul,
Post by Mark Brown
Post by Paul Kocialkowski
+ gpio = lp->pdata->enable_gpio;
+ if (!gpio_is_valid(gpio))
+ return 0;
+
+ /* Always set enable GPIO high. */
+ ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN");
+ if (ret) {
+ dev_err(lp->dev, "gpio request err: %d\n", ret);
+ return ret;
+ }
This isn't really adding support for the enable GPIO as the changelog
suggests, it's requesting but not managing the GPIO. Since there is
core support for manging enable GPIOs this seems especially silly,
please tell the core about the GPIO and then it will work at runtime
too.
With reference to my previous mail, external GPIOs for LDO3 and BUCK2 in
LP8725 can be specified through regulator_config.ena_gpio. BUCK2 only
can be controlled by external pin when CONFIG pin is grounded.
Please see the description at page 5 of the datasheet.
http://www.ti.com/lit/ds/symlink/lp8725.pdf
After reading the datasheets thoroughly, it seems to me that for the
lp8720, the EN pin is used to enable the regulators output, which is a
good fit for the core regulator GPIO framework, as there is no reason to
keep it on when no regulator is in use. The serial interface is already
available when EN=0 and regulators can be configured in that state. The
lp8725 seems seems to behave the same when CONFIG=0 (the datasheet
clearly states: "CONFIG=0: EN=1 turns on outputs or standby mode if
EN=0"). On the other hand, it is indeed used as a power-on pin when
CONFIG=1.
I think it's different use case. LP8720/5 are designed for system PMU,
so some regulators are enabled by default after the device is on. EN pin
is used for turning on/off the chip. This pin does not control regulator
outputs directly. It's separate functional block in the silicon.
Well, I really don't understand why the EN pin would turn on/off the
chip. All it does it enable the regulators outputs (by entering IDLE
mode), the serial block is already available in STANDBY state.
If we want some regulators enabled at boot, the best thing to do seems
to be to request the GPIO with the GPIOF_INIT_HIGH flag, as done in e.g.
if (pdata->reg_data->constraints.boot_on)
config.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
According to MAX8952 datasheet, output regulator is enabled/disabled
using EN pin, so ena_gpio is used correctly.
However, LP8720/5 regulators are enabled/disabled through I2C command.
Only few regulators of LP8725 can be on/off by separate external pins.
(B2_EN and LDO3_EN)
Please note that EN pin in LP8720/5 is not the control pin for
enabling/disabling regulators.
Oh, I should have mentionned that I also suggest making it possible to
use *both* an enable GPIO and registers to enable a regulator. You are
of course right in saying that the GPIO alone is not enough, and the
core regulator framework will currently only do GPIO or registers to
enable not both.

In my opinion, it would be more elegant to adapt the core regulator
framework to first enable the GPIO and then call the regulator enable
ops callback instead of handling the GPIO in the driver.

However, if that change is not welcome to the core regulator framework,
we'll have to deal with the GPIO in the driver (and probably keep it
enabled at all times there).

Cheers,
--
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/
Mark Brown
2015-12-30 16:40:02 UTC
Permalink
Post by Paul Kocialkowski
In my opinion, it would be more elegant to adapt the core regulator
framework to first enable the GPIO and then call the regulator enable
ops callback instead of handling the GPIO in the driver.
Why would we want to actively manage both things at runtime? It's more
work, what do we gain from it?
Paul Kocialkowski
2015-12-30 18:40:02 UTC
Permalink
Post by Mark Brown
Post by Paul Kocialkowski
In my opinion, it would be more elegant to adapt the core regulator
framework to first enable the GPIO and then call the regulator enable
ops callback instead of handling the GPIO in the driver.
Why would we want to actively manage both things at runtime? It's more
work, what do we gain from it?
Well, I figured that it would be best to disable the EN pin when we're
not using any of the regulators, since that allows the chip to enter
standby mode (and thus consume less power).

Implementing that logic in the driver seems very redundant when we're
one step away from doing it with the core regulator framework.
It is also likely that in the future, other chips will use and need to
handle a global enable pin for the same purpose, while allowing
regulator configuration and enable via registers.

It also doesn't hurt regulators that only use a GPIO for enable.

The diff to do this is very minimal, I already have a patch ready, that
I could send if there is a chance for it to get through.
--
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/
Paul Kocialkowski
2015-12-31 22:00:02 UTC
Permalink
Post by Paul Kocialkowski
Post by Mark Brown
Post by Paul Kocialkowski
In my opinion, it would be more elegant to adapt the core regulator
framework to first enable the GPIO and then call the regulator enable
ops callback instead of handling the GPIO in the driver.
Why would we want to actively manage both things at runtime? It's more
work, what do we gain from it?
Well, I figured that it would be best to disable the EN pin when we're
not using any of the regulators, since that allows the chip to enter
standby mode (and thus consume less power).
This doesn't sound like it's anything to do with the regulators, that's
a chip wide power management function which should be implemented via
runtime PM if there's any value in implementing it at all (if the device
is a primary PMIC normally this would be handled by the CPU core when it
enters low power state without any software). It's not something we
should be considering on a per regulator basis since it's at the chip
level and on a per regulator basis it's not doing anything useful for
the reasons above.
I understand, thanks for pointing this out. Well, for my use case, there
is no use in disabling the chip at any point as it powers the external
mmc.

Would you agree to have the enable pin handled directly (and by that, I
mean enabled once, when requested, as I first suggested in the patchset)
in the driver then?
Post by Paul Kocialkowski
It also doesn't hurt regulators that only use a GPIO for enable.
It causes problems for any device with an optional GPIO, it means that
we end up mantaining both GPIO and register which as I've said a couple
of times now defeats the point of having the GPIO.
Fair enough.
Mark Brown
2015-12-31 22:20:03 UTC
Permalink
Post by Paul Kocialkowski
I understand, thanks for pointing this out. Well, for my use case, there
is no use in disabling the chip at any point as it powers the external
mmc.
Presumably someone might decide not to use the MMC in some case (perhaps
only mounting it when explicitly needed in order to save power for
example, or the MMC subsystem might figure out a way to power down an
idle MMC block device).
Post by Paul Kocialkowski
Would you agree to have the enable pin handled directly (and by that, I
mean enabled once, when requested, as I first suggested in the patchset)
in the driver then?
That's probably fine, or do it via runtime PM (the framework is fairly
simple to use, I'll probably go add support in the core for it in the
next day or two as this seems like a sensible use case). I can't
remember if this device is a MFD or not and I'm just on my way out the
door.
Paul Kocialkowski
2016-01-03 10:20:01 UTC
Permalink
Post by Mark Brown
Post by Paul Kocialkowski
I understand, thanks for pointing this out. Well, for my use case, there
is no use in disabling the chip at any point as it powers the external
mmc.
Presumably someone might decide not to use the MMC in some case (perhaps
only mounting it when explicitly needed in order to save power for
example, or the MMC subsystem might figure out a way to power down an
idle MMC block device).
Makes sense, I'll keep that in mind.
Post by Mark Brown
Post by Paul Kocialkowski
Would you agree to have the enable pin handled directly (and by that, I
mean enabled once, when requested, as I first suggested in the patchset)
in the driver then?
That's probably fine, or do it via runtime PM (the framework is fairly
simple to use, I'll probably go add support in the core for it in the
next day or two as this seems like a sensible use case). I can't
remember if this device is a MFD or not and I'm just on my way out the
door.
Runtime PM seems like a good fit (though I hadn't heard about it before:
you can guess I'm fairly new to kernel development), please let me know
whether you end up implementing it so I can try to handle the GPIO this
way.

Thanks!
--
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/
Paul Kocialkowski
2016-01-16 07:40:01 UTC
Permalink
Post by Mark Brown
Post by Paul Kocialkowski
I understand, thanks for pointing this out. Well, for my use case, there
is no use in disabling the chip at any point as it powers the external
mmc.
Presumably someone might decide not to use the MMC in some case (perhaps
only mounting it when explicitly needed in order to save power for
example, or the MMC subsystem might figure out a way to power down an
idle MMC block device).
Post by Paul Kocialkowski
Would you agree to have the enable pin handled directly (and by that, I
mean enabled once, when requested, as I first suggested in the patchset)
in the driver then?
That's probably fine, or do it via runtime PM (the framework is fairly
simple to use, I'll probably go add support in the core for it in the
next day or two as this seems like a sensible use case). I can't
remember if this device is a MFD or not and I'm just on my way out the
door.
Is there some git tree I can work with that has regulator runtime PM
support at this point? I'll certainly end up handling the GPIO in the
driver otherwise.
--
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/
Mark Brown
2016-01-18 16:40:02 UTC
Permalink
Post by Paul Kocialkowski
Is there some git tree I can work with that has regulator runtime PM
support at this point? I'll certainly end up handling the GPIO in the
driver otherwise.
No, the last week was much more busy than I was expecting. I'm hoping
for this week (or you could have a go at implementing it, it'd be adding
the PM operations in _regulator_enable() or _regulator_do_enable() if a
flag was set in the regulator_desc).
Paul Kocialkowski
2016-02-05 18:50:03 UTC
Permalink
Hi,
Post by Mark Brown
Post by Paul Kocialkowski
Is there some git tree I can work with that has regulator runtime PM
support at this point? I'll certainly end up handling the GPIO in the
driver otherwise.
No, the last week was much more busy than I was expecting. I'm hoping
for this week (or you could have a go at implementing it, it'd be adding
the PM operations in _regulator_enable() or _regulator_do_enable() if a
flag was set in the regulator_desc).
I decided to go with a function simply requesting the GPIO for v2.
Support for runtime PM can be added later. I'll get back to you on this
on the thread related to your patches, since it appears to be somewhat
broken for now.

Thanks!
--
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/
Mark Brown
2015-12-31 22:00:02 UTC
Permalink
Post by Paul Kocialkowski
Post by Mark Brown
Post by Paul Kocialkowski
In my opinion, it would be more elegant to adapt the core regulator
framework to first enable the GPIO and then call the regulator enable
ops callback instead of handling the GPIO in the driver.
Why would we want to actively manage both things at runtime? It's more
work, what do we gain from it?
Well, I figured that it would be best to disable the EN pin when we're
not using any of the regulators, since that allows the chip to enter
standby mode (and thus consume less power).
This doesn't sound like it's anything to do with the regulators, that's
a chip wide power management function which should be implemented via
runtime PM if there's any value in implementing it at all (if the device
is a primary PMIC normally this would be handled by the CPU core when it
enters low power state without any software). It's not something we
should be considering on a per regulator basis since it's at the chip
level and on a per regulator basis it's not doing anything useful for
the reasons above.
Post by Paul Kocialkowski
It also doesn't hurt regulators that only use a GPIO for enable.
It causes problems for any device with an optional GPIO, it means that
we end up mantaining both GPIO and register which as I've said a couple
of times now defeats the point of having the GPIO.
Milo Kim
2015-12-28 02:50:18 UTC
Permalink
Hi Paul,

Thanks for the patches. Please see my comments below.
Post by Paul Kocialkowski
LP872x regulators are made active via the EN pin, which might be hooked to a
GPIO. This adds support for driving the GPIO high when the driver is in use.
EN pin is used for enabling HW logic like I2C block. It's not regulator
enable pin. Please check the block diagram in the datasheet.

All regulators of LP8720 and LP8725 are controlled through I2C
registers. Additionally, LP8725 provides external pin control for LDO3
and BUCK2. In this case, you can use 'regulator_config.ena_gpio' when a
regulator is registered.
Post by Paul Kocialkowski
---
.../devicetree/bindings/regulator/lp872x.txt | 1 +
drivers/regulator/lp872x.c | 33 ++++++++++++++++++++--
include/linux/regulator/lp872x.h | 2 ++
3 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/regulator/lp872x.txt b/Documentation/devicetree/bindings/regulator/lp872x.txt
index 7818318..0559c25 100644
--- a/Documentation/devicetree/bindings/regulator/lp872x.txt
+++ b/Documentation/devicetree/bindings/regulator/lp872x.txt
- ti,dvs-gpio: GPIO specifier for external DVS pin control of LP872x devices.
- ti,dvs-vsel: DVS selector. 0 = SEL_V1, 1 = SEL_V2.
- ti,dvs-state: initial DVS pin state. 0 = DVS_LOW, 1 = DVS_HIGH.
+ - ti,enable-gpio: GPIO specifier for EN pin control of LP872x devices.
Please use general property, "enable-gpios" instead of "ti,enable-gpio".
Post by Paul Kocialkowski
Sub nodes for regulator_init_data
LP8720 has maximum 6 nodes. (child name: ldo1 ~ 5 and buck)
diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index 21c49d8..c8855f3 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -726,6 +726,27 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
};
+static int lp872x_init_enable(struct lp872x *lp)
lp872x_enable_hw() would be better.
Post by Paul Kocialkowski
+{
+ int ret, gpio;
+
+ if (!lp->pdata)
+ return -EINVAL;
+
+ gpio = lp->pdata->enable_gpio;
+ if (!gpio_is_valid(gpio))
+ return 0;
+
+ /* Always set enable GPIO high. */
+ ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN");
+ if (ret) {
+ dev_err(lp->dev, "gpio request err: %d\n", ret);
+ return ret;
+ }
LP8720 device needs max 200usec for startup time.
LP8725 also requires enable time about 30ms.
Please use usleep_range() after EN pin control.
Post by Paul Kocialkowski
+
+ return 0;
+}
+
static int lp872x_init_dvs(struct lp872x *lp)
{
int ret, gpio;
@@ -763,14 +784,18 @@ static int lp872x_config(struct lp872x *lp)
int ret;
if (!pdata || !pdata->update_config)
- goto init_dvs;
+ goto init_dvs_enable;
ret = lp872x_write_byte(lp, LP872X_GENERAL_CFG, pdata->general_config);
if (ret)
return ret;
- return lp872x_init_dvs(lp);
+ ret = lp872x_init_dvs(lp);
+ if (ret)
+ return ret;
+
+ return lp872x_init_enable(lp);
}
Logic should be enabled prior to DVS configuration. And please call
lp872x_enable_hw() in _probe().
Post by Paul Kocialkowski
static struct regulator_init_data
@@ -875,6 +900,8 @@ static struct lp872x_platform_data
of_property_read_u8(np, "ti,dvs-state", &dvs_state);
pdata->dvs->init_state = dvs_state ? DVS_HIGH : DVS_LOW;
+ pdata->enable_gpio = of_get_named_gpio(np, "ti,enable-gpio", 0);
+
Please replace "ti,enable-gpio" with "enable-gpios".

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Paul Kocialkowski
2016-02-05 18:50:02 UTC
Permalink
Post by Milo Kim
Hi Paul,
Thanks for the patches. Please see my comments below.
Thanks for the review Milo, I have just submitted v2 with those
suggestions integrated.
Post by Milo Kim
Post by Paul Kocialkowski
LP872x regulators are made active via the EN pin, which might be hooked to a
GPIO. This adds support for driving the GPIO high when the driver is in use.
EN pin is used for enabling HW logic like I2C block. It's not regulator
enable pin. Please check the block diagram in the datasheet.
All regulators of LP8720 and LP8725 are controlled through I2C
registers. Additionally, LP8725 provides external pin control for LDO3
and BUCK2. In this case, you can use 'regulator_config.ena_gpio' when a
regulator is registered.
Post by Paul Kocialkowski
---
.../devicetree/bindings/regulator/lp872x.txt | 1 +
drivers/regulator/lp872x.c | 33 ++++++++++++++++++++--
include/linux/regulator/lp872x.h | 2 ++
3 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/regulator/lp872x.txt b/Documentation/devicetree/bindings/regulator/lp872x.txt
index 7818318..0559c25 100644
--- a/Documentation/devicetree/bindings/regulator/lp872x.txt
+++ b/Documentation/devicetree/bindings/regulator/lp872x.txt
- ti,dvs-gpio: GPIO specifier for external DVS pin control of LP872x devices.
- ti,dvs-vsel: DVS selector. 0 = SEL_V1, 1 = SEL_V2.
- ti,dvs-state: initial DVS pin state. 0 = DVS_LOW, 1 = DVS_HIGH.
+ - ti,enable-gpio: GPIO specifier for EN pin control of LP872x devices.
Please use general property, "enable-gpios" instead of "ti,enable-gpio".
Post by Paul Kocialkowski
Sub nodes for regulator_init_data
LP8720 has maximum 6 nodes. (child name: ldo1 ~ 5 and buck)
diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index 21c49d8..c8855f3 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -726,6 +726,27 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
};
+static int lp872x_init_enable(struct lp872x *lp)
lp872x_enable_hw() would be better.
Post by Paul Kocialkowski
+{
+ int ret, gpio;
+
+ if (!lp->pdata)
+ return -EINVAL;
+
+ gpio = lp->pdata->enable_gpio;
+ if (!gpio_is_valid(gpio))
+ return 0;
+
+ /* Always set enable GPIO high. */
+ ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN");
+ if (ret) {
+ dev_err(lp->dev, "gpio request err: %d\n", ret);
+ return ret;
+ }
LP8720 device needs max 200usec for startup time.
LP8725 also requires enable time about 30ms.
Please use usleep_range() after EN pin control.
Post by Paul Kocialkowski
+
+ return 0;
+}
+
static int lp872x_init_dvs(struct lp872x *lp)
{
int ret, gpio;
@@ -763,14 +784,18 @@ static int lp872x_config(struct lp872x *lp)
int ret;
if (!pdata || !pdata->update_config)
- goto init_dvs;
+ goto init_dvs_enable;
ret = lp872x_write_byte(lp, LP872X_GENERAL_CFG, pdata->general_config);
if (ret)
return ret;
- return lp872x_init_dvs(lp);
+ ret = lp872x_init_dvs(lp);
+ if (ret)
+ return ret;
+
+ return lp872x_init_enable(lp);
}
Logic should be enabled prior to DVS configuration. And please call
lp872x_enable_hw() in _probe().
Post by Paul Kocialkowski
static struct regulator_init_data
@@ -875,6 +900,8 @@ static struct lp872x_platform_data
of_property_read_u8(np, "ti,dvs-state", &dvs_state);
pdata->dvs->init_state = dvs_state ? DVS_HIGH : DVS_LOW;
+ pdata->enable_gpio = of_get_named_gpio(np, "ti,enable-gpio", 0);
+
Please replace "ti,enable-gpio" with "enable-gpios".
Best regards,
Milo
Rob Herring
2015-12-29 20:10:01 UTC
Permalink
Post by Paul Kocialkowski
LP872x regulators are made active via the EN pin, which might be hooked to a
GPIO. This adds support for driving the GPIO high when the driver is in use.
---
.../devicetree/bindings/regulator/lp872x.txt | 1 +
drivers/regulator/lp872x.c | 33 ++++++++++++++++++++--
include/linux/regulator/lp872x.h | 2 ++
3 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/regulator/lp872x.txt b/Documentation/devicetree/bindings/regulator/lp872x.txt
index 7818318..0559c25 100644
--- a/Documentation/devicetree/bindings/regulator/lp872x.txt
+++ b/Documentation/devicetree/bindings/regulator/lp872x.txt
- ti,dvs-gpio: GPIO specifier for external DVS pin control of LP872x devices.
- ti,dvs-vsel: DVS selector. 0 = SEL_V1, 1 = SEL_V2.
- ti,dvs-state: initial DVS pin state. 0 = DVS_LOW, 1 = DVS_HIGH.
+ - ti,enable-gpio: GPIO specifier for EN pin control of LP872x devices.
Should be "-gpios" instead of "-gpio".

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Paul Kocialkowski
2015-12-29 21:30:02 UTC
Permalink
Post by Rob Herring
Post by Paul Kocialkowski
LP872x regulators are made active via the EN pin, which might be hooked to a
GPIO. This adds support for driving the GPIO high when the driver is in use.
---
.../devicetree/bindings/regulator/lp872x.txt | 1 +
drivers/regulator/lp872x.c | 33 ++++++++++++++++++++--
include/linux/regulator/lp872x.h | 2 ++
3 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/regulator/lp872x.txt b/Documentation/devicetree/bindings/regulator/lp872x.txt
index 7818318..0559c25 100644
--- a/Documentation/devicetree/bindings/regulator/lp872x.txt
+++ b/Documentation/devicetree/bindings/regulator/lp872x.txt
- ti,dvs-gpio: GPIO specifier for external DVS pin control of LP872x devices.
- ti,dvs-vsel: DVS selector. 0 = SEL_V1, 1 = SEL_V2.
- ti,dvs-state: initial DVS pin state. 0 = DVS_LOW, 1 = DVS_HIGH.
+ - ti,enable-gpio: GPIO specifier for EN pin control of LP872x devices.
Should be "-gpios" instead of "-gpio".
Care to comment why? There is only one GPIO that can be used here, since
there is only one single EN pin. I thought this matched what is done
already with "ti,dvs-gpio".

Thanks!
Rob Herring
2015-12-29 22:00:02 UTC
Permalink
Post by Paul Kocialkowski
Post by Rob Herring
Post by Paul Kocialkowski
LP872x regulators are made active via the EN pin, which might be hooked to a
GPIO. This adds support for driving the GPIO high when the driver is in use.
---
.../devicetree/bindings/regulator/lp872x.txt | 1 +
drivers/regulator/lp872x.c | 33 ++++++++++++++++++++--
include/linux/regulator/lp872x.h | 2 ++
3 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/regulator/lp872x.txt b/Documentation/devicetree/bindings/regulator/lp872x.txt
index 7818318..0559c25 100644
--- a/Documentation/devicetree/bindings/regulator/lp872x.txt
+++ b/Documentation/devicetree/bindings/regulator/lp872x.txt
- ti,dvs-gpio: GPIO specifier for external DVS pin control of LP872x devices.
- ti,dvs-vsel: DVS selector. 0 = SEL_V1, 1 = SEL_V2.
- ti,dvs-state: initial DVS pin state. 0 = DVS_LOW, 1 = DVS_HIGH.
+ - ti,enable-gpio: GPIO specifier for EN pin control of LP872x devices.
Should be "-gpios" instead of "-gpio".
Care to comment why? There is only one GPIO that can be used here, since
there is only one single EN pin. I thought this matched what is done
already with "ti,dvs-gpio".
To be consistent. We use "clocks" and "interrupts" always whether one
or more for example.

-gpio is documented as deprecated now.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Paul Kocialkowski
2016-02-05 18:50:02 UTC
Permalink
Hi,
Post by Rob Herring
Post by Paul Kocialkowski
Post by Rob Herring
Post by Paul Kocialkowski
LP872x regulators are made active via the EN pin, which might be hooked to a
GPIO. This adds support for driving the GPIO high when the driver is in use.
---
.../devicetree/bindings/regulator/lp872x.txt | 1 +
drivers/regulator/lp872x.c | 33 ++++++++++++++++++++--
include/linux/regulator/lp872x.h | 2 ++
3 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/regulator/lp872x.txt b/Documentation/devicetree/bindings/regulator/lp872x.txt
index 7818318..0559c25 100644
--- a/Documentation/devicetree/bindings/regulator/lp872x.txt
+++ b/Documentation/devicetree/bindings/regulator/lp872x.txt
- ti,dvs-gpio: GPIO specifier for external DVS pin control of LP872x devices.
- ti,dvs-vsel: DVS selector. 0 = SEL_V1, 1 = SEL_V2.
- ti,dvs-state: initial DVS pin state. 0 = DVS_LOW, 1 = DVS_HIGH.
+ - ti,enable-gpio: GPIO specifier for EN pin control of LP872x devices.
Should be "-gpios" instead of "-gpio".
Thanks for the review Rob, I have just submitted v2 with those
suggestions integrated.
Post by Rob Herring
Post by Paul Kocialkowski
Care to comment why? There is only one GPIO that can be used here, since
there is only one single EN pin. I thought this matched what is done
already with "ti,dvs-gpio".
To be consistent. We use "clocks" and "interrupts" always whether one
or more for example.
-gpio is documented as deprecated now.
Rob
Paul Kocialkowski
2015-12-23 11:10:03 UTC
Permalink
The LP872x regulator is used in the LG Optimus Black (P970) codename sniper
to supply the external mmc card.

Signed-off-by: Paul Kocialkowski <***@paulk.fr>
---
arch/arm/configs/multi_v7_defconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 69a22fd..e2c01f0 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -425,6 +425,7 @@ CONFIG_REGULATOR_RK808=y
CONFIG_REGULATOR_GPIO=y
CONFIG_MFD_SYSCON=y
CONFIG_POWER_RESET_SYSCON=y
+CONFIG_REGULATOR_LP872X=y
CONFIG_REGULATOR_MAX14577=m
CONFIG_REGULATOR_MAX8907=y
CONFIG_REGULATOR_MAX8973=y
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Paul Kocialkowski
2015-12-23 11:10:03 UTC
Permalink
The lp872x structure holds a reference to the DVS GPIO, but it is never actually
used anywhere, since a first reference exists from the lp872x_dvs structure.

Signed-off-by: Paul Kocialkowski <***@paulk.fr>
---
drivers/regulator/lp872x.c | 2 --
1 file changed, 2 deletions(-)

diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index 9412353..19d7584 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -108,7 +108,6 @@ struct lp872x {
struct lp872x_platform_data *pdata;
int num_regulators;
enum lp872x_dvs_state dvs_pin;
- int dvs_gpio;
};

/* LP8720/LP8725 shared voltage table for LDOs */
@@ -752,7 +751,6 @@ static int lp872x_init_dvs(struct lp872x *lp)
}

lp->dvs_pin = pinstate;
- lp->dvs_gpio = gpio;

return 0;
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Mark Brown
2015-12-23 12:20:02 UTC
Permalink
The patch

regulator: lp872x: Get rid of duplicate reference to DVS GPIO

has been applied to the regulator tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 8a99cc6ff5710780e3d9bfc41027c142725089e5 Mon Sep 17 00:00:00 2001
From: Paul Kocialkowski <***@paulk.fr>
Date: Wed, 23 Dec 2015 11:58:35 +0100
Subject: [PATCH] regulator: lp872x: Get rid of duplicate reference to DVS GPIO

The lp872x structure holds a reference to the DVS GPIO, but it is never actually
used anywhere, since a first reference exists from the lp872x_dvs structure.

Signed-off-by: Paul Kocialkowski <***@paulk.fr>
Signed-off-by: Mark Brown <***@kernel.org>
---
drivers/regulator/lp872x.c | 2 --
1 file changed, 2 deletions(-)

diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index 9412353da622..19d758486553 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -108,7 +108,6 @@ struct lp872x {
struct lp872x_platform_data *pdata;
int num_regulators;
enum lp872x_dvs_state dvs_pin;
- int dvs_gpio;
};

/* LP8720/LP8725 shared voltage table for LDOs */
@@ -752,7 +751,6 @@ static int lp872x_init_dvs(struct lp872x *lp)
}

lp->dvs_pin = pinstate;
- lp->dvs_gpio = gpio;

return 0;
--
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Milo Kim
2015-12-28 02:50:23 UTC
Permalink
Post by Paul Kocialkowski
The lp872x structure holds a reference to the DVS GPIO, but it is never actually
used anywhere, since a first reference exists from the lp872x_dvs structure.
---
drivers/regulator/lp872x.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index 9412353..19d7584 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -108,7 +108,6 @@ struct lp872x {
struct lp872x_platform_data *pdata;
int num_regulators;
enum lp872x_dvs_state dvs_pin;
- int dvs_gpio;
};
/* LP8720/LP8725 shared voltage table for LDOs */
@@ -752,7 +751,6 @@ static int lp872x_init_dvs(struct lp872x *lp)
}
lp->dvs_pin = pinstate;
- lp->dvs_gpio = gpio;
return 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Mark Brown
2015-12-23 12:20:03 UTC
Permalink
The patch

regulator: lp872x: Add missing of_match in regulators descriptions

has been applied to the regulator tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 1f97fe4777217123428e3212a95a67c0de7a0132 Mon Sep 17 00:00:00 2001
From: Paul Kocialkowski <***@paulk.fr>
Date: Wed, 23 Dec 2015 11:58:34 +0100
Subject: [PATCH] regulator: lp872x: Add missing of_match in regulators
descriptions

In order to select the regulators via of_find_regulator_by_node (and thus use
them in devicetree), defining of_match for each regulator is required.

Signed-off-by: Paul Kocialkowski <***@paulk.fr>
Signed-off-by: Mark Brown <***@kernel.org>
---
drivers/regulator/lp872x.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index e5af07208f9d..9412353da622 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -520,6 +520,7 @@ static struct regulator_ops lp8725_buck_ops = {
static struct regulator_desc lp8720_regulator_desc[] = {
{
.name = "ldo1",
+ .of_match = of_match_ptr("ldo1"),
.id = LP8720_ID_LDO1,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -533,6 +534,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo2",
+ .of_match = of_match_ptr("ldo2"),
.id = LP8720_ID_LDO2,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -546,6 +548,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo3",
+ .of_match = of_match_ptr("ldo3"),
.id = LP8720_ID_LDO3,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -559,6 +562,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo4",
+ .of_match = of_match_ptr("ldo4"),
.id = LP8720_ID_LDO4,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp8720_ldo4_vtbl),
@@ -572,6 +576,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo5",
+ .of_match = of_match_ptr("ldo5"),
.id = LP8720_ID_LDO5,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -585,6 +590,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "buck",
+ .of_match = of_match_ptr("buck"),
.id = LP8720_ID_BUCK,
.ops = &lp8720_buck_ops,
.n_voltages = ARRAY_SIZE(lp8720_buck_vtbl),
@@ -599,6 +605,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
static struct regulator_desc lp8725_regulator_desc[] = {
{
.name = "ldo1",
+ .of_match = of_match_ptr("ldo1"),
.id = LP8725_ID_LDO1,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -612,6 +619,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo2",
+ .of_match = of_match_ptr("ldo2"),
.id = LP8725_ID_LDO2,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -625,6 +633,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo3",
+ .of_match = of_match_ptr("ldo3"),
.id = LP8725_ID_LDO3,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -638,6 +647,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo4",
+ .of_match = of_match_ptr("ldo4"),
.id = LP8725_ID_LDO4,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -651,6 +661,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo5",
+ .of_match = of_match_ptr("ldo5"),
.id = LP8725_ID_LDO5,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -664,6 +675,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "lilo1",
+ .of_match = of_match_ptr("lilo1"),
.id = LP8725_ID_LILO1,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl),
@@ -677,6 +689,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "lilo2",
+ .of_match = of_match_ptr("lilo2"),
.id = LP8725_ID_LILO2,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl),
@@ -690,6 +703,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "buck1",
+ .of_match = of_match_ptr("buck1"),
.id = LP8725_ID_BUCK1,
.ops = &lp8725_buck_ops,
.n_voltages = ARRAY_SIZE(lp8725_buck_vtbl),
@@ -701,6 +715,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "buck2",
+ .of_match = of_match_ptr("buck2"),
.id = LP8725_ID_BUCK2,
.ops = &lp8725_buck_ops,
.n_voltages = ARRAY_SIZE(lp8725_buck_vtbl),
--
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Milo Kim
2015-12-28 02:50:39 UTC
Permalink
Post by Paul Kocialkowski
In order to select the regulators via of_find_regulator_by_node (and thus use
them in devicetree), defining of_match for each regulator is required.
---
drivers/regulator/lp872x.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index e5af072..9412353 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -520,6 +520,7 @@ static struct regulator_ops lp8725_buck_ops = {
static struct regulator_desc lp8720_regulator_desc[] = {
{
.name = "ldo1",
+ .of_match = of_match_ptr("ldo1"),
.id = LP8720_ID_LDO1,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -533,6 +534,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo2",
+ .of_match = of_match_ptr("ldo2"),
.id = LP8720_ID_LDO2,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -546,6 +548,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo3",
+ .of_match = of_match_ptr("ldo3"),
.id = LP8720_ID_LDO3,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -559,6 +562,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo4",
+ .of_match = of_match_ptr("ldo4"),
.id = LP8720_ID_LDO4,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp8720_ldo4_vtbl),
@@ -572,6 +576,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo5",
+ .of_match = of_match_ptr("ldo5"),
.id = LP8720_ID_LDO5,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -585,6 +590,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "buck",
+ .of_match = of_match_ptr("buck"),
.id = LP8720_ID_BUCK,
.ops = &lp8720_buck_ops,
.n_voltages = ARRAY_SIZE(lp8720_buck_vtbl),
@@ -599,6 +605,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
static struct regulator_desc lp8725_regulator_desc[] = {
{
.name = "ldo1",
+ .of_match = of_match_ptr("ldo1"),
.id = LP8725_ID_LDO1,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -612,6 +619,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo2",
+ .of_match = of_match_ptr("ldo2"),
.id = LP8725_ID_LDO2,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -625,6 +633,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo3",
+ .of_match = of_match_ptr("ldo3"),
.id = LP8725_ID_LDO3,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -638,6 +647,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo4",
+ .of_match = of_match_ptr("ldo4"),
.id = LP8725_ID_LDO4,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -651,6 +661,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo5",
+ .of_match = of_match_ptr("ldo5"),
.id = LP8725_ID_LDO5,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -664,6 +675,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "lilo1",
+ .of_match = of_match_ptr("lilo1"),
.id = LP8725_ID_LILO1,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl),
@@ -677,6 +689,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "lilo2",
+ .of_match = of_match_ptr("lilo2"),
.id = LP8725_ID_LILO2,
.ops = &lp872x_ldo_ops,
.n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl),
@@ -690,6 +703,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "buck1",
+ .of_match = of_match_ptr("buck1"),
.id = LP8725_ID_BUCK1,
.ops = &lp8725_buck_ops,
.n_voltages = ARRAY_SIZE(lp8725_buck_vtbl),
@@ -701,6 +715,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "buck2",
+ .of_match = of_match_ptr("buck2"),
.id = LP8725_ID_BUCK2,
.ops = &lp8725_buck_ops,
.n_voltages = ARRAY_SIZE(lp8725_buck_vtbl),
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Loading...