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

[PATCH v2 1/4] 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>
---

Changes since v1:
 - Added some comments in patch 4 to explain the exact regulator voltage
   ranges
 - Removed the reset patch, whilst I work on the suggestions by Mark, the
   rest of the series is useful in its own right and doesn't need to be
   blocked by that

Thanks,
Charles

 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 v2 2/4] 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 v2 3/4] 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>
Acked-by: Mark Brown <broonie@kernel.org>
---
 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

[PATCH v2 4/4] 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 |   52 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 8f61ccf..d208c20 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -407,6 +407,37 @@ 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 {
+			/*
+			 * As this is only called for the internal regulator
+			 * (where we know voltage ranges available) it is ok
+			 * to request an exact range.
+			 */
+			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 +488,27 @@ static int arizona_runtime_suspend(struct device *dev)
 				ret);
 			return ret;
 		}
+	} else {
+		switch (arizona->type) {
+		case WM5110:
+		case WM8280:
+			/*
+			 * As this is only called for the internal regulator
+			 * (where we know voltage ranges available) it is ok
+			 * to request an exact range.
+			 */
+			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 v2 4/4] mfd: wm5110: Set DCVDD voltage to 1.175V before entering sleep mode

[Mark Brown]

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

On Tue, Mar 17, 2015 at 04:11:12PM +0000, Charles Keepax wrote:
> 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.

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

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

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

[Lee Jones]

On Tue, 17 Mar 2015, Charles Keepax wrote:

> 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>
> ---
> 
> Changes since v1:
>  - Added some comments in patch 4 to explain the exact regulator voltage
>    ranges
>  - Removed the reset patch, whilst I work on the suggestions by Mark, the
>    rest of the series is useful in its own right and doesn't need to be
>    blocked by that
> 
> Thanks,
> Charles
> 
>  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);

Not sure I like this much.

What's the reason for over-complicating this?  Can you just:

arizona_exec_with_sysclk(arizona);
arizona_hardware_patch_wseq(arizona); 

... or if you need that call to be in the middle, split the calls up
further.

> +}
> +
>  #ifdef CONFIG_PM
>  static int arizona_runtime_resume(struct device *dev)
>  {

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

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

[Lee Jones]

On Tue, 17 Mar 2015, Charles Keepax wrote:

> 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 },
> +};

Really not keen on all these magic numbers.

Would you be kind enough to define them please?

> +static inline int wm5110_sleep_patch_wseq(struct arizona *arizona)

What is 'wseq'?

I suggest if I'm having to ask that it's not a very good name.

> +{
> +	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) {

if (ret) {

> +				dev_err(arizona->dev,
> +					"Failed to apply sleep patch: %d\n",
> +					ret);
> +				goto err_reset;
> +			}
> +			break;
>  		default:
>  			break;
>  		}

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

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

[Lee Jones]

On Tue, 17 Mar 2015, Charles Keepax wrote:

> 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 |   52 ++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 52 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index 8f61ccf..d208c20 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -407,6 +407,37 @@ static int arizona_runtime_resume(struct device *dev)
>  			goto err;
>  		}
>  		break;
> +	case WM5110:
> +	case WM8280:
> +		ret = arizona_wait_for_boot(arizona);
> +		if (ret != 0)

Nit: if (ret)

> +			goto err;
> +
> +		if (arizona->external_dcvdd) {
> +			ret = regmap_update_bits(arizona->regmap,
> +						 ARIZONA_ISOLATION_CONTROL,
> +						 ARIZONA_ISOLATE_DCVDD1, 0);
> +			if (ret != 0) {

Nit: And so on ...

> +				dev_err(arizona->dev,
> +					"Failed to connect DCVDD: %d\n", ret);
> +				goto err;
> +			}
> +		} else {
> +			/*
> +			 * As this is only called for the internal regulator
> +			 * (where we know voltage ranges available) it is ok
> +			 * to request an exact range.
> +			 */
> +			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 +488,27 @@ static int arizona_runtime_suspend(struct device *dev)
>  				ret);
>  			return ret;
>  		}
> +	} else {
> +		switch (arizona->type) {
> +		case WM5110:
> +		case WM8280:
> +			/*
> +			 * As this is only called for the internal regulator
> +			 * (where we know voltage ranges available) it is ok
> +			 * to request an exact range.
> +			 */
> +			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);

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

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

[Charles Keepax]

On Mon, Mar 23, 2015 at 02:13:07PM +0000, Lee Jones wrote:
> On Tue, 17 Mar 2015, Charles Keepax wrote:
> 
> > 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 },
> > +};
> 
> Really not keen on all these magic numbers.
> 
> Would you be kind enough to define them please?

This is basically just a register patch, its only not included in
the normal patch due to the constraint on SYSCLK being enabled.
Its not very normal to add defines for register patch entries and
often not really practical in terms of IP. I would prefer to
leave this as is if possible, I am not sure the sort of naming I
am going to put on it in terms of defines is likely to add much
more clarity to the situation.

> 
> > +static inline int wm5110_sleep_patch_wseq(struct arizona *arizona)
> 
> What is 'wseq'?
> 
> I suggest if I'm having to ask that it's not a very good name.

Yeah good point I will update the name here.

Thanks,
Charles

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

[Charles Keepax]

On Mon, Mar 23, 2015 at 01:52:13PM +0000, Lee Jones wrote:
> On Tue, 17 Mar 2015, Charles Keepax wrote:
> 
> > 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>
> > ---
<snip>
> > +
> > +static inline int arizona_apply_hardware_patch(struct arizona *arizona)
> > +{
> > +	return arizona_exec_with_sysclk(arizona, arizona_hardware_patch_wseq);
> 
> Not sure I like this much.
> 
> What's the reason for over-complicating this?  Can you just:
> 
> arizona_exec_with_sysclk(arizona);
> arizona_hardware_patch_wseq(arizona); 
> 
> ... or if you need that call to be in the middle, split the calls up
> further.
> 

Yeah, it was just a handy way to store the state and keep the
amount of code down. But I don't really have any problem with
explicitly storing the state if you prefer that.

So would probably look something like:

struct sysclk_state;
int ret;

ret = arizona_force_sysclk(arizona, &sysclk_state);
if (ret) {
	//Handle error
}
ret = arizona_hardware_patch_wseq(arizona);
if (ret) {
	//Handle error
}
ret = arizona_restore_sysclk(arizona, &sysclk_state);
if (ret) {
	//Handle error
}

I will assume you want it updated to look like this so let me
know if not.

Thanks,
Charles

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

[Lee Jones]

On Mon, 23 Mar 2015, Charles Keepax wrote:

> On Mon, Mar 23, 2015 at 01:52:13PM +0000, Lee Jones wrote:
> > On Tue, 17 Mar 2015, Charles Keepax wrote:
> > 
> > > 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>
> > > ---
> <snip>
> > > +
> > > +static inline int arizona_apply_hardware_patch(struct arizona *arizona)
> > > +{
> > > +	return arizona_exec_with_sysclk(arizona, arizona_hardware_patch_wseq);
> > 
> > Not sure I like this much.
> > 
> > What's the reason for over-complicating this?  Can you just:
> > 
> > arizona_exec_with_sysclk(arizona);
> > arizona_hardware_patch_wseq(arizona); 
> > 
> > ... or if you need that call to be in the middle, split the calls up
> > further.
> > 
> 
> Yeah, it was just a handy way to store the state and keep the
> amount of code down. But I don't really have any problem with
> explicitly storing the state if you prefer that.
> 
> So would probably look something like:
> 
> struct sysclk_state;
> int ret;
> 
> ret = arizona_force_sysclk(arizona, &sysclk_state);
> if (ret) {
> 	//Handle error
> }
> ret = arizona_hardware_patch_wseq(arizona);
> if (ret) {
> 	//Handle error
> }
> ret = arizona_restore_sysclk(arizona, &sysclk_state);
> if (ret) {
> 	//Handle error
> }
> 
> I will assume you want it updated to look like this so let me
> know if not.

Looks much cleaner/less hacky.

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

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

[Lee Jones]

On Mon, 23 Mar 2015, Charles Keepax wrote:

> On Mon, Mar 23, 2015 at 02:13:07PM +0000, Lee Jones wrote:
> > On Tue, 17 Mar 2015, Charles Keepax wrote:
> > 
> > > 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 },
> > > +};
> > 
> > Really not keen on all these magic numbers.
> > 
> > Would you be kind enough to define them please?
> 
> This is basically just a register patch, its only not included in
> the normal patch due to the constraint on SYSCLK being enabled.
> Its not very normal to add defines for register patch entries and
> often not really practical in terms of IP. I would prefer to
> leave this as is if possible, I am not sure the sort of naming I
> am going to put on it in terms of defines is likely to add much
> more clarity to the situation.

At least supply a nice descriptive comment then please.

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