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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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.
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 <[email protected]>
---
.../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
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.
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 <[email protected]>
---
.../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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> .../bindings/regulator/twl-regulator.txt | 6 ++++++
Acked-by: Rob Herring <[email protected]>
> drivers/regulator/twl-regulator.c | 22 +++++++++++++++++++---
> 2 files changed, 25 insertions(+), 3 deletions(-)
>
* Rob Herring <[email protected]> [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 <[email protected]>
> > ---
> > .../bindings/regulator/twl-regulator.txt | 6 ++++++
>
> Acked-by: Rob Herring <[email protected]>
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
Hi Tony,
On Fri, Apr 08, 2016 at 08:49:07AM -0700, Tony Lindgren wrote:
> * Rob Herring <[email protected]> [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 <[email protected]>
> > > ---
> > > .../bindings/regulator/twl-regulator.txt | 6 ++++++
> >
> > Acked-by: Rob Herring <[email protected]>
>
> 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
* Sebastian Reichel <[email protected]> [160408 09:09]:
> Hi Tony,
>
> On Fri, Apr 08, 2016 at 08:49:07AM -0700, Tony Lindgren wrote:
> > * Rob Herring <[email protected]> [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 <[email protected]>
> > > > ---
> > > > .../bindings/regulator/twl-regulator.txt | 6 ++++++
> > >
> > > Acked-by: Rob Herring <[email protected]>
> >
> > 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
* Tony Lindgren <[email protected]> [160408 09:21]:
> * Sebastian Reichel <[email protected]> [160408 09:09]:
> > Hi Tony,
> >
> > On Fri, Apr 08, 2016 at 08:49:07AM -0700, Tony Lindgren wrote:
> > > * Rob Herring <[email protected]> [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 <[email protected]>
> > > > > ---
> > > > > .../bindings/regulator/twl-regulator.txt | 6 ++++++
> > > >
> > > > Acked-by: Rob Herring <[email protected]>
> > >
> > > 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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);
>> }
>>
>
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 <[email protected]>
>> ---
>> 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;
>> +}
>