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

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

[Alberto Panizzo]

Resend this series of patches.

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]

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 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>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 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 3953297..185139e 100644
--- a/drivers/mfd/mc13783-core.c
+++ b/drivers/mfd/mc13783-core.c
@@ -629,6 +629,8 @@ err_revision:
 	}
 	/* This should go away (END) */
 
+	mc13783_unlock(mc13783);
+
 	if (pdata->flags & MC13783_USE_ADC)
 		mc13783_add_subdevice(mc13783, "mc13783-adc");
 
@@ -651,8 +653,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 v2 .

[Alberto Panizzo]

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

version 2 diffs:
- delete the "Switchers PLL" enable and multiplication factor value 
  selecting capability because it is not a voltage or current regulator.
  This will be a part of Main switcher supporting task.
- Correct many coding style problems pointed me out.

Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/regulator/mc13783-regulator.c |  345 ++++++++++++++++++++++++++++++---
 1 files changed, 315 insertions(+), 30 deletions(-)

diff --git a/drivers/regulator/mc13783-regulator.c b/drivers/regulator/mc13783-regulator.c
index 9f99862..3d5d349 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
@@ -16,11 +17,44 @@
 #include <linux/init.h>
 #include <linux/err.h>
 
-#define MC13783_REG_SWITCHERS4			28
-#define MC13783_REG_SWITCHERS4_PLLEN			(1 << 18)
-
 #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,14 +87,88 @@ 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_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;
+static struct regulator_ops mc13783_fixed_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,		\
@@ -68,36 +176,83 @@ static struct regulator_ops mc13783_regulator_ops;
 		},							\
 		.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_DEFINE_SW(_name, _reg) MC13783_DEFINE(SW, _name, _reg)
-#define MC13783_DEFINE_REGU(_name, _reg) MC13783_DEFINE(REGU, _name, _reg)
+#define MC13783_FIXED_DEFINE(prefix, _name, _reg, _voltages)		\
+	[MC13783_ ## prefix ## _ ## _name] = {				\
+		.desc = {						\
+			.name = #prefix "_" #_name,			\
+			.n_voltages = ARRAY_SIZE(_voltages),		\
+			.ops = &mc13783_fixed_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,			\
+			.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,	\
+	}
+
+#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_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 +309,140 @@ 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] >= min_uV &&
+		    mc13783_regulators[reg_id].voltages[i] < bestmatch) {
+			bestmatch = mc13783_regulators[reg_id].voltages[i];
+			bestindex = i;
+		}
+	}
+
+	if (bestindex < 0 || bestmatch > max_uV) {
+		dev_warn(&rdev->dev, "no possible value for %d<=x<=%d uV\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);
+
+	/* 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);
+
+	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 mc13783_fixed_regulator_set_voltage(struct regulator_dev *rdev,
+						int min_uV, int max_uV)
+{
+	int id = rdev_get_id(rdev);
+
+	dev_dbg(rdev_get_dev(rdev), "%s id: %d min_uV: %d max_uV: %d\n",
+		__func__, id, min_uV, max_uV);
+
+	if (min_uV > mc13783_regulators[id].voltages[0] &&
+	    max_uV < mc13783_regulators[id].voltages[0])
+		return 0;
+	else
+		return -EINVAL;
+}
+
+static int mc13783_fixed_regulator_get_voltage(struct regulator_dev *rdev)
+{
+	int id = rdev_get_id(rdev);
+
+	dev_dbg(rdev_get_dev(rdev), "%s id: %d\n", __func__, id);
+
+	return mc13783_regulators[id].voltages[0];
+}
+
+static struct regulator_ops mc13783_fixed_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_fixed_regulator_set_voltage,
+	.get_voltage = mc13783_fixed_regulator_get_voltage,
 };
 
 static int __devinit mc13783_regulator_probe(struct platform_device *pdev)
-- 
1.6.3.3

[PATCH 4/4] regulator: mc13783-regulator: correct the probing time.

[Alberto Panizzo]

When the mc13783-regulator driver is built in kernel, probing it during
the regulator subsystem initialisation result in a fault.

That is because regulator subsystem is planned to be initialised very early
in the boot process, before the mfd subsystem initialisation.

The mc12783-regulator probing process need to access to the mc13783-core
functionality to read/write mc13783 registers and so must be called after
the mc13783-core driver initialisation.

The way to do this is to let the kernel probe the mc13783-regulator driver when 
mc13783-core register his regulator subdevice.

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 3d5d349..a40e35a 100644
--- a/drivers/regulator/mc13783-regulator.c
+++ b/drivers/regulator/mc13783-regulator.c
@@ -509,12 +509,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

Re: [PATCH 4/4] regulator: mc13783-regulator: correct the probing time.

[Liam Girdwood]

On Mon, 2009-12-14 at 18:53 +0100, Alberto Panizzo wrote:
> When the mc13783-regulator driver is built in kernel, probing it during
> the regulator subsystem initialisation result in a fault.
> 
> That is because regulator subsystem is planned to be initialised very early
> in the boot process, before the mfd subsystem initialisation.
> 
> The mc12783-regulator probing process need to access to the mc13783-core
> functionality to read/write mc13783 registers and so must be called after
> the mc13783-core driver initialisation.
> 
> The way to do this is to let the kernel probe the mc13783-regulator driver when 
> mc13783-core register his regulator subdevice.
> 
> Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>

Applied to 2.6.33

Thanks

Liam

Re: [PATCH 4/4] regulator: mc13783-regulator: correct the probing time.

[Mark Brown]

On Mon, Dec 14, 2009 at 06:53:35PM +0100, Alberto Panizzo wrote:
> When the mc13783-regulator driver is built in kernel, probing it during
> the regulator subsystem initialisation result in a fault.
> 
> That is because regulator subsystem is planned to be initialised very early
> in the boot process, before the mfd subsystem initialisation.
> 
> The mc12783-regulator probing process need to access to the mc13783-core
> functionality to read/write mc13783 registers and so must be called after
> the mc13783-core driver initialisation.
> 
> The way to do this is to let the kernel probe the mc13783-regulator driver when 
> mc13783-core register his regulator subdevice.
> 
> Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>

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

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

[Liam Girdwood]

On Mon, 2009-12-14 at 18:26 +0100, Alberto Panizzo wrote:
> This patch, complete the mc13783 regulator subsystem driver with 
> voltage selecting capability.
> Main Switches (SW1AB, SW2AB) are not supported yet.
> 
> version 2 diffs:
> - delete the "Switchers PLL" enable and multiplication factor value 
>   selecting capability because it is not a voltage or current regulator.
>   This will be a part of Main switcher supporting task.
> - Correct many coding style problems pointed me out.
> 
> Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  drivers/regulator/mc13783-regulator.c |  345 ++++++++++++++++++++++++++++++---
>  1 files changed, 315 insertions(+), 30 deletions(-)

Applied.

Thanks

Liam

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

[Alberto Panizzo]

Ping :)

PATCH 1 & 2 are fixes that can go to .33

Thanks for the time giving to this.

Alberto!

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

[Uwe Kleine-König]

Hello,

On Fri, Dec 18, 2009 at 05:12:26PM +0100, Alberto Panizzo wrote:
> Ping :)
> 
> PATCH 1 & 2 are fixes that can go to .33
I don't like patch 1.  I'd prefer that drivers touching
MC13783_REG_POWER_MISCELLANEOUS are aware of the bit in question and
wouldn't rely on mc13783-core.

Best regards
Uwe

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

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

[Alberto Panizzo]

Il giorno sab, 19/12/2009 alle 21.18 +0100, Uwe Kleine-König ha
scritto: 
> Hello,
> 
> On Fri, Dec 18, 2009 at 05:12:26PM +0100, Alberto Panizzo wrote:
> > Ping :)
> > 
> > PATCH 1 & 2 are fixes that can go to .33
> I don't like patch 1.  I'd prefer that drivers touching
> MC13783_REG_POWER_MISCELLANEOUS are aware of the bit in question and
> wouldn't rely on mc13783-core.
> 
> Best regards
> Uwe
> 

Yes, but MC13783_REG_POWER_MISCELLANEOUS contains bit that control
different aspect of mc13783 chip.
GPO are regulator related, but those two bits in question maybe 
apply to a power management driver, so this problem is a matter
of mc13783-core.

Another possible solution, is to trace the writings to those two bits
in mc13783_reg_rmw storing the value written an reproducing it in next
mc13783_reg_rmw calls.
But the problem for this is that we don't know if the bootloader 
had initialised those with another non default value.
Another problem is that if another driver make use of 
mc13783_reg_write for modifying those bits, the state stored will
be inconsistent.

And so? what kind of solution can you suggest?

I am working to support i.MX31 PDK board that make a strong use of mc13783
and GPO's controls important power supplies.

Alberto!

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

[Uwe Kleine-König]

Hi Alberto,

On Sun, Dec 20, 2009 at 02:48:38PM +0100, Alberto Panizzo wrote:
> > > PATCH 1 & 2 are fixes that can go to .33
> > I don't like patch 1.  I'd prefer that drivers touching
> > MC13783_REG_POWER_MISCELLANEOUS are aware of the bit in question and
> > wouldn't rely on mc13783-core.
> 
> Yes, but MC13783_REG_POWER_MISCELLANEOUS contains bit that control
> different aspect of mc13783 chip.
> GPO are regulator related, but those two bits in question maybe 
> apply to a power management driver, so this problem is a matter
> of mc13783-core.
maybe?
 
> Another possible solution, is to trace the writings to those two bits
> in mc13783_reg_rmw storing the value written an reproducing it in next
> mc13783_reg_rmw calls.
> But the problem for this is that we don't know if the bootloader 
> had initialised those with another non default value.
> Another problem is that if another driver make use of 
> mc13783_reg_write for modifying those bits, the state stored will
> be inconsistent.
The next best thing I would consider acceptable are dedicated functions
for MC13783_REG_POWER_MISCELLANEOUS.  I havn't checked what the register
in question is used for, but I think the bootloader isn't generally a
problem as Linux shouldn't rely on things setup by the bootloader (apart
from the things described in
http://www.arm.linux.org.uk/developer/booting.php of course).  And I
don't see a problem for in-kernel users of mc13783_reg_write to modify
the register.  If the mc13783-API is changed that
MC13783_REG_POWER_MISCELLANEOUS must only be modified by using (say)
mc13783_powermisc_rmw() it's a (probably uncatched) bug to use
mc13783_reg_write.

And patch 1 is definitly *not* material for .33, as there is currently
no user of MC13783_REG_POWER_MISCELLANEOUS in .33-rc1, so there is
nothing to fix.

Best regards
Uwe

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

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

[Alberto Panizzo]

Hi Uwe,

Il giorno dom, 20/12/2009 alle 19.49 +0100, Uwe Kleine-König ha scritto:
> Hi Alberto,
> 
> On Sun, Dec 20, 2009 at 02:48:38PM +0100, Alberto Panizzo wrote:
> > > > PATCH 1 & 2 are fixes that can go to .33
> > > I don't like patch 1.  I'd prefer that drivers touching
> > > MC13783_REG_POWER_MISCELLANEOUS are aware of the bit in question and
> > > wouldn't rely on mc13783-core.
> > 
> > Yes, but MC13783_REG_POWER_MISCELLANEOUS contains bit that control
> > different aspect of mc13783 chip.
> > GPO are regulator related, but those two bits in question maybe 
> > apply to a power management driver, so this problem is a matter
> > of mc13783-core.
> maybe?
>  
> > Another possible solution, is to trace the writings to those two bits
> > in mc13783_reg_rmw storing the value written an reproducing it in next
> > mc13783_reg_rmw calls.
> > But the problem for this is that we don't know if the bootloader 
> > had initialised those with another non default value.
> > Another problem is that if another driver make use of 
> > mc13783_reg_write for modifying those bits, the state stored will
> > be inconsistent.
> The next best thing I would consider acceptable are dedicated functions
> for MC13783_REG_POWER_MISCELLANEOUS.  I havn't checked what the register
> in question is used for, but I think the bootloader isn't generally a
> problem as Linux shouldn't rely on things setup by the bootloader (apart
> from the things described in
> http://www.arm.linux.org.uk/developer/booting.php of course).  And I
> don't see a problem for in-kernel users of mc13783_reg_write to modify
> the register.  If the mc13783-API is changed that
> MC13783_REG_POWER_MISCELLANEOUS must only be modified by using (say)
> mc13783_powermisc_rmw() it's a (probably uncatched) bug to use
> mc13783_reg_write.

Thanks for your point of view. What I proposed is a functionality cut
for mc13783 and I understand this.

MC13783_REG_POWER_MISCELLANEOUS contain configuration bits for GPO's
(two bits for everyone: "Enable" and "Controlled by Standby")
those two Power Gates Enable and VIBPINCTRL that behave the same as the
two Power Gates Enable.

GPO's are considered as regulators by the present driver in the term 
that they are thought to be used as external power supplies controllers.

Power Gates behave light different: PWGTnDRV are digital output 
controlled by hardware pins (PWGTnEN) and enabled (the hardware control)
by those two bits.

In this sense, can they be considered digital regulators too?

If yes, they can be controlled in the regulator driver and there can
be written a new mc13783_regulator_powermisc_rmw() function.

In the i.MX31 PDK board PWGT1DRV controls the MCU power supply and
PWGT2DRV control the L2 cache power supply. PWGTnEN are connected
to the corresponding i.MX31 Power Management interface outputs
in this way, in power saving mode, voltage for MCU is lowered at the
minimum and L2 cache is turned off.

> 
> And patch 1 is definitly *not* material for .33, as there is currently
> no user of MC13783_REG_POWER_MISCELLANEOUS in .33-rc1, so there is
> nothing to fix.
> 
> Best regards
> Uwe

You are right, there is no directly user, what I thought to fix is
the capability to enable / disable GPOs in the mc13783 that is a 
existent functionality for the present .33 regulator driver.

I must make much familiarity with the kernel developing process.

Thanks a lot!
Best regards
Alberto!

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

[Alberto Panizzo]

Samuel, although patch [1/4] is not a matter of .33 
I think this one [2/4] can go to mainline in this subversion.

As submitted by Valentin Longchamp here:

http://thread.gmane.org/gmane.linux.ports.arm.kernel/71128/focus=71127

this patch, Acked by Uwe, fix a little regression in mc13783-core.

Best Regards,
Alberto!

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

[Samuel Ortiz]

Hi Alberto,

On Fri, Jan 08, 2010 at 12:13:35PM +0100, Alberto Panizzo wrote:
> Samuel, although patch [1/4] is not a matter of .33 
> I think this one [2/4] can go to mainline in this subversion.
> 
> As submitted by Valentin Longchamp here:
> 
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/71128/focus=71127
> 
> this patch, Acked by Uwe, fix a little regression in mc13783-core.
Right, I'll push that upstream soon.

Cheers,
Samuel.


> Best Regards,
> Alberto!
> 

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

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

[Sergei Shtylyov]

Alberto Panizzo wrote:
> 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 |
>   

   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

Re: [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

Re: [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/

Re: [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/  |

Re: [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
> 

[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

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

Re: [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/  |

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