[PATCH 0/3] regulator: twl: Fix regulator mode support

[PATCH 0/3] regulator: twl: Fix regulator mode support

[Ivaylo Dimitrov]

On Nokia N900 regulators are left in the mode last set by the bootloader
or by the stock kernel, depends on whether it is power-on or reboot from
stock kernel to mainline. That leads to problem with devices connected
to vmmc2 regulator - when the device is rebooted from stock kernel vmmc2
is left in "sleep" mode (REGULATOR_STATUS_STANDBY in terms of regulator
framework) and as noone in mainline kernel switches vmmc2 regulator to
normal (REGULATOR_STATUS_NORMAL) mode, devices supplied by it does not
get enough power to operate normally.

Fix that by providing the correct functionality for initial mode setting
from the board DTS. Fix i2c access to powerbus while at it.

I will send a follow-up patch for N900 board DTS that sets initial
regulators mode once the $subject series is assumed to be ok.

Ivaylo Dimitrov (3):
  regulator: twl: Make sure we have access to powerbus before trying to
    write to it
  regulator: twl: Provide of_map_mode for twl4030
  regulator: twl: Regulator mode should not depend on regulator enabled
    state

 drivers/regulator/twl-regulator.c | 100 +++++++++++++++++++++++++++++++-------
 1 file changed, 82 insertions(+), 18 deletions(-)

-- 
1.9.1

[PATCH 1/3] regulator: twl: Make sure we have access to powerbus before trying to write to it

[Ivaylo Dimitrov]

According to the TRM, we need to enable i2c access to powerbus before
writing to it. Also, a new write to powerbus should not be attempted if
there is a pending transfer. The current code does not implement that
functionality and while there are no known problems caused by that, it is
better to follow what TRM says.

Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
 drivers/regulator/twl-regulator.c | 78 +++++++++++++++++++++++++++++++++++----
 1 file changed, 70 insertions(+), 8 deletions(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 955a6fb..aad748b0 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -21,7 +21,7 @@
 #include <linux/regulator/machine.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/i2c/twl.h>
-
+#include <linux/delay.h>
 
 /*
  * The TWL4030/TW5030/TPS659x0/TWL6030 family chips include power management, a
@@ -188,6 +188,74 @@ static int twl6030reg_is_enabled(struct regulator_dev *rdev)
 	return grp && (val == TWL6030_CFG_STATE_ON);
 }
 
+#define PB_I2C_BUSY	BIT(0)
+#define PB_I2C_BWEN	BIT(1)
+
+/* Wait until buffer empty/ready to send a word on power bus. */
+static int twl4030_wait_pb_ready(void)
+{
+
+	int	ret;
+	int	timeout = 10;
+	u8	val;
+
+	do {
+		ret = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val,
+				      TWL4030_PM_MASTER_PB_CFG);
+		if (ret < 0)
+			return ret;
+
+		if (!(val & PB_I2C_BUSY))
+			return 0;
+
+		mdelay(1);
+		timeout--;
+	} while (timeout);
+
+	return -ETIMEDOUT;
+}
+
+/* Send a word over the powerbus */
+static int twl4030_send_pb_msg(unsigned msg)
+{
+	u8	val;
+	int	ret;
+
+	/* save powerbus configuration */
+	ret = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val,
+			      TWL4030_PM_MASTER_PB_CFG);
+	if (ret < 0)
+		return ret;
+
+	/* Enable i2c access to powerbus */
+	ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, val | PB_I2C_BWEN,
+			       TWL4030_PM_MASTER_PB_CFG);
+	if (ret < 0)
+		return ret;
+
+	ret = twl4030_wait_pb_ready();
+	if (ret < 0)
+		return ret;
+
+	ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, msg >> 8,
+			       TWL4030_PM_MASTER_PB_WORD_MSB);
+	if (ret < 0)
+		return ret;
+
+	ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, msg & 0xff,
+			       TWL4030_PM_MASTER_PB_WORD_LSB);
+	if (ret < 0)
+		return ret;
+
+	ret = twl4030_wait_pb_ready();
+	if (ret < 0)
+		return ret;
+
+	/* Restore powerbus configuration */
+	return twl_i2c_write_u8(TWL_MODULE_PM_MASTER, val,
+				TWL_MODULE_PM_MASTER);
+}
+
 static int twl4030reg_enable(struct regulator_dev *rdev)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
@@ -324,13 +392,7 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
 	if (!(status & (P3_GRP_4030 | P2_GRP_4030 | P1_GRP_4030)))
 		return -EACCES;
 
-	status = twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
-			message >> 8, TWL4030_PM_MASTER_PB_WORD_MSB);
-	if (status < 0)
-		return status;
-
-	return twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
-			message & 0xff, TWL4030_PM_MASTER_PB_WORD_LSB);
+	return twl4030_send_pb_msg(message);
 }
 
 static int twl6030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
-- 
1.9.1

[PATCH 2/3] regulator: twl: Provide of_map_mode for twl4030

[Ivaylo Dimitrov]

of_map_mode is needed so to be possible to set initial regulators mode from
the board DTS. Otherwise, for DT boot, regulators are left in their default
state after reset/reboot.

Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
 drivers/regulator/twl-regulator.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index aad748b0..be8d05e 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -395,6 +395,12 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
 	return twl4030_send_pb_msg(message);
 }
 
+static inline unsigned int twl4030reg_map_mode(unsigned int mode)
+{
+	return mode == RES_STATE_ACTIVE ?
+				REGULATOR_MODE_NORMAL : REGULATOR_MODE_STANDBY;
+}
+
 static int twl6030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
@@ -897,10 +903,11 @@ static struct regulator_ops twlsmps_ops = {
 #define TWL4030_FIXED_LDO(label, offset, mVolts, num, turnon_delay, \
 			remap_conf) \
 		TWL_FIXED_LDO(label, offset, mVolts, num, turnon_delay, \
-			remap_conf, TWL4030, twl4030fixed_ops)
+			remap_conf, TWL4030, twl4030fixed_ops, \
+			twl4030reg_map_mode)
 #define TWL6030_FIXED_LDO(label, offset, mVolts, turnon_delay) \
 		TWL_FIXED_LDO(label, offset, mVolts, 0x0, turnon_delay, \
-			0x0, TWL6030, twl6030fixed_ops)
+			0x0, TWL6030, twl6030fixed_ops, 0x0)
 
 #define TWL4030_ADJUSTABLE_LDO(label, offset, num, turnon_delay, remap_conf) \
 static const struct twlreg_info TWL4030_INFO_##label = { \
@@ -917,6 +924,7 @@ static const struct twlreg_info TWL4030_INFO_##label = { \
 		.type = REGULATOR_VOLTAGE, \
 		.owner = THIS_MODULE, \
 		.enable_time = turnon_delay, \
+		.of_map_mode = twl4030reg_map_mode, \
 		}, \
 	}
 
@@ -932,6 +940,7 @@ static const struct twlreg_info TWL4030_INFO_##label = { \
 		.type = REGULATOR_VOLTAGE, \
 		.owner = THIS_MODULE, \
 		.enable_time = turnon_delay, \
+		.of_map_mode = twl4030reg_map_mode, \
 		}, \
 	}
 
@@ -977,7 +986,7 @@ static const struct twlreg_info TWL6032_INFO_##label = { \
 	}
 
 #define TWL_FIXED_LDO(label, offset, mVolts, num, turnon_delay, remap_conf, \
-		family, operations) \
+		family, operations, map_mode) \
 static const struct twlreg_info TWLFIXED_INFO_##label = { \
 	.base = offset, \
 	.id = num, \
@@ -992,6 +1001,7 @@ static const struct twlreg_info TWLFIXED_INFO_##label = { \
 		.owner = THIS_MODULE, \
 		.min_uV = mVolts * 1000, \
 		.enable_time = turnon_delay, \
+		.of_map_mode = map_mode, \
 		}, \
 	}
 
-- 
1.9.1

[PATCH 3/3] regulator: twl: Regulator mode should not depend on regulator enabled state

[Ivaylo Dimitrov]

When machine constraints are applied, regulator framework first sets
initial mode (if any) and then enables the regulator if needed. The current
code in twl4030reg_set_mode always checks if the regulator is enabled
before applying the mode. That results in -EACCES error returned for
"always-on" regulators which have "initial-mode" set in the board DTS. Fix
that by removing the unneeded check.

Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
 drivers/regulator/twl-regulator.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index be8d05e..d8f6ad6 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -371,7 +371,6 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
 	unsigned		message;
-	int			status;
 
 	/* We can only set the mode through state machine commands... */
 	switch (mode) {
@@ -385,13 +384,6 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
 		return -EINVAL;
 	}
 
-	/* Ensure the resource is associated with some group */
-	status = twlreg_grp(rdev);
-	if (status < 0)
-		return status;
-	if (!(status & (P3_GRP_4030 | P2_GRP_4030 | P1_GRP_4030)))
-		return -EACCES;
-
 	return twl4030_send_pb_msg(message);
 }
 
-- 
1.9.1

Re: [PATCH 2/3] regulator: twl: Provide of_map_mode for twl4030

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 365 bytes --]

On Sat, Mar 26, 2016 at 10:28:14AM +0200, Ivaylo Dimitrov wrote:
> of_map_mode is needed so to be possible to set initial regulators mode from
> the board DTS. Otherwise, for DT boot, regulators are left in their default
> state after reset/reboot.

This should also update the DT binding document for the device to
specify what the valid modes for the device are.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH] regulator: twl: Provide of_map_mode for twl4030

[Ivaylo Dimitrov]

of_map_mode is needed so to be possible to set initial regulators mode from
the board DTS. Otherwise, for DT boot, regulators are left in their default
state after reset/reboot. Document device specific modes as well.

Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
 .../devicetree/bindings/regulator/twl-regulator.txt      |  8 ++++++++
 drivers/regulator/twl-regulator.c                        | 16 +++++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/twl-regulator.txt b/Documentation/devicetree/bindings/regulator/twl-regulator.txt
index 75b0c16..fe759903 100644
--- a/Documentation/devicetree/bindings/regulator/twl-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/twl-regulator.txt
@@ -57,6 +57,14 @@ For twl4030 regulators/LDOs
 
 Optional properties:
 - Any optional property defined in bindings/regulator/regulator.txt
+For twl4030 regulators/LDOs:
+ - regulator-initial-mode:
+  - 0x00 - Off mode, the output voltage is not maintained and voltage regulator
+           power consumption is 0.
+  - 0x08 - Sleep mode, the nominal output voltage is maintained with low power
+           consumption with low load current capability.
+  - 0x0e - Active mode, the regulator can deliver its nominal output voltage
+           with full-load current capability.
 
 Example:
 
diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index aad748b0..be8d05e 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -395,6 +395,12 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
 	return twl4030_send_pb_msg(message);
 }
 
+static inline unsigned int twl4030reg_map_mode(unsigned int mode)
+{
+	return mode == RES_STATE_ACTIVE ?
+				REGULATOR_MODE_NORMAL : REGULATOR_MODE_STANDBY;
+}
+
 static int twl6030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
@@ -897,10 +903,11 @@ static struct regulator_ops twlsmps_ops = {
 #define TWL4030_FIXED_LDO(label, offset, mVolts, num, turnon_delay, \
 			remap_conf) \
 		TWL_FIXED_LDO(label, offset, mVolts, num, turnon_delay, \
-			remap_conf, TWL4030, twl4030fixed_ops)
+			remap_conf, TWL4030, twl4030fixed_ops, \
+			twl4030reg_map_mode)
 #define TWL6030_FIXED_LDO(label, offset, mVolts, turnon_delay) \
 		TWL_FIXED_LDO(label, offset, mVolts, 0x0, turnon_delay, \
-			0x0, TWL6030, twl6030fixed_ops)
+			0x0, TWL6030, twl6030fixed_ops, 0x0)
 
 #define TWL4030_ADJUSTABLE_LDO(label, offset, num, turnon_delay, remap_conf) \
 static const struct twlreg_info TWL4030_INFO_##label = { \
@@ -917,6 +924,7 @@ static const struct twlreg_info TWL4030_INFO_##label = { \
 		.type = REGULATOR_VOLTAGE, \
 		.owner = THIS_MODULE, \
 		.enable_time = turnon_delay, \
+		.of_map_mode = twl4030reg_map_mode, \
 		}, \
 	}
 
@@ -932,6 +940,7 @@ static const struct twlreg_info TWL4030_INFO_##label = { \
 		.type = REGULATOR_VOLTAGE, \
 		.owner = THIS_MODULE, \
 		.enable_time = turnon_delay, \
+		.of_map_mode = twl4030reg_map_mode, \
 		}, \
 	}
 
@@ -977,7 +986,7 @@ static const struct twlreg_info TWL6032_INFO_##label = { \
 	}
 
 #define TWL_FIXED_LDO(label, offset, mVolts, num, turnon_delay, remap_conf, \
-		family, operations) \
+		family, operations, map_mode) \
 static const struct twlreg_info TWLFIXED_INFO_##label = { \
 	.base = offset, \
 	.id = num, \
@@ -992,6 +1001,7 @@ static const struct twlreg_info TWLFIXED_INFO_##label = { \
 		.owner = THIS_MODULE, \
 		.min_uV = mVolts * 1000, \
 		.enable_time = turnon_delay, \
+		.of_map_mode = map_mode, \
 		}, \
 	}
 
-- 
1.9.1

[PATCH] regulator: twl: Provide of_map_mode for twl4030

[Ivaylo Dimitrov]

of_map_mode is needed so to be possible to set initial regulators mode from
the board DTS. Otherwise, for DT boot, regulators are left in their default
state after reset/reboot. Document device specific modes as well.

Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/regulator/twl-regulator.txt      |  8 ++++++++
 drivers/regulator/twl-regulator.c                        | 16 +++++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/twl-regulator.txt b/Documentation/devicetree/bindings/regulator/twl-regulator.txt
index 75b0c16..fe759903 100644
--- a/Documentation/devicetree/bindings/regulator/twl-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/twl-regulator.txt
@@ -57,6 +57,14 @@ For twl4030 regulators/LDOs
 
 Optional properties:
 - Any optional property defined in bindings/regulator/regulator.txt
+For twl4030 regulators/LDOs:
+ - regulator-initial-mode:
+  - 0x00 - Off mode, the output voltage is not maintained and voltage regulator
+           power consumption is 0.
+  - 0x08 - Sleep mode, the nominal output voltage is maintained with low power
+           consumption with low load current capability.
+  - 0x0e - Active mode, the regulator can deliver its nominal output voltage
+           with full-load current capability.
 
 Example:
 
diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index aad748b0..be8d05e 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -395,6 +395,12 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
 	return twl4030_send_pb_msg(message);
 }
 
+static inline unsigned int twl4030reg_map_mode(unsigned int mode)
+{
+	return mode == RES_STATE_ACTIVE ?
+				REGULATOR_MODE_NORMAL : REGULATOR_MODE_STANDBY;
+}
+
 static int twl6030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
@@ -897,10 +903,11 @@ static struct regulator_ops twlsmps_ops = {
 #define TWL4030_FIXED_LDO(label, offset, mVolts, num, turnon_delay, \
 			remap_conf) \
 		TWL_FIXED_LDO(label, offset, mVolts, num, turnon_delay, \
-			remap_conf, TWL4030, twl4030fixed_ops)
+			remap_conf, TWL4030, twl4030fixed_ops, \
+			twl4030reg_map_mode)
 #define TWL6030_FIXED_LDO(label, offset, mVolts, turnon_delay) \
 		TWL_FIXED_LDO(label, offset, mVolts, 0x0, turnon_delay, \
-			0x0, TWL6030, twl6030fixed_ops)
+			0x0, TWL6030, twl6030fixed_ops, 0x0)
 
 #define TWL4030_ADJUSTABLE_LDO(label, offset, num, turnon_delay, remap_conf) \
 static const struct twlreg_info TWL4030_INFO_##label = { \
@@ -917,6 +924,7 @@ static const struct twlreg_info TWL4030_INFO_##label = { \
 		.type = REGULATOR_VOLTAGE, \
 		.owner = THIS_MODULE, \
 		.enable_time = turnon_delay, \
+		.of_map_mode = twl4030reg_map_mode, \
 		}, \
 	}
 
@@ -932,6 +940,7 @@ static const struct twlreg_info TWL4030_INFO_##label = { \
 		.type = REGULATOR_VOLTAGE, \
 		.owner = THIS_MODULE, \
 		.enable_time = turnon_delay, \
+		.of_map_mode = twl4030reg_map_mode, \
 		}, \
 	}
 
@@ -977,7 +986,7 @@ static const struct twlreg_info TWL6032_INFO_##label = { \
 	}
 
 #define TWL_FIXED_LDO(label, offset, mVolts, num, turnon_delay, remap_conf, \
-		family, operations) \
+		family, operations, map_mode) \
 static const struct twlreg_info TWLFIXED_INFO_##label = { \
 	.base = offset, \
 	.id = num, \
@@ -992,6 +1001,7 @@ static const struct twlreg_info TWLFIXED_INFO_##label = { \
 		.owner = THIS_MODULE, \
 		.min_uV = mVolts * 1000, \
 		.enable_time = turnon_delay, \
+		.of_map_mode = map_mode, \
 		}, \
 	}
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] regulator: twl: Provide of_map_mode for twl4030

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

On Mon, Apr 04, 2016 at 09:57:14PM +0300, Ivaylo Dimitrov wrote:

> +For twl4030 regulators/LDOs:
> + - regulator-initial-mode:
> +  - 0x00 - Off mode, the output voltage is not maintained and voltage regulator
> +           power consumption is 0.

This isn't a mode, it's just the regulator being off.  Just drop it.

> +static inline unsigned int twl4030reg_map_mode(unsigned int mode)
> +{
> +	return mode == RES_STATE_ACTIVE ?
> +				REGULATOR_MODE_NORMAL : REGULATOR_MODE_STANDBY;
> +}

Please write normal if statements, the code should be readable.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v1] regulator: twl: Provide of_map_mode for twl4030

[Ivaylo Dimitrov]

of_map_mode is needed so to be possible to set initial regulators mode from
the board DTS. Otherwise, for DT boot, regulators are left in their default
state after reset/reboot. Document device specific modes as well.

Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
 .../bindings/regulator/twl-regulator.txt           |  6 ++++++
 drivers/regulator/twl-regulator.c                  | 22 +++++++++++++++++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/twl-regulator.txt b/Documentation/devicetree/bindings/regulator/twl-regulator.txt
index 75b0c16..74a91c4 100644
--- a/Documentation/devicetree/bindings/regulator/twl-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/twl-regulator.txt
@@ -57,6 +57,12 @@ For twl4030 regulators/LDOs
 
 Optional properties:
 - Any optional property defined in bindings/regulator/regulator.txt
+For twl4030 regulators/LDOs:
+ - regulator-initial-mode:
+  - 0x08 - Sleep mode, the nominal output voltage is maintained with low power
+           consumption with low load current capability.
+  - 0x0e - Active mode, the regulator can deliver its nominal output voltage
+           with full-load current capability.
 
 Example:
 
diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index aad748b0..53fcbb0 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -395,6 +395,18 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
 	return twl4030_send_pb_msg(message);
 }
 
+static inline unsigned int twl4030reg_map_mode(unsigned int mode)
+{
+	switch (mode) {
+	case RES_STATE_ACTIVE:
+		return REGULATOR_MODE_NORMAL;
+	case RES_STATE_SLEEP:
+		return REGULATOR_MODE_STANDBY;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int twl6030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
@@ -897,10 +909,11 @@ static struct regulator_ops twlsmps_ops = {
 #define TWL4030_FIXED_LDO(label, offset, mVolts, num, turnon_delay, \
 			remap_conf) \
 		TWL_FIXED_LDO(label, offset, mVolts, num, turnon_delay, \
-			remap_conf, TWL4030, twl4030fixed_ops)
+			remap_conf, TWL4030, twl4030fixed_ops, \
+			twl4030reg_map_mode)
 #define TWL6030_FIXED_LDO(label, offset, mVolts, turnon_delay) \
 		TWL_FIXED_LDO(label, offset, mVolts, 0x0, turnon_delay, \
-			0x0, TWL6030, twl6030fixed_ops)
+			0x0, TWL6030, twl6030fixed_ops, 0x0)
 
 #define TWL4030_ADJUSTABLE_LDO(label, offset, num, turnon_delay, remap_conf) \
 static const struct twlreg_info TWL4030_INFO_##label = { \
@@ -917,6 +930,7 @@ static const struct twlreg_info TWL4030_INFO_##label = { \
 		.type = REGULATOR_VOLTAGE, \
 		.owner = THIS_MODULE, \
 		.enable_time = turnon_delay, \
+		.of_map_mode = twl4030reg_map_mode, \
 		}, \
 	}
 
@@ -932,6 +946,7 @@ static const struct twlreg_info TWL4030_INFO_##label = { \
 		.type = REGULATOR_VOLTAGE, \
 		.owner = THIS_MODULE, \
 		.enable_time = turnon_delay, \
+		.of_map_mode = twl4030reg_map_mode, \
 		}, \
 	}
 
@@ -977,7 +992,7 @@ static const struct twlreg_info TWL6032_INFO_##label = { \
 	}
 
 #define TWL_FIXED_LDO(label, offset, mVolts, num, turnon_delay, remap_conf, \
-		family, operations) \
+		family, operations, map_mode) \
 static const struct twlreg_info TWLFIXED_INFO_##label = { \
 	.base = offset, \
 	.id = num, \
@@ -992,6 +1007,7 @@ static const struct twlreg_info TWLFIXED_INFO_##label = { \
 		.owner = THIS_MODULE, \
 		.min_uV = mVolts * 1000, \
 		.enable_time = turnon_delay, \
+		.of_map_mode = map_mode, \
 		}, \
 	}
 
-- 
1.9.1

Re: [PATCH 1/3] regulator: twl: Make sure we have access to powerbus before trying to write to it

[Ivaylo Dimitrov]

On 26.03.2016 10:28, Ivaylo Dimitrov wrote:
> According to the TRM, we need to enable i2c access to powerbus before
> writing to it. Also, a new write to powerbus should not be attempted if
> there is a pending transfer. The current code does not implement that
> functionality and while there are no known problems caused by that, it is
> better to follow what TRM says.
>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> ---
>   drivers/regulator/twl-regulator.c | 78 +++++++++++++++++++++++++++++++++++----
>   1 file changed, 70 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
> index 955a6fb..aad748b0 100644
> --- a/drivers/regulator/twl-regulator.c
> +++ b/drivers/regulator/twl-regulator.c
> @@ -21,7 +21,7 @@
>   #include <linux/regulator/machine.h>
>   #include <linux/regulator/of_regulator.h>
>   #include <linux/i2c/twl.h>
> -
> +#include <linux/delay.h>
>
>   /*
>    * The TWL4030/TW5030/TPS659x0/TWL6030 family chips include power management, a
> @@ -188,6 +188,74 @@ static int twl6030reg_is_enabled(struct regulator_dev *rdev)
>   	return grp && (val == TWL6030_CFG_STATE_ON);
>   }
>
> +#define PB_I2C_BUSY	BIT(0)
> +#define PB_I2C_BWEN	BIT(1)
> +
> +/* Wait until buffer empty/ready to send a word on power bus. */
> +static int twl4030_wait_pb_ready(void)
> +{
> +
> +	int	ret;
> +	int	timeout = 10;
> +	u8	val;
> +
> +	do {
> +		ret = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val,
> +				      TWL4030_PM_MASTER_PB_CFG);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (!(val & PB_I2C_BUSY))
> +			return 0;
> +
> +		mdelay(1);
> +		timeout--;
> +	} while (timeout);
> +
> +	return -ETIMEDOUT;
> +}
> +
> +/* Send a word over the powerbus */
> +static int twl4030_send_pb_msg(unsigned msg)
> +{
> +	u8	val;
> +	int	ret;
> +
> +	/* save powerbus configuration */
> +	ret = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val,
> +			      TWL4030_PM_MASTER_PB_CFG);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Enable i2c access to powerbus */
> +	ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, val | PB_I2C_BWEN,
> +			       TWL4030_PM_MASTER_PB_CFG);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = twl4030_wait_pb_ready();
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, msg >> 8,
> +			       TWL4030_PM_MASTER_PB_WORD_MSB);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, msg & 0xff,
> +			       TWL4030_PM_MASTER_PB_WORD_LSB);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = twl4030_wait_pb_ready();
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Restore powerbus configuration */
> +	return twl_i2c_write_u8(TWL_MODULE_PM_MASTER, val,
> +				TWL_MODULE_PM_MASTER);

Hmm, I made a copy/paste error here, this should be:

	return twl_i2c_write_u8(TWL_MODULE_PM_MASTER, val,
				TWL4030_PM_MASTER_PB_CFG);

Will send a new version of the patch.

> +}
> +
>   static int twl4030reg_enable(struct regulator_dev *rdev)
>   {
>   	struct twlreg_info	*info = rdev_get_drvdata(rdev);
> @@ -324,13 +392,7 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
>   	if (!(status & (P3_GRP_4030 | P2_GRP_4030 | P1_GRP_4030)))
>   		return -EACCES;
>
> -	status = twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
> -			message >> 8, TWL4030_PM_MASTER_PB_WORD_MSB);
> -	if (status < 0)
> -		return status;
> -
> -	return twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
> -			message & 0xff, TWL4030_PM_MASTER_PB_WORD_LSB);
> +	return twl4030_send_pb_msg(message);
>   }
>
>   static int twl6030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
>

Ivo

[PATCH] regulator: twl: Fix a typo in twl4030_send_pb_msg

[Ivaylo Dimitrov]

Commit <2330b05c095bdeaaf1261c54cd2d4b9127496996> ("regulator: twl: Make
sure we have access to powerbus before trying to write to it")
has implemented the needed logic to correctly access powerbus through i2c,
however it brought a typo when powerbus configuration is restored, which
results in writing to a wrong register. Fix that by providing the correct
register value.

Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
 drivers/regulator/twl-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 11c6a5c..faeb5ee 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -253,7 +253,7 @@ static int twl4030_send_pb_msg(unsigned msg)
 
 	/* Restore powerbus configuration */
 	return twl_i2c_write_u8(TWL_MODULE_PM_MASTER, val,
-				TWL_MODULE_PM_MASTER);
+				TWL4030_PM_MASTER_PB_CFG);
 }
 
 static int twl4030reg_enable(struct regulator_dev *rdev)
-- 
1.9.1

Re: [PATCH v1] regulator: twl: Provide of_map_mode for twl4030

[Rob Herring]

On Tue, Apr 05, 2016 at 08:59:34AM +0300, Ivaylo Dimitrov wrote:
> of_map_mode is needed so to be possible to set initial regulators mode from
> the board DTS. Otherwise, for DT boot, regulators are left in their default
> state after reset/reboot. Document device specific modes as well.
> 
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> ---
>  .../bindings/regulator/twl-regulator.txt           |  6 ++++++

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/regulator/twl-regulator.c                  | 22 +++++++++++++++++++---
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 

Re: [PATCH v1] regulator: twl: Provide of_map_mode for twl4030

[Tony Lindgren]

* Rob Herring <robh@kernel.org> [160407 10:58]:
> On Tue, Apr 05, 2016 at 08:59:34AM +0300, Ivaylo Dimitrov wrote:
> > of_map_mode is needed so to be possible to set initial regulators mode from
> > the board DTS. Otherwise, for DT boot, regulators are left in their default
> > state after reset/reboot. Document device specific modes as well.
> > 
> > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > ---
> >  .../bindings/regulator/twl-regulator.txt           |  6 ++++++
> 
> Acked-by: Rob Herring <robh@kernel.org>

I'd like to test these patches, but I don't know which combination
of patches needed? It seems we're waiting for an update on at least
one of the patches in this series?

Might be best to repost the whole series so people can test the
right patches.

Regards,

Tony

Re: [PATCH v1] regulator: twl: Provide of_map_mode for twl4030

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

Hi Tony,

On Fri, Apr 08, 2016 at 08:49:07AM -0700, Tony Lindgren wrote:
> * Rob Herring <robh@kernel.org> [160407 10:58]:
> > On Tue, Apr 05, 2016 at 08:59:34AM +0300, Ivaylo Dimitrov wrote:
> > > of_map_mode is needed so to be possible to set initial regulators mode from
> > > the board DTS. Otherwise, for DT boot, regulators are left in their default
> > > state after reset/reboot. Document device specific modes as well.
> > > 
> > > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > > ---
> > >  .../bindings/regulator/twl-regulator.txt           |  6 ++++++
> > 
> > Acked-by: Rob Herring <robh@kernel.org>
> 
> I'd like to test these patches, but I don't know which combination
> of patches needed? It seems we're waiting for an update on at least
> one of the patches in this series?
> 
> Might be best to repost the whole series so people can test the
> right patches.

As far as I can see Mark has already queued all patches in his
for-next branch and they are already in today's linux-next, so
you can just test linux-next.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1] regulator: twl: Provide of_map_mode for twl4030

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

Hi Tony,

On Fri, Apr 08, 2016 at 08:49:07AM -0700, Tony Lindgren wrote:
> * Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [160407 10:58]:
> > On Tue, Apr 05, 2016 at 08:59:34AM +0300, Ivaylo Dimitrov wrote:
> > > of_map_mode is needed so to be possible to set initial regulators mode from
> > > the board DTS. Otherwise, for DT boot, regulators are left in their default
> > > state after reset/reboot. Document device specific modes as well.
> > > 
> > > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > ---
> > >  .../bindings/regulator/twl-regulator.txt           |  6 ++++++
> > 
> > Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 
> I'd like to test these patches, but I don't know which combination
> of patches needed? It seems we're waiting for an update on at least
> one of the patches in this series?
> 
> Might be best to repost the whole series so people can test the
> right patches.

As far as I can see Mark has already queued all patches in his
for-next branch and they are already in today's linux-next, so
you can just test linux-next.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1] regulator: twl: Provide of_map_mode for twl4030

[Tony Lindgren]

* Sebastian Reichel <sre@ring0.de> [160408 09:09]:
> Hi Tony,
> 
> On Fri, Apr 08, 2016 at 08:49:07AM -0700, Tony Lindgren wrote:
> > * Rob Herring <robh@kernel.org> [160407 10:58]:
> > > On Tue, Apr 05, 2016 at 08:59:34AM +0300, Ivaylo Dimitrov wrote:
> > > > of_map_mode is needed so to be possible to set initial regulators mode from
> > > > the board DTS. Otherwise, for DT boot, regulators are left in their default
> > > > state after reset/reboot. Document device specific modes as well.
> > > > 
> > > > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > > > ---
> > > >  .../bindings/regulator/twl-regulator.txt           |  6 ++++++
> > > 
> > > Acked-by: Rob Herring <robh@kernel.org>
> > 
> > I'd like to test these patches, but I don't know which combination
> > of patches needed? It seems we're waiting for an update on at least
> > one of the patches in this series?
> > 
> > Might be best to repost the whole series so people can test the
> > right patches.
> 
> As far as I can see Mark has already queued all patches in his
> for-next branch and they are already in today's linux-next, so
> you can just test linux-next.

OK thanks will do.

Tony

Re: [PATCH v1] regulator: twl: Provide of_map_mode for twl4030

[Tony Lindgren]

* Sebastian Reichel <sre-GFxCN5SEZAc@public.gmane.org> [160408 09:09]:
> Hi Tony,
> 
> On Fri, Apr 08, 2016 at 08:49:07AM -0700, Tony Lindgren wrote:
> > * Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [160407 10:58]:
> > > On Tue, Apr 05, 2016 at 08:59:34AM +0300, Ivaylo Dimitrov wrote:
> > > > of_map_mode is needed so to be possible to set initial regulators mode from
> > > > the board DTS. Otherwise, for DT boot, regulators are left in their default
> > > > state after reset/reboot. Document device specific modes as well.
> > > > 
> > > > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > ---
> > > >  .../bindings/regulator/twl-regulator.txt           |  6 ++++++
> > > 
> > > Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > 
> > I'd like to test these patches, but I don't know which combination
> > of patches needed? It seems we're waiting for an update on at least
> > one of the patches in this series?
> > 
> > Might be best to repost the whole series so people can test the
> > right patches.
> 
> As far as I can see Mark has already queued all patches in his
> for-next branch and they are already in today's linux-next, so
> you can just test linux-next.

OK thanks will do.

Tony


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] regulator: twl: Provide of_map_mode for twl4030

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [160408 09:21]:
> * Sebastian Reichel <sre@ring0.de> [160408 09:09]:
> > Hi Tony,
> > 
> > On Fri, Apr 08, 2016 at 08:49:07AM -0700, Tony Lindgren wrote:
> > > * Rob Herring <robh@kernel.org> [160407 10:58]:
> > > > On Tue, Apr 05, 2016 at 08:59:34AM +0300, Ivaylo Dimitrov wrote:
> > > > > of_map_mode is needed so to be possible to set initial regulators mode from
> > > > > the board DTS. Otherwise, for DT boot, regulators are left in their default
> > > > > state after reset/reboot. Document device specific modes as well.
> > > > > 
> > > > > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > > > > ---
> > > > >  .../bindings/regulator/twl-regulator.txt           |  6 ++++++
> > > > 
> > > > Acked-by: Rob Herring <robh@kernel.org>
> > > 
> > > I'd like to test these patches, but I don't know which combination
> > > of patches needed? It seems we're waiting for an update on at least
> > > one of the patches in this series?
> > > 
> > > Might be best to repost the whole series so people can test the
> > > right patches.
> > 
> > As far as I can see Mark has already queued all patches in his
> > for-next branch and they are already in today's linux-next, so
> > you can just test linux-next.
> 
> OK thanks will do.

Yup PM seems to work just fine with these.

Tony

Re: [PATCH v1] regulator: twl: Provide of_map_mode for twl4030

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160408 09:21]:
> * Sebastian Reichel <sre-GFxCN5SEZAc@public.gmane.org> [160408 09:09]:
> > Hi Tony,
> > 
> > On Fri, Apr 08, 2016 at 08:49:07AM -0700, Tony Lindgren wrote:
> > > * Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [160407 10:58]:
> > > > On Tue, Apr 05, 2016 at 08:59:34AM +0300, Ivaylo Dimitrov wrote:
> > > > > of_map_mode is needed so to be possible to set initial regulators mode from
> > > > > the board DTS. Otherwise, for DT boot, regulators are left in their default
> > > > > state after reset/reboot. Document device specific modes as well.
> > > > > 
> > > > > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > > ---
> > > > >  .../bindings/regulator/twl-regulator.txt           |  6 ++++++
> > > > 
> > > > Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > 
> > > I'd like to test these patches, but I don't know which combination
> > > of patches needed? It seems we're waiting for an update on at least
> > > one of the patches in this series?
> > > 
> > > Might be best to repost the whole series so people can test the
> > > right patches.
> > 
> > As far as I can see Mark has already queued all patches in his
> > for-next branch and they are already in today's linux-next, so
> > you can just test linux-next.
> 
> OK thanks will do.

Yup PM seems to work just fine with these.

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] regulator: twl: Make sure we have access to powerbus before trying to write to it

[Pavel Machek]

Hi!

> According to the TRM, we need to enable i2c access to powerbus before
> writing to it. Also, a new write to powerbus should not be attempted if
> there is a pending transfer. The current code does not implement that
> functionality and while there are no known problems caused by that, it is
> better to follow what TRM says.
> 
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> ---
>  drivers/regulator/twl-regulator.c | 78 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 70 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
> index 955a6fb..aad748b0 100644
> --- a/drivers/regulator/twl-regulator.c
> +++ b/drivers/regulator/twl-regulator.c
> @@ -21,7 +21,7 @@
>  #include <linux/regulator/machine.h>
>  #include <linux/regulator/of_regulator.h>
>  #include <linux/i2c/twl.h>
> -
> +#include <linux/delay.h>
>  
>  /*
>   * The TWL4030/TW5030/TPS659x0/TWL6030 family chips include power management, a
> @@ -188,6 +188,74 @@ static int twl6030reg_is_enabled(struct regulator_dev *rdev)
>  	return grp && (val == TWL6030_CFG_STATE_ON);
>  }
>  
> +#define PB_I2C_BUSY	BIT(0)
> +#define PB_I2C_BWEN	BIT(1)
> +
> +/* Wait until buffer empty/ready to send a word on power bus. */
> +static int twl4030_wait_pb_ready(void)
> +{
> +
> +	int	ret;
> +	int	timeout = 10;
> +	u8	val;
> +

Can we do this plain

    while (timeout--) {
    }...

? Also... if the bit is not immediately available, it will wait for
1msec. Would it make sense to have timeout = 1000 but wait only 10usec
each time or something?

Thanks,
								Pavel
								
> +	do {
> +		ret = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val,
> +				      TWL4030_PM_MASTER_PB_CFG);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (!(val & PB_I2C_BUSY))
> +			return 0;
> +
> +		mdelay(1);
> +		timeout--;
> +	} while (timeout);
> +
> +	return -ETIMEDOUT;
> +}

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 3/3] regulator: twl: Regulator mode should not depend on regulator enabled state

[Pavel Machek]

On Sat 2016-03-26 10:28:15, Ivaylo Dimitrov wrote:
> When machine constraints are applied, regulator framework first sets
> initial mode (if any) and then enables the regulator if needed. The current
> code in twl4030reg_set_mode always checks if the regulator is enabled
> before applying the mode. That results in -EACCES error returned for
> "always-on" regulators which have "initial-mode" set in the board DTS. Fix
> that by removing the unneeded check.

Should we keep the check for the regulators that are _not_ always-on?

Thanks,
									Pavel

> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> ---
>  drivers/regulator/twl-regulator.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
> index be8d05e..d8f6ad6 100644
> --- a/drivers/regulator/twl-regulator.c
> +++ b/drivers/regulator/twl-regulator.c
> @@ -371,7 +371,6 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
>  {
>  	struct twlreg_info	*info = rdev_get_drvdata(rdev);
>  	unsigned		message;
> -	int			status;
>  
>  	/* We can only set the mode through state machine commands... */
>  	switch (mode) {
> @@ -385,13 +384,6 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
>  		return -EINVAL;
>  	}
>  
> -	/* Ensure the resource is associated with some group */
> -	status = twlreg_grp(rdev);
> -	if (status < 0)
> -		return status;
> -	if (!(status & (P3_GRP_4030 | P2_GRP_4030 | P1_GRP_4030)))
> -		return -EACCES;
> -
>  	return twl4030_send_pb_msg(message);
>  }
>  

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 3/3] regulator: twl: Regulator mode should not depend on regulator enabled state

[Ivaylo Dimitrov]

Hi,

On 24.04.2016 19:11, Pavel Machek wrote:
> On Sat 2016-03-26 10:28:15, Ivaylo Dimitrov wrote:
>> When machine constraints are applied, regulator framework first sets
>> initial mode (if any) and then enables the regulator if needed. The current
>> code in twl4030reg_set_mode always checks if the regulator is enabled
>> before applying the mode. That results in -EACCES error returned for
>> "always-on" regulators which have "initial-mode" set in the board DTS. Fix
>> that by removing the unneeded check.
>
> Should we keep the check for the regulators that are _not_ always-on?
>

I don't think so, as there is no other way to set mode to a regulator 
but through regulator framework and board DTS (for twl4030 that is) - 
and this is done only once during boot, see 
https://lkml.org/lkml/2016/4/17/78 as well. So, no matter if the 
regulator is always-on or not, that check is redundant IMO.

Ivo



>
>> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
>> ---
>>   drivers/regulator/twl-regulator.c | 8 --------
>>   1 file changed, 8 deletions(-)
>>
>> diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
>> index be8d05e..d8f6ad6 100644
>> --- a/drivers/regulator/twl-regulator.c
>> +++ b/drivers/regulator/twl-regulator.c
>> @@ -371,7 +371,6 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
>>   {
>>   	struct twlreg_info	*info = rdev_get_drvdata(rdev);
>>   	unsigned		message;
>> -	int			status;
>>
>>   	/* We can only set the mode through state machine commands... */
>>   	switch (mode) {
>> @@ -385,13 +384,6 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
>>   		return -EINVAL;
>>   	}
>>
>> -	/* Ensure the resource is associated with some group */
>> -	status = twlreg_grp(rdev);
>> -	if (status < 0)
>> -		return status;
>> -	if (!(status & (P3_GRP_4030 | P2_GRP_4030 | P1_GRP_4030)))
>> -		return -EACCES;
>> -
>>   	return twl4030_send_pb_msg(message);
>>   }
>>
>

Re: [PATCH 1/3] regulator: twl: Make sure we have access to powerbus before trying to write to it

[Ivaylo Dimitrov]

Hi,

On 24.04.2016 19:10, Pavel Machek wrote:
> Hi!
>
>> According to the TRM, we need to enable i2c access to powerbus before
>> writing to it. Also, a new write to powerbus should not be attempted if
>> there is a pending transfer. The current code does not implement that
>> functionality and while there are no known problems caused by that, it is
>> better to follow what TRM says.
>>
>> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
>> ---
>>   drivers/regulator/twl-regulator.c | 78 +++++++++++++++++++++++++++++++++++----
>>   1 file changed, 70 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
>> index 955a6fb..aad748b0 100644
>> --- a/drivers/regulator/twl-regulator.c
>> +++ b/drivers/regulator/twl-regulator.c
>> @@ -21,7 +21,7 @@
>>   #include <linux/regulator/machine.h>
>>   #include <linux/regulator/of_regulator.h>
>>   #include <linux/i2c/twl.h>
>> -
>> +#include <linux/delay.h>
>>
>>   /*
>>    * The TWL4030/TW5030/TPS659x0/TWL6030 family chips include power management, a
>> @@ -188,6 +188,74 @@ static int twl6030reg_is_enabled(struct regulator_dev *rdev)
>>   	return grp && (val == TWL6030_CFG_STATE_ON);
>>   }
>>
>> +#define PB_I2C_BUSY	BIT(0)
>> +#define PB_I2C_BWEN	BIT(1)
>> +
>> +/* Wait until buffer empty/ready to send a word on power bus. */
>> +static int twl4030_wait_pb_ready(void)
>> +{
>> +
>> +	int	ret;
>> +	int	timeout = 10;
>> +	u8	val;
>> +
>
> Can we do this plain
>
>      while (timeout--) {
>      }...
>

Now looking at the code, yes, while(timeout--) looks prettier, but as 
the $subject patch is in the linux-next for a couple of weeks already, 
that change will need another patch. I'll put that in my TODO, right 
after the RFC for the N900 cameras :) .

> ? Also... if the bit is not immediately available, it will wait for
> 1msec. Would it make sense to have timeout = 1000 but wait only 10usec
> each time or something?
>

Well, anyway these look like a kind of arbitrary values, I guess it will 
work both ways. But, I borrowed the code in the patch from stock Nokia 
kernel, it has been field tested on tens of thousands of devices, so, 
unless you have hard values giving a reason to do it the other way, I 
prefer to keep it as it is.

Regards,
Ivo

>> +	do {
>> +		ret = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val,
>> +				      TWL4030_PM_MASTER_PB_CFG);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		if (!(val & PB_I2C_BUSY))
>> +			return 0;
>> +
>> +		mdelay(1);
>> +		timeout--;
>> +	} while (timeout);
>> +
>> +	return -ETIMEDOUT;
>> +}
>