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
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 <[email protected]>
---
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
With this, mc13783 subsystems drivers can configure the mc13783 chip
reading and writing registers.
Signed-off-by: Alberto Panizzo <[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 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
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 <[email protected]>
---
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 <[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
@@ -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
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 <[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 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
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.
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 <[email protected]>
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 <[email protected]>
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 (.
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 <[email protected]>
Acked-by: Mark Brown <[email protected]>
I think this one ought to go into 2.6.33 as a fix.
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/ |
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 <[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 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/ |
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 <[email protected]>
> ---
> 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 <[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
> @@ -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/ |
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 <[email protected]>
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/ |
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.
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 <[email protected]>
> > ---
> > 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 <[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
> > @@ -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.
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 <[email protected]>
> 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!
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).