GPO regulators are digital outputs that can be enabled or disabled by a
dedicated bit in mc13783 POWERMISC register.
In this family can be count in also Power Gates (PWGT1 and 2): enabled by
a dedicated pin a Power Gate is an hardware driven supply where the output
(PWGTnDRV) follow this law:
Bit PWGTxSPIEN | Pin PWGTxEN | PWGTxDRV | Read Back
0 = default | | | PWGTxSPIEN
---------------+-------------+----------+------------
1 | x | Low | 0
0 | 0 | High | 1
0 | 1 | Low | 0
As read back value of control bit reflects the PWGTxDRV state and not the
control value previously written, a dedicated function to manage bits of
POWERMISC register is created and the state of PWGTxEN bits is stored in memory.
All POWERMISC users _must_ make use of the new function to not accidentally
disable Power Gates supplies.
Signed-off-by: Alberto Panizzo <[email protected]>
---
drivers/regulator/mc13783-regulator.c | 97 ++++++++++++++++++++++++++++++++-
include/linux/mfd/mc13783.h | 2 +
2 files changed, 98 insertions(+), 1 deletions(-)
diff --git a/drivers/regulator/mc13783-regulator.c b/drivers/regulator/mc13783-regulator.c
index a40e35a..8570fce 100644
--- a/drivers/regulator/mc13783-regulator.c
+++ b/drivers/regulator/mc13783-regulator.c
@@ -82,6 +82,10 @@
#define MC13783_REG_POWERMISC_GPO2EN (1 << 8)
#define MC13783_REG_POWERMISC_GPO3EN (1 << 10)
#define MC13783_REG_POWERMISC_GPO4EN (1 << 12)
+#define MC13783_REG_POWERMISC_PWGTSPI_M (3 << 15)
+#define MC13783_REG_POWERMISC_PWGT1SPIEN (1 << 15)
+#define MC13783_REG_POWERMISC_PWGT2SPIEN (1 << 16)
+
struct mc13783_regulator {
struct regulator_desc desc;
@@ -163,6 +167,7 @@ static const int const mc13783_vrf_val[] = {
static struct regulator_ops mc13783_regulator_ops;
static struct regulator_ops mc13783_fixed_regulator_ops;
+static struct regulator_ops mc13783_regulator_ops_gpo;
#define MC13783_DEFINE(prefix, _name, _reg, _vsel_reg, _voltages) \
[MC13783_ ## prefix ## _ ## _name] = { \
@@ -201,7 +206,7 @@ static struct regulator_ops mc13783_fixed_regulator_ops;
[MC13783_ ## prefix ## _ ## _name] = { \
.desc = { \
.name = #prefix "_" #_name, \
- .ops = &mc13783_regulator_ops, \
+ .ops = &mc13783_regulator_ops_gpo, \
.type = REGULATOR_VOLTAGE, \
.id = MC13783_ ## prefix ## _ ## _name, \
.owner = THIS_MODULE, \
@@ -253,6 +258,8 @@ static struct mc13783_regulator mc13783_regulators[] = {
MC13783_GPO_DEFINE(REGU, GPO2, POWERMISC),
MC13783_GPO_DEFINE(REGU, GPO3, POWERMISC),
MC13783_GPO_DEFINE(REGU, GPO4, POWERMISC),
+ MC13783_GPO_DEFINE(REGU, PWGT1SPI, POWERMISC),
+ MC13783_GPO_DEFINE(REGU, PWGT2SPI, POWERMISC),
};
struct mc13783_regulator_priv {
@@ -445,6 +452,94 @@ static struct regulator_ops mc13783_fixed_regulator_ops = {
.get_voltage = mc13783_fixed_regulator_get_voltage,
};
+int mc13783_state_powermisc_pwgt;
+int mc13783_reg_rmw_powermisc(struct mc13783 *mc13783, u32 mask, u32 val)
+{
+ int ret;
+ u32 valread;
+
+ BUG_ON(val & ~mask);
+
+ /* Update the stored state for Power Gates.
+ * As from specs the meaning is inverted (0: en, 1: dis) */
+ if (mask & MC13783_REG_POWERMISC_PWGTSPI_M)
+ mc13783_state_powermisc_pwgt =
+ (mc13783_state_powermisc_pwgt & ~mask) |
+ ((val ^ mask) & MC13783_REG_POWERMISC_PWGTSPI_M);
+
+ ret = mc13783_reg_read(mc13783, MC13783_REG_POWERMISC, &valread);
+ if (ret)
+ return ret;
+
+ valread = (valread & ~mask) | val;
+
+ /* Re propose the stored state for Power Gates */
+ valread = (valread & ~MC13783_REG_POWERMISC_PWGTSPI_M) |
+ mc13783_state_powermisc_pwgt;
+
+ return mc13783_reg_write(mc13783, MC13783_REG_POWERMISC, valread);
+}
+
+static int mc13783_regulator_enable_gpo(struct regulator_dev *rdev)
+{
+ struct mc13783_regulator_priv *priv = rdev_get_drvdata(rdev);
+ int id = rdev_get_id(rdev);
+ int ret;
+
+ dev_dbg(rdev_get_dev(rdev), "%s id: %d\n", __func__, id);
+
+ mc13783_lock(priv->mc13783);
+ ret = mc13783_reg_rmw_powermisc(priv->mc13783,
+ mc13783_regulators[id].enable_bit,
+ mc13783_regulators[id].enable_bit);
+ mc13783_unlock(priv->mc13783);
+
+ return ret;
+}
+
+static int mc13783_regulator_disable_gpo(struct regulator_dev *rdev)
+{
+ struct mc13783_regulator_priv *priv = rdev_get_drvdata(rdev);
+ int id = rdev_get_id(rdev);
+ int ret;
+
+ dev_dbg(rdev_get_dev(rdev), "%s id: %d\n", __func__, id);
+
+ mc13783_lock(priv->mc13783);
+ ret = mc13783_reg_rmw_powermisc(priv->mc13783,
+ mc13783_regulators[id].enable_bit, 0);
+ mc13783_unlock(priv->mc13783);
+
+ return ret;
+}
+
+static int mc13783_regulator_is_enabled_gpo(struct regulator_dev *rdev)
+{
+ struct mc13783_regulator_priv *priv = rdev_get_drvdata(rdev);
+ int ret, id = rdev_get_id(rdev);
+ unsigned int val;
+
+ mc13783_lock(priv->mc13783);
+ ret = mc13783_reg_read(priv->mc13783, mc13783_regulators[id].reg, &val);
+ mc13783_unlock(priv->mc13783);
+
+ /* Power Gates state is stored in mc13783_state_powermisc_pwgt
+ * where the meaning is inverted */
+ val = (val & ~MC13783_REG_POWERMISC_PWGTSPI_M) |
+ (mc13783_state_powermisc_pwgt ^ MC13783_REG_POWERMISC_PWGTSPI_M);
+
+ if (ret)
+ return ret;
+
+ return (val & mc13783_regulators[id].enable_bit) != 0;
+}
+
+static struct regulator_ops mc13783_regulator_ops_gpo = {
+ .enable = mc13783_regulator_enable_gpo,
+ .disable = mc13783_regulator_disable_gpo,
+ .is_enabled = mc13783_regulator_is_enabled_gpo,
+};
+
static int __devinit mc13783_regulator_probe(struct platform_device *pdev)
{
struct mc13783_regulator_priv *priv;
diff --git a/include/linux/mfd/mc13783.h b/include/linux/mfd/mc13783.h
index 3568040..94cb51a 100644
--- a/include/linux/mfd/mc13783.h
+++ b/include/linux/mfd/mc13783.h
@@ -108,6 +108,8 @@ int mc13783_adc_do_conversion(struct mc13783 *mc13783, unsigned int mode,
#define MC13783_REGU_V2 28
#define MC13783_REGU_V3 29
#define MC13783_REGU_V4 30
+#define MC13783_REGU_PWGT1SPI 31
+#define MC13783_REGU_PWGT2SPI 32
#define MC13783_IRQ_ADCDONE 0
#define MC13783_IRQ_ADCBISDONE 1
--
1.6.3.3
On Mon, Jan 18, 2010 at 05:02:03PM +0100, Alberto Panizzo wrote:
> +int mc13783_state_powermisc_pwgt;
> +int mc13783_reg_rmw_powermisc(struct mc13783 *mc13783, u32 mask, u32 val)
You're missing some statics here and some whitespace to separate the
function from the variable.
> + /* Update the stored state for Power Gates.
> + * As from specs the meaning is inverted (0: en, 1: dis) */
> + if (mask & MC13783_REG_POWERMISC_PWGTSPI_M)
> + mc13783_state_powermisc_pwgt =
> + (mc13783_state_powermisc_pwgt & ~mask) |
> + ((val ^ mask) & MC13783_REG_POWERMISC_PWGTSPI_M);
Could this code be written in a clearer fashion? The bit manipluation
isn't entirely obvious, especially given the multiple masks in use...
> + ret = mc13783_reg_read(mc13783, MC13783_REG_POWERMISC, &valread);
> + if (ret)
> + return ret;
> +
> + valread = (valread & ~mask) | val;
> +
> + /* Re propose the stored state for Power Gates */
> + valread = (valread & ~MC13783_REG_POWERMISC_PWGTSPI_M) |
> + mc13783_state_powermisc_pwgt;
...and this further mainpulation.
On lun, 2010-01-18 at 16:32 +0000, Mark Brown wrote:
> On Mon, Jan 18, 2010 at 05:02:03PM +0100, Alberto Panizzo wrote:
>
> > +int mc13783_state_powermisc_pwgt;
> > +int mc13783_reg_rmw_powermisc(struct mc13783 *mc13783, u32 mask, u32 val)
>
> You're missing some statics here and some whitespace to separate the
> function from the variable.
Ok, mc13783_state_powermisc_pwgt will be static (and u32..)
>
> > + /* Update the stored state for Power Gates.
> > + * As from specs the meaning is inverted (0: en, 1: dis) */
> > + if (mask & MC13783_REG_POWERMISC_PWGTSPI_M)
> > + mc13783_state_powermisc_pwgt =
> > + (mc13783_state_powermisc_pwgt & ~mask) |
> > + ((val ^ mask) & MC13783_REG_POWERMISC_PWGTSPI_M);
>
> Could this code be written in a clearer fashion? The bit manipluation
> isn't entirely obvious, especially given the multiple masks in use...
Something like this?
if (mask & MC13783_REG_POWERMISC_PWGTSPI_M) {
u32 new_state = (val & MC13783_REG_POWERMISC_PWGTSPI_M) ^ mask;
mc13783_state_powermisc_pwgt =
(mc13783_state_powermisc_pwgt & ~mask) | new_state;
}
I use the EXOR with one to negate bits.
The state stored in mc13783_state_powermisc_pwgt would be what the next
functions write.
>
> > + ret = mc13783_reg_read(mc13783, MC13783_REG_POWERMISC, &valread);
> > + if (ret)
> > + return ret;
> > +
> > + valread = (valread & ~mask) | val;
> > +
> > + /* Re propose the stored state for Power Gates */
> > + valread = (valread & ~MC13783_REG_POWERMISC_PWGTSPI_M) |
> > + mc13783_state_powermisc_pwgt;
>
> ...and this further mainpulation.
What is obscure in this? it is the same operation as the previous
MC13783_REG_POWERMISC_PWGTSPI_M is the mask for PWGT1 and 2 bits and in
mc13783_state_powermisc_pwgt there is the stored state for those two
bits.
As I just wrote...
On Mon, Jan 18, 2010 at 06:07:53PM +0100, Alberto Panizzo wrote:
> Something like this?
> if (mask & MC13783_REG_POWERMISC_PWGTSPI_M) {
> u32 new_state = (val & MC13783_REG_POWERMISC_PWGTSPI_M) ^ mask;
>
> mc13783_state_powermisc_pwgt =
> (mc13783_state_powermisc_pwgt & ~mask) | new_state;
> }
Yes, that's clearer.
> > > + if (ret)
> > > + return ret;
> > > +
> > > + valread = (valread & ~mask) | val;
> > > +
> > > + /* Re propose the stored state for Power Gates */
> > > + valread = (valread & ~MC13783_REG_POWERMISC_PWGTSPI_M) |
> > > + mc13783_state_powermisc_pwgt;
> >
> > ...and this further mainpulation.
> What is obscure in this? it is the same operation as the previous
> MC13783_REG_POWERMISC_PWGTSPI_M is the mask for PWGT1 and 2 bits and in
> mc13783_state_powermisc_pwgt there is the stored state for those two bits.
Part of it is the fact that the first bit was almost completely opaque
but even so it would be less surprising if you first worked out the
value you wanted to set, then did whatever manipulation was required to
translate into the format that actually gets written.
On lun, 2010-01-18 at 17:20 +0000, Mark Brown wrote:
> On Mon, Jan 18, 2010 at 06:07:53PM +0100, Alberto Panizzo wrote:
>
> > Something like this?
> > if (mask & MC13783_REG_POWERMISC_PWGTSPI_M) {
> > u32 new_state = (val & MC13783_REG_POWERMISC_PWGTSPI_M) ^ mask;
> >
> > mc13783_state_powermisc_pwgt =
> > (mc13783_state_powermisc_pwgt & ~mask) | new_state;
> > }
>
> Yes, that's clearer.
>
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + valread = (valread & ~mask) | val;
> > > > +
> > > > + /* Re propose the stored state for Power Gates */
> > > > + valread = (valread & ~MC13783_REG_POWERMISC_PWGTSPI_M) |
> > > > + mc13783_state_powermisc_pwgt;
> > >
> > > ...and this further mainpulation.
>
> > What is obscure in this? it is the same operation as the previous
> > MC13783_REG_POWERMISC_PWGTSPI_M is the mask for PWGT1 and 2 bits and in
> > mc13783_state_powermisc_pwgt there is the stored state for those two bits.
>
> Part of it is the fact that the first bit was almost completely opaque
> but even so it would be less surprising if you first worked out the
> value you wanted to set, then did whatever manipulation was required to
> translate into the format that actually gets written.
Maybe I not deep explained what's going on..
In POWERMISC register there are other controls bits than PWGTxEN that follow
the convention of 1= enable 0= disable and for those bits read and write value
are consistent: what is written could be read.
So, for all these bits the way to manipulate is the normal:
valread = (valread & ~mask) | val;
where the mask can indicate the manipulation of not only one bit.
As "mask" could contain manipulation of PWGTxEN bits, what I do is to overwrite
those with the previously updated value:
valread = (valread & ~MC13783_REG_POWERMISC_PWGTSPI_M) |
mc13783_state_powermisc_pwgt;
mc13783_state_powermisc_pwgt is maintained to be 0 in bits other than
MC13783_REG_POWERMISC_PWGTSPI_M mask.
I got me much clear?
I misunderstood the question?
Sorry my English please.. :)
Thanks!
Alberto.
On Mon, Jan 18, 2010 at 06:50:04PM +0100, Alberto Panizzo wrote:
> Maybe I not deep explained what's going on..
Like I say, half the problem was that the first bit of code was very
opaque so by the time you got past that the reader was already likely to
be lost.
Hello Alberto,
Can you post an updated patch addressing the discussion with Mark,
please?
>From the first look I don't like the name
MC13783_REG_POWERMISC_PWGTSPI_M as I don't understand it's purpose
without going through mc13783_reg_rmw_powermisc (which I didn't do yet).
What is the base for your patch? (Hm, it seems next could work.
mc13783-regulator seems to have gotten some more #defines ending in _M.
You seem to mean "mask". IMHO it's a bit unfortunate, because the
nameing scheme doesn't match the already existing names :-()
Shouldn't mc13783_state_powermisc_pwgt be a per-device variable instead
of a static variable?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On lun, 2010-01-18 at 20:04 +0100, Uwe Kleine-König wrote:
> Hello Alberto,
Hello Uwe!
>
> Can you post an updated patch addressing the discussion with Mark,
> please?
>
> From the first look I don't like the name
> MC13783_REG_POWERMISC_PWGTSPI_M as I don't understand it's purpose
>From now the present driver naming schema is mask ends in _M.
>
> What is the base for your patch? (Hm, it seems next could work.
Yes, this patch is based on:
git://git.kernel.org/pub/scm/linux/kernel/git/lrg/voltage-2.6.git for-next
> mc13783-regulator seems to have gotten some more #defines ending in _M.
> You seem to mean "mask". IMHO it's a bit unfortunate, because the
> nameing scheme doesn't match the already existing names :-()
If the naming schema adopted, conflicts other I will propose a correction..
>
> Shouldn't mc13783_state_powermisc_pwgt be a per-device variable instead
> of a static variable?
Thanks for point me out this. It should.
the new patch below:
--------------------------------------------------------------------------
GPO regulators are digital outputs that can be enabled or disabled by a
dedicated bit in mc13783 POWERMISC register.
In this family can be count in also Power Gates (PWGT1 and 2): enabled by
a dedicated pin a Power Gate is an hardware driven supply where the output
(PWGTnDRV) follow this law:
Bit PWGTxSPIEN | Pin PWGTxEN | PWGTxDRV | Read Back
0 = default | | | PWGTxSPIEN
---------------+-------------+----------+------------
1 | x | Low | 0
0 | 0 | High | 1
0 | 1 | Low | 0
As read back value of control bit reflects the PWGTxDRV state (not the
control value previously written) and mc13783 POWERMISC register contain
only regulator related bits, a dedicated function to manage these bits is
created here with the aim of tracing the real value of PWGTxSPIEN bits
and reproduce it on next writes.
All POWERMISC users _must_ use the new function to not accidentally
disable Power Gates supplies.
Signed-off-by: Alberto Panizzo <[email protected]>
---
drivers/regulator/mc13783-regulator.c | 109 ++++++++++++++++++++++++++++++++-
include/linux/mfd/mc13783.h | 2 +
2 files changed, 110 insertions(+), 1 deletions(-)
diff --git a/drivers/regulator/mc13783-regulator.c b/drivers/regulator/mc13783-regulator.c
index a40e35a..555eb29 100644
--- a/drivers/regulator/mc13783-regulator.c
+++ b/drivers/regulator/mc13783-regulator.c
@@ -82,6 +82,11 @@
#define MC13783_REG_POWERMISC_GPO2EN (1 << 8)
#define MC13783_REG_POWERMISC_GPO3EN (1 << 10)
#define MC13783_REG_POWERMISC_GPO4EN (1 << 12)
+#define MC13783_REG_POWERMISC_PWGT1SPIEN (1 << 15)
+#define MC13783_REG_POWERMISC_PWGT2SPIEN (1 << 16)
+
+#define MC13783_REG_POWERMISC_PWGTSPI_M (3 << 15)
+
struct mc13783_regulator {
struct regulator_desc desc;
@@ -163,6 +168,7 @@ static const int const mc13783_vrf_val[] = {
static struct regulator_ops mc13783_regulator_ops;
static struct regulator_ops mc13783_fixed_regulator_ops;
+static struct regulator_ops mc13783_gpo_regulator_ops;
#define MC13783_DEFINE(prefix, _name, _reg, _vsel_reg, _voltages) \
[MC13783_ ## prefix ## _ ## _name] = { \
@@ -201,7 +207,7 @@ static struct regulator_ops mc13783_fixed_regulator_ops;
[MC13783_ ## prefix ## _ ## _name] = { \
.desc = { \
.name = #prefix "_" #_name, \
- .ops = &mc13783_regulator_ops, \
+ .ops = &mc13783_gpo_regulator_ops, \
.type = REGULATOR_VOLTAGE, \
.id = MC13783_ ## prefix ## _ ## _name, \
.owner = THIS_MODULE, \
@@ -253,10 +259,13 @@ static struct mc13783_regulator mc13783_regulators[] = {
MC13783_GPO_DEFINE(REGU, GPO2, POWERMISC),
MC13783_GPO_DEFINE(REGU, GPO3, POWERMISC),
MC13783_GPO_DEFINE(REGU, GPO4, POWERMISC),
+ MC13783_GPO_DEFINE(REGU, PWGT1SPI, POWERMISC),
+ MC13783_GPO_DEFINE(REGU, PWGT2SPI, POWERMISC),
};
struct mc13783_regulator_priv {
struct mc13783 *mc13783;
+ u32 powermisc_pwgt_state;
struct regulator_dev *regulators[];
};
@@ -445,6 +454,104 @@ static struct regulator_ops mc13783_fixed_regulator_ops = {
.get_voltage = mc13783_fixed_regulator_get_voltage,
};
+int mc13783_powermisc_rmw(struct mc13783_regulator_priv *priv, u32 mask,
+ u32 val)
+{
+ struct mc13783 *mc13783 = priv->mc13783;
+ int ret;
+ u32 valread;
+
+ BUG_ON(val & ~mask);
+
+ ret = mc13783_reg_read(mc13783, MC13783_REG_POWERMISC, &valread);
+ if (ret)
+ return ret;
+
+ /* Update the stored state for Power Gates. */
+ priv->powermisc_pwgt_state =
+ (priv->powermisc_pwgt_state & ~mask) | val;
+ priv->powermisc_pwgt_state &= MC13783_REG_POWERMISC_PWGTSPI_M;
+
+ /* Construct the new register value */
+ valread = (valread & ~mask) | val;
+ /* Overwrite the PWGTxEN with the stored version */
+ valread = (valread & ~MC13783_REG_POWERMISC_PWGTSPI_M) |
+ priv->powermisc_pwgt_state;
+
+ return mc13783_reg_write(mc13783, MC13783_REG_POWERMISC, valread);
+}
+
+static int mc13783_gpo_regulator_enable(struct regulator_dev *rdev)
+{
+ struct mc13783_regulator_priv *priv = rdev_get_drvdata(rdev);
+ int id = rdev_get_id(rdev);
+ int ret;
+ u32 en_val = mc13783_regulators[id].enable_bit;
+
+ dev_dbg(rdev_get_dev(rdev), "%s id: %d\n", __func__, id);
+
+ /* Power Gate enable value is 0 */
+ if (id == MC13783_REGU_PWGT1SPI ||
+ id == MC13783_REGU_PWGT2SPI)
+ en_val = 0;
+
+ mc13783_lock(priv->mc13783);
+ ret = mc13783_powermisc_rmw(priv, mc13783_regulators[id].enable_bit,
+ en_val);
+ mc13783_unlock(priv->mc13783);
+
+ return ret;
+}
+
+static int mc13783_gpo_regulator_disable(struct regulator_dev *rdev)
+{
+ struct mc13783_regulator_priv *priv = rdev_get_drvdata(rdev);
+ int id = rdev_get_id(rdev);
+ int ret;
+ u32 dis_val = 0;
+
+ dev_dbg(rdev_get_dev(rdev), "%s id: %d\n", __func__, id);
+
+ /* Power Gate disable value is 1 */
+ if (id == MC13783_REGU_PWGT1SPI ||
+ id == MC13783_REGU_PWGT2SPI)
+ dis_val = mc13783_regulators[id].enable_bit;
+
+ mc13783_lock(priv->mc13783);
+ ret = mc13783_powermisc_rmw(priv, mc13783_regulators[id].enable_bit,
+ dis_val);
+ mc13783_unlock(priv->mc13783);
+
+ return ret;
+}
+
+static int mc13783_gpo_regulator_is_enabled(struct regulator_dev *rdev)
+{
+ struct mc13783_regulator_priv *priv = rdev_get_drvdata(rdev);
+ int ret, id = rdev_get_id(rdev);
+ unsigned int val;
+
+ mc13783_lock(priv->mc13783);
+ ret = mc13783_reg_read(priv->mc13783, mc13783_regulators[id].reg, &val);
+ mc13783_unlock(priv->mc13783);
+
+ if (ret)
+ return ret;
+
+ /* Power Gates state is stored in powermisc_pwgt_state
+ * where the meaning of bits is negated */
+ val = (val & ~MC13783_REG_POWERMISC_PWGTSPI_M) |
+ (priv->powermisc_pwgt_state ^ MC13783_REG_POWERMISC_PWGTSPI_M);
+
+ return (val & mc13783_regulators[id].enable_bit) != 0;
+}
+
+static struct regulator_ops mc13783_gpo_regulator_ops = {
+ .enable = mc13783_gpo_regulator_enable,
+ .disable = mc13783_gpo_regulator_disable,
+ .is_enabled = mc13783_gpo_regulator_is_enabled,
+};
+
static int __devinit mc13783_regulator_probe(struct platform_device *pdev)
{
struct mc13783_regulator_priv *priv;
diff --git a/include/linux/mfd/mc13783.h b/include/linux/mfd/mc13783.h
index 3568040..94cb51a 100644
--- a/include/linux/mfd/mc13783.h
+++ b/include/linux/mfd/mc13783.h
@@ -108,6 +108,8 @@ int mc13783_adc_do_conversion(struct mc13783 *mc13783, unsigned int mode,
#define MC13783_REGU_V2 28
#define MC13783_REGU_V3 29
#define MC13783_REGU_V4 30
+#define MC13783_REGU_PWGT1SPI 31
+#define MC13783_REGU_PWGT2SPI 32
#define MC13783_IRQ_ADCDONE 0
#define MC13783_IRQ_ADCBISDONE 1
--
1.6.3.3
Looks a lot better. Just one question below.
On Mon, 2010-01-18 at 22:01 +0100, Alberto Panizzo wrote:
> On lun, 2010-01-18 at 20:04 +0100, Uwe Kleine-König wrote:
> > Hello Alberto,
> Hello Uwe!
>
> >
> > Can you post an updated patch addressing the discussion with Mark,
> > please?
> >
> > From the first look I don't like the name
> > MC13783_REG_POWERMISC_PWGTSPI_M as I don't understand it's purpose
>
> >From now the present driver naming schema is mask ends in _M.
>
> >
> > What is the base for your patch? (Hm, it seems next could work.
>
> Yes, this patch is based on:
> git://git.kernel.org/pub/scm/linux/kernel/git/lrg/voltage-2.6.git for-next
>
> > mc13783-regulator seems to have gotten some more #defines ending in _M.
> > You seem to mean "mask". IMHO it's a bit unfortunate, because the
> > nameing scheme doesn't match the already existing names :-()
>
> If the naming schema adopted, conflicts other I will propose a correction..
>
> >
> > Shouldn't mc13783_state_powermisc_pwgt be a per-device variable instead
> > of a static variable?
>
> Thanks for point me out this. It should.
>
>
> the new patch below:
>
> --------------------------------------------------------------------------
> GPO regulators are digital outputs that can be enabled or disabled by a
> dedicated bit in mc13783 POWERMISC register.
> In this family can be count in also Power Gates (PWGT1 and 2): enabled by
> a dedicated pin a Power Gate is an hardware driven supply where the output
> (PWGTnDRV) follow this law:
>
> Bit PWGTxSPIEN | Pin PWGTxEN | PWGTxDRV | Read Back
> 0 = default | | | PWGTxSPIEN
> ---------------+-------------+----------+------------
> 1 | x | Low | 0
> 0 | 0 | High | 1
> 0 | 1 | Low | 0
>
> As read back value of control bit reflects the PWGTxDRV state (not the
> control value previously written) and mc13783 POWERMISC register contain
> only regulator related bits, a dedicated function to manage these bits is
> created here with the aim of tracing the real value of PWGTxSPIEN bits
> and reproduce it on next writes.
>
> All POWERMISC users _must_ use the new function to not accidentally
> disable Power Gates supplies.
>
> Signed-off-by: Alberto Panizzo <[email protected]>
> ---
> drivers/regulator/mc13783-regulator.c | 109 ++++++++++++++++++++++++++++++++-
> include/linux/mfd/mc13783.h | 2 +
> 2 files changed, 110 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/regulator/mc13783-regulator.c b/drivers/regulator/mc13783-regulator.c
> index a40e35a..555eb29 100644
> --- a/drivers/regulator/mc13783-regulator.c
> +++ b/drivers/regulator/mc13783-regulator.c
> @@ -82,6 +82,11 @@
> #define MC13783_REG_POWERMISC_GPO2EN (1 << 8)
> #define MC13783_REG_POWERMISC_GPO3EN (1 << 10)
> #define MC13783_REG_POWERMISC_GPO4EN (1 << 12)
> +#define MC13783_REG_POWERMISC_PWGT1SPIEN (1 << 15)
> +#define MC13783_REG_POWERMISC_PWGT2SPIEN (1 << 16)
> +
> +#define MC13783_REG_POWERMISC_PWGTSPI_M (3 << 15)
> +
>
> struct mc13783_regulator {
> struct regulator_desc desc;
> @@ -163,6 +168,7 @@ static const int const mc13783_vrf_val[] = {
>
> static struct regulator_ops mc13783_regulator_ops;
> static struct regulator_ops mc13783_fixed_regulator_ops;
> +static struct regulator_ops mc13783_gpo_regulator_ops;
>
> #define MC13783_DEFINE(prefix, _name, _reg, _vsel_reg, _voltages) \
> [MC13783_ ## prefix ## _ ## _name] = { \
> @@ -201,7 +207,7 @@ static struct regulator_ops mc13783_fixed_regulator_ops;
> [MC13783_ ## prefix ## _ ## _name] = { \
> .desc = { \
> .name = #prefix "_" #_name, \
> - .ops = &mc13783_regulator_ops, \
> + .ops = &mc13783_gpo_regulator_ops, \
> .type = REGULATOR_VOLTAGE, \
> .id = MC13783_ ## prefix ## _ ## _name, \
> .owner = THIS_MODULE, \
> @@ -253,10 +259,13 @@ static struct mc13783_regulator mc13783_regulators[] = {
> MC13783_GPO_DEFINE(REGU, GPO2, POWERMISC),
> MC13783_GPO_DEFINE(REGU, GPO3, POWERMISC),
> MC13783_GPO_DEFINE(REGU, GPO4, POWERMISC),
> + MC13783_GPO_DEFINE(REGU, PWGT1SPI, POWERMISC),
> + MC13783_GPO_DEFINE(REGU, PWGT2SPI, POWERMISC),
> };
>
> struct mc13783_regulator_priv {
> struct mc13783 *mc13783;
> + u32 powermisc_pwgt_state;
> struct regulator_dev *regulators[];
> };
>
> @@ -445,6 +454,104 @@ static struct regulator_ops mc13783_fixed_regulator_ops = {
> .get_voltage = mc13783_fixed_regulator_get_voltage,
> };
>
> +int mc13783_powermisc_rmw(struct mc13783_regulator_priv *priv, u32 mask,
> + u32 val)
> +{
> + struct mc13783 *mc13783 = priv->mc13783;
> + int ret;
> + u32 valread;
> +
> + BUG_ON(val & ~mask);
> +
> + ret = mc13783_reg_read(mc13783, MC13783_REG_POWERMISC, &valread);
> + if (ret)
> + return ret;
> +
> + /* Update the stored state for Power Gates. */
> + priv->powermisc_pwgt_state =
> + (priv->powermisc_pwgt_state & ~mask) | val;
> + priv->powermisc_pwgt_state &= MC13783_REG_POWERMISC_PWGTSPI_M;
> +
> + /* Construct the new register value */
> + valread = (valread & ~mask) | val;
> + /* Overwrite the PWGTxEN with the stored version */
> + valread = (valread & ~MC13783_REG_POWERMISC_PWGTSPI_M) |
> + priv->powermisc_pwgt_state;
> +
> + return mc13783_reg_write(mc13783, MC13783_REG_POWERMISC, valread);
> +}
> +
> +static int mc13783_gpo_regulator_enable(struct regulator_dev *rdev)
> +{
> + struct mc13783_regulator_priv *priv = rdev_get_drvdata(rdev);
> + int id = rdev_get_id(rdev);
> + int ret;
> + u32 en_val = mc13783_regulators[id].enable_bit;
> +
> + dev_dbg(rdev_get_dev(rdev), "%s id: %d\n", __func__, id);
> +
> + /* Power Gate enable value is 0 */
> + if (id == MC13783_REGU_PWGT1SPI ||
> + id == MC13783_REGU_PWGT2SPI)
> + en_val = 0;
> +
> + mc13783_lock(priv->mc13783);
> + ret = mc13783_powermisc_rmw(priv, mc13783_regulators[id].enable_bit,
> + en_val);
> + mc13783_unlock(priv->mc13783);
> +
> + return ret;
> +}
> +
> +static int mc13783_gpo_regulator_disable(struct regulator_dev *rdev)
> +{
> + struct mc13783_regulator_priv *priv = rdev_get_drvdata(rdev);
> + int id = rdev_get_id(rdev);
> + int ret;
> + u32 dis_val = 0;
> +
> + dev_dbg(rdev_get_dev(rdev), "%s id: %d\n", __func__, id);
> +
> + /* Power Gate disable value is 1 */
> + if (id == MC13783_REGU_PWGT1SPI ||
> + id == MC13783_REGU_PWGT2SPI)
> + dis_val = mc13783_regulators[id].enable_bit;
> +
> + mc13783_lock(priv->mc13783);
> + ret = mc13783_powermisc_rmw(priv, mc13783_regulators[id].enable_bit,
> + dis_val);
> + mc13783_unlock(priv->mc13783);
> +
> + return ret;
> +}
> +
> +static int mc13783_gpo_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> + struct mc13783_regulator_priv *priv = rdev_get_drvdata(rdev);
> + int ret, id = rdev_get_id(rdev);
> + unsigned int val;
> +
> + mc13783_lock(priv->mc13783);
> + ret = mc13783_reg_read(priv->mc13783, mc13783_regulators[id].reg, &val);
> + mc13783_unlock(priv->mc13783);
> +
> + if (ret)
> + return ret;
> +
> + /* Power Gates state is stored in powermisc_pwgt_state
> + * where the meaning of bits is negated */
> + val = (val & ~MC13783_REG_POWERMISC_PWGTSPI_M) |
> + (priv->powermisc_pwgt_state ^ MC13783_REG_POWERMISC_PWGTSPI_M);
> +
> + return (val & mc13783_regulators[id].enable_bit) != 0;
> +}
> +
> +static struct regulator_ops mc13783_gpo_regulator_ops = {
> + .enable = mc13783_gpo_regulator_enable,
> + .disable = mc13783_gpo_regulator_disable,
> + .is_enabled = mc13783_gpo_regulator_is_enabled,
How is the voltage output on the GPO regulator governed. Is it
controlled by a parent regulator or fixed ?
We probably want a way to query the voltage here or specify the parent
relationship.
> +};
> +
> static int __devinit mc13783_regulator_probe(struct platform_device *pdev)
> {
> struct mc13783_regulator_priv *priv;
> diff --git a/include/linux/mfd/mc13783.h b/include/linux/mfd/mc13783.h
> index 3568040..94cb51a 100644
> --- a/include/linux/mfd/mc13783.h
> +++ b/include/linux/mfd/mc13783.h
> @@ -108,6 +108,8 @@ int mc13783_adc_do_conversion(struct mc13783 *mc13783, unsigned int mode,
> #define MC13783_REGU_V2 28
> #define MC13783_REGU_V3 29
> #define MC13783_REGU_V4 30
> +#define MC13783_REGU_PWGT1SPI 31
> +#define MC13783_REGU_PWGT2SPI 32
>
> #define MC13783_IRQ_ADCDONE 0
> #define MC13783_IRQ_ADCBISDONE 1
On mar, 2010-01-19 at 10:26 +0000, Liam Girdwood wrote:
> > +static struct regulator_ops mc13783_gpo_regulator_ops = {
> > + .enable = mc13783_gpo_regulator_enable,
> > + .disable = mc13783_gpo_regulator_disable,
> > + .is_enabled = mc13783_gpo_regulator_is_enabled,
>
> How is the voltage output on the GPO regulator governed. Is it
> controlled by a parent regulator or fixed ?
>
> We probably want a way to query the voltage here or specify the parent
> relationship.
Good question.
>From datasheet GPO's are marked as LV (Low Voltage)-> 3.1V
Power Gates Driver are marked as EMV (Extended Medium Voltage) -> 5.5V
and no way to modify that -> they are fixed regulators.
I will add voltage query capability.