[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 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 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 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 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 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 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 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 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.

[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 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 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 1/4] mfd: mc13783: Take care of semantic inversion between read and write value of two bits in POWER_MISCELLANEUS register.

[Alberto Panizzo]

PWGT1DRV and PWGT1DRV are two digital output controlled by two corresponding
hardware signals (Pin PWGTnEN) that are meant to be used to control core power
supplies.
The register MC13783_REG_POWER_MISCELLANEOUS contain the two control and
status bit (PWGTnSPIEN) where write and read meaning is summarised by 
the following table:

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

Writing a 1 to those bits will turn off the corresponding core
power supply. As there is no way to read back the value of 
PWGTnSPIEN, the behaviour chosen is to let always the hardware
control itself leaving those bits at the default value.

This patch is especially needed for manipulate the other bits
in the same register, where the read-modify-write operation
can produce unwanted power fault.

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

diff --git a/drivers/mfd/mc13783-core.c b/drivers/mfd/mc13783-core.c
index a1ade23..3953297 100644
--- a/drivers/mfd/mc13783-core.c
+++ b/drivers/mfd/mc13783-core.c
@@ -171,6 +171,9 @@ int mc13783_reg_read(struct mc13783 *mc13783, unsigned int offset, u32 *val)
 }
 EXPORT_SYMBOL(mc13783_reg_read);
 
+#define MC13783_REG_POWER_MISCELLANEOUS	34
+#define MC13783_REGCTRL_PWGT1SPIEN	(1 << 15)
+#define MC13783_REGCTRL_PWGT2SPIEN	(1 << 16)
 int mc13783_reg_write(struct mc13783 *mc13783, unsigned int offset, u32 val)
 {
 	u32 buf;
@@ -187,6 +190,13 @@ int mc13783_reg_write(struct mc13783 *mc13783, unsigned int offset, u32 val)
 
 	buf = 1 << 31 | offset << MC13783_REGOFFSET_SHIFT | val;
 
+	/* Take care of table 4-24 in Freescale MC13783IGPLDRM.pdf making
+	 * the assumption that PWGTnDRV signals controls core power supplies
+	 * that software must not disable. */
+	if (offset == MC13783_REG_POWER_MISCELLANEOUS)
+		buf &= !(MC13783_REGCTRL_PWGT1SPIEN |
+			 MC13783_REGCTRL_PWGT2SPIEN);
+
 	memset(&t, 0, sizeof(t));
 
 	t.tx_buf = &buf;
-- 
1.6.3.3

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

[Alberto Panizzo]

Il giorno lun, 14/12/2009 alle 20.45 +0300, Sergei Shtylyov ha scritto:
> > +     if (offset == MC13783_REG_POWER_MISCELLANEOUS)
> > +             buf &= !(MC13783_REGCTRL_PWGT1SPIEN |
> >   
> 
>    Are you sure ! shouldn't be ~ here?
> 
> > +                      MC13783_REGCTRL_PWGT2SPIEN);
> >   
> 
>    !(MC13783_REGCTRL_PWGT1SPIEN | MC13783_REGCTRL_PWGT2SPIEN) would 
> evaluate to 0 which is most probably not what you want.
> 
> WBR, Sergei 

For sure, you are right. The correct patch below..

PWGT1DRV and PWGT1DRV are two digital output controlled by two corresponding
hardware signals (Pin PWGTnEN) that are meant to be used to control core power
supplies.
The register MC13783_REG_POWER_MISCELLANEOUS contain the two control and
status bit (PWGTnSPIEN) where write and read meaning is summarised by 
the following table:

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

Writing a 1 to those bits will turn off the corresponding core
power supply. As there is no way to read back the value of 
PWGTnSPIEN, the behaviour chosen is to let always the hardware
control itself leaving those bits at the default value.

This patch is especially needed for manipulate the other bits
in the same register, where the read-modify-write operation
can produce unwanted power fault.

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

diff --git a/drivers/mfd/mc13783-core.c b/drivers/mfd/mc13783-core.c
index a1ade23..3953297 100644
--- a/drivers/mfd/mc13783-core.c
+++ b/drivers/mfd/mc13783-core.c
@@ -171,6 +171,9 @@ int mc13783_reg_read(struct mc13783 *mc13783, unsigned int offset, u32 *val)
 }
 EXPORT_SYMBOL(mc13783_reg_read);
 
+#define MC13783_REG_POWER_MISCELLANEOUS        34
+#define MC13783_REGCTRL_PWGT1SPIEN     (1 << 15)
+#define MC13783_REGCTRL_PWGT2SPIEN     (1 << 16)
 int mc13783_reg_write(struct mc13783 *mc13783, unsigned int offset, u32 val)
 {
        u32 buf;
@@ -187,6 +190,13 @@ int mc13783_reg_write(struct mc13783 *mc13783, unsigned int offset, u32 val)
 
        buf = 1 << 31 | offset << MC13783_REGOFFSET_SHIFT | val;
 
+       /* Take care of table 4-24 in Freescale MC13783IGPLDRM.pdf making
+        * the assumption that PWGTnDRV signals controls core power supplies
+        * that software must not disable. */
+       if (offset == MC13783_REG_POWER_MISCELLANEOUS)
+               buf &= ~(MC13783_REGCTRL_PWGT1SPIEN |
+                        MC13783_REGCTRL_PWGT2SPIEN);
+
        memset(&t, 0, sizeof(t));
 
        t.tx_buf = &buf;
-- 
1.6.3.3

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

[Samuel Ortiz]

Hi Alberto,

On Mon, Dec 14, 2009 at 06:59:00PM +0100, Alberto Panizzo wrote:
> Il giorno lun, 14/12/2009 alle 20.45 +0300, Sergei Shtylyov ha scritto:
> > > +     if (offset == MC13783_REG_POWER_MISCELLANEOUS)
> > > +             buf &= !(MC13783_REGCTRL_PWGT1SPIEN |
> > >   
> > 
> >    Are you sure ! shouldn't be ~ here?
> > 
> > > +                      MC13783_REGCTRL_PWGT2SPIEN);
> > >   
> > 
> >    !(MC13783_REGCTRL_PWGT1SPIEN | MC13783_REGCTRL_PWGT2SPIEN) would 
> > evaluate to 0 which is most probably not what you want.
> > 
> > WBR, Sergei 
> 
> For sure, you are right. The correct patch below..
> 
> PWGT1DRV and PWGT1DRV are two digital output controlled by two corresponding
> hardware signals (Pin PWGTnEN) that are meant to be used to control core power
> supplies.
> The register MC13783_REG_POWER_MISCELLANEOUS contain the two control and
> status bit (PWGTnSPIEN) where write and read meaning is summarised by 
> the following table:
> 
>  Bit PWGTxSPIEN | Pin PWGTxEN | PWGTxDRV |  Read Back
>    0 = default  |             |          | PWGTxSPIEN
>  ---------------+-------------+----------+------------
>        1        |      x      |   Low    |     0
>        0        |      0      |   High   |     1
>        0        |      1      |   Low    |     0
> 
> Writing a 1 to those bits will turn off the corresponding core
> power supply. As there is no way to read back the value of 
> PWGTnSPIEN, 
Nice hardware :(


> the behaviour chosen is to let always the hardware
> control itself leaving those bits at the default value.
> 
> This patch is especially needed for manipulate the other bits
> in the same register, where the read-modify-write operation
> can produce unwanted power fault.
> 
> Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
> ---
>  drivers/mfd/mc13783-core.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mfd/mc13783-core.c b/drivers/mfd/mc13783-core.c
> index a1ade23..3953297 100644
> --- a/drivers/mfd/mc13783-core.c
> +++ b/drivers/mfd/mc13783-core.c
> @@ -171,6 +171,9 @@ int mc13783_reg_read(struct mc13783 *mc13783, unsigned int offset, u32 *val)
>  }
>  EXPORT_SYMBOL(mc13783_reg_read);
>  
> +#define MC13783_REG_POWER_MISCELLANEOUS        34
> +#define MC13783_REGCTRL_PWGT1SPIEN     (1 << 15)
> +#define MC13783_REGCTRL_PWGT2SPIEN     (1 << 16)
>  int mc13783_reg_write(struct mc13783 *mc13783, unsigned int offset, u32 val)
>  {
>         u32 buf;
> @@ -187,6 +190,13 @@ int mc13783_reg_write(struct mc13783 *mc13783, unsigned int offset, u32 val)
>  
>         buf = 1 << 31 | offset << MC13783_REGOFFSET_SHIFT | val;
>  
> +       /* Take care of table 4-24 in Freescale MC13783IGPLDRM.pdf making
> +        * the assumption that PWGTnDRV signals controls core power supplies
> +        * that software must not disable. */
> +       if (offset == MC13783_REG_POWER_MISCELLANEOUS)
> +               buf &= ~(MC13783_REGCTRL_PWGT1SPIEN |
> +                        MC13783_REGCTRL_PWGT2SPIEN);
> +
Although I see where you want to go, I dont really enjoy that solution.
I would prefere to have specific register write/rmw routines for
MC13783_REG_POWER_MISCELLANEOUS, and at the same time forbid to access the
latter from the current mc13783_reg_* API.

Cheers,
Samuel.


>         memset(&t, 0, sizeof(t));
>  
>         t.tx_buf = &buf;
> -- 
> 1.6.3.3
> 

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[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 Tue, Jan 05, 2010 at 07:15:42PM +0100, Samuel Ortiz wrote:
> > @@ -187,6 +190,13 @@ int mc13783_reg_write(struct mc13783 *mc13783, unsigned int offset, u32 val)
> >  
> >         buf = 1 << 31 | offset << MC13783_REGOFFSET_SHIFT | val;
> >  
> > +       /* Take care of table 4-24 in Freescale MC13783IGPLDRM.pdf making
> > +        * the assumption that PWGTnDRV signals controls core power supplies
> > +        * that software must not disable. */
> > +       if (offset == MC13783_REG_POWER_MISCELLANEOUS)
> > +               buf &= ~(MC13783_REGCTRL_PWGT1SPIEN |
> > +                        MC13783_REGCTRL_PWGT2SPIEN);
> > +
> Although I see where you want to go, I dont really enjoy that solution.
> I would prefere to have specific register write/rmw routines for
> MC13783_REG_POWER_MISCELLANEOUS, and at the same time forbid to access the
> latter from the current mc13783_reg_* API.
Ack.  This is what I already suggested in

	http://thread.gmane.org/gmane.linux.kernel/927112/focus=930317

(This happend to be a reply to patch 2/4 as I replied to Alberto's ping
for patches 1 and 2.)

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]

Sorry for late response.. as Uwe said, we have discuss about this 
and I will prepare another patch that involve the regulator driver.

Alberto!

Il giorno mar, 05/01/2010 alle 20.55 +0100, Uwe Kleine-K?nig ha scritto:
> On Tue, Jan 05, 2010 at 07:15:42PM +0100, Samuel Ortiz wrote:
> > > @@ -187,6 +190,13 @@ int mc13783_reg_write(struct mc13783 *mc13783, unsigned int offset, u32 val)
> > >  
> > >         buf = 1 << 31 | offset << MC13783_REGOFFSET_SHIFT | val;
> > >  
> > > +       /* Take care of table 4-24 in Freescale MC13783IGPLDRM.pdf making
> > > +        * the assumption that PWGTnDRV signals controls core power supplies
> > > +        * that software must not disable. */
> > > +       if (offset == MC13783_REG_POWER_MISCELLANEOUS)
> > > +               buf &= ~(MC13783_REGCTRL_PWGT1SPIEN |
> > > +                        MC13783_REGCTRL_PWGT2SPIEN);
> > > +
> > Although I see where you want to go, I dont really enjoy that solution.
> > I would prefere to have specific register write/rmw routines for
> > MC13783_REG_POWER_MISCELLANEOUS, and at the same time forbid to access the
> > latter from the current mc13783_reg_* API.
> Ack.  This is what I already suggested in
> 
> 	http://thread.gmane.org/gmane.linux.kernel/927112/focus=930317
> 
> (This happend to be a reply to patch 2/4 as I replied to Alberto's ping
> for patches 1 and 2.)
> 
> Best regards
> Uwe
>