[PATCH 0/4] Patch series for introduce voltage selecting for mc13783 regulators.

[PATCH 0/4] Patch series for introduce voltage selecting for mc13783 regulators.

[Alberto Panizzo]

This series of patches fix some problems in mfd mc13783 core driver
and extend mc13783-regulator driver with voltage selection capability.


First two patches apply to  git://git.kernel.org/pub/scm/linux/kernel/git/sameo/mfd-2.6.git for-next

Last two patches apply to git://git.kernel.org/pub/scm/linux/kernel/git/lrg/voltage-2.6.git for-next

[PATCH 1/4] mfd: mc13783: Take care of semantic inversion between read and write value of two bits in POWER_MISCELLANEUS register.

[Alberto Panizzo]

MC13783_REGCTRL_PWGTnSPIEN controls the states of the corresponding
PWGTn_DRV output.
Reading 1 on the corresponding bit mean that the output is enabled
Writing 1 on the corresponding bit disable that output!

So, if not asked directly to modify those bits, write the inverted
value.

Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
---
 drivers/mfd/mc13783-core.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/mc13783-core.c b/drivers/mfd/mc13783-core.c
index a1ade23..aa1f79a 100644
--- a/drivers/mfd/mc13783-core.c
+++ b/drivers/mfd/mc13783-core.c
@@ -207,6 +207,9 @@ int mc13783_reg_write(struct mc13783 *mc13783, unsigned int offset, u32 val)
 }
 EXPORT_SYMBOL(mc13783_reg_write);
 
+#define MC13783_REG_POWER_MISCELLANEOUS	34
+#define MC13783_REGCTRL_PWGT1SPIEN	(1 << 15)
+#define MC13783_REGCTRL_PWGT2SPIEN	(1 << 16)
 int mc13783_reg_rmw(struct mc13783 *mc13783, unsigned int offset,
 		u32 mask, u32 val)
 {
@@ -221,6 +225,14 @@ int mc13783_reg_rmw(struct mc13783 *mc13783, unsigned int offset,
 
 	valread = (valread & ~mask) | val;
 
+	if ((offset == MC13783_REG_POWER_MISCELLANEOUS) &&
+	   !(mask & MC13783_REGCTRL_PWGT1SPIEN))
+		valread ^= MC13783_REGCTRL_PWGT1SPIEN;
+
+	if ((offset == MC13783_REG_POWER_MISCELLANEOUS) &&
+	   !(mask & MC13783_REGCTRL_PWGT2SPIEN))
+		valread ^= MC13783_REGCTRL_PWGT2SPIEN;
+
 	return mc13783_reg_write(mc13783, offset, valread);
 }
 EXPORT_SYMBOL(mc13783_reg_rmw);
-- 
1.6.3.3

[PATCH 2/4] mfd: mc13783: When probing, unlock the mc13783 before subsystems initialisation.

[Alberto Panizzo]

With this, mc13783 subsystems drivers can configure the mc13783 chip
reading and writing registers.

Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
---
 drivers/mfd/mc13783-core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/mc13783-core.c b/drivers/mfd/mc13783-core.c
index aa1f79a..35dcc2a 100644
--- a/drivers/mfd/mc13783-core.c
+++ b/drivers/mfd/mc13783-core.c
@@ -631,6 +631,8 @@ err_revision:
 	}
 	/* This should go away (END) */
 
+	mc13783_unlock(mc13783);
+
 	if (pdata->flags & MC13783_USE_ADC)
 		mc13783_add_subdevice(mc13783, "mc13783-adc");
 
@@ -653,8 +655,6 @@ err_revision:
 	if (pdata->flags & MC13783_USE_TOUCHSCREEN)
 		mc13783_add_subdevice(mc13783, "mc13783-ts");
 
-	mc13783_unlock(mc13783);
-
 	return 0;
 }
 
-- 
1.6.3.3

[PATCH 3/4] regulator: add voltage selection capability to mc13783 regulators.

[Alberto Panizzo]

This patch, complete the mc13783 regulator subsystem driver with 
voltage selecting capability.
Main Switches (SW1AB, SW2AB) are not supported yet.

Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
---
 drivers/regulator/mc13783-regulator.c |  375 ++++++++++++++++++++++++++++++---
 1 files changed, 348 insertions(+), 27 deletions(-)

diff --git a/drivers/regulator/mc13783-regulator.c b/drivers/regulator/mc13783-regulator.c
index 9f99862..ed78137 100644
--- a/drivers/regulator/mc13783-regulator.c
+++ b/drivers/regulator/mc13783-regulator.c
@@ -2,6 +2,7 @@
  * Regulator Driver for Freescale MC13783 PMIC
  *
  * Copyright (C) 2008 Sascha Hauer, Pengutronix <s.hauer@pengutronix.de>
+ * Copyright 2009 Alberto Panizzo <maramaopercheseimorto@gmail.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -18,9 +19,47 @@
 
 #define MC13783_REG_SWITCHERS4			28
 #define MC13783_REG_SWITCHERS4_PLLEN			(1 << 18)
+#define MC13783_REG_SWITCHERS4_PLLVSEL			(1 << 19)
+#define MC13783_REG_SWITCHERS4_PLLVSEL_M		(7 << 19)
 
 #define MC13783_REG_SWITCHERS5			29
 #define MC13783_REG_SWITCHERS5_SW3EN			(1 << 20)
+#define MC13783_REG_SWITCHERS5_SW3VSEL			18
+#define MC13783_REG_SWITCHERS5_SW3VSEL_M		(3 << 18)
+
+#define MC13783_REG_REGULATORSETTING0		30
+#define MC13783_REG_REGULATORSETTING0_VIOLOVSEL		2
+#define MC13783_REG_REGULATORSETTING0_VDIGVSEL		4
+#define MC13783_REG_REGULATORSETTING0_VGENVSEL		6
+#define MC13783_REG_REGULATORSETTING0_VRFDIGVSEL	9
+#define MC13783_REG_REGULATORSETTING0_VRFREFVSEL	11
+#define MC13783_REG_REGULATORSETTING0_VRFCPVSEL		13
+#define MC13783_REG_REGULATORSETTING0_VSIMVSEL		14
+#define MC13783_REG_REGULATORSETTING0_VESIMVSEL		15
+#define MC13783_REG_REGULATORSETTING0_VCAMVSEL		16
+
+#define MC13783_REG_REGULATORSETTING0_VIOLOVSEL_M	(3 << 2)
+#define MC13783_REG_REGULATORSETTING0_VDIGVSEL_M	(3 << 4)
+#define MC13783_REG_REGULATORSETTING0_VGENVSEL_M	(7 << 6)
+#define MC13783_REG_REGULATORSETTING0_VRFDIGVSEL_M	(3 << 9)
+#define MC13783_REG_REGULATORSETTING0_VRFREFVSEL_M	(3 << 11)
+#define MC13783_REG_REGULATORSETTING0_VRFCPVSEL_M	(1 << 13)
+#define MC13783_REG_REGULATORSETTING0_VSIMVSEL_M	(1 << 14)
+#define MC13783_REG_REGULATORSETTING0_VESIMVSEL_M	(1 << 15)
+#define MC13783_REG_REGULATORSETTING0_VCAMVSEL_M	(7 << 16)
+
+#define MC13783_REG_REGULATORSETTING1		31
+#define MC13783_REG_REGULATORSETTING1_VVIBVSEL		0
+#define MC13783_REG_REGULATORSETTING1_VRF1VSEL		2
+#define MC13783_REG_REGULATORSETTING1_VRF2VSEL		4
+#define MC13783_REG_REGULATORSETTING1_VMMC1VSEL		6
+#define MC13783_REG_REGULATORSETTING1_VMMC2VSEL		9
+
+#define MC13783_REG_REGULATORSETTING1_VVIBVSEL_M	(3 << 0)
+#define MC13783_REG_REGULATORSETTING1_VRF1VSEL_M	(3 << 2)
+#define MC13783_REG_REGULATORSETTING1_VRF2VSEL_M	(3 << 4)
+#define MC13783_REG_REGULATORSETTING1_VMMC1VSEL_M	(7 << 6)
+#define MC13783_REG_REGULATORSETTING1_VMMC2VSEL_M	(7 << 9)
 
 #define MC13783_REG_REGULATORMODE0		32
 #define MC13783_REG_REGULATORMODE0_VAUDIOEN		(1 << 0)
@@ -53,11 +92,164 @@ struct mc13783_regulator {
 	struct regulator_desc desc;
 	int reg;
 	int enable_bit;
+	int vsel_reg;
+	int vsel_shift;
+	int vsel_mask;
+	int const *voltages;
+};
+
+/* Voltage Values */
+static const int const mc13783_sw3_val[] = {
+	5000000,
+	5000000,
+	5000000,
+	5500000,
+};
+
+static const int const mc13783_pll_val[] = {
+	28,29,30,31,
+	32,33,34,35,
+};
+
+static const int const mc13783_vaudio_val[] = {
+	2775000,
+};
+
+static const int const mc13783_viohi_val[] = {
+	2775000,
+};
+
+static const int const mc13783_violo_val[] = {
+	1200000,
+	1300000,
+	1500000,
+	1800000,
+};
+
+static const int const mc13783_vdig_val[] = {
+	1200000,
+	1300000,
+	1500000,
+	1800000,
+};
+
+static const int const mc13783_vgen_val[] = {
+	1200000,
+	1300000,
+	1500000,
+	1800000,
+	1100000,
+	2000000,
+	2775000,
+	2400000,
+};
+
+static const int const mc13783_vrfdig_val[] = {
+	1200000,
+	1500000,
+	1800000,
+	1875000,
+};
+
+static const int const mc13783_vrfref_val[] = {
+	2475000,
+	2600000,
+	2700000,
+	2775000,
+};
+
+static const int const mc13783_vrfcp_val[] = {
+	2700000,
+	2775000,
+};
+
+static const int const mc13783_vsim_val[] = {
+	1800000,
+	2900000,
+	3000000,
+};
+
+static const int const mc13783_vesim_val[] = {
+	1800000,
+	2900000,
+};
+
+static const int const mc13783_vcam_val[] = {
+	1500000,
+	1800000,
+	2500000,
+	2550000,
+	2600000,
+	2750000,
+	2800000,
+	3000000,
+};
+
+static const int const mc13783_vrfbg_val[] = {
+	1250000,
+};
+
+static const int const mc13783_vvib_val[] = {
+	1300000,
+	1800000,
+	2000000,
+	3000000,
+};
+
+static const int const mc13783_vmmc_val[] = {
+	1600000,
+	1800000,
+	2000000,
+	2600000,
+	2700000,
+	2800000,
+	2900000,
+	3000000,
+};
+
+static const int const mc13783_vrf_val[] = {
+	1500000,
+	1875000,
+	2700000,
+	2775000,
 };
 
 static struct regulator_ops mc13783_regulator_ops;
 
-#define MC13783_DEFINE(prefix, _name, _reg)				\
+#define MC13783_DEFINE(prefix, _name, _reg, _vsel_reg, _voltages)	\
+	[MC13783_ ## prefix ## _ ## _name] = {				\
+		.desc = {						\
+			.name = #prefix "_" #_name,			\
+			.n_voltages = ARRAY_SIZE(_voltages),		\
+			.ops = &mc13783_regulator_ops,			\
+			.type = REGULATOR_VOLTAGE,			\
+			.id = MC13783_ ## prefix ## _ ## _name,		\
+			.owner = THIS_MODULE,				\
+		},							\
+		.reg = MC13783_REG_ ## _reg,				\
+		.enable_bit = MC13783_REG_ ## _reg ## _ ## _name ## EN,	\
+		.vsel_reg = MC13783_REG_ ## _vsel_reg,			\
+		.vsel_shift = MC13783_REG_ ## _vsel_reg ## _ ## _name ## VSEL,\
+		.vsel_mask = MC13783_REG_ ## _vsel_reg ## _ ## _name ## VSEL_M,\
+		.voltages =  _voltages,					\
+	}
+
+#define MC13783_FIXED_DEFINE(prefix, _name, _reg, _voltages)		\
+	[MC13783_ ## prefix ## _ ## _name] = {				\
+		.desc = {						\
+			.name = #prefix "_" #_name,			\
+			.n_voltages = ARRAY_SIZE(_voltages),		\
+			.ops = &mc13783_regulator_ops,			\
+			.type = REGULATOR_VOLTAGE,			\
+			.id = MC13783_ ## prefix ## _ ## _name,		\
+			.owner = THIS_MODULE,				\
+		},							\
+		.reg = MC13783_REG_ ## _reg,				\
+		.enable_bit = MC13783_REG_ ## _reg ## _ ## _name ## EN,	\
+		.voltages =  _voltages,					\
+	}
+
+#define MC13783_GPO_DEFINE(prefix, _name, _reg)				\
 	[MC13783_ ## prefix ## _ ## _name] = {				\
 		.desc = {						\
 			.name = #prefix "_" #_name,			\
@@ -70,34 +262,50 @@ static struct regulator_ops mc13783_regulator_ops;
 		.enable_bit = MC13783_REG_ ## _reg ## _ ## _name ## EN,	\
 	}
 
-#define MC13783_DEFINE_SW(_name, _reg) MC13783_DEFINE(SW, _name, _reg)
-#define MC13783_DEFINE_REGU(_name, _reg) MC13783_DEFINE(REGU, _name, _reg)
+#define MC13783_DEFINE_SW(_name, _reg, _vsel_reg, _voltages)		\
+	MC13783_DEFINE(SW, _name, _reg, _vsel_reg, _voltages)
+#define MC13783_DEFINE_REGU(_name, _reg, _vsel_reg, _voltages)		\
+	MC13783_DEFINE(REGU, _name, _reg, _vsel_reg, _voltages)
 
 static struct mc13783_regulator mc13783_regulators[] = {
-	MC13783_DEFINE_SW(SW3, SWITCHERS5),
-	MC13783_DEFINE_SW(PLL, SWITCHERS4),
-
-	MC13783_DEFINE_REGU(VAUDIO, REGULATORMODE0),
-	MC13783_DEFINE_REGU(VIOHI, REGULATORMODE0),
-	MC13783_DEFINE_REGU(VIOLO, REGULATORMODE0),
-	MC13783_DEFINE_REGU(VDIG, REGULATORMODE0),
-	MC13783_DEFINE_REGU(VGEN, REGULATORMODE0),
-	MC13783_DEFINE_REGU(VRFDIG, REGULATORMODE0),
-	MC13783_DEFINE_REGU(VRFREF, REGULATORMODE0),
-	MC13783_DEFINE_REGU(VRFCP, REGULATORMODE0),
-	MC13783_DEFINE_REGU(VSIM, REGULATORMODE1),
-	MC13783_DEFINE_REGU(VESIM, REGULATORMODE1),
-	MC13783_DEFINE_REGU(VCAM, REGULATORMODE1),
-	MC13783_DEFINE_REGU(VRFBG, REGULATORMODE1),
-	MC13783_DEFINE_REGU(VVIB, REGULATORMODE1),
-	MC13783_DEFINE_REGU(VRF1, REGULATORMODE1),
-	MC13783_DEFINE_REGU(VRF2, REGULATORMODE1),
-	MC13783_DEFINE_REGU(VMMC1, REGULATORMODE1),
-	MC13783_DEFINE_REGU(VMMC2, REGULATORMODE1),
-	MC13783_DEFINE_REGU(GPO1, POWERMISC),
-	MC13783_DEFINE_REGU(GPO2, POWERMISC),
-	MC13783_DEFINE_REGU(GPO3, POWERMISC),
-	MC13783_DEFINE_REGU(GPO4, POWERMISC),
+	MC13783_DEFINE_SW(SW3, SWITCHERS5, SWITCHERS5, mc13783_sw3_val),
+	MC13783_DEFINE_SW(PLL, SWITCHERS4, SWITCHERS4, mc13783_pll_val),
+
+	MC13783_FIXED_DEFINE(REGU, VAUDIO, REGULATORMODE0, mc13783_vaudio_val),
+	MC13783_FIXED_DEFINE(REGU, VIOHI, REGULATORMODE0, mc13783_viohi_val),
+	MC13783_DEFINE_REGU(VIOLO, REGULATORMODE0, REGULATORSETTING0,	\
+			    mc13783_violo_val),
+	MC13783_DEFINE_REGU(VDIG, REGULATORMODE0, REGULATORSETTING0,	\
+			    mc13783_vdig_val),
+	MC13783_DEFINE_REGU(VGEN, REGULATORMODE0, REGULATORSETTING0,	\
+			    mc13783_vgen_val),
+	MC13783_DEFINE_REGU(VRFDIG, REGULATORMODE0, REGULATORSETTING0,	\
+			    mc13783_vrfdig_val),
+	MC13783_DEFINE_REGU(VRFREF, REGULATORMODE0, REGULATORSETTING0,	\
+			    mc13783_vrfref_val),
+	MC13783_DEFINE_REGU(VRFCP, REGULATORMODE0, REGULATORSETTING0,	\
+			    mc13783_vrfcp_val),
+	MC13783_DEFINE_REGU(VSIM, REGULATORMODE1, REGULATORSETTING0,	\
+			    mc13783_vsim_val),
+	MC13783_DEFINE_REGU(VESIM, REGULATORMODE1, REGULATORSETTING0,	\
+			    mc13783_vesim_val),
+	MC13783_DEFINE_REGU(VCAM, REGULATORMODE1, REGULATORSETTING0,	\
+			    mc13783_vcam_val),
+	MC13783_FIXED_DEFINE(REGU,VRFBG, REGULATORMODE1, mc13783_vrfbg_val),
+	MC13783_DEFINE_REGU(VVIB, REGULATORMODE1, REGULATORSETTING1,	\
+			    mc13783_vvib_val),
+	MC13783_DEFINE_REGU(VRF1, REGULATORMODE1, REGULATORSETTING1,	\
+			    mc13783_vrf_val),
+	MC13783_DEFINE_REGU(VRF2, REGULATORMODE1, REGULATORSETTING1,	\
+			    mc13783_vrf_val),
+	MC13783_DEFINE_REGU(VMMC1, REGULATORMODE1, REGULATORSETTING1,	\
+			    mc13783_vmmc_val),
+	MC13783_DEFINE_REGU(VMMC2, REGULATORMODE1, REGULATORSETTING1,	\
+			    mc13783_vmmc_val),
+	MC13783_GPO_DEFINE(REGU, GPO1, POWERMISC),
+	MC13783_GPO_DEFINE(REGU, GPO2, POWERMISC),
+	MC13783_GPO_DEFINE(REGU, GPO3, POWERMISC),
+	MC13783_GPO_DEFINE(REGU, GPO4, POWERMISC),
 };
 
 struct mc13783_regulator_priv {
@@ -154,10 +362,123 @@ static int mc13783_regulator_is_enabled(struct regulator_dev *rdev)
 	return (val & mc13783_regulators[id].enable_bit) != 0;
 }
 
+static int mc13783_regulator_list_voltage (struct regulator_dev *rdev, unsigned selector)
+{
+	int id = rdev_get_id(rdev);
+
+	if (selector >= mc13783_regulators[id].desc.n_voltages)
+		return -EINVAL;
+
+	return mc13783_regulators[id].voltages[selector];
+}
+
+static int mc13783_get_best_voltage_index(struct regulator_dev *rdev,
+						int min_uV, int max_uV)
+{
+	int reg_id = rdev_get_id(rdev);
+	int i;
+	int bestmatch;
+	int bestindex;
+
+	/*
+	 * Locate the minimum voltage fitting the criteria on
+	 * this regulator. The switchable voltages are not
+	 * in strict falling order so we need to check them
+	 * all for the best match.
+	 */
+	bestmatch = INT_MAX;
+	bestindex = -1;
+	for (i = 0; i < mc13783_regulators[reg_id].desc.n_voltages; i++) {
+		if (mc13783_regulators[reg_id].voltages[i] <= max_uV &&
+		    mc13783_regulators[reg_id].voltages[i] >= min_uV &&
+		    mc13783_regulators[reg_id].voltages[i] < bestmatch) {
+			bestmatch = mc13783_regulators[reg_id].voltages[i];
+			bestindex = i;
+		}
+	}
+
+	if (bestindex < 0) {
+		dev_warn(&rdev->dev, "requested %d<=x<=%d uV, out of range!\n",
+				min_uV, max_uV);
+		return -EINVAL;
+	}
+	return bestindex;
+}
+
+static int mc13783_regulator_set_voltage (struct regulator_dev *rdev, int min_uV, int max_uV)
+{
+	struct mc13783_regulator_priv *priv = rdev_get_drvdata(rdev);
+	int value, id = rdev_get_id(rdev);
+	int ret;
+
+	dev_dbg(rdev_get_dev(rdev), "%s id: %d min_uV: %d max_uV: %d\n",
+		__func__, id, min_uV, max_uV);
+
+	dev_dbg(rdev_get_dev(rdev), "%s n_voltages: %d \n",
+		__func__, mc13783_regulators[id].desc.n_voltages);
+
+	/* If it is a fixed regulator*/
+	if (mc13783_regulators[id].desc.n_voltages == 1)
+	{
+		if (min_uV > mc13783_regulators[id].voltages[0] &&
+		    max_uV < mc13783_regulators[id].voltages[0])
+			return 0;
+		else
+			return -EINVAL;
+	}
+
+	/* Find the best index */
+	value = mc13783_get_best_voltage_index(rdev, min_uV, max_uV);
+	dev_dbg(rdev_get_dev(rdev), "%s best value: %d \n", __func__, value);
+	if (value < 0)
+		return value;
+
+	mc13783_lock(priv->mc13783);
+	ret = mc13783_reg_rmw(priv->mc13783, mc13783_regulators[id].vsel_reg,
+			mc13783_regulators[id].vsel_mask,
+			value << mc13783_regulators[id].vsel_shift);
+	mc13783_unlock(priv->mc13783);
+
+	return ret;
+}
+
+static int mc13783_regulator_get_voltage (struct regulator_dev *rdev)
+{
+	struct mc13783_regulator_priv *priv = rdev_get_drvdata(rdev);
+	int ret, id = rdev_get_id(rdev);
+	unsigned int val;
+
+	dev_dbg(rdev_get_dev(rdev), "%s id: %d\n", __func__, id);
+
+	/* If it is a fixed voltage regulator */
+	if (mc13783_regulators[id].desc.n_voltages == 1)
+		return mc13783_regulators[id].voltages[0];
+
+	mc13783_lock(priv->mc13783);
+	ret = mc13783_reg_read( priv->mc13783,
+				mc13783_regulators[id].vsel_reg, &val);
+	mc13783_unlock(priv->mc13783);
+
+	if (ret)
+		return ret;
+
+	val =(val & mc13783_regulators[id].vsel_mask)
+		>> mc13783_regulators[id].vsel_shift;
+	dev_dbg(rdev_get_dev(rdev), "%s id: %d val: %d\n", __func__, id, val);
+
+	BUG_ON(val < 0 || val > mc13783_regulators[id].desc.n_voltages);
+
+	return mc13783_regulators[id].voltages[val];
+}
+
+
 static struct regulator_ops mc13783_regulator_ops = {
 	.enable = mc13783_regulator_enable,
 	.disable = mc13783_regulator_disable,
 	.is_enabled = mc13783_regulator_is_enabled,
+	.list_voltage = mc13783_regulator_list_voltage,
+	.set_voltage = mc13783_regulator_set_voltage,
+	.get_voltage = mc13783_regulator_get_voltage,
 };
 
 static int __devinit mc13783_regulator_probe(struct platform_device *pdev)
-- 
1.6.3.3

[PATCH 4/4] regulator: mc13783 change to platform_driver_register.

[Alberto Panizzo]

Change the instant when regulator driver is probed. 
To have a correct regulators initialisation (enable, disable and voltages 
selection), the driver must have access to mc13783 registers and so 
mc13783-core must be loaded before this.

With this patch mc13783_regulator_probe is called when mc13783-core
register the regulator subsystem.

Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
---
 drivers/regulator/mc13783-regulator.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/mc13783-regulator.c b/drivers/regulator/mc13783-regulator.c
index ed78137..1ae9017 100644
--- a/drivers/regulator/mc13783-regulator.c
+++ b/drivers/regulator/mc13783-regulator.c
@@ -545,12 +545,12 @@ static struct platform_driver mc13783_regulator_driver = {
 		.owner	= THIS_MODULE,
 	},
 	.remove		= __devexit_p(mc13783_regulator_remove),
+	.probe		= mc13783_regulator_probe,
 };
 
 static int __init mc13783_regulator_init(void)
 {
-	return platform_driver_probe(&mc13783_regulator_driver,
-			mc13783_regulator_probe);
+	return platform_driver_register(&mc13783_regulator_driver);
 }
 subsys_initcall(mc13783_regulator_init);
 
-- 
1.6.3.3

[PATCH 4/4] regulator: mc13783 change to platform_driver_register.

[Mark Brown]

On Sat, Dec 12, 2009 at 06:06:17PM +0100, Alberto Panizzo wrote:
> Change the instant when regulator driver is probed. 
> To have a correct regulators initialisation (enable, disable and voltages 
> selection), the driver must have access to mc13783 registers and so 
> mc13783-core must be loaded before this.
> 
> With this patch mc13783_regulator_probe is called when mc13783-core
> register the regulator subsystem.
> 
> Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

I think this one ought to go into 2.6.33 as a fix.

[PATCH 4/4] regulator: mc13783 change to platform_driver_register.

[Uwe Kleine-König]

Hello,

On Sat, Dec 12, 2009 at 06:06:17PM +0100, Alberto Panizzo wrote:
> Change the instant when regulator driver is probed. 
> To have a correct regulators initialisation (enable, disable and voltages 
> selection), the driver must have access to mc13783 registers and so 
> mc13783-core must be loaded before this.
> 
> With this patch mc13783_regulator_probe is called when mc13783-core
> register the regulator subsystem.
> 
> Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
I think the change is OK, the commit log isn't optimal though.

You might want to point out that the problem only occurs if the driver
is built-in and that mc13783_regulator_probe doesn't need to be changed
as it already lives in .devinit.text

As if mc13783-regulator is built-in mc13783-core is built-in, too, the
wording isn't good.  The problem is (I suppose) that regulators are
linked first and so mc13783-core isn't *probed* early enough and so the
mc13783-regulator device isn't available at mc13783-regulator probing
time.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH 4/4] regulator: mc13783 change to platform_driver_register.

[Alberto Panizzo]

Il giorno dom, 13/12/2009 alle 21.05 +0100, Uwe Kleine-K?nig ha scritto:
> Hello,
> 
> On Sat, Dec 12, 2009 at 06:06:17PM +0100, Alberto Panizzo wrote:
> > Change the instant when regulator driver is probed. 
> > To have a correct regulators initialisation (enable, disable and voltages 
> > selection), the driver must have access to mc13783 registers and so 
> > mc13783-core must be loaded before this.
> > 
> > With this patch mc13783_regulator_probe is called when mc13783-core
> > register the regulator subsystem.
> > 
> > Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
> I think the change is OK, the commit log isn't optimal though.
> 
> You might want to point out that the problem only occurs if the driver
> is built-in and that mc13783_regulator_probe doesn't need to be changed
> as it already lives in .devinit.text
> 

My problem is a not great knowledge of different types of initcall, I 
have to improve myself in this! 

> As if mc13783-regulator is built-in mc13783-core is built-in, too, the
> wording isn't good.  The problem is (I suppose) that regulators are
> linked first and so mc13783-core isn't *probed* early enough and so the
> mc13783-regulator device isn't available at mc13783-regulator probing
> time.

This is the problem. As I understand subsystem_initcall it is the function
called by the system when that subsystem is initialised.
Because of mc13783-regulator is in the regulator subsystem it is called very 
early in the boot process (regulators are meant to be initialised in a very
early phase).

The way that mc13783-regulator's subsystem_initcall call mc13783_regulator_probe
is correct, is only if mc13783-regulator is a subsystem of mc13783-core (mfd?).


Thanks all for reviewing!

Best regards.
Alberto!

[PATCH 3/4] regulator: add voltage selection capability to mc13783 regulators.

[Mark Brown]

On Sat, Dec 12, 2009 at 05:56:16PM +0100, Alberto Panizzo wrote:
> This patch, complete the mc13783 regulator subsystem driver with 
> voltage selecting capability.
> Main Switches (SW1AB, SW2AB) are not supported yet.
> 
> Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>

This is broadly OK but there are a number of issues, mostly stylistic
which it would be better to fix.  scripts/checkpatch.pl will catch a lot
of these.

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

on the basis that the code is correct, though.

> +static int mc13783_regulator_list_voltage (struct regulator_dev *rdev, unsigned selector)

These long lines really ought to be wrapped, and you've got an extra
space before the ( which isn't the usual coding style.  There's lots of
other odditities with things like this which checkpatch should catch.

> +static int mc13783_get_best_voltage_index(struct regulator_dev *rdev,
> +						int min_uV, int max_uV)
> +{
> +	int reg_id = rdev_get_id(rdev);
> +	int i;
> +	int bestmatch;
> +	int bestindex;
> +
> +	/*
> +	 * Locate the minimum voltage fitting the criteria on
> +	 * this regulator. The switchable voltages are not
> +	 * in strict falling order so we need to check them
> +	 * all for the best match.
> +	 */
> +	bestmatch = INT_MAX;
> +	bestindex = -1;
> +	for (i = 0; i < mc13783_regulators[reg_id].desc.n_voltages; i++) {
> +		if (mc13783_regulators[reg_id].voltages[i] <= max_uV &&
> +		    mc13783_regulators[reg_id].voltages[i] >= min_uV &&
> +		    mc13783_regulators[reg_id].voltages[i] < bestmatch) {
> +			bestmatch = mc13783_regulators[reg_id].voltages[i];
> +			bestindex = i;
> +		}
> +	}

Not that it makes much difference but you could just ignore max_uV until
you're done then check it once at the end since you're selecting for the
lowest matching voltage anyway.

> +	/* If it is a fixed regulator*/
> +	if (mc13783_regulators[id].desc.n_voltages == 1)
> +	{

if (...)  {

though it might be as well to define the fixed voltage regulators
separately with their own get and list functions rather than special
casing like this.

> +	mc13783_lock(priv->mc13783);
> +	ret = mc13783_reg_read( priv->mc13783,
> +				mc13783_regulators[id].vsel_reg, &val);

Extra space after the (.

[PATCH 3/4] regulator: add voltage selection capability to mc13783 regulators.

[Uwe Kleine-König]

On Sat, Dec 12, 2009 at 05:56:16PM +0100, Alberto Panizzo wrote:
> This patch, complete the mc13783 regulator subsystem driver with 
> voltage selecting capability.
> Main Switches (SW1AB, SW2AB) are not supported yet.
> 
> Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
> ---
>  drivers/regulator/mc13783-regulator.c |  375 ++++++++++++++++++++++++++++++---
>  1 files changed, 348 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/regulator/mc13783-regulator.c b/drivers/regulator/mc13783-regulator.c
> index 9f99862..ed78137 100644
> --- a/drivers/regulator/mc13783-regulator.c
> +++ b/drivers/regulator/mc13783-regulator.c
> @@ -2,6 +2,7 @@
>   * Regulator Driver for Freescale MC13783 PMIC
>   *
>   * Copyright (C) 2008 Sascha Hauer, Pengutronix <s.hauer@pengutronix.de>
> + * Copyright 2009 Alberto Panizzo <maramaopercheseimorto@gmail.com>
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -18,9 +19,47 @@
>  
>  #define MC13783_REG_SWITCHERS4			28
>  #define MC13783_REG_SWITCHERS4_PLLEN			(1 << 18)
> +#define MC13783_REG_SWITCHERS4_PLLVSEL			(1 << 19)
> +#define MC13783_REG_SWITCHERS4_PLLVSEL_M		(7 << 19)
>  
>  #define MC13783_REG_SWITCHERS5			29
>  #define MC13783_REG_SWITCHERS5_SW3EN			(1 << 20)
> +#define MC13783_REG_SWITCHERS5_SW3VSEL			18
This looks inconsitent:
	MC13783_REG_SWITCHERS4_PLLVSEL	(1 << 19)
	MC13783_REG_SWITCHERS5_SW3VSEL	18

I didn't check the rest of the patch though it would be great if you
wouldn't need all those arrays as they occupy much memory.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH 3/4] regulator: add voltage selection capability to mc13783 regulators.

[Alberto Panizzo]

Il giorno dom, 13/12/2009 alle 21.01 +0100, Uwe Kleine-K?nig ha scritto:
> On Sat, Dec 12, 2009 at 05:56:16PM +0100, Alberto Panizzo wrote:
> > This patch, complete the mc13783 regulator subsystem driver with 
> > voltage selecting capability.
> > Main Switches (SW1AB, SW2AB) are not supported yet.
> > 
> > Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
> > ---
> >  drivers/regulator/mc13783-regulator.c |  375 ++++++++++++++++++++++++++++++---
> >  1 files changed, 348 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/regulator/mc13783-regulator.c b/drivers/regulator/mc13783-regulator.c
> > index 9f99862..ed78137 100644
> > --- a/drivers/regulator/mc13783-regulator.c
> > +++ b/drivers/regulator/mc13783-regulator.c
> > @@ -2,6 +2,7 @@
> >   * Regulator Driver for Freescale MC13783 PMIC
> >   *
> >   * Copyright (C) 2008 Sascha Hauer, Pengutronix <s.hauer@pengutronix.de>
> > + * Copyright 2009 Alberto Panizzo <maramaopercheseimorto@gmail.com>
> >   *
> >   * This program is free software; you can redistribute it and/or modify
> >   * it under the terms of the GNU General Public License version 2 as
> > @@ -18,9 +19,47 @@
> >  
> >  #define MC13783_REG_SWITCHERS4			28
> >  #define MC13783_REG_SWITCHERS4_PLLEN			(1 << 18)
> > +#define MC13783_REG_SWITCHERS4_PLLVSEL			(1 << 19)
> > +#define MC13783_REG_SWITCHERS4_PLLVSEL_M		(7 << 19)
> >  
> >  #define MC13783_REG_SWITCHERS5			29
> >  #define MC13783_REG_SWITCHERS5_SW3EN			(1 << 20)
> > +#define MC13783_REG_SWITCHERS5_SW3VSEL			18
> This looks inconsitent:
> 	MC13783_REG_SWITCHERS4_PLLVSEL	(1 << 19)
> 	MC13783_REG_SWITCHERS5_SW3VSEL	18
> 
> I didn't check the rest of the patch though it would be great if you
> wouldn't need all those arrays as they occupy much memory.
> 
> Best regards
> Uwe
> 

Yes this is a mistake and.. I'm not sure to embrace SWITCHERS4_PLL in
the regulators stuff at all.
The code that I propose can enable/disable and set the multiplication
factor for the PLL but this is not a voltage regulator!

Maybe the PLL must be initialised and controlled via another driver, 
in the audio codec?

For the arrays, also for me it is not the better code that I wrote
but voltages values have no regular stepping and this way is a great 
self explain way.
Look at tables 4-18 and 4-19 of the mc13783 information for GPL drivers..

The other ways are:
- Compress arrays in different phases, with complex initialisation.
- Write as many function as different regulators there are, increasing
  the driver complexity and also the text instead of data memory..

Sure, I have to correct all the coding style issues asserted by Mark!

Alberto.

[PATCH 3/4] regulator: add voltage selection capability to mc13783 regulators.

[Mark Brown]

On Mon, Dec 14, 2009 at 11:41:38AM +0100, Alberto Panizzo wrote:

> Maybe the PLL must be initialised and controlled via another driver, 
> in the audio codec?

That would be the normal place to control a PLL used to clock the audio
subsystem, yes.  If it's more generally used then it probably ought to
go in the core driver - something along the lines of the clock API would
be useful for arbitration (the clock API itself can't really be used
since it only really works with on-SoC clocks).

[PATCH 2/4] mfd: mc13783: When probing, unlock the mc13783 before subsystems initialisation.

[Uwe Kleine-König]

Hello Alberto,

On Sat, Dec 12, 2009 at 05:53:39PM +0100, Alberto Panizzo wrote:
> With this, mc13783 subsystems drivers can configure the mc13783 chip
> reading and writing registers.
> 
> Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
> ---
>  drivers/mfd/mc13783-core.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/mc13783-core.c b/drivers/mfd/mc13783-core.c
> index aa1f79a..35dcc2a 100644
> --- a/drivers/mfd/mc13783-core.c
> +++ b/drivers/mfd/mc13783-core.c
> @@ -631,6 +631,8 @@ err_revision:
>  	}
>  	/* This should go away (END) */
>  
> +	mc13783_unlock(mc13783);
> +
>  	if (pdata->flags & MC13783_USE_ADC)
>  		mc13783_add_subdevice(mc13783, "mc13783-adc");
>  
> @@ -653,8 +655,6 @@ err_revision:
>  	if (pdata->flags & MC13783_USE_TOUCHSCREEN)
>  		mc13783_add_subdevice(mc13783, "mc13783-ts");
>  
> -	mc13783_unlock(mc13783);
> -
>  	return 0;
>  }
Looks reasonable.  You can take my Acked-by: for that.

Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH 1/4] mfd: mc13783: Take care of semantic inversion between read and write value of two bits in POWER_MISCELLANEUS register.

[Mark Brown]

On Sat, Dec 12, 2009 at 05:48:43PM +0100, Alberto Panizzo wrote:
> MC13783_REGCTRL_PWGTnSPIEN controls the states of the corresponding
> PWGTn_DRV output.
> Reading 1 on the corresponding bit mean that the output is enabled
> Writing 1 on the corresponding bit disable that output!

> So, if not asked directly to modify those bits, write the inverted
> value.

Some comments in the code explaining what's going on wouild help a lot -
it's not going to be obvious to a reader why the code is doing this and
may well confuse them.

[PATCH 1/4] mfd: mc13783: Take care of semantic inversion between read and write value of two bits in POWER_MISCELLANEUS register.

[Uwe Kleine-König]

On Sat, Dec 12, 2009 at 05:48:43PM +0100, Alberto Panizzo wrote:
> MC13783_REGCTRL_PWGTnSPIEN controls the states of the corresponding
> PWGTn_DRV output.
> Reading 1 on the corresponding bit mean that the output is enabled
> Writing 1 on the corresponding bit disable that output!
> 
> So, if not asked directly to modify those bits, write the inverted
> value.
Hmm, I'm not sure this completely right.  The Spec has:

        Bit PWGTxSPIEN | Pin PWGTxEN | PWGTxDRV |  Read Back
          0 = default  |             |          | PWGTxSPIEN
        ---------------+-------------+----------+------------
              1        |      x      |   Low    |     0
              0        |      0      |   High   |     1
              0        |      1      |   Low    |     0

So it looks a bit harder than just inverting the read bit.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH 1/4] mfd: mc13783: Take care of semantic inversion between read and write value of two bits in POWER_MISCELLANEUS register.

[Alberto Panizzo]

Hi Uwe..

Il giorno dom, 13/12/2009 alle 20.56 +0100, Uwe Kleine-K?nig ha scritto:
> On Sat, Dec 12, 2009 at 05:48:43PM +0100, Alberto Panizzo wrote:
> > MC13783_REGCTRL_PWGTnSPIEN controls the states of the corresponding
> > PWGTn_DRV output.
> > Reading 1 on the corresponding bit mean that the output is enabled
> > Writing 1 on the corresponding bit disable that output!
> > 
> > So, if not asked directly to modify those bits, write the inverted
> > value.
> Hmm, I'm not sure this completely right.  The Spec has:
> 
>         Bit PWGTxSPIEN | Pin PWGTxEN | PWGTxDRV |  Read Back
>           0 = default  |             |          | PWGTxSPIEN
>         ---------------+-------------+----------+------------
>               1        |      x      |   Low    |     0
>               0        |      0      |   High   |     1
>               0        |      1      |   Low    |     0
> 
> So it looks a bit harder than just inverting the read bit.
> 
> Best regards
> Uwe
> 

Yes, it is a bit harder, and because we don't have the complete 
information (we cannot check via software the state of Pin PWGTxEN)
the problem have no complete solution: if the read back value is 0 what I
choose is to assign to the software the master part.

We have to decide what to do, the other option is to write always 0 
(that's what the freescale code do) to let the hardware control itself.
This one for my board work as well, but it is the same, it is not a
complete solution.

Maybe we can trace via software the state of those two bits, starting
from an initial value, 0? (maybe the bootloader wrote 1 on those..)

Alberto.