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
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 <[email protected]>
---
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
With this, mc13783 subsystems drivers can configure the mc13783 chip
reading and writing registers.
Signed-off-by: Alberto Panizzo <[email protected]>
Acked-by: Uwe Kleine-König <[email protected]>
---
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
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 <[email protected]>
Acked-by: Mark Brown <[email protected]>
---
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 <[email protected]>
+ * Copyright 2009 Alberto Panizzo <[email protected]>
*
* 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Applied to 2.6.33
Thanks
Liam
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 <[email protected]>
Acked-by: Mark Brown <[email protected]>
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 <[email protected]>
> Acked-by: Mark Brown <[email protected]>
> ---
> drivers/regulator/mc13783-regulator.c | 345 ++++++++++++++++++++++++++++++---
> 1 files changed, 315 insertions(+), 30 deletions(-)
Applied.
Thanks
Liam
Ping :)
PATCH 1 & 2 are fixes that can go to .33
Thanks for the time giving to this.
Alberto!
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/ |
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!
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/ |
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!
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 <[email protected]>
> ---
> 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/
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/ |
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
>
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!
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/