mfd: arizona: Factor out SYSCLK enable from wm5102 hardware patch

PATCH Superseded Charles Keepax 2 participants 9 messages Mar 23, 2015 – Mar 31, 2015

Subsystems (3)

Multifunction devices (MFD) Voltage and current regulator framework Wolfson microelectronics drivers

lore

Share & tools

lore

Charles Keepax

author

2015-03-23 16:23 UTC #0 lore raw

PATCH v3 1/4 mfd: arizona: Factor out SYSCLK enable from wm5102 hardware patch

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 <[email protected]>

---

Changes since v3:
- Split out enable and disable for the freerunning SYSCLK instead of having a single function that takes a function pointer.
- Improve some naming for the sake of clarity
- Update error handling to use if (err) rather than if (err != 0)
- Added comment on register patch

Thanks,
Charles

 drivers/mfd/arizona-core.c |  103 ++++++++++++++++++++++++++++++-------------
 1 files changed, 72 insertions(+), 31 deletions(-)

drivers/mfd/arizona-core.c +72 ~~-31~~

index 6ca6dfa..695c68e 100644
@@ -250,20 +250,26 @@ static int arizona_wait_for_boot(struct arizona *arizona)
 	return ret;
 }
 
-static int arizona_apply_hardware_patch(struct arizona* arizona)
+struct arizona_sysclk_state {
+	unsigned int fll;
+	unsigned int sysclk;
+};
+
+static int arizona_enable_freerun_sysclk(struct arizona *arizona,
+					 struct arizona_sysclk_state *state)
 {
-	unsigned int fll, sysclk;
 	int ret, err;
 
 	/* Cache existing FLL and SYSCLK settings */
-	ret = regmap_read(arizona->regmap, ARIZONA_FLL1_CONTROL_1, &fll);
-	if (ret != 0) {
+	ret = regmap_read(arizona->regmap, ARIZONA_FLL1_CONTROL_1, &state->fll);
+	if (ret) {
 		dev_err(arizona->dev, "Failed to cache FLL settings: %d\n",
 			ret);
 		return ret;
 	}
-	ret = regmap_read(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1, &sysclk);
-	if (ret != 0) {
+	ret = regmap_read(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1,
+			  &state->sysclk);
+	if (ret) {
 		dev_err(arizona->dev, "Failed to cache SYSCLK settings: %d\n",
 			ret);
 		return ret;

@@ -272,7 +278,7 @@ static int arizona_apply_hardware_patch(struct arizona* arizona)
 	/* Start up SYSCLK using the FLL in free running mode */
 	ret = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1,
 			ARIZONA_FLL1_ENA | ARIZONA_FLL1_FREERUN);
-	if (ret != 0) {
+	if (ret) {
 		dev_err(arizona->dev,
 			"Failed to start FLL in freerunning mode: %d\n",
 			ret);

@@ -281,50 +287,85 @@ static int arizona_apply_hardware_patch(struct arizona* arizona)
 	ret = arizona_poll_reg(arizona, 25, ARIZONA_INTERRUPT_RAW_STATUS_5,
 			       ARIZONA_FLL1_CLOCK_OK_STS,
 			       ARIZONA_FLL1_CLOCK_OK_STS);
-	if (ret != 0) {
+	if (ret) {
 		ret = -ETIMEDOUT;
 		goto err_fll;
 	}
 
 	ret = regmap_write(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1, 0x0144);
-	if (ret != 0) {
+	if (ret) {
 		dev_err(arizona->dev, "Failed to start SYSCLK: %d\n", ret);
 		goto err_fll;
 	}
 
+	return 0;
+
+err_fll:
+	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, state->fll);
+	if (err)
+		dev_err(arizona->dev,
+			"Failed to re-apply old FLL settings: %d\n",
+			err);
+
+	return ret;
+}
+
+static int arizona_disable_freerun_sysclk(struct arizona *arizona,
+					  struct arizona_sysclk_state *state)
+{
+	int ret = 0;
+	int err;
+
+	err = regmap_write(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1,
+			   state->sysclk);
+	if (err) {
+		dev_err(arizona->dev,
+			"Failed to re-apply old SYSCLK settings: %d\n",
+			err);
+		ret = err;
+	}
+
+	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, state->fll);
+	if (err) {
+		dev_err(arizona->dev,
+			"Failed to re-apply old FLL settings: %d\n",
+			err);
+		ret = err;
+	}
+
+	return ret;
+}
+
+static int wm5102_apply_hardware_patch(struct arizona *arizona)
+{
+	struct arizona_sysclk_state state;
+	int err, ret;
+
+	ret = arizona_enable_freerun_sysclk(arizona, &state);
+	if (ret)
+		return 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) {
+	if (ret) {
 		dev_err(arizona->dev, "Failed to start write sequencer: %d\n",
 			ret);
-		goto err_sysclk;
+		goto err;
 	}
+
 	ret = arizona_poll_reg(arizona, 5, ARIZONA_WRITE_SEQUENCER_CTRL_1,
 			       ARIZONA_WSEQ_BUSY, 0);
-	if (ret != 0) {
+	if (ret) {
 		regmap_write(arizona->regmap, ARIZONA_WRITE_SEQUENCER_CTRL_0,
-				ARIZONA_WSEQ_ABORT);
+			     ARIZONA_WSEQ_ABORT);
 		ret = -ETIMEDOUT;
 	}
 
-err_sysclk:
-	err = regmap_write(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1, sysclk);
-	if (err != 0) {
-		dev_err(arizona->dev,
-			"Failed to re-apply old SYSCLK settings: %d\n",
-			err);
-	}
-
-err_fll:
-	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, fll);
-	if (err != 0) {
-		dev_err(arizona->dev,
-			"Failed to re-apply old FLL settings: %d\n",
-			err);
-	}
+err:
+	err = arizona_disable_freerun_sysclk(arizona, &state);
 
-	if (ret != 0)
+	if (ret)
 		return ret;
 	else
 		return err;

@@ -366,7 +407,7 @@ static int arizona_runtime_resume(struct device *dev)
 			goto err;
 		}
 
-		ret = arizona_apply_hardware_patch(arizona);
+		ret = wm5102_apply_hardware_patch(arizona);
 		if (ret != 0) {
 			dev_err(arizona->dev,
 				"Failed to apply hardware patch: %d\n",

@@ -891,7 +932,7 @@ int arizona_dev_init(struct arizona *arizona)
 
 		switch (arizona->type) {
 		case WM5102:
-			ret = arizona_apply_hardware_patch(arizona);
+			ret = wm5102_apply_hardware_patch(arizona);
 			if (ret != 0) {
 				dev_err(arizona->dev,
 					"Failed to apply hardware patch: %d\n",

Signature · 1 line

1.7.2.5

Charles Keepax

author

2015-03-23 16:23 UTC #1 lore raw

PATCH v3 2/4 mfd: wm5110: Add register patch required for low power sleep

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 <[email protected]>

---
 drivers/mfd/arizona-core.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)

drivers/mfd/arizona-core.c +42 -0

index 695c68e..b82b909 100644
@@ -371,6 +371,38 @@ err:
 		return err;
 }
 
+/*
+ * Register patch to some of the CODECs internal write sequences
+ * to ensure a clean exit from the low power sleep state.
+ */
+static const struct reg_default wm5110_sleep_patch[] = {
+	{ 0x337A, 0xC100 },
+	{ 0x337B, 0x0041 },
+	{ 0x3300, 0xA210 },
+	{ 0x3301, 0x050C },
+};
+
+static inline int wm5110_apply_sleep_patch(struct arizona *arizona)
+{
+	struct arizona_sysclk_state state;
+	int err, ret;
+
+	ret = arizona_enable_freerun_sysclk(arizona, &state);
+	if (ret)
+		return ret;
+
+	ret = regmap_multi_reg_write_bypassed(arizona->regmap,
+					      wm5110_sleep_patch,
+					      ARRAY_SIZE(wm5110_sleep_patch));
+
+	err = arizona_disable_freerun_sysclk(arizona, &state);
+
+	if (ret)
+		return ret;
+	else
+		return err;
+}
+
 #ifdef CONFIG_PM
 static int arizona_runtime_resume(struct device *dev)
 {

@@ -940,6 +972,16 @@ int arizona_dev_init(struct arizona *arizona)
 				goto err_reset;
 			}
 			break;
+		case WM5110:
+		case WM8280:
+			ret = wm5110_apply_sleep_patch(arizona);
+			if (ret) {
+				dev_err(arizona->dev,
+					"Failed to apply sleep patch: %d\n",
+					ret);
+				goto err_reset;
+			}
+			break;
 		default:
 			break;
 		}

Signature · 1 line

1.7.2.5

Lee Jones

Acked-by on v3 2/4

2015-03-30 11:25 UTC #2 lore raw

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

Quoted 10 lines · 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 <[email protected]>
---
 drivers/mfd/arizona-core.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)

No real problems with this:

Acked-by Lee Jones <[email protected]>

Quoted 60 lines

drivers/mfd/arizona-core.c +42 -0

index 695c68e..b82b909 100644
@@ -371,6 +371,38 @@ err:
 		return err;
 }
 
+/*
+ * Register patch to some of the CODECs internal write sequences
+ * to ensure a clean exit from the low power sleep state.
+ */
+static const struct reg_default wm5110_sleep_patch[] = {
+	{ 0x337A, 0xC100 },
+	{ 0x337B, 0x0041 },
+	{ 0x3300, 0xA210 },
+	{ 0x3301, 0x050C },
+};
+
+static inline int wm5110_apply_sleep_patch(struct arizona *arizona)
+{
+	struct arizona_sysclk_state state;
+	int err, ret;
+
+	ret = arizona_enable_freerun_sysclk(arizona, &state);
+	if (ret)
+		return ret;
+
+	ret = regmap_multi_reg_write_bypassed(arizona->regmap,
+					      wm5110_sleep_patch,
+					      ARRAY_SIZE(wm5110_sleep_patch));
+
+	err = arizona_disable_freerun_sysclk(arizona, &state);
+
+	if (ret)
+		return ret;
+	else
+		return err;
+}
+
 #ifdef CONFIG_PM
 static int arizona_runtime_resume(struct device *dev)
 {

@@ -940,6 +972,16 @@ int arizona_dev_init(struct arizona *arizona)
 				goto err_reset;
 			}
 			break;
+		case WM5110:
+		case WM8280:
+			ret = wm5110_apply_sleep_patch(arizona);
+			if (ret) {
+				dev_err(arizona->dev,
+					"Failed to apply sleep patch: %d\n",
+					ret);
+				goto err_reset;
+			}
+			break;
 		default:
 			break;
 		}

Signature · 4 lines

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

Charles Keepax

author

2015-03-23 16:23 UTC #3 lore raw

PATCH v3 3/4 regulator: arizona-ldo1: Add additional supported voltage

In reply to 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 <[email protected]>

Acked-by Mark Brown <[email protected]>

---
 drivers/regulator/arizona-ldo1.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

drivers/regulator/arizona-ldo1.c +15 -0

index 8169165..c9f6302 100644
@@ -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;

Signature · 1 line

1.7.2.5

Charles Keepax

author

2015-03-23 16:24 UTC #4 lore raw

PATCH v3 4/4 mfd: wm5110: Set DCVDD voltage to 1.175V before entering sleep mode

In reply to 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 <[email protected]>

Acked-by Mark Brown <[email protected]>

---
 drivers/mfd/arizona-core.c |   52 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)

drivers/mfd/arizona-core.c +52 -0

index b82b909..01fb9eb 100644
@@ -447,6 +447,37 @@ static int arizona_runtime_resume(struct device *dev)
 			goto err;
 		}
 		break;
+	case WM5110:
+	case WM8280:
+		ret = arizona_wait_for_boot(arizona);
+		if (ret)
+			goto err;
+
+		if (arizona->external_dcvdd) {
+			ret = regmap_update_bits(arizona->regmap,
+						 ARIZONA_ISOLATION_CONTROL,
+						 ARIZONA_ISOLATE_DCVDD1, 0);
+			if (ret) {
+				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) {

@@ -497,6 +528,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);

Signature · 1 line

1.7.2.5

Lee Jones

Acked-by on v3 4/4

2015-03-30 11:26 UTC #5 lore raw

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

Quoted 9 lines · 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 <[email protected]>
Acked-by Mark Brown <[email protected]>
---
 drivers/mfd/arizona-core.c |   52 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)

Acked-by Lee Jones <[email protected]>

Quoted 70 lines

drivers/mfd/arizona-core.c +52 -0

index b82b909..01fb9eb 100644
@@ -447,6 +447,37 @@ static int arizona_runtime_resume(struct device *dev)
 			goto err;
 		}
 		break;
+	case WM5110:
+	case WM8280:
+		ret = arizona_wait_for_boot(arizona);
+		if (ret)
+			goto err;
+
+		if (arizona->external_dcvdd) {
+			ret = regmap_update_bits(arizona->regmap,
+						 ARIZONA_ISOLATION_CONTROL,
+						 ARIZONA_ISOLATE_DCVDD1, 0);
+			if (ret) {
+				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) {

@@ -497,6 +528,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);

Signature · 4 lines

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

Lee Jones

2015-03-30 11:23 UTC #6 lore raw

In reply to Charles Keepax

Quoted 82 lines · Charles Keepax

Signed-off-by Charles Keepax <[email protected]>

---

Thanks,
Charles

 drivers/mfd/arizona-core.c |  103 ++++++++++++++++++++++++++++++-------------
 1 files changed, 72 insertions(+), 31 deletions(-)

drivers/mfd/arizona-core.c +15 -9

index 6ca6dfa..695c68e 100644
@@ -250,20 +250,26 @@ static int arizona_wait_for_boot(struct arizona *arizona)
 	return ret;
 }
 
-static int arizona_apply_hardware_patch(struct arizona* arizona)
+struct arizona_sysclk_state {
+	unsigned int fll;
+	unsigned int sysclk;
+};
+
+static int arizona_enable_freerun_sysclk(struct arizona *arizona,
+					 struct arizona_sysclk_state *state)
 {
-	unsigned int fll, sysclk;
 	int ret, err;
 
 	/* Cache existing FLL and SYSCLK settings */
-	ret = regmap_read(arizona->regmap, ARIZONA_FLL1_CONTROL_1, &fll);
-	if (ret != 0) {
+	ret = regmap_read(arizona->regmap, ARIZONA_FLL1_CONTROL_1, &state->fll);
+	if (ret) {
 		dev_err(arizona->dev, "Failed to cache FLL settings: %d\n",
 			ret);
 		return ret;
 	}
-	ret = regmap_read(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1, &sysclk);
-	if (ret != 0) {
+	ret = regmap_read(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1,
+			  &state->sysclk);
+	if (ret) {
 		dev_err(arizona->dev, "Failed to cache SYSCLK settings: %d\n",
 			ret);
 		return ret;

@@ -272,7 +278,7 @@ static int arizona_apply_hardware_patch(struct arizona* arizona)
 	/* Start up SYSCLK using the FLL in free running mode */
 	ret = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1,
 			ARIZONA_FLL1_ENA | ARIZONA_FLL1_FREERUN);
-	if (ret != 0) {
+	if (ret) {
 		dev_err(arizona->dev,
 			"Failed to start FLL in freerunning mode: %d\n",
 			ret);

@@ -281,50 +287,85 @@ static int arizona_apply_hardware_patch(struct arizona* arizona)
 	ret = arizona_poll_reg(arizona, 25, ARIZONA_INTERRUPT_RAW_STATUS_5,
 			       ARIZONA_FLL1_CLOCK_OK_STS,
 			       ARIZONA_FLL1_CLOCK_OK_STS);
-	if (ret != 0) {
+	if (ret) {
 		ret = -ETIMEDOUT;
 		goto err_fll;
 	}
 
 	ret = regmap_write(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1, 0x0144);
-	if (ret != 0) {
+	if (ret) {
 		dev_err(arizona->dev, "Failed to start SYSCLK: %d\n", ret);
 		goto err_fll;

drivers/mfd/arizona-core.c static int arizona_apply_hardware_patch(struct arizona* arizona) · line 308

Quoted 10 lines

drivers/mfd/arizona-core.c +8 -0

 	}
 
+	return 0;
+
+err_fll:
+	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, state->fll);
+	if (err)
+		dev_err(arizona->dev,
+			"Failed to re-apply old FLL settings: %d\n",
+			err);

Nit: How is it that the regmap_write() line fit on 80 chars and the "err);" bit can't?

Quoted 5 lines

drivers/mfd/arizona-core.c +5 -0

+	return ret;
+}
+
+static int arizona_disable_freerun_sysclk(struct arizona *arizona,
+					  struct arizona_sysclk_state *state)

drivers/mfd/arizona-core.c static int arizona_apply_hardware_patch(struct arizona* arizona) · line 324

Quoted 10 lines

drivers/mfd/arizona-core.c +10 -0

+{
+	int ret = 0;
+	int err;
+
+	err = regmap_write(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1,
+			   state->sysclk);
+	if (err) {
+		dev_err(arizona->dev,
+			"Failed to re-apply old SYSCLK settings: %d\n",
+			err);

Same there, this extra linewrap seems unnecessary.

drivers/mfd/arizona-core.c static int arizona_apply_hardware_patch(struct arizona* arizona) · line 325

Quoted 2 lines

drivers/mfd/arizona-core.c +2 -0
+		ret = err;
+	}

Can you explain the resson for using 'err' and and not 'ret'?

Quoted 54 lines

drivers/mfd/arizona-core.c +25 ~~-17~~

+	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, state->fll);
+	if (err) {
+		dev_err(arizona->dev,
+			"Failed to re-apply old FLL settings: %d\n",
+			err);
+		ret = err;
+	}
+
+	return ret;
+}
+
+static int wm5102_apply_hardware_patch(struct arizona *arizona)
+{
+	struct arizona_sysclk_state state;
+	int err, ret;
+
+	ret = arizona_enable_freerun_sysclk(arizona, &state);
+	if (ret)
+		return 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) {
+	if (ret) {
 		dev_err(arizona->dev, "Failed to start write sequencer: %d\n",
 			ret);
-		goto err_sysclk;
+		goto err;
 	}
+
 	ret = arizona_poll_reg(arizona, 5, ARIZONA_WRITE_SEQUENCER_CTRL_1,
 			       ARIZONA_WSEQ_BUSY, 0);
-	if (ret != 0) {
+	if (ret) {
 		regmap_write(arizona->regmap, ARIZONA_WRITE_SEQUENCER_CTRL_0,
-				ARIZONA_WSEQ_ABORT);
+			     ARIZONA_WSEQ_ABORT);
 		ret = -ETIMEDOUT;
 	}
 
-err_sysclk:
-	err = regmap_write(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1, sysclk);
-	if (err != 0) {
-		dev_err(arizona->dev,
-			"Failed to re-apply old SYSCLK settings: %d\n",
-			err);
-	}
-
-err_fll:
-	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, fll);
-	if (err != 0) {
-		dev_err(arizona->dev,
-			"Failed to re-apply old FLL settings: %d\n",

drivers/mfd/arizona-core.c static int arizona_apply_hardware_patch(struct arizona* arizona) · line 371

Quoted 10 lines

drivers/mfd/arizona-core.c +3 -3

-			err);
-	}
+err:
+	err = arizona_disable_freerun_sysclk(arizona, &state);
 
-	if (ret != 0)
+	if (ret)
 		return ret;
 	else
 		return err;

return ret ?: err;

Quoted 18 lines

drivers/mfd/arizona-core.c +2 -2

@@ -366,7 +407,7 @@ static int arizona_runtime_resume(struct device *dev)
 			goto err;
 		}
 
-		ret = arizona_apply_hardware_patch(arizona);
+		ret = wm5102_apply_hardware_patch(arizona);
 		if (ret != 0) {
 			dev_err(arizona->dev,
 				"Failed to apply hardware patch: %d\n",

@@ -891,7 +932,7 @@ int arizona_dev_init(struct arizona *arizona)
 
 		switch (arizona->type) {
 		case WM5102:
-			ret = arizona_apply_hardware_patch(arizona);
+			ret = wm5102_apply_hardware_patch(arizona);
 			if (ret != 0) {
 				dev_err(arizona->dev,
 					"Failed to apply hardware patch: %d\n",

Signature · 4 lines

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

Charles Keepax

author

2015-03-30 16:39 UTC #7 lore raw

Quoted 38 lines · Lee Jones

Quoted 36 lines · 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 <[email protected]>
---
Changes since v3:
- Split out enable and disable for the freerunning SYSCLK instead of having a single function that takes a function pointer.
- Improve some naming for the sake of clarity
- Update error handling to use if (err) rather than if (err != 0)
- Added comment on register patch
Thanks,
Charles
 drivers/mfd/arizona-core.c |  103 ++++++++++++++++++++++++++++++-------------
 1 files changed, 72 insertions(+), 31 deletions(-)
drivers/mfd/arizona-core.c +6 -1
index 6ca6dfa..695c68e 100644
@@ -250,20 +250,26 @@ static int arizona_wait_for_boot(struct arizona *arizona)
 	return ret;
 }
 
-static int arizona_apply_hardware_patch(struct arizona* arizona)
+struct arizona_sysclk_state {
+	unsigned int fll;
+	unsigned int sysclk;
+};
+
+

<snip>

Quoted 9 lines

drivers/mfd/arizona-core.c static int arizona_apply_hardware_patch(struct arizona* arizona) · line 308
Quoted 6 lines
drivers/mfd/arizona-core.c +6 -0
+err_fll:
+	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, state->fll);
+	if (err)
+		dev_err(arizona->dev,
+			"Failed to re-apply old FLL settings: %d\n",
+			err);
Nit: How is it that the regmap_write() line fit on 80 chars and the "err);" bit can't?

It can, I just thought it looked a bit nicer this way. But happy to change.

Quoted 10 lines

drivers/mfd/arizona-core.c +9 -0

+	return ret;
+}
+
+static int arizona_disable_freerun_sysclk(struct arizona *arizona,
+					  struct arizona_sysclk_state *state)
+{
+	int ret = 0;
+	int err;
+

Quoted 8 lines

drivers/mfd/arizona-core.c static int arizona_apply_hardware_patch(struct arizona* arizona) · line 324
Quoted 6 lines
drivers/mfd/arizona-core.c +6 -0
+	err = regmap_write(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1,
+			   state->sysclk);
+	if (err) {
+		dev_err(arizona->dev,
+			"Failed to re-apply old SYSCLK settings: %d\n",
+			err);
Same there, this extra linewrap seems unnecessary.

Ditto. Can change.

Quoted 5 lines

drivers/mfd/arizona-core.c
Quoted 3 lines
drivers/mfd/arizona-core.c +2 -0
+		ret = err;
+	}
Can you explain the resson for using 'err' and and not 'ret'?

It is to return an error if either write failed but still execute both writes. Although looking at things again with fresh eyes, it is pretty epically catastrophic if either one fails so I am not so sure we care in the case one has failed if we do the other. I will fix this up to.

Quoted 12 lines

drivers/mfd/arizona-core.c +11 -0

+	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, state->fll);
+	if (err) {
+		dev_err(arizona->dev,
+			"Failed to re-apply old FLL settings: %d\n",
+			err);
+		ret = err;
+	}
+
+	return ret;
+}
+

<snip>

Quoted 10 lines

drivers/mfd/arizona-core.c +2 -7

-err_fll:
-	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, fll);
-	if (err != 0) {
-		dev_err(arizona->dev,
-			"Failed to re-apply old FLL settings: %d\n",
-			err);
-	}
+err:
+	err = arizona_disable_freerun_sysclk(arizona, &state);

Quoted 7 lines

drivers/mfd/arizona-core.c static int arizona_apply_hardware_patch(struct arizona* arizona) · line 371
Quoted 5 lines
drivers/mfd/arizona-core.c +1 -1
-	if (ret != 0)
+	if (ret)
 		return ret;
 	else
 		return err;
return ret ?: err;

Hard to keep tabs on people's preferences around these ternary operators. I am happy to update this too.

Will fire out a new spin tomorrow.

Thanks,
Charles

Lee Jones

2015-03-31 08:01 UTC #8 lore raw

Quoted 50 lines · Charles Keepax

Quoted 38 lines · Lee Jones
Quoted 36 lines · 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 <[email protected]>
---
Changes since v3:
- Split out enable and disable for the freerunning SYSCLK instead of having a single function that takes a function pointer.
- Improve some naming for the sake of clarity
- Update error handling to use if (err) rather than if (err != 0)
- Added comment on register patch
Thanks,
Charles
 drivers/mfd/arizona-core.c |  103 ++++++++++++++++++++++++++++++-------------
 1 files changed, 72 insertions(+), 31 deletions(-)
drivers/mfd/arizona-core.c +6 -1
index 6ca6dfa..695c68e 100644
@@ -250,20 +250,26 @@ static int arizona_wait_for_boot(struct arizona *arizona)
 	return ret;
 }
 
-static int arizona_apply_hardware_patch(struct arizona* arizona)
+struct arizona_sysclk_state {
+	unsigned int fll;
+	unsigned int sysclk;
+};
+
+
<snip>
Quoted 9 lines
drivers/mfd/arizona-core.c static int arizona_apply_hardware_patch(struct arizona* arizona) · line 308
Quoted 6 lines
drivers/mfd/arizona-core.c +6 -0
+err_fll:
+	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, state->fll);
+	if (err)
+		dev_err(arizona->dev,
+			"Failed to re-apply old FLL settings: %d\n",
+			err);
Nit: How is it that the regmap_write() line fit on 80 chars and the "err);" bit can't?

Quoted 2 lines

It can, I just thought it looked a bit nicer this way. But happy to change.

I don't usually like unnecessary line wraps, but it's only a nit, so you decide.

Quoted 26 lines

Quoted 9 lines
Quoted 9 lines
drivers/mfd/arizona-core.c +9 -0
+	return ret;
+}
+
+static int arizona_disable_freerun_sysclk(struct arizona *arizona,
+					  struct arizona_sysclk_state *state)
+{
+	int ret = 0;
+	int err;
+
Quoted 8 lines
drivers/mfd/arizona-core.c static int arizona_apply_hardware_patch(struct arizona* arizona) · line 324
Quoted 6 lines
drivers/mfd/arizona-core.c +6 -0
+	err = regmap_write(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1,
+			   state->sysclk);
+	if (err) {
+		dev_err(arizona->dev,
+			"Failed to re-apply old SYSCLK settings: %d\n",
+			err);
Same there, this extra linewrap seems unnecessary.
Ditto. Can change.
Quoted 5 lines
drivers/mfd/arizona-core.c
Quoted 3 lines
drivers/mfd/arizona-core.c +2 -0
+		ret = err;
+	}
Can you explain the resson for using 'err' and and not 'ret'?

Quoted 5 lines

It is to return an error if either write failed but still execute both writes. Although looking at things again with fresh eyes, it is pretty epically catastrophic if either one fails so I am not so sure we care in the case one has failed if we do the other. I will fix this up to.

I can see why you used 'err' below, but this one can be 'ret'.

But if it's a proper failure then this point is moot and you can use conventional error handling instead.

Quoted 37 lines

Quoted 11 lines
Quoted 11 lines
drivers/mfd/arizona-core.c +11 -0
+	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, state->fll);
+	if (err) {
+		dev_err(arizona->dev,
+			"Failed to re-apply old FLL settings: %d\n",
+			err);
+		ret = err;
+	}
+
+	return ret;
+}
+
<snip>
Quoted 10 lines
Quoted 10 lines
drivers/mfd/arizona-core.c +2 -7
-err_fll:
-	err = regmap_write(arizona->regmap, ARIZONA_FLL1_CONTROL_1, fll);
-	if (err != 0) {
-		dev_err(arizona->dev,
-			"Failed to re-apply old FLL settings: %d\n",
-			err);
-	}
+err:
+	err = arizona_disable_freerun_sysclk(arizona, &state);
Quoted 7 lines
drivers/mfd/arizona-core.c static int arizona_apply_hardware_patch(struct arizona* arizona) · line 371
Quoted 5 lines
drivers/mfd/arizona-core.c +1 -1
-	if (ret != 0)
+	if (ret)
 		return ret;
 	else
 		return err;
return ret ?: err;
Hard to keep tabs on people's preferences around these ternary operators. I am happy to update this too.
Will fire out a new spin tomorrow.
Thanks,
Charles

Signature · 4 lines

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