[PATCH v3 0/5] Fixes for tps65090 for Samsung ARM Chromebook

[PATCH v3 0/5] Fixes for tps65090 for Samsung ARM Chromebook

[Doug Anderson]

These five patches bring tps65090 up to speed with what's currently
in the Chromium OS kernel 3.8 tree and running on the Samsung ARM
Chromebook.  Changes were tested atop the current linux tree
(v3.15-rc1).  FET retries were tested on a machine with a known flaky
tps65090.  Since display isn't working on pure upstream, I augmented
the code to turn FET1 (vcd_led) on/off 500 times at bootup.  When
testing I included <https://patchwork.kernel.org/patch/3980731/> to
make sure tps65090 was in exynos5250-snow's device tree.

Dependencies:
- Patch #1 (mfd no irq) has no dependencies, though patch #2 won't
  work without it.
- Patch #2 (charger polling) can be applied without patch #1 but won't
  actually make charger polling work without it.
- Patch #3 (caching) can be applied before retries patch but not
  after.
- Patch #4 (overcurrent wait time) can be applied before retries patch
  but not after (just due to merge conflicts, no other reason).
- Patch #5 (retries) absolutely requires patch #3 (caching).

Changes in v3:
- Fixed kernel-doc notation for return

Changes in v2:
- Split noirq (polling mode) changes into MFD and charger
- Leave cache on for the registers that can be cached.
- Move register offsets to mfd header file.
- Separated the overcurrent and retries changes into two patches.
- Now set overcurrent at probe time since it doesn't change.
- Separated the overcurrent and retries changes into two patches.
- No longer open code fet_is_enabled().
- Fixed tps6090 typo.
- For loop => "while true".
- Removed a set of braces.

Doug Anderson (5):
  mfd: tps65090: Don't tell child devices we have an IRQ if we don't
  charger: tps65090: Allow charger module to be used when no irq
  mfd: tps65090: Stop caching most registers
  regulator: tps65090: Allow setting the overcurrent wait time
  regulator: tps65090: Make FETs more reliable by adding retries

 .../devicetree/bindings/regulator/tps65090.txt     |   4 +
 drivers/mfd/tps65090.c                             |  41 ++--
 drivers/power/tps65090-charger.c                   |  87 ++++++---
 drivers/regulator/tps65090-regulator.c             | 211 +++++++++++++++++++--
 include/linux/mfd/tps65090.h                       |  19 ++
 5 files changed, 303 insertions(+), 59 deletions(-)

-- 
1.9.1.423.g4596e3a

[PATCH v3 1/5] mfd: tps65090: Don't tell child devices we have an IRQ if we don't

[Doug Anderson]

If we weren't given an interrupt we shouldn't tell child devices (like
the tps65090 charger) that they have an interrupt.  This is needed so
that we can support polling mode in the tps65090 charger driver.

See also (charger: tps65090: Allow charger module to be used when no
irq).

Signed-off-by: Doug Anderson <dianders@chromium.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
Changes in v3: None
Changes in v2:
- Split noirq (polling mode) changes into MFD and charger

 drivers/mfd/tps65090.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
index ba1a25d..c3cddb4 100644
--- a/drivers/mfd/tps65090.c
+++ b/drivers/mfd/tps65090.c
@@ -64,11 +64,16 @@ static struct resource charger_resources[] = {
 	}
 };
 
-static const struct mfd_cell tps65090s[] = {
-	{
+enum tps65090_cells {
+	PMIC = 0,
+	CHARGER = 1,
+};
+
+static struct mfd_cell tps65090s[] = {
+	[PMIC] = {
 		.name = "tps65090-pmic",
 	},
-	{
+	[CHARGER] = {
 		.name = "tps65090-charger",
 		.num_resources = ARRAY_SIZE(charger_resources),
 		.resources = &charger_resources[0],
@@ -211,6 +216,9 @@ static int tps65090_i2c_probe(struct i2c_client *client,
 					"IRQ init failed with err: %d\n", ret);
 			return ret;
 		}
+	} else {
+		/* Don't tell children they have an IRQ that'll never fire */
+		tps65090s[CHARGER].num_resources = 0;
 	}
 
 	ret = mfd_add_devices(tps65090->dev, -1, tps65090s,
-- 
1.9.1.423.g4596e3a

Re: [PATCH v3 1/5] mfd: tps65090: Don't tell child devices we have an IRQ if we don't

[Lee Jones]

> If we weren't given an interrupt we shouldn't tell child devices (like
> the tps65090 charger) that they have an interrupt.  This is needed so
> that we can support polling mode in the tps65090 charger driver.
> 
> See also (charger: tps65090: Allow charger module to be used when no
> irq).
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> ---
> Changes in v3: None
> Changes in v2:
> - Split noirq (polling mode) changes into MFD and charger
> 
>  drivers/mfd/tps65090.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

Applied, thanks.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v3 2/5] charger: tps65090: Allow charger module to be used when no irq

[Doug Anderson]

On the ARM Chromebook tps65090 has two masters: the AP (the main
processor running linux) and the EC (the embedded controller).  The AP
is allowed to mess with FETs but the EC is in charge of charge control.

The tps65090 interupt line is routed to both the AP and the EC, which
can cause quite a headache.  Having two people adjusting masks and
acking interrupts is a recipe for disaster.

In the shipping kernel we had a hack to have the AP pay attention to
the IRQ but not to ack it.  It also wasn't supposed to configure the
IRQ in any way.  That hack allowed us to detect when the device was
charging without messing with the EC's state.

The current tps65090 infrastructure makes the above difficult, and it
was a bit of a hack to begin with.  Rather than uglify the driver to
support it, just extend the driver's existing notion of "no irq" to
the charger.  This makes the charger code poll every 2 seconds for AC
detect, which is sufficient.

For proper functioning, requires (mfd: tps65090: Don't tell child
devices we have an IRQ if we don't).  If we don't have that patch
we'll simply fail to probe on devices without an interrupt (just like
we did before this patch).

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v3: None
Changes in v2:
- Split noirq (polling mode) changes into MFD and charger

 drivers/power/tps65090-charger.c | 76 +++++++++++++++++++++++++++++++---------
 1 file changed, 59 insertions(+), 17 deletions(-)

diff --git a/drivers/power/tps65090-charger.c b/drivers/power/tps65090-charger.c
index 8fc9d6d..cc26c12 100644
--- a/drivers/power/tps65090-charger.c
+++ b/drivers/power/tps65090-charger.c
@@ -17,9 +17,11 @@
  */
 #include <linux/delay.h>
 #include <linux/err.h>
+#include <linux/freezer.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
+#include <linux/kthread.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
@@ -43,11 +45,15 @@
 #define TPS65090_VACG		BIT(1)
 #define TPS65090_NOITERM	BIT(5)
 
+#define POLL_INTERVAL		(HZ * 2)	/* Used when no irq */
+
 struct tps65090_charger {
 	struct	device	*dev;
 	int	ac_online;
 	int	prev_ac_online;
 	int	irq;
+	struct task_struct	*poll_task;
+	bool			passive_mode;
 	struct power_supply	ac;
 	struct tps65090_platform_data *pdata;
 };
@@ -60,6 +66,9 @@ static int tps65090_low_chrg_current(struct tps65090_charger *charger)
 {
 	int ret;
 
+	if (charger->passive_mode)
+		return 0;
+
 	ret = tps65090_write(charger->dev->parent, TPS65090_REG_CG_CTRL5,
 			TPS65090_NOITERM);
 	if (ret < 0) {
@@ -75,6 +84,9 @@ static int tps65090_enable_charging(struct tps65090_charger *charger)
 	int ret;
 	uint8_t ctrl0 = 0;
 
+	if (charger->passive_mode)
+		return 0;
+
 	ret = tps65090_read(charger->dev->parent, TPS65090_REG_CG_CTRL0,
 			    &ctrl0);
 	if (ret < 0) {
@@ -98,6 +110,9 @@ static int tps65090_config_charger(struct tps65090_charger *charger)
 	uint8_t intrmask = 0;
 	int ret;
 
+	if (charger->passive_mode)
+		return 0;
+
 	if (charger->pdata->enable_low_current_chrg) {
 		ret = tps65090_low_chrg_current(charger);
 		if (ret < 0) {
@@ -175,10 +190,14 @@ static irqreturn_t tps65090_charger_isr(int irq, void *dev_id)
 	}
 
 	/* Clear interrupts. */
-	ret = tps65090_write(charger->dev->parent, TPS65090_REG_INTR_STS, 0x00);
-	if (ret < 0) {
-		dev_err(charger->dev, "%s(): Error in writing reg 0x%x\n",
+	if (!charger->passive_mode) {
+		ret = tps65090_write(charger->dev->parent,
+				     TPS65090_REG_INTR_STS, 0x00);
+		if (ret < 0) {
+			dev_err(charger->dev,
+				"%s(): Error in writing reg 0x%x\n",
 				__func__, TPS65090_REG_INTR_STS);
+		}
 	}
 
 	if (charger->prev_ac_online != charger->ac_online)
@@ -209,6 +228,18 @@ static struct tps65090_platform_data *
 
 }
 
+static int tps65090_charger_poll_task(void *data)
+{
+	set_freezable();
+
+	while (!kthread_should_stop()) {
+		schedule_timeout_interruptible(POLL_INTERVAL);
+		try_to_freeze();
+		tps65090_charger_isr(-1, data);
+	}
+	return 0;
+}
+
 static int tps65090_charger_probe(struct platform_device *pdev)
 {
 	struct tps65090_charger *cdata;
@@ -255,22 +286,10 @@ static int tps65090_charger_probe(struct platform_device *pdev)
 	}
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq <= 0) {
-		dev_warn(&pdev->dev, "Unable to get charger irq = %d\n", irq);
-		ret = irq;
-		goto fail_unregister_supply;
-	}
-
+	if (irq < 0)
+		irq = NO_IRQ;
 	cdata->irq = irq;
 
-	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
-		tps65090_charger_isr, 0, "tps65090-charger", cdata);
-	if (ret) {
-		dev_err(cdata->dev, "Unable to register irq %d err %d\n", irq,
-			ret);
-		goto fail_unregister_supply;
-	}
-
 	ret = tps65090_config_charger(cdata);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "charger config failed, err %d\n", ret);
@@ -296,6 +315,27 @@ static int tps65090_charger_probe(struct platform_device *pdev)
 		power_supply_changed(&cdata->ac);
 	}
 
+	if (irq != NO_IRQ) {
+		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+			tps65090_charger_isr, 0, "tps65090-charger", cdata);
+		if (ret) {
+			dev_err(cdata->dev,
+				"Unable to register irq %d err %d\n", irq,
+				ret);
+			goto fail_unregister_supply;
+		}
+	} else {
+		cdata->poll_task = kthread_run(tps65090_charger_poll_task,
+					      cdata, "ktps65090charger");
+		cdata->passive_mode = true;
+		if (IS_ERR(cdata->poll_task)) {
+			ret = PTR_ERR(cdata->poll_task);
+			dev_err(cdata->dev,
+				"Unable to run kthread err %d\n", ret);
+			goto fail_unregister_supply;
+		}
+	}
+
 	return 0;
 
 fail_unregister_supply:
@@ -308,6 +348,8 @@ static int tps65090_charger_remove(struct platform_device *pdev)
 {
 	struct tps65090_charger *cdata = platform_get_drvdata(pdev);
 
+	if (cdata->irq == NO_IRQ)
+		kthread_stop(cdata->poll_task);
 	power_supply_unregister(&cdata->ac);
 
 	return 0;
-- 
1.9.1.423.g4596e3a

[PATCH v3 3/5] mfd: tps65090: Stop caching most registers

[Doug Anderson]

Nearly all of the registers in tps65090 combine control bits and
status bits.  Turn off caching of all registers except the select few
that can be cached.

In order to avoid adding more duplicate #defines, we also move some
register offset definitions to the mfd driver (and resolve
inconsistent names).

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v3: None
Changes in v2:
- Leave cache on for the registers that can be cached.
- Move register offsets to mfd header file.

 drivers/mfd/tps65090.c           | 27 ++++++++++++++-------------
 drivers/power/tps65090-charger.c | 11 -----------
 include/linux/mfd/tps65090.h     | 14 ++++++++++++++
 3 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
index c3cddb4..1c3e6e2 100644
--- a/drivers/mfd/tps65090.c
+++ b/drivers/mfd/tps65090.c
@@ -32,14 +32,6 @@
 #define NUM_INT_REG 2
 #define TOTAL_NUM_REG 0x18
 
-/* interrupt status registers */
-#define TPS65090_INT_STS	0x0
-#define TPS65090_INT_STS2	0x1
-
-/* interrupt mask registers */
-#define TPS65090_INT_MSK	0x2
-#define TPS65090_INT_MSK2	0x3
-
 #define TPS65090_INT1_MASK_VAC_STATUS_CHANGE		1
 #define TPS65090_INT1_MASK_VSYS_STATUS_CHANGE		2
 #define TPS65090_INT1_MASK_BAT_STATUS_CHANGE		3
@@ -144,17 +136,26 @@ static struct regmap_irq_chip tps65090_irq_chip = {
 	.irqs = tps65090_irqs,
 	.num_irqs = ARRAY_SIZE(tps65090_irqs),
 	.num_regs = NUM_INT_REG,
-	.status_base = TPS65090_INT_STS,
-	.mask_base = TPS65090_INT_MSK,
+	.status_base = TPS65090_REG_INTR_STS,
+	.mask_base = TPS65090_REG_INTR_MASK,
 	.mask_invert = true,
 };
 
 static bool is_volatile_reg(struct device *dev, unsigned int reg)
 {
-	if ((reg == TPS65090_INT_STS) || (reg == TPS65090_INT_STS2))
-		return true;
-	else
+	/* Nearly all registers have status bits mixed in, except a few */
+	switch (reg) {
+	case TPS65090_REG_INTR_MASK:
+	case TPS65090_REG_INTR_MASK2:
+	case TPS65090_REG_CG_CTRL0:
+	case TPS65090_REG_CG_CTRL1:
+	case TPS65090_REG_CG_CTRL2:
+	case TPS65090_REG_CG_CTRL3:
+	case TPS65090_REG_CG_CTRL4:
+	case TPS65090_REG_CG_CTRL5:
 		return false;
+	}
+	return true;
 }
 
 static const struct regmap_config tps65090_regmap_config = {
diff --git a/drivers/power/tps65090-charger.c b/drivers/power/tps65090-charger.c
index cc26c12..31a3ba4 100644
--- a/drivers/power/tps65090-charger.c
+++ b/drivers/power/tps65090-charger.c
@@ -30,17 +30,6 @@
 
 #include <linux/mfd/tps65090.h>
 
-#define TPS65090_REG_INTR_STS	0x00
-#define TPS65090_REG_INTR_MASK	0x02
-#define TPS65090_REG_CG_CTRL0	0x04
-#define TPS65090_REG_CG_CTRL1	0x05
-#define TPS65090_REG_CG_CTRL2	0x06
-#define TPS65090_REG_CG_CTRL3	0x07
-#define TPS65090_REG_CG_CTRL4	0x08
-#define TPS65090_REG_CG_CTRL5	0x09
-#define TPS65090_REG_CG_STATUS1	0x0a
-#define TPS65090_REG_CG_STATUS2	0x0b
-
 #define TPS65090_CHARGER_ENABLE	BIT(0)
 #define TPS65090_VACG		BIT(1)
 #define TPS65090_NOITERM	BIT(5)
diff --git a/include/linux/mfd/tps65090.h b/include/linux/mfd/tps65090.h
index 3f43069..45f0f9d 100644
--- a/include/linux/mfd/tps65090.h
+++ b/include/linux/mfd/tps65090.h
@@ -64,6 +64,20 @@ enum {
 	TPS65090_REGULATOR_MAX,
 };
 
+/* Register addresses */
+#define TPS65090_REG_INTR_STS	0x00
+#define TPS65090_REG_INTR_STS2	0x01
+#define TPS65090_REG_INTR_MASK	0x02
+#define TPS65090_REG_INTR_MASK2	0x03
+#define TPS65090_REG_CG_CTRL0	0x04
+#define TPS65090_REG_CG_CTRL1	0x05
+#define TPS65090_REG_CG_CTRL2	0x06
+#define TPS65090_REG_CG_CTRL3	0x07
+#define TPS65090_REG_CG_CTRL4	0x08
+#define TPS65090_REG_CG_CTRL5	0x09
+#define TPS65090_REG_CG_STATUS1	0x0a
+#define TPS65090_REG_CG_STATUS2	0x0b
+
 struct tps65090 {
 	struct device		*dev;
 	struct regmap		*rmap;
-- 
1.9.1.423.g4596e3a

Re: [PATCH v3 3/5] mfd: tps65090: Stop caching most registers

[Lee Jones]

> Nearly all of the registers in tps65090 combine control bits and
> status bits.  Turn off caching of all registers except the select few
> that can be cached.
> 
> In order to avoid adding more duplicate #defines, we also move some
> register offset definitions to the mfd driver (and resolve
> inconsistent names).
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v3: None
> Changes in v2:
> - Leave cache on for the registers that can be cached.
> - Move register offsets to mfd header file.
> 
>  drivers/mfd/tps65090.c           | 27 ++++++++++++++-------------
>  drivers/power/tps65090-charger.c | 11 -----------
>  include/linux/mfd/tps65090.h     | 14 ++++++++++++++
>  3 files changed, 28 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
> index c3cddb4..1c3e6e2 100644
> --- a/drivers/mfd/tps65090.c
> +++ b/drivers/mfd/tps65090.c
> @@ -32,14 +32,6 @@
>  #define NUM_INT_REG 2
>  #define TOTAL_NUM_REG 0x18
>  
> -/* interrupt status registers */
> -#define TPS65090_INT_STS	0x0
> -#define TPS65090_INT_STS2	0x1
> -
> -/* interrupt mask registers */
> -#define TPS65090_INT_MSK	0x2
> -#define TPS65090_INT_MSK2	0x3
> -
>  #define TPS65090_INT1_MASK_VAC_STATUS_CHANGE		1
>  #define TPS65090_INT1_MASK_VSYS_STATUS_CHANGE		2
>  #define TPS65090_INT1_MASK_BAT_STATUS_CHANGE		3
> @@ -144,17 +136,26 @@ static struct regmap_irq_chip tps65090_irq_chip = {
>  	.irqs = tps65090_irqs,
>  	.num_irqs = ARRAY_SIZE(tps65090_irqs),
>  	.num_regs = NUM_INT_REG,
> -	.status_base = TPS65090_INT_STS,
> -	.mask_base = TPS65090_INT_MSK,
> +	.status_base = TPS65090_REG_INTR_STS,
> +	.mask_base = TPS65090_REG_INTR_MASK,
>  	.mask_invert = true,
>  };
>  
>  static bool is_volatile_reg(struct device *dev, unsigned int reg)
>  {
> -	if ((reg == TPS65090_INT_STS) || (reg == TPS65090_INT_STS2))
> -		return true;
> -	else
> +	/* Nearly all registers have status bits mixed in, except a few */
> +	switch (reg) {
> +	case TPS65090_REG_INTR_MASK:
> +	case TPS65090_REG_INTR_MASK2:
> +	case TPS65090_REG_CG_CTRL0:
> +	case TPS65090_REG_CG_CTRL1:
> +	case TPS65090_REG_CG_CTRL2:
> +	case TPS65090_REG_CG_CTRL3:
> +	case TPS65090_REG_CG_CTRL4:
> +	case TPS65090_REG_CG_CTRL5:
>  		return false;
> +	}
> +	return true;
>  }

I'll not force the issue, but if you wanted to do this more succinctly
you could also do:

  case TPS65090_REG_INTR_MASK ... TPS65090_REG_INTR_MASK:
  case TPS65090_REG_CG_CTRL0  ... TPS65090_REG_CG_CTRL5:

or

  if (reg >= TPS65090_REG_INTR_MASK && reg <= TPS65090_REG_CG_CTRL5)

Ect.

Otherwise patch looks fine:
  Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 3/5] mfd: tps65090: Stop caching most registers

[Doug Anderson]

Lee,

On Thu, Apr 17, 2014 at 4:00 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> Nearly all of the registers in tps65090 combine control bits and
>> status bits.  Turn off caching of all registers except the select few
>> that can be cached.
>>
>> In order to avoid adding more duplicate #defines, we also move some
>> register offset definitions to the mfd driver (and resolve
>> inconsistent names).
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> ---
>> Changes in v3: None
>> Changes in v2:
>> - Leave cache on for the registers that can be cached.
>> - Move register offsets to mfd header file.
>>
>>  drivers/mfd/tps65090.c           | 27 ++++++++++++++-------------
>>  drivers/power/tps65090-charger.c | 11 -----------
>>  include/linux/mfd/tps65090.h     | 14 ++++++++++++++
>>  3 files changed, 28 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
>> index c3cddb4..1c3e6e2 100644
>> --- a/drivers/mfd/tps65090.c
>> +++ b/drivers/mfd/tps65090.c
>> @@ -32,14 +32,6 @@
>>  #define NUM_INT_REG 2
>>  #define TOTAL_NUM_REG 0x18
>>
>> -/* interrupt status registers */
>> -#define TPS65090_INT_STS     0x0
>> -#define TPS65090_INT_STS2    0x1
>> -
>> -/* interrupt mask registers */
>> -#define TPS65090_INT_MSK     0x2
>> -#define TPS65090_INT_MSK2    0x3
>> -
>>  #define TPS65090_INT1_MASK_VAC_STATUS_CHANGE         1
>>  #define TPS65090_INT1_MASK_VSYS_STATUS_CHANGE                2
>>  #define TPS65090_INT1_MASK_BAT_STATUS_CHANGE         3
>> @@ -144,17 +136,26 @@ static struct regmap_irq_chip tps65090_irq_chip = {
>>       .irqs = tps65090_irqs,
>>       .num_irqs = ARRAY_SIZE(tps65090_irqs),
>>       .num_regs = NUM_INT_REG,
>> -     .status_base = TPS65090_INT_STS,
>> -     .mask_base = TPS65090_INT_MSK,
>> +     .status_base = TPS65090_REG_INTR_STS,
>> +     .mask_base = TPS65090_REG_INTR_MASK,
>>       .mask_invert = true,
>>  };
>>
>>  static bool is_volatile_reg(struct device *dev, unsigned int reg)
>>  {
>> -     if ((reg == TPS65090_INT_STS) || (reg == TPS65090_INT_STS2))
>> -             return true;
>> -     else
>> +     /* Nearly all registers have status bits mixed in, except a few */
>> +     switch (reg) {
>> +     case TPS65090_REG_INTR_MASK:
>> +     case TPS65090_REG_INTR_MASK2:
>> +     case TPS65090_REG_CG_CTRL0:
>> +     case TPS65090_REG_CG_CTRL1:
>> +     case TPS65090_REG_CG_CTRL2:
>> +     case TPS65090_REG_CG_CTRL3:
>> +     case TPS65090_REG_CG_CTRL4:
>> +     case TPS65090_REG_CG_CTRL5:
>>               return false;
>> +     }
>> +     return true;
>>  }
>
> I'll not force the issue, but if you wanted to do this more succinctly
> you could also do:
>
>   case TPS65090_REG_INTR_MASK ... TPS65090_REG_INTR_MASK:
>   case TPS65090_REG_CG_CTRL0  ... TPS65090_REG_CG_CTRL5:
>
> or
>
>   if (reg >= TPS65090_REG_INTR_MASK && reg <= TPS65090_REG_CG_CTRL5)
>
> Ect.
>
> Otherwise patch looks fine:
>   Acked-by: Lee Jones <lee.jones@linaro.org>

If I need to spin the series for another reason I'll make that change.
 Otherwise I won't plan to spin.  Thanks for the review!

-Doug

Re: [PATCH v3 3/5] mfd: tps65090: Stop caching most registers

[Lee Jones]

Anton,

> Nearly all of the registers in tps65090 combine control bits and
> status bits.  Turn off caching of all registers except the select few
> that can be cached.
> 
> In order to avoid adding more duplicate #defines, we also move some
> register offset definitions to the mfd driver (and resolve
> inconsistent names).
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v3: None
> Changes in v2:
> - Leave cache on for the registers that can be cached.
> - Move register offsets to mfd header file.
> 
>  drivers/mfd/tps65090.c           | 27 ++++++++++++++-------------
>  drivers/power/tps65090-charger.c | 11 -----------

This patch still requires your Ack.

>  include/linux/mfd/tps65090.h     | 14 ++++++++++++++
>  3 files changed, 28 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
> index c3cddb4..1c3e6e2 100644
> --- a/drivers/mfd/tps65090.c
> +++ b/drivers/mfd/tps65090.c
> @@ -32,14 +32,6 @@
>  #define NUM_INT_REG 2
>  #define TOTAL_NUM_REG 0x18
>  
> -/* interrupt status registers */
> -#define TPS65090_INT_STS	0x0
> -#define TPS65090_INT_STS2	0x1
> -
> -/* interrupt mask registers */
> -#define TPS65090_INT_MSK	0x2
> -#define TPS65090_INT_MSK2	0x3
> -
>  #define TPS65090_INT1_MASK_VAC_STATUS_CHANGE		1
>  #define TPS65090_INT1_MASK_VSYS_STATUS_CHANGE		2
>  #define TPS65090_INT1_MASK_BAT_STATUS_CHANGE		3
> @@ -144,17 +136,26 @@ static struct regmap_irq_chip tps65090_irq_chip = {
>  	.irqs = tps65090_irqs,
>  	.num_irqs = ARRAY_SIZE(tps65090_irqs),
>  	.num_regs = NUM_INT_REG,
> -	.status_base = TPS65090_INT_STS,
> -	.mask_base = TPS65090_INT_MSK,
> +	.status_base = TPS65090_REG_INTR_STS,
> +	.mask_base = TPS65090_REG_INTR_MASK,
>  	.mask_invert = true,
>  };
>  
>  static bool is_volatile_reg(struct device *dev, unsigned int reg)
>  {
> -	if ((reg == TPS65090_INT_STS) || (reg == TPS65090_INT_STS2))
> -		return true;
> -	else
> +	/* Nearly all registers have status bits mixed in, except a few */
> +	switch (reg) {
> +	case TPS65090_REG_INTR_MASK:
> +	case TPS65090_REG_INTR_MASK2:
> +	case TPS65090_REG_CG_CTRL0:
> +	case TPS65090_REG_CG_CTRL1:
> +	case TPS65090_REG_CG_CTRL2:
> +	case TPS65090_REG_CG_CTRL3:
> +	case TPS65090_REG_CG_CTRL4:
> +	case TPS65090_REG_CG_CTRL5:
>  		return false;
> +	}
> +	return true;
>  }
>  
>  static const struct regmap_config tps65090_regmap_config = {
> diff --git a/drivers/power/tps65090-charger.c b/drivers/power/tps65090-charger.c
> index cc26c12..31a3ba4 100644
> --- a/drivers/power/tps65090-charger.c
> +++ b/drivers/power/tps65090-charger.c
> @@ -30,17 +30,6 @@
>  
>  #include <linux/mfd/tps65090.h>
>  
> -#define TPS65090_REG_INTR_STS	0x00
> -#define TPS65090_REG_INTR_MASK	0x02
> -#define TPS65090_REG_CG_CTRL0	0x04
> -#define TPS65090_REG_CG_CTRL1	0x05
> -#define TPS65090_REG_CG_CTRL2	0x06
> -#define TPS65090_REG_CG_CTRL3	0x07
> -#define TPS65090_REG_CG_CTRL4	0x08
> -#define TPS65090_REG_CG_CTRL5	0x09
> -#define TPS65090_REG_CG_STATUS1	0x0a
> -#define TPS65090_REG_CG_STATUS2	0x0b
> -
>  #define TPS65090_CHARGER_ENABLE	BIT(0)
>  #define TPS65090_VACG		BIT(1)
>  #define TPS65090_NOITERM	BIT(5)
> diff --git a/include/linux/mfd/tps65090.h b/include/linux/mfd/tps65090.h
> index 3f43069..45f0f9d 100644
> --- a/include/linux/mfd/tps65090.h
> +++ b/include/linux/mfd/tps65090.h
> @@ -64,6 +64,20 @@ enum {
>  	TPS65090_REGULATOR_MAX,
>  };
>  
> +/* Register addresses */
> +#define TPS65090_REG_INTR_STS	0x00
> +#define TPS65090_REG_INTR_STS2	0x01
> +#define TPS65090_REG_INTR_MASK	0x02
> +#define TPS65090_REG_INTR_MASK2	0x03
> +#define TPS65090_REG_CG_CTRL0	0x04
> +#define TPS65090_REG_CG_CTRL1	0x05
> +#define TPS65090_REG_CG_CTRL2	0x06
> +#define TPS65090_REG_CG_CTRL3	0x07
> +#define TPS65090_REG_CG_CTRL4	0x08
> +#define TPS65090_REG_CG_CTRL5	0x09
> +#define TPS65090_REG_CG_STATUS1	0x0a
> +#define TPS65090_REG_CG_STATUS2	0x0b
> +
>  struct tps65090 {
>  	struct device		*dev;
>  	struct regmap		*rmap;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 3/5] mfd: tps65090: Stop caching most registers

[Lee Jones]

> Nearly all of the registers in tps65090 combine control bits and
> status bits.  Turn off caching of all registers except the select few
> that can be cached.
> 
> In order to avoid adding more duplicate #defines, we also move some
> register offset definitions to the mfd driver (and resolve
> inconsistent names).
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v3: None
> Changes in v2:
> - Leave cache on for the registers that can be cached.
> - Move register offsets to mfd header file.
> 
>  drivers/mfd/tps65090.c           | 27 ++++++++++++++-------------
>  drivers/power/tps65090-charger.c | 11 -----------
>  include/linux/mfd/tps65090.h     | 14 ++++++++++++++
>  3 files changed, 28 insertions(+), 24 deletions(-)

Applied now, thanks.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v3 4/5] regulator: tps65090: Allow setting the overcurrent wait time

[Doug Anderson]

The tps65090 regulator allows you to specify how long you want it to
wait before detecting an overcurrent condition.  Allow specifying that
through the device tree (or through platform data).

Signed-off-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Michael Spang <spang@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
Changes in v3:
- Fixed kernel-doc notation for return

Changes in v2:
- Separated the overcurrent and retries changes into two patches.
- Now set overcurrent at probe time since it doesn't change.

 .../devicetree/bindings/regulator/tps65090.txt     |  4 ++
 drivers/regulator/tps65090-regulator.c             | 56 ++++++++++++++++++++++
 include/linux/mfd/tps65090.h                       |  5 ++
 3 files changed, 65 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/tps65090.txt b/Documentation/devicetree/bindings/regulator/tps65090.txt
index 313a60b..34098023 100644
--- a/Documentation/devicetree/bindings/regulator/tps65090.txt
+++ b/Documentation/devicetree/bindings/regulator/tps65090.txt
@@ -21,6 +21,10 @@ Optional properties:
   number should be provided. If it is externally controlled and no GPIO
   entry then driver will just configure this rails as external control
   and will not provide any enable/disable APIs.
+- ti,overcurrent-wait: This is applicable to FET registers, which have a
+  poorly defined "overcurrent wait" field.  If this property is present it
+  should be between 0 - 3.  If this property isn't present we won't touch the
+  "overcurrent wait" field and we'll leave it to the BIOS/EC to deal with.
 
 Each regulator is defined using the standard binding for regulators.
 
diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c
index 2e92ef6..ca04e9f 100644
--- a/drivers/regulator/tps65090-regulator.c
+++ b/drivers/regulator/tps65090-regulator.c
@@ -28,15 +28,58 @@
 #include <linux/regulator/of_regulator.h>
 #include <linux/mfd/tps65090.h>
 
+#define CTRL_WT_BIT		2 /* Regulator wait time 0 bit */
+
+#define MAX_OVERCURRENT_WAIT	3 /* Overcurrent wait must be <= this */
+
+/**
+ * struct tps65090_regulator - Per-regulator data for a tps65090 regulator
+ *
+ * @dev: Pointer to our device.
+ * @desc: The struct regulator_desc for the regulator.
+ * @rdev: The struct regulator_dev for the regulator.
+ * @overcurrent_wait_valid: True if overcurrent_wait is valid.
+ * @overcurrent_wait: For FETs, the value to put in the WTFET bitfield.
+ */
+
 struct tps65090_regulator {
 	struct device		*dev;
 	struct regulator_desc	*desc;
 	struct regulator_dev	*rdev;
+	bool			overcurrent_wait_valid;
+	int			overcurrent_wait;
 };
 
 static struct regulator_ops tps65090_ext_control_ops = {
 };
 
+/**
+ * tps65090_reg_set_overcurrent_wait - Setup overcurrent wait
+ *
+ * This will set the overcurrent wait time based on what's in the regulator
+ * info.
+ *
+ * @ri:		Overall regulator data
+ * @rdev:	Regulator device
+ *
+ * Return: 0 if no error, non-zero if there was an error writing the register.
+ */
+static int tps65090_reg_set_overcurrent_wait(struct tps65090_regulator *ri,
+					     struct regulator_dev *rdev)
+{
+	int ret;
+
+	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+				 MAX_OVERCURRENT_WAIT << CTRL_WT_BIT,
+				 ri->overcurrent_wait << CTRL_WT_BIT);
+	if (ret) {
+		dev_err(&rdev->dev, "Error updating overcurrent wait %#x\n",
+			rdev->desc->enable_reg);
+	}
+
+	return ret;
+}
+
 static struct regulator_ops tps65090_reg_contol_ops = {
 	.enable		= regulator_enable_regmap,
 	.disable	= regulator_disable_regmap,
@@ -209,6 +252,11 @@ static struct tps65090_platform_data *tps65090_parse_dt_reg_data(
 			rpdata->gpio = of_get_named_gpio(np,
 					"dcdc-ext-control-gpios", 0);
 
+		if (of_property_read_u32(tps65090_matches[idx].of_node,
+					 "ti,overcurrent-wait",
+					 &rpdata->overcurrent_wait) == 0)
+			rpdata->overcurrent_wait_valid = true;
+
 		tps65090_pdata->reg_pdata[idx] = rpdata;
 	}
 	return tps65090_pdata;
@@ -258,6 +306,8 @@ static int tps65090_regulator_probe(struct platform_device *pdev)
 		ri = &pmic[num];
 		ri->dev = &pdev->dev;
 		ri->desc = &tps65090_regulator_desc[num];
+		ri->overcurrent_wait_valid = tps_pdata->overcurrent_wait_valid;
+		ri->overcurrent_wait = tps_pdata->overcurrent_wait;
 
 		/*
 		 * TPS5090 DCDC support the control from external digital input.
@@ -299,6 +349,12 @@ static int tps65090_regulator_probe(struct platform_device *pdev)
 		}
 		ri->rdev = rdev;
 
+		if (ri->overcurrent_wait_valid) {
+			ret = tps65090_reg_set_overcurrent_wait(ri, rdev);
+			if (ret < 0)
+				return ret;
+		}
+
 		/* Enable external control if it is require */
 		if (tps_pdata && is_dcdc(num) && tps_pdata->reg_init_data &&
 				tps_pdata->enable_ext_control) {
diff --git a/include/linux/mfd/tps65090.h b/include/linux/mfd/tps65090.h
index 45f0f9d..0bf2708 100644
--- a/include/linux/mfd/tps65090.h
+++ b/include/linux/mfd/tps65090.h
@@ -92,11 +92,16 @@ struct tps65090 {
  *     DCDC1, DCDC2 and DCDC3.
  * @gpio: Gpio number if external control is enabled and controlled through
  *     gpio.
+ * @overcurrent_wait_valid: True if the overcurrent_wait should be applied.
+ * @overcurrent_wait: Value to set as the overcurrent wait time.  This is the
+ *     actual bitfield value, not a time in ms (valid value are 0 - 3).
  */
 struct tps65090_regulator_plat_data {
 	struct regulator_init_data *reg_init_data;
 	bool enable_ext_control;
 	int gpio;
+	bool overcurrent_wait_valid;
+	int overcurrent_wait;
 };
 
 struct tps65090_platform_data {
-- 
1.9.1.423.g4596e3a

Re: [PATCH v3 4/5] regulator: tps65090: Allow setting the overcurrent wait time

[Mark Brown]

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

On Wed, Apr 16, 2014 at 04:12:28PM -0700, Doug Anderson wrote:
> The tps65090 regulator allows you to specify how long you want it to
> wait before detecting an overcurrent condition.  Allow specifying that
> through the device tree (or through platform data).

Applied, thanks.

> +- ti,overcurrent-wait: This is applicable to FET registers, which have a
> +  poorly defined "overcurrent wait" field.  If this property is present it
> +  should be between 0 - 3.  If this property isn't present we won't touch the
> +  "overcurrent wait" field and we'll leave it to the BIOS/EC to deal with.

What I was driving at by asking if this was the raw register value was
that the binding should make this clearer ideally.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 4/5] regulator: tps65090: Allow setting the overcurrent wait time

[Lee Jones]

> The tps65090 regulator allows you to specify how long you want it to
> wait before detecting an overcurrent condition.  Allow specifying that
> through the device tree (or through platform data).
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Michael Spang <spang@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> Changes in v3:
> - Fixed kernel-doc notation for return
> 
> Changes in v2:
> - Separated the overcurrent and retries changes into two patches.
> - Now set overcurrent at probe time since it doesn't change.
> 
>  .../devicetree/bindings/regulator/tps65090.txt     |  4 ++
>  drivers/regulator/tps65090-regulator.c             | 56 ++++++++++++++++++++++
>  include/linux/mfd/tps65090.h                       |  5 ++
>  3 files changed, 65 insertions(+)

Applied, thanks.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 4/5] regulator: tps65090: Allow setting the overcurrent wait time

[Doug Anderson]

Lee,

On Wed, Apr 23, 2014 at 4:51 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> The tps65090 regulator allows you to specify how long you want it to
>> wait before detecting an overcurrent condition.  Allow specifying that
>> through the device tree (or through platform data).
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Michael Spang <spang@chromium.org>
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> ---
>> Changes in v3:
>> - Fixed kernel-doc notation for return
>>
>> Changes in v2:
>> - Separated the overcurrent and retries changes into two patches.
>> - Now set overcurrent at probe time since it doesn't change.
>>
>>  .../devicetree/bindings/regulator/tps65090.txt     |  4 ++
>>  drivers/regulator/tps65090-regulator.c             | 56 ++++++++++++++++++++++
>>  include/linux/mfd/tps65090.h                       |  5 ++
>>  3 files changed, 65 insertions(+)
>
> Applied, thanks.

Ummmm, Mark said that he had already applied this patch to his tree (I
mentioned it in my recent summary and you can see it in this thread
too).  I don't see it on git.kernel.org though
<https://git.kernel.org/cgit/linux/kernel/git/broonie/regulator.git/log/?h=for-next>

I'm worried this will cause a merge conflict if you both apply it.

-Doug

Re: [PATCH v3 4/5] regulator: tps65090: Allow setting the overcurrent wait time

[Olof Johansson]

Lee, Doug,

I've bisected a boot failure on Tegra Dalmore (which has a tps65090)
down to this patch. It started in -next 0501, so I guess Lee might
have pushed some patches out now even though the commit date is a
little while back?

The commit is:

commit 60e91b51b515b20f85697fcd397911fdb97bbdca
Author:     Doug Anderson <dianders@chromium.org>
AuthorDate: Wed Apr 16 16:12:28 2014 -0700
Commit:     Lee Jones <lee.jones@linaro.org>
CommitDate: Wed Apr 23 12:34:01 2014 +0100

    regulator: tps65090: Allow setting the overcurrent wait time

    The tps65090 regulator allows you to specify how long you want it to
    wait before detecting an overcurrent condition.  Allow specifying that
    through the device tree (or through platform data).

    Signed-off-by: Doug Anderson <dianders@chromium.org>
    Acked-by: Simon Glass <sjg@chromium.org>
    Acked-by: Michael Spang <spang@chromium.org>
    Acked-by: Sean Paul <seanpaul@chromium.org>
    Acked-by: Mark Brown <broonie@kernel.org>
    Signed-off-by: Lee Jones <lee.jones@linaro.org>


Panic is line 309:

303         for (num = 0; num < TPS65090_REGULATOR_MAX; num++) {
304                 tps_pdata = tps65090_pdata->reg_pdata[num];
305
306                 ri = &pmic[num];
307                 ri->dev = &pdev->dev;
308                 ri->desc = &tps65090_regulator_desc[num];
309                 ri->overcurrent_wait_valid =
tps_pdata->overcurrent_wait_valid;
310                 ri->overcurrent_wait = tps_pdata->overcurrent_wait;

so it looks like tps_pdata is NULL. Should likely be a check for it?


-Olof



On Wed, Apr 23, 2014 at 8:48 AM, Doug Anderson <dianders@chromium.org> wrote:
> Lee,
>
> On Wed, Apr 23, 2014 at 4:51 AM, Lee Jones <lee.jones@linaro.org> wrote:
>>> The tps65090 regulator allows you to specify how long you want it to
>>> wait before detecting an overcurrent condition.  Allow specifying that
>>> through the device tree (or through platform data).
>>>
>>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> Signed-off-by: Michael Spang <spang@chromium.org>
>>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>>> ---
>>> Changes in v3:
>>> - Fixed kernel-doc notation for return
>>>
>>> Changes in v2:
>>> - Separated the overcurrent and retries changes into two patches.
>>> - Now set overcurrent at probe time since it doesn't change.
>>>
>>>  .../devicetree/bindings/regulator/tps65090.txt     |  4 ++
>>>  drivers/regulator/tps65090-regulator.c             | 56 ++++++++++++++++++++++
>>>  include/linux/mfd/tps65090.h                       |  5 ++
>>>  3 files changed, 65 insertions(+)
>>
>> Applied, thanks.
>
> Ummmm, Mark said that he had already applied this patch to his tree (I
> mentioned it in my recent summary and you can see it in this thread
> too).  I don't see it on git.kernel.org though
> <https://git.kernel.org/cgit/linux/kernel/git/broonie/regulator.git/log/?h=for-next>
>
> I'm worried this will cause a merge conflict if you both apply it.
>
> -Doug

Re: [PATCH v3 4/5] regulator: tps65090: Allow setting the overcurrent wait time

[Mark Brown]

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

On Thu, May 01, 2014 at 11:17:58AM -0700, Olof Johansson wrote:

> so it looks like tps_pdata is NULL. Should likely be a check for it?

Yes, just about to post a fix.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 4/5] regulator: tps65090: Allow setting the overcurrent wait time

[Mark Brown]

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

On Thu, May 01, 2014 at 11:17:58AM -0700, Olof Johansson wrote:

> so it looks like tps_pdata is NULL. Should likely be a check for it?

Yes, just about to post a fix.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 4/5] regulator: tps65090: Allow setting the overcurrent wait time

[Doug Anderson]

Mark,

On Thu, May 1, 2014 at 11:49 AM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, May 01, 2014 at 11:17:58AM -0700, Olof Johansson wrote:
>
>> so it looks like tps_pdata is NULL. Should likely be a check for it?
>
> Yes, just about to post a fix.

Doh, was working on it at the same time.

https://patchwork.kernel.org/patch/4099981/

Sorry for the bug.  :(  Thanks for the report!

-Doug

Re: [PATCH v3 4/5] regulator: tps65090: Allow setting the overcurrent wait time

[Mark Brown]

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

On Thu, May 01, 2014 at 11:52:28AM -0700, Doug Anderson wrote:

> > Yes, just about to post a fix.

> Doh, was working on it at the same time.

No worries, I dropped my patch in favour of yours now (though they look
to be identical apart from the metadata!).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v3 5/5] regulator: tps65090: Make FETs more reliable by adding retries

[Doug Anderson]

An issue was discovered with tps65090 where sometimes the FETs
wouldn't actually turn on when requested (they would report
overcurrent).  The most problematic FET was the one used for the LCD
backlight on the Samsung ARM Chromebook (FET1).  Problems were
especially prevalent when the device was plugged in to AC power (when
the backlight voltage was higher).

Mitigate the problem by adding retries on the enables of the FETs,
which works around the problem fairly effectively.

Signed-off-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Michael Spang <spang@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
Changes in v3:
- Fixed kernel-doc notation for return

Changes in v2:
- Separated the overcurrent and retries changes into two patches.
- No longer open code fet_is_enabled().
- Fixed tps6090 typo.
- For loop => "while true".
- Removed a set of braces.

 drivers/regulator/tps65090-regulator.c | 155 +++++++++++++++++++++++++++++----
 1 file changed, 140 insertions(+), 15 deletions(-)

diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c
index ca04e9f..2057e2e 100644
--- a/drivers/regulator/tps65090-regulator.c
+++ b/drivers/regulator/tps65090-regulator.c
@@ -17,6 +17,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
@@ -28,7 +29,13 @@
 #include <linux/regulator/of_regulator.h>
 #include <linux/mfd/tps65090.h>
 
+#define MAX_CTRL_READ_TRIES	5
+#define MAX_FET_ENABLE_TRIES	1000
+
+#define CTRL_EN_BIT		0 /* Regulator enable bit, active high */
 #define CTRL_WT_BIT		2 /* Regulator wait time 0 bit */
+#define CTRL_PG_BIT		4 /* Regulator power good bit, 1=good */
+#define CTRL_TO_BIT		7 /* Regulator timeout bit, 1=wait */
 
 #define MAX_OVERCURRENT_WAIT	3 /* Overcurrent wait must be <= this */
 
@@ -80,40 +87,158 @@ static int tps65090_reg_set_overcurrent_wait(struct tps65090_regulator *ri,
 	return ret;
 }
 
-static struct regulator_ops tps65090_reg_contol_ops = {
+/**
+ * tps65090_try_enable_fet - Try to enable a FET
+ *
+ * @rdev:	Regulator device
+ *
+ * Return: 0 if ok, -ENOTRECOVERABLE if the FET power good bit did not get
+ * set, or some other -ve value if another error occurred (e.g. i2c error)
+ */
+static int tps65090_try_enable_fet(struct regulator_dev *rdev)
+{
+	unsigned int control;
+	int ret, i;
+
+	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+				 rdev->desc->enable_mask,
+				 rdev->desc->enable_mask);
+	if (ret < 0) {
+		dev_err(&rdev->dev, "Error in updating reg %#x\n",
+			rdev->desc->enable_reg);
+		return ret;
+	}
+
+	for (i = 0; i < MAX_CTRL_READ_TRIES; i++) {
+		ret = regmap_read(rdev->regmap, rdev->desc->enable_reg,
+				  &control);
+		if (ret < 0)
+			return ret;
+
+		if (!(control & BIT(CTRL_TO_BIT)))
+			break;
+
+		usleep_range(1000, 1500);
+	}
+	if (!(control & BIT(CTRL_PG_BIT)))
+		return -ENOTRECOVERABLE;
+
+	return 0;
+}
+
+/**
+ * tps65090_fet_enable - Enable a FET, trying a few times if it fails
+ *
+ * Some versions of the tps65090 have issues when turning on the FETs.
+ * This function goes through several steps to ensure the best chance of the
+ * FET going on.  Specifically:
+ * - We'll make sure that we bump the "overcurrent wait" to the maximum, which
+ *   increases the chances that we'll turn on properly.
+ * - We'll retry turning the FET on multiple times (turning off in between).
+ *
+ * @rdev:	Regulator device
+ *
+ * Return: 0 if ok, non-zero if it fails.
+ */
+static int tps65090_fet_enable(struct regulator_dev *rdev)
+{
+	int ret, tries;
+
+	/*
+	 * Try enabling multiple times until we succeed since sometimes the
+	 * first try times out.
+	 */
+	tries = 0;
+	while (true) {
+		ret = tps65090_try_enable_fet(rdev);
+		if (!ret)
+			break;
+		if (ret != -ENOTRECOVERABLE || tries == MAX_FET_ENABLE_TRIES)
+			goto err;
+
+		/* Try turning the FET off (and then on again) */
+		ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+					 rdev->desc->enable_mask, 0);
+		if (ret)
+			goto err;
+
+		tries++;
+	}
+
+	if (tries)
+		dev_warn(&rdev->dev, "reg %#x enable ok after %d tries\n",
+			 rdev->desc->enable_reg, tries);
+
+	return 0;
+err:
+	dev_warn(&rdev->dev, "reg %#x enable failed\n", rdev->desc->enable_reg);
+	WARN_ON(1);
+
+	return ret;
+}
+
+static struct regulator_ops tps65090_reg_control_ops = {
 	.enable		= regulator_enable_regmap,
 	.disable	= regulator_disable_regmap,
 	.is_enabled	= regulator_is_enabled_regmap,
 };
 
+static struct regulator_ops tps65090_fet_control_ops = {
+	.enable		= tps65090_fet_enable,
+	.disable	= regulator_disable_regmap,
+	.is_enabled	= regulator_is_enabled_regmap,
+};
+
 static struct regulator_ops tps65090_ldo_ops = {
 };
 
-#define tps65090_REG_DESC(_id, _sname, _en_reg, _ops)	\
+#define tps65090_REG_DESC(_id, _sname, _en_reg, _en_bits, _ops)	\
 {							\
 	.name = "TPS65090_RAILS"#_id,			\
 	.supply_name = _sname,				\
 	.id = TPS65090_REGULATOR_##_id,			\
 	.ops = &_ops,					\
 	.enable_reg = _en_reg,				\
-	.enable_mask = BIT(0),				\
+	.enable_val = _en_bits,				\
+	.enable_mask = _en_bits,			\
 	.type = REGULATOR_VOLTAGE,			\
 	.owner = THIS_MODULE,				\
 }
 
 static struct regulator_desc tps65090_regulator_desc[] = {
-	tps65090_REG_DESC(DCDC1, "vsys1",   0x0C, tps65090_reg_contol_ops),
-	tps65090_REG_DESC(DCDC2, "vsys2",   0x0D, tps65090_reg_contol_ops),
-	tps65090_REG_DESC(DCDC3, "vsys3",   0x0E, tps65090_reg_contol_ops),
-	tps65090_REG_DESC(FET1,  "infet1",  0x0F, tps65090_reg_contol_ops),
-	tps65090_REG_DESC(FET2,  "infet2",  0x10, tps65090_reg_contol_ops),
-	tps65090_REG_DESC(FET3,  "infet3",  0x11, tps65090_reg_contol_ops),
-	tps65090_REG_DESC(FET4,  "infet4",  0x12, tps65090_reg_contol_ops),
-	tps65090_REG_DESC(FET5,  "infet5",  0x13, tps65090_reg_contol_ops),
-	tps65090_REG_DESC(FET6,  "infet6",  0x14, tps65090_reg_contol_ops),
-	tps65090_REG_DESC(FET7,  "infet7",  0x15, tps65090_reg_contol_ops),
-	tps65090_REG_DESC(LDO1,  "vsys-l1", 0,    tps65090_ldo_ops),
-	tps65090_REG_DESC(LDO2,  "vsys-l2", 0,    tps65090_ldo_ops),
+	tps65090_REG_DESC(DCDC1, "vsys1",   0x0C, BIT(CTRL_EN_BIT),
+			  tps65090_reg_control_ops),
+	tps65090_REG_DESC(DCDC2, "vsys2",   0x0D, BIT(CTRL_EN_BIT),
+			  tps65090_reg_control_ops),
+	tps65090_REG_DESC(DCDC3, "vsys3",   0x0E, BIT(CTRL_EN_BIT),
+			  tps65090_reg_control_ops),
+
+	tps65090_REG_DESC(FET1,  "infet1",  0x0F,
+			  BIT(CTRL_EN_BIT) | BIT(CTRL_PG_BIT),
+			  tps65090_fet_control_ops),
+	tps65090_REG_DESC(FET2,  "infet2",  0x10,
+			  BIT(CTRL_EN_BIT) | BIT(CTRL_PG_BIT),
+			  tps65090_fet_control_ops),
+	tps65090_REG_DESC(FET3,  "infet3",  0x11,
+			  BIT(CTRL_EN_BIT) | BIT(CTRL_PG_BIT),
+			  tps65090_fet_control_ops),
+	tps65090_REG_DESC(FET4,  "infet4",  0x12,
+			  BIT(CTRL_EN_BIT) | BIT(CTRL_PG_BIT),
+			  tps65090_fet_control_ops),
+	tps65090_REG_DESC(FET5,  "infet5",  0x13,
+			  BIT(CTRL_EN_BIT) | BIT(CTRL_PG_BIT),
+			  tps65090_fet_control_ops),
+	tps65090_REG_DESC(FET6,  "infet6",  0x14,
+			  BIT(CTRL_EN_BIT) | BIT(CTRL_PG_BIT),
+			  tps65090_fet_control_ops),
+	tps65090_REG_DESC(FET7,  "infet7",  0x15,
+			  BIT(CTRL_EN_BIT) | BIT(CTRL_PG_BIT),
+			  tps65090_fet_control_ops),
+
+	tps65090_REG_DESC(LDO1,  "vsys-l1", 0, 0,
+			  tps65090_ldo_ops),
+	tps65090_REG_DESC(LDO2,  "vsys-l2", 0, 0,
+			  tps65090_ldo_ops),
 };
 
 static inline bool is_dcdc(int id)
-- 
1.9.1.423.g4596e3a

Re: [PATCH v3 5/5] regulator: tps65090: Make FETs more reliable by adding retries

[Mark Brown]

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

On Wed, Apr 16, 2014 at 04:12:29PM -0700, Doug Anderson wrote:
> An issue was discovered with tps65090 where sometimes the FETs
> wouldn't actually turn on when requested (they would report
> overcurrent).  The most problematic FET was the one used for the LCD

This is basically fine but you said it depends on one of the previous
patches which you didn't CC me on so I've no idea what's going on
there?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 5/5] regulator: tps65090: Make FETs more reliable by adding retries

[Doug Anderson]

Mark,

On Fri, Apr 18, 2014 at 10:43 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Apr 16, 2014 at 04:12:29PM -0700, Doug Anderson wrote:
>> An issue was discovered with tps65090 where sometimes the FETs
>> wouldn't actually turn on when requested (they would report
>> overcurrent).  The most problematic FET was the one used for the LCD
>
> This is basically fine but you said it depends on one of the previous
> patches which you didn't CC me on so I've no idea what's going on
> there?

Sorry about that.  :(

Lee has Acked the caching patch.  Lee: what's the best way for you and
Mark to coordinate there?  Should he apply the caching patch with your
Ack?

-Doug

Re: [PATCH v3 5/5] regulator: tps65090: Make FETs more reliable by adding retries

[Lee Jones]

> Mark,
> 
> On Fri, Apr 18, 2014 at 10:43 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Apr 16, 2014 at 04:12:29PM -0700, Doug Anderson wrote:
> >> An issue was discovered with tps65090 where sometimes the FETs
> >> wouldn't actually turn on when requested (they would report
> >> overcurrent).  The most problematic FET was the one used for the LCD
> >
> > This is basically fine but you said it depends on one of the previous
> > patches which you didn't CC me on so I've no idea what's going on
> > there?
> 
> Sorry about that.  :(
> 
> Lee has Acked the caching patch.  Lee: what's the best way for you and
> Mark to coordinate there?  Should he apply the caching patch with your
> Ack?

If there are cross-subsystem dependencies I prefer to use immutable
branches to eliminate any change of merge conflicts in -next or the
next merge window. I'm happy to either create on with Mark's Acks, or
receive a pull-request from Mark with mine applied.

Up to Mark.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 5/5] regulator: tps65090: Make FETs more reliable by adding retries

[Mark Brown]

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

On Tue, Apr 22, 2014 at 08:47:09AM +0100, Lee Jones wrote:

> If there are cross-subsystem dependencies I prefer to use immutable
> branches to eliminate any change of merge conflicts in -next or the
> next merge window. I'm happy to either create on with Mark's Acks, or
> receive a pull-request from Mark with mine applied.

> Up to Mark.

Well, Doug didn't send me the MFD patch so I can't do anything with it
anyway.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 5/5] regulator: tps65090: Make FETs more reliable by adding retries

[Lee Jones]

> > If there are cross-subsystem dependencies I prefer to use immutable
> > branches to eliminate any change of merge conflicts in -next or the
> > next merge window. I'm happy to either create on with Mark's Acks, or
> > receive a pull-request from Mark with mine applied.
> 
> > Up to Mark.
> 
> Well, Doug didn't send me the MFD patch so I can't do anything with it
> anyway.

Ah!

Doug,
  what exactly are the dependencies within the set?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 5/5] regulator: tps65090: Make FETs more reliable by adding retries

[Doug Anderson]

Hi,

On Tue, Apr 22, 2014 at 8:07 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> > If there are cross-subsystem dependencies I prefer to use immutable
>> > branches to eliminate any change of merge conflicts in -next or the
>> > next merge window. I'm happy to either create on with Mark's Acks, or
>> > receive a pull-request from Mark with mine applied.
>>
>> > Up to Mark.
>>
>> Well, Doug didn't send me the MFD patch so I can't do anything with it
>> anyway.
>
> Ah!
>
> Doug,
>   what exactly are the dependencies within the set?

I will repost the patch shortly with Mark on the CC list.  Status of
the patches in this series:

- Patch #1 (mfd no irq) has no dependencies, though patch #2 won't
  work without it.

https://patchwork.kernel.org/patch/4004441/

Acked by you.  Not landed anywhere that I'm aware of.

--

- Patch #2 (charger polling) can be applied without patch #1 but won't
  actually make charger polling work without it.

https://patchwork.kernel.org/patch/4004461/

Not acked or applied anywhere.

---

- Patch #3 (caching) can be applied before retries patch but not
  after.

https://patchwork.kernel.org/patch/4033361/

This is the patch we need.  I just resent with Mark on the CC list
(including your ack).

---

- Patch #4 (overcurrent wait time) can be applied before retries patch
  but not after (just due to merge conflicts, no other reason).

https://patchwork.kernel.org/patch/4004471/

I believe Mark has already applied.

---

- Patch #5 (retries) absolutely requires patch #3 (caching).

https://patchwork.kernel.org/patch/4004521/