[PATCH 1/5] mfd: arizona: Factor out SYSCLK enable from wm5102 hardware patch

[PATCH 1/5] mfd: arizona: Factor out SYSCLK enable from wm5102 hardware patch

[Charles Keepax]

wm5102 applies a custom hardware boot sequence, for this the SYSCLK
needs to be enabled. This patch factors out the code that enables
SYSCLK for this sequence such that it can be used for other boot time
operations that require SYSCLK.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c |   48 ++++++++++++++++++++++++++++---------------
 1 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 6ca6dfa..ef1f8aa 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -250,7 +250,8 @@ static int arizona_wait_for_boot(struct arizona *arizona)
 	return ret;
 }
 
-static int arizona_apply_hardware_patch(struct arizona* arizona)
+static int arizona_exec_with_sysclk(struct arizona *arizona,
+				    int (*exec)(struct arizona *))
 {
 	unsigned int fll, sysclk;
 	int ret, err;
@@ -292,23 +293,8 @@ static int arizona_apply_hardware_patch(struct arizona* arizona)
 		goto err_fll;
 	}
 
-	/* Start the write sequencer and wait for it to finish */
-	ret = regmap_write(arizona->regmap, ARIZONA_WRITE_SEQUENCER_CTRL_0,
-			ARIZONA_WSEQ_ENA | ARIZONA_WSEQ_START | 160);
-	if (ret != 0) {
-		dev_err(arizona->dev, "Failed to start write sequencer: %d\n",
-			ret);
-		goto err_sysclk;
-	}
-	ret = arizona_poll_reg(arizona, 5, ARIZONA_WRITE_SEQUENCER_CTRL_1,
-			       ARIZONA_WSEQ_BUSY, 0);
-	if (ret != 0) {
-		regmap_write(arizona->regmap, ARIZONA_WRITE_SEQUENCER_CTRL_0,
-				ARIZONA_WSEQ_ABORT);
-		ret = -ETIMEDOUT;
-	}
+	ret = exec(arizona);
 
-err_sysclk:
 	err = regmap_write(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1, sysclk);
 	if (err != 0) {
 		dev_err(arizona->dev,
@@ -330,6 +316,34 @@ err_fll:
 		return err;
 }
 
+static int arizona_hardware_patch_wseq(struct arizona *arizona)
+{
+	int ret;
+
+	/* Start the write sequencer and wait for it to finish */
+	ret = regmap_write(arizona->regmap, ARIZONA_WRITE_SEQUENCER_CTRL_0,
+			ARIZONA_WSEQ_ENA | ARIZONA_WSEQ_START | 160);
+	if (ret != 0) {
+		dev_err(arizona->dev, "Failed to start write sequencer: %d\n",
+			ret);
+		return ret;
+	}
+	ret = arizona_poll_reg(arizona, 5, ARIZONA_WRITE_SEQUENCER_CTRL_1,
+			       ARIZONA_WSEQ_BUSY, 0);
+	if (ret != 0) {
+		regmap_write(arizona->regmap, ARIZONA_WRITE_SEQUENCER_CTRL_0,
+				ARIZONA_WSEQ_ABORT);
+		ret = -ETIMEDOUT;
+	}
+
+	return ret;
+}
+
+static inline int arizona_apply_hardware_patch(struct arizona *arizona)
+{
+	return arizona_exec_with_sysclk(arizona, arizona_hardware_patch_wseq);
+}
+
 #ifdef CONFIG_PM
 static int arizona_runtime_resume(struct device *dev)
 {
-- 
1.7.2.5

[PATCH 2/5] mfd: wm5110: Add register patch required for low power sleep

[Charles Keepax]

Some register settings must be applied before the first time low power
sleep mode is entered on the wm5110 to ensure optimium performance.
These settings require SYSCLK to be enabled whilst they are being
applied. This patch applies the settings using the recently factored out
boot time SYSCLK functionality.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index ef1f8aa..8f61ccf 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -344,6 +344,25 @@ static inline int arizona_apply_hardware_patch(struct arizona *arizona)
 	return arizona_exec_with_sysclk(arizona, arizona_hardware_patch_wseq);
 }
 
+static const struct reg_default wm5110_sleep_patch[] = {
+	{ 0x337A, 0xC100 },
+	{ 0x337B, 0x0041 },
+	{ 0x3300, 0xA210 },
+	{ 0x3301, 0x050C },
+};
+
+static inline int wm5110_sleep_patch_wseq(struct arizona *arizona)
+{
+	return regmap_multi_reg_write_bypassed(arizona->regmap,
+					       wm5110_sleep_patch,
+					       ARRAY_SIZE(wm5110_sleep_patch));
+}
+
+static inline int wm5110_apply_sleep_patch(struct arizona *arizona)
+{
+	return arizona_exec_with_sysclk(arizona, wm5110_sleep_patch_wseq);
+}
+
 #ifdef CONFIG_PM
 static int arizona_runtime_resume(struct device *dev)
 {
@@ -913,6 +932,16 @@ int arizona_dev_init(struct arizona *arizona)
 				goto err_reset;
 			}
 			break;
+		case WM5110:
+		case WM8280:
+			ret = wm5110_apply_sleep_patch(arizona);
+			if (ret != 0) {
+				dev_err(arizona->dev,
+					"Failed to apply sleep patch: %d\n",
+					ret);
+				goto err_reset;
+			}
+			break;
 		default:
 			break;
 		}
-- 
1.7.2.5

[PATCH 3/5] mfd: wm5110: Add delay before releasing reset line on cold boot

[Charles Keepax]

On the first boot of the wm5110 it is important the reset line is held
for slightly longer to ensure the device starts up well. This patch adds
a 5mS delay for this.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 8f61ccf..1ecf9f5 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -784,6 +784,15 @@ int arizona_dev_init(struct arizona *arizona)
 		goto err_enable;
 	}
 
+	switch (arizona->type) {
+	case WM5110:
+	case WM8280:
+		msleep(5);
+		break;
+	default:
+		break;
+	}
+
 	if (arizona->pdata.reset) {
 		gpio_set_value_cansleep(arizona->pdata.reset, 1);
 		msleep(1);
-- 
1.7.2.5

Re: [PATCH 3/5] mfd: wm5110: Add delay before releasing reset line on cold boot

[Mark Brown]

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

On Mon, Mar 16, 2015 at 04:58:42PM +0000, Charles Keepax wrote:

> On the first boot of the wm5110 it is important the reset line is held
> for slightly longer to ensure the device starts up well. This patch adds
> a 5mS delay for this.

How can we tell what first boot is - what happens if the device is fully
powered off during system suspend for example?  I'd expect to see this
done for system resume as well if we don't know power was maintained (or
whatever else the distinction is).

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

Re: [PATCH 3/5] mfd: wm5110: Add delay before releasing reset line on cold boot

[Charles Keepax]

On Mon, Mar 16, 2015 at 05:12:32PM +0000, Mark Brown wrote:
> On Mon, Mar 16, 2015 at 04:58:42PM +0000, Charles Keepax wrote:
> 
> > On the first boot of the wm5110 it is important the reset line is held
> > for slightly longer to ensure the device starts up well. This patch adds
> > a 5mS delay for this.
> 
> How can we tell what first boot is - what happens if the device is fully
> powered off during system suspend for example?  I'd expect to see this
> done for system resume as well if we don't know power was maintained (or
> whatever else the distinction is).

Internally the device has some state, so effectively we define
the first boot as the first time DCVDD is applied since either
the last physical reset or time the core_supplies were missing.

I think your suspend example is pretty tricky, we enable the
regulators for the core_supplies at boot, so I guess we have
requested that the system never removes those so if it does so
anyway perhaps that is a system problem? There isn't really
anyway to tell if they were removed, since we can't talk to the
CODEC without DCVDD (even if we could I don't think we can find
out) and we need to know before we power it up. Also presumably
if the system removed the regulator when we told it not to it
won't report anything through the regulator framework.

That would leave the only possible solution being a hard reset
during every runtime resume but that makes me very nervous about
the AoD interrupts as state for those would be lost upon that
reset.

All in all, I really struggle to see what more the driver could
do here.

Thanks,
Charles

Re: [PATCH 3/5] mfd: wm5110: Add delay before releasing reset line on cold boot

[Mark Brown]

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

On Mon, Mar 16, 2015 at 06:45:18PM +0000, Charles Keepax wrote:

> I think your suspend example is pretty tricky, we enable the
> regulators for the core_supplies at boot, so I guess we have
> requested that the system never removes those so if it does so
> anyway perhaps that is a system problem? There isn't really

No, there's no guarantee that the current state is maintained over
system suspend - system suspend can turn anything off (at least from an
API point of view).

> That would leave the only possible solution being a hard reset
> during every runtime resume but that makes me very nervous about
> the AoD interrupts as state for those would be lost upon that
> reset.

No, you're guaranteed that the supply will stay on while the system is
running so runtime PM is not an issue - it's system suspend that's an
issue.

> All in all, I really struggle to see what more the driver could
> do here.

As I suggested in my original reply handle system suspend.

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

Re: [PATCH 3/5] mfd: wm5110: Add delay before releasing reset line on cold boot

[Charles Keepax]

On Mon, Mar 16, 2015 at 08:47:10PM +0000, Mark Brown wrote:
> On Mon, Mar 16, 2015 at 06:45:18PM +0000, Charles Keepax wrote:
> 
> > I think your suspend example is pretty tricky, we enable the
> > regulators for the core_supplies at boot, so I guess we have
> > requested that the system never removes those so if it does so
> > anyway perhaps that is a system problem? There isn't really
> 
> No, there's no guarantee that the current state is maintained over
> system suspend - system suspend can turn anything off (at least from an
> API point of view).
> 
> > That would leave the only possible solution being a hard reset
> > during every runtime resume but that makes me very nervous about
> > the AoD interrupts as state for those would be lost upon that
> > reset.
> 
> No, you're guaranteed that the supply will stay on while the system is
> running so runtime PM is not an issue - it's system suspend that's an
> issue.
> 
> > All in all, I really struggle to see what more the driver could
> > do here.
> 
> As I suggested in my original reply handle system suspend.

Just to confirm when we say system suspend here we are talking
about the suspend and resume callbacks in dev_pm_ops? As I am
slightly concerned I have my wires very crossed here.

Assuming I don't have my wires crossed.

There are use-cases were we expect the CODEC to be powered up
across system suspend, is that something we should not expect to
be guaranteed possible? For example compressed off-load or always
on voice.

If these use-cases are expected to be reasonable then it would be
reasonable to assume the regulators would not be removed if a
runtime reference to the CODEC is held. If we can't assume that
then how do we know if we should reset the CODEC?

So I guess we could reset the chip in system resume if no runtime
references are held, but that still has the same problem as
resetting in runtime resume, the chip may have been active
running jack detection and you have just blatted the
settings/state. I guess you could have the extcon driver restore
the settings at least in its system resume, but it really feels
like we are likely to be introducing issues worse than those this
patch is there to fix.

Apologies if I am missing something very basic here. Does this
really need to be done before this patch could be accepted?

Thanks,
Charles

Re: [PATCH 3/5] mfd: wm5110: Add delay before releasing reset line on cold boot

[Mark Brown]

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

On Tue, Mar 17, 2015 at 11:50:30AM +0000, Charles Keepax wrote:
> On Mon, Mar 16, 2015 at 08:47:10PM +0000, Mark Brown wrote:

> > As I suggested in my original reply handle system suspend.

> Just to confirm when we say system suspend here we are talking
> about the suspend and resume callbacks in dev_pm_ops? As I am
> slightly concerned I have my wires very crossed here.

Yes.

> There are use-cases were we expect the CODEC to be powered up
> across system suspend, is that something we should not expect to
> be guaranteed possible? For example compressed off-load or always
> on voice.

Sure, that's perfectly fine - jack detection is also very common.

> If these use-cases are expected to be reasonable then it would be
> reasonable to assume the regulators would not be removed if a
> runtime reference to the CODEC is held. If we can't assume that
> then how do we know if we should reset the CODEC?

Given jack detection I don't think you can base this purely on the CODEC
part of the device.

> So I guess we could reset the chip in system resume if no runtime
> references are held, but that still has the same problem as
> resetting in runtime resume, the chip may have been active
> running jack detection and you have just blatted the
> settings/state. I guess you could have the extcon driver restore
> the settings at least in its system resume, but it really feels
> like we are likely to be introducing issues worse than those this
> patch is there to fix.

> Apologies if I am missing something very basic here. Does this
> really need to be done before this patch could be accepted?

It seems easy enough to check if the device is active and it's very
common for drivers to do this (wm8994 has an example) - for most uses
you should be able to check pm_runtime_is_enabled() and there should
just be a single bitfield you can look at to figure out if jack
detection was running.  Looking at the extcon driver briefly
ARIZONA_JD1_ENA looks hopeful.

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

Re: [PATCH 3/5] mfd: wm5110: Add delay before releasing reset line on cold boot

[Charles Keepax]

On Tue, Mar 17, 2015 at 12:04:21PM +0000, Mark Brown wrote:
> On Tue, Mar 17, 2015 at 11:50:30AM +0000, Charles Keepax wrote:
> > On Mon, Mar 16, 2015 at 08:47:10PM +0000, Mark Brown wrote:
<snip>
> It seems easy enough to check if the device is active and it's very
> common for drivers to do this (wm8994 has an example) - for most uses
> you should be able to check pm_runtime_is_enabled() and there should
> just be a single bitfield you can look at to figure out if jack
> detection was running.  Looking at the extcon driver briefly
> ARIZONA_JD1_ENA looks hopeful.

Cool ok, sorry about that. All makes more sense now, thanks. I
will respin.

Thanks,
Charles

[PATCH 4/5] regulator: arizona-ldo1: Add additional supported voltage

[Charles Keepax]

This patch adds support for the 1.175V mode on the LDO1 regulator on the
wm5110. This is need as part of the low power sleep mode operation.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/regulator/arizona-ldo1.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/arizona-ldo1.c b/drivers/regulator/arizona-ldo1.c
index 8169165..c9f6302 100644
--- a/drivers/regulator/arizona-ldo1.c
+++ b/drivers/regulator/arizona-ldo1.c
@@ -178,6 +178,16 @@ static const struct regulator_init_data arizona_ldo1_default = {
 	.num_consumer_supplies = 1,
 };
 
+static const struct regulator_init_data arizona_ldo1_wm5110 = {
+	.constraints = {
+		.min_uV = 1175000,
+		.max_uV = 1200000,
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS |
+				  REGULATOR_CHANGE_VOLTAGE,
+	},
+	.num_consumer_supplies = 1,
+};
+
 static int arizona_ldo1_of_get_pdata(struct arizona *arizona,
 				     struct regulator_config *config,
 				     const struct regulator_desc *desc)
@@ -243,6 +253,11 @@ static int arizona_ldo1_probe(struct platform_device *pdev)
 		desc = &arizona_ldo1_hc;
 		ldo1->init_data = arizona_ldo1_dvfs;
 		break;
+	case WM5110:
+	case WM8280:
+		desc = &arizona_ldo1;
+		ldo1->init_data = arizona_ldo1_wm5110;
+		break;
 	default:
 		desc = &arizona_ldo1;
 		ldo1->init_data = arizona_ldo1_default;
-- 
1.7.2.5

Re: [PATCH 4/5] regulator: arizona-ldo1: Add additional supported voltage

[Mark Brown]

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

On Mon, Mar 16, 2015 at 04:58:43PM +0000, Charles Keepax wrote:
> This patch adds support for the 1.175V mode on the LDO1 regulator on the
> wm5110. This is need as part of the low power sleep mode operation.

Acked-by: Mark Brown <broonie@kernel.org>

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

[PATCH 5/5] mfd: wm5110: Set DCVDD voltage to 1.175V before entering sleep mode

[Charles Keepax]

The low power sleep mode on wm5110 requires that the LDO1 regulator be
set to 1.175V prior to entering sleep, then returned to 1.2V after
exiting sleep mode. This patch apply these regulator settings.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 1ecf9f5..aa1bd77 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -407,6 +407,32 @@ static int arizona_runtime_resume(struct device *dev)
 			goto err;
 		}
 		break;
+	case WM5110:
+	case WM8280:
+		ret = arizona_wait_for_boot(arizona);
+		if (ret != 0)
+			goto err;
+
+		if (arizona->external_dcvdd) {
+			ret = regmap_update_bits(arizona->regmap,
+						 ARIZONA_ISOLATION_CONTROL,
+						 ARIZONA_ISOLATE_DCVDD1, 0);
+			if (ret != 0) {
+				dev_err(arizona->dev,
+					"Failed to connect DCVDD: %d\n", ret);
+				goto err;
+			}
+		} else {
+			ret = regulator_set_voltage(arizona->dcvdd,
+						    1200000, 1200000);
+			if (ret < 0) {
+				dev_err(arizona->dev,
+					"Failed to set resume voltage: %d\n",
+					ret);
+				goto err;
+			}
+		}
+		break;
 	default:
 		ret = arizona_wait_for_boot(arizona);
 		if (ret != 0) {
@@ -457,6 +483,22 @@ static int arizona_runtime_suspend(struct device *dev)
 				ret);
 			return ret;
 		}
+	} else {
+		switch (arizona->type) {
+		case WM5110:
+		case WM8280:
+			ret = regulator_set_voltage(arizona->dcvdd,
+						    1175000, 1175000);
+			if (ret < 0) {
+				dev_err(arizona->dev,
+					"Failed to set suspend voltage: %d\n",
+					ret);
+				return ret;
+			}
+			break;
+		default:
+			break;
+		}
 	}
 
 	regcache_cache_only(arizona->regmap, true);
-- 
1.7.2.5

Re: [PATCH 5/5] mfd: wm5110: Set DCVDD voltage to 1.175V before entering sleep mode

[Mark Brown]

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

On Mon, Mar 16, 2015 at 04:58:44PM +0000, Charles Keepax wrote:

> +	} else {
> +		switch (arizona->type) {
> +		case WM5110:
> +		case WM8280:
> +			ret = regulator_set_voltage(arizona->dcvdd,
> +						    1175000, 1175000);
> +			if (ret < 0) {

Can we please have a comment about this being the internal LDO and us
therefore knowing exactly what voltages it supports (or just that it's
the on chip LDO)?  Going for a single voltage with zero tolerance is
usually bad practice since it causes needless interoperability issues
(eg, here the chances that the hardware would mind if an external
regulator were only able to deliver 1.174V rather than 1.175V are
minimal).  This is fine here but people might be looking for examples
and get the wrong idea.

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

Re: [PATCH 5/5] mfd: wm5110: Set DCVDD voltage to 1.175V before entering sleep mode

[Charles Keepax]

On Mon, Mar 16, 2015 at 05:17:55PM +0000, Mark Brown wrote:
> On Mon, Mar 16, 2015 at 04:58:44PM +0000, Charles Keepax wrote:
> 
> > +	} else {
> > +		switch (arizona->type) {
> > +		case WM5110:
> > +		case WM8280:
> > +			ret = regulator_set_voltage(arizona->dcvdd,
> > +						    1175000, 1175000);
> > +			if (ret < 0) {
> 
> Can we please have a comment about this being the internal LDO and us
> therefore knowing exactly what voltages it supports (or just that it's
> the on chip LDO)?  Going for a single voltage with zero tolerance is
> usually bad practice since it causes needless interoperability issues
> (eg, here the chances that the hardware would mind if an external
> regulator were only able to deliver 1.174V rather than 1.175V are
> minimal).  This is fine here but people might be looking for examples
> and get the wrong idea.

Yeah no problem I will respin and add that.

Thanks,
Charles