2012-06-13 03:27:18

by Axel Lin

[permalink] [raw]
Subject: [PATCH RFT] regulator: tps65217: Convert to regulator_[is_enabled|get_voltage_sel]_regmap

This patch converts .is_enabled and .get_voltage_sel to
regulator_is_enabled_regmap and regulator_get_voltage_sel_regmap.

For .enable, .disable, and .set_voltage_sel, the write protect level is either
1 or 2. So we cannot use regulator_[enable|disable|set_voltage_sel]_regmap.

Now we store the enable reg/mask and vsel reg/mask in regulator_desc,
so we can remove enable_mask, set_vout_reg, and set_vout_mask from
struct tps_info.

Signed-off-by: Axel Lin <[email protected]>
---
Hi AnilKumar,
Any chance to test this patch on your hardware?

Thanks,
Axel

drivers/regulator/tps65217-regulator.c | 114 ++++++++++++--------------------
include/linux/mfd/tps65217.h | 6 --
2 files changed, 42 insertions(+), 78 deletions(-)

diff --git a/drivers/regulator/tps65217-regulator.c b/drivers/regulator/tps65217-regulator.c
index 9d371d2..f5fa05b 100644
--- a/drivers/regulator/tps65217-regulator.c
+++ b/drivers/regulator/tps65217-regulator.c
@@ -26,7 +26,7 @@
#include <linux/regulator/machine.h>
#include <linux/mfd/tps65217.h>

-#define TPS65217_REGULATOR(_name, _id, _ops, _n) \
+#define TPS65217_REGULATOR(_name, _id, _ops, _n, _vr, _vm, _em) \
{ \
.name = _name, \
.id = _id, \
@@ -34,9 +34,13 @@
.n_voltages = _n, \
.type = REGULATOR_VOLTAGE, \
.owner = THIS_MODULE, \
+ .vsel_reg = _vr, \
+ .vsel_mask = _vm, \
+ .enable_reg = TPS65217_REG_ENABLE, \
+ .enable_mask = _em, \
} \

-#define TPS65217_INFO(_nm, _min, _max, _f1, _f2, _t, _n, _em, _vr, _vm) \
+#define TPS65217_INFO(_nm, _min, _max, _f1, _f2, _t, _n)\
{ \
.name = _nm, \
.min_uV = _min, \
@@ -45,9 +49,6 @@
.uv_to_vsel = _f2, \
.table = _t, \
.table_len = _n, \
- .enable_mask = _em, \
- .set_vout_reg = _vr, \
- .set_vout_mask = _vm, \
}

static const int LDO1_VSEL_table[] = {
@@ -127,46 +128,21 @@ static int tps65217_uv_to_vsel2(int uV, unsigned int *vsel)

static struct tps_info tps65217_pmic_regs[] = {
TPS65217_INFO("DCDC1", 900000, 1800000, tps65217_vsel_to_uv1,
- tps65217_uv_to_vsel1, NULL, 64, TPS65217_ENABLE_DC1_EN,
- TPS65217_REG_DEFDCDC1, TPS65217_DEFDCDCX_DCDC_MASK),
+ tps65217_uv_to_vsel1, NULL, 64),
TPS65217_INFO("DCDC2", 900000, 3300000, tps65217_vsel_to_uv1,
- tps65217_uv_to_vsel1, NULL, 64, TPS65217_ENABLE_DC2_EN,
- TPS65217_REG_DEFDCDC2, TPS65217_DEFDCDCX_DCDC_MASK),
+ tps65217_uv_to_vsel1, NULL, 64),
TPS65217_INFO("DCDC3", 900000, 1500000, tps65217_vsel_to_uv1,
- tps65217_uv_to_vsel1, NULL, 64, TPS65217_ENABLE_DC3_EN,
- TPS65217_REG_DEFDCDC3, TPS65217_DEFDCDCX_DCDC_MASK),
+ tps65217_uv_to_vsel1, NULL, 64),
TPS65217_INFO("LDO1", 1000000, 3300000, NULL, NULL, LDO1_VSEL_table,
- 16, TPS65217_ENABLE_LDO1_EN, TPS65217_REG_DEFLDO1,
- TPS65217_DEFLDO1_LDO1_MASK),
+ 16),
TPS65217_INFO("LDO2", 900000, 3300000, tps65217_vsel_to_uv1,
- tps65217_uv_to_vsel1, NULL, 64, TPS65217_ENABLE_LDO2_EN,
- TPS65217_REG_DEFLDO2, TPS65217_DEFLDO2_LDO2_MASK),
+ tps65217_uv_to_vsel1, NULL, 64),
TPS65217_INFO("LDO3", 1800000, 3300000, tps65217_vsel_to_uv2,
- tps65217_uv_to_vsel2, NULL, 32,
- TPS65217_ENABLE_LS1_EN | TPS65217_DEFLDO3_LDO3_EN,
- TPS65217_REG_DEFLS1, TPS65217_DEFLDO3_LDO3_MASK),
+ tps65217_uv_to_vsel2, NULL, 32),
TPS65217_INFO("LDO4", 1800000, 3300000, tps65217_vsel_to_uv2,
- tps65217_uv_to_vsel2, NULL, 32,
- TPS65217_ENABLE_LS2_EN | TPS65217_DEFLDO4_LDO4_EN,
- TPS65217_REG_DEFLS2, TPS65217_DEFLDO4_LDO4_MASK),
+ tps65217_uv_to_vsel2, NULL, 32),
};

-static int tps65217_pmic_is_enabled(struct regulator_dev *dev)
-{
- int ret;
- struct tps65217 *tps = rdev_get_drvdata(dev);
- unsigned int data, rid = rdev_get_id(dev);
-
- if (rid < TPS65217_DCDC_1 || rid > TPS65217_LDO_4)
- return -EINVAL;
-
- ret = tps65217_reg_read(tps, TPS65217_REG_ENABLE, &data);
- if (ret)
- return ret;
-
- return (data & tps->info[rid]->enable_mask) ? 1 : 0;
-}
-
static int tps65217_pmic_enable(struct regulator_dev *dev)
{
struct tps65217 *tps = rdev_get_drvdata(dev);
@@ -177,9 +153,8 @@ static int tps65217_pmic_enable(struct regulator_dev *dev)

/* Enable the regulator and password protection is level 1 */
return tps65217_set_bits(tps, TPS65217_REG_ENABLE,
- tps->info[rid]->enable_mask,
- tps->info[rid]->enable_mask,
- TPS65217_PROTECT_L1);
+ dev->desc->enable_mask, dev->desc->enable_mask,
+ TPS65217_PROTECT_L1);
}

static int tps65217_pmic_disable(struct regulator_dev *dev)
@@ -192,25 +167,7 @@ static int tps65217_pmic_disable(struct regulator_dev *dev)

/* Disable the regulator and password protection is level 1 */
return tps65217_clear_bits(tps, TPS65217_REG_ENABLE,
- tps->info[rid]->enable_mask, TPS65217_PROTECT_L1);
-}
-
-static int tps65217_pmic_get_voltage_sel(struct regulator_dev *dev)
-{
- int ret;
- struct tps65217 *tps = rdev_get_drvdata(dev);
- unsigned int selector, rid = rdev_get_id(dev);
-
- if (rid < TPS65217_DCDC_1 || rid > TPS65217_LDO_4)
- return -EINVAL;
-
- ret = tps65217_reg_read(tps, tps->info[rid]->set_vout_reg, &selector);
- if (ret)
- return ret;
-
- selector &= tps->info[rid]->set_vout_mask;
-
- return selector;
+ dev->desc->enable_mask, TPS65217_PROTECT_L1);
}

static int tps65217_pmic_set_voltage_sel(struct regulator_dev *dev,
@@ -221,8 +178,7 @@ static int tps65217_pmic_set_voltage_sel(struct regulator_dev *dev,
unsigned int rid = rdev_get_id(dev);

/* Set the voltage based on vsel value and write protect level is 2 */
- ret = tps65217_set_bits(tps, tps->info[rid]->set_vout_reg,
- tps->info[rid]->set_vout_mask,
+ ret = tps65217_set_bits(tps, dev->desc->vsel_reg, dev->desc->vsel_mask,
selector, TPS65217_PROTECT_L2);

/* Set GO bit for DCDCx to initiate voltage transistion */
@@ -285,10 +241,10 @@ static int tps65217_pmic_list_voltage(struct regulator_dev *dev,

/* Operations permitted on DCDCx, LDO2, LDO3 and LDO4 */
static struct regulator_ops tps65217_pmic_ops = {
- .is_enabled = tps65217_pmic_is_enabled,
+ .is_enabled = regulator_is_enabled_regmap,
.enable = tps65217_pmic_enable,
.disable = tps65217_pmic_disable,
- .get_voltage_sel = tps65217_pmic_get_voltage_sel,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
.set_voltage_sel = tps65217_pmic_set_voltage_sel,
.list_voltage = tps65217_pmic_list_voltage,
.map_voltage = tps65217_pmic_map_voltage,
@@ -296,22 +252,36 @@ static struct regulator_ops tps65217_pmic_ops = {

/* Operations permitted on LDO1 */
static struct regulator_ops tps65217_pmic_ldo1_ops = {
- .is_enabled = tps65217_pmic_is_enabled,
+ .is_enabled = regulator_is_enabled_regmap,
.enable = tps65217_pmic_enable,
.disable = tps65217_pmic_disable,
- .get_voltage_sel = tps65217_pmic_get_voltage_sel,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
.set_voltage_sel = tps65217_pmic_set_voltage_sel,
.list_voltage = tps65217_pmic_list_voltage,
};

static const struct regulator_desc regulators[] = {
- TPS65217_REGULATOR("DCDC1", TPS65217_DCDC_1, tps65217_pmic_ops, 64),
- TPS65217_REGULATOR("DCDC2", TPS65217_DCDC_2, tps65217_pmic_ops, 64),
- TPS65217_REGULATOR("DCDC3", TPS65217_DCDC_3, tps65217_pmic_ops, 64),
- TPS65217_REGULATOR("LDO1", TPS65217_LDO_1, tps65217_pmic_ldo1_ops, 16),
- TPS65217_REGULATOR("LDO2", TPS65217_LDO_2, tps65217_pmic_ops, 64),
- TPS65217_REGULATOR("LDO3", TPS65217_LDO_3, tps65217_pmic_ops, 32),
- TPS65217_REGULATOR("LDO4", TPS65217_LDO_4, tps65217_pmic_ops, 32),
+ TPS65217_REGULATOR("DCDC1", TPS65217_DCDC_1, tps65217_pmic_ops, 64,
+ TPS65217_REG_DEFDCDC1, TPS65217_DEFDCDCX_DCDC_MASK,
+ TPS65217_ENABLE_DC1_EN),
+ TPS65217_REGULATOR("DCDC2", TPS65217_DCDC_2, tps65217_pmic_ops, 64,
+ TPS65217_REG_DEFDCDC2, TPS65217_DEFDCDCX_DCDC_MASK,
+ TPS65217_ENABLE_DC2_EN),
+ TPS65217_REGULATOR("DCDC3", TPS65217_DCDC_3, tps65217_pmic_ops, 64,
+ TPS65217_REG_DEFDCDC3, TPS65217_DEFDCDCX_DCDC_MASK,
+ TPS65217_ENABLE_DC3_EN),
+ TPS65217_REGULATOR("LDO1", TPS65217_LDO_1, tps65217_pmic_ldo1_ops, 16,
+ TPS65217_REG_DEFLDO1, TPS65217_DEFLDO1_LDO1_MASK,
+ TPS65217_ENABLE_LDO1_EN),
+ TPS65217_REGULATOR("LDO2", TPS65217_LDO_2, tps65217_pmic_ops, 64,
+ TPS65217_REG_DEFLDO2, TPS65217_DEFLDO2_LDO2_MASK,
+ TPS65217_ENABLE_LDO2_EN),
+ TPS65217_REGULATOR("LDO3", TPS65217_LDO_3, tps65217_pmic_ops, 32,
+ TPS65217_REG_DEFLS1, TPS65217_DEFLDO3_LDO3_MASK,
+ TPS65217_ENABLE_LS1_EN | TPS65217_DEFLDO3_LDO3_EN),
+ TPS65217_REGULATOR("LDO4", TPS65217_LDO_4, tps65217_pmic_ops, 32,
+ TPS65217_REG_DEFLS2, TPS65217_DEFLDO4_LDO4_MASK,
+ TPS65217_ENABLE_LS2_EN | TPS65217_DEFLDO4_LDO4_EN),
};

static int __devinit tps65217_regulator_probe(struct platform_device *pdev)
diff --git a/include/linux/mfd/tps65217.h b/include/linux/mfd/tps65217.h
index e030ef9..4e035a4 100644
--- a/include/linux/mfd/tps65217.h
+++ b/include/linux/mfd/tps65217.h
@@ -229,9 +229,6 @@ struct tps65217_board {
* @uv_to_vsel: Function pointer to get selector from voltage
* @table: Table for non-uniform voltage step-size
* @table_len: Length of the voltage table
- * @enable_mask: Regulator enable mask bits
- * @set_vout_reg: Regulator output voltage set register
- * @set_vout_mask: Regulator output voltage set mask
*
* This data is used to check the regualtor voltage limits while setting.
*/
@@ -243,9 +240,6 @@ struct tps_info {
int (*uv_to_vsel)(int uV, unsigned int *vsel);
const int *table;
unsigned int table_len;
- unsigned int enable_mask;
- unsigned int set_vout_reg;
- unsigned int set_vout_mask;
};

/**
--
1.7.9.5



2012-06-13 09:09:35

by AnilKumar, Chimata

[permalink] [raw]
Subject: RE: [PATCH RFT] regulator: tps65217: Convert to regulator_[is_enabled|get_voltage_sel]_regmap

Hi Axel,

On Wed, Jun 13, 2012 at 08:57:11, Axel Lin wrote:
> This patch converts .is_enabled and .get_voltage_sel to
> regulator_is_enabled_regmap and regulator_get_voltage_sel_regmap.
>
> For .enable, .disable, and .set_voltage_sel, the write protect level is either
> 1 or 2. So we cannot use regulator_[enable|disable|set_voltage_sel]_regmap.
>
> Now we store the enable reg/mask and vsel reg/mask in regulator_desc,
> so we can remove enable_mask, set_vout_reg, and set_vout_mask from
> struct tps_info.
>
> Signed-off-by: Axel Lin <[email protected]>
> ---
> Hi AnilKumar,
> Any chance to test this patch on your hardware?

I will give the status in a day or two.

Regards
AnilKumar-

2012-06-13 17:58:46

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH RFT] regulator: tps65217: Convert to regulator_[is_enabled|get_voltage_sel]_regmap

On Wed, Jun 13, 2012 at 11:27:11AM +0800, Axel Lin wrote:
> This patch converts .is_enabled and .get_voltage_sel to
> regulator_is_enabled_regmap and regulator_get_voltage_sel_regmap.

Applied, thanks.


Attachments:
(No filename) (202.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2012-06-18 11:48:42

by AnilKumar, Chimata

[permalink] [raw]
Subject: RE: [PATCH RFT] regulator: tps65217: Convert to regulator_[is_enabled|get_voltage_sel]_regmap


Hi Axel,

Thanks for the patch,

On Wed, Jun 13, 2012 at 14:39:27, AnilKumar, Chimata wrote:
> Hi Axel,
>
> On Wed, Jun 13, 2012 at 08:57:11, Axel Lin wrote:
> > This patch converts .is_enabled and .get_voltage_sel to
> > regulator_is_enabled_regmap and regulator_get_voltage_sel_regmap.
> >
> > For .enable, .disable, and .set_voltage_sel, the write protect level is either
> > 1 or 2. So we cannot use regulator_[enable|disable|set_voltage_sel]_regmap.
> >
> > Now we store the enable reg/mask and vsel reg/mask in regulator_desc,
> > so we can remove enable_mask, set_vout_reg, and set_vout_mask from
> > struct tps_info.
> >
> > Signed-off-by: Axel Lin <[email protected]>
> > ---
> > Hi AnilKumar,
> > Any chance to test this patch on your hardware?
>
> I will give the status in a day or two.

Your patch seems to be working but we have to fix the below issue which is missed in
earlier patch "regulator: core: Use a struct to pass in regulator runtime configuration"
from Mark.

Basically regmap pointer is missed out in runtime configutaion data. Without this we
can see NULL pointer exception because config.regmap is NULL.

+ config.regmap = tps->regmap;

If this is required then same applicable to all the regulators which is modified for
"runtime config".

If not, I missed out some basic stuff here could you please tell me how regmap pointer
is used across the regulator in the current tree?

Regards
AnilKumar-

2012-06-18 11:56:21

by Axel Lin

[permalink] [raw]
Subject: Re: [PATCH RFT] regulator: tps65217: Convert to regulator_[is_enabled|get_voltage_sel]_regmap

> Basically regmap pointer is missed out in runtime configutaion data. Without this we
> can see NULL pointer exception because config.regmap is NULL.
>
> + ? ? ? config.regmap = tps->regmap;
>
Since this patch is target for 3.6, this is not required.
The regulator core will set it automatically.
In drivers/regulator/core.c
if (config->regmap)
rdev->regmap = config->regmap;
else
rdev->regmap = dev_get_regmap(dev, NULL);


Regards,
Axel

2012-06-18 12:02:03

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH RFT] regulator: tps65217: Convert to regulator_[is_enabled|get_voltage_sel]_regmap

On Mon, Jun 18, 2012 at 07:55:59PM +0800, Axel Lin wrote:

> Since this patch is target for 3.6, this is not required.
> The regulator core will set it automatically.

Do we need to fix it for 3.5?


Attachments:
(No filename) (198.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2012-06-18 12:05:38

by AnilKumar, Chimata

[permalink] [raw]
Subject: RE: [PATCH RFT] regulator: tps65217: Convert to regulator_[is_enabled|get_voltage_sel]_regmap

Hi Mark,

On Mon, Jun 18, 2012 at 17:31:58, Mark Brown wrote:
> On Mon, Jun 18, 2012 at 07:55:59PM +0800, Axel Lin wrote:
>
> > Since this patch is target for 3.6, this is not required.
> > The regulator core will set it automatically.
>
> Do we need to fix it for 3.5?

Yes, it's required for 3.5.

Regards
AnilKumar

2012-06-18 12:06:36

by Axel Lin

[permalink] [raw]
Subject: Re: [PATCH RFT] regulator: tps65217: Convert to regulator_[is_enabled|get_voltage_sel]_regmap

2012/6/18 Mark Brown <[email protected]>:
> On Mon, Jun 18, 2012 at 07:55:59PM +0800, Axel Lin wrote:
>
>> Since this patch is target for 3.6, this is not required.
>> The regulator core will set it automatically.
>
> Do we need to fix it for 3.5?
No. In current Linus' tree, this driver does not use
regulator_[is_enabled|get_voltage_sel]_regmap.

Regards,
Axel

2012-06-18 12:15:08

by AnilKumar, Chimata

[permalink] [raw]
Subject: RE: [PATCH RFT] regulator: tps65217: Convert to regulator_[is_enabled|get_voltage_sel]_regmap

Hi Axel,

On Mon, Jun 18, 2012 at 17:36:14, Axel Lin wrote:
> 2012/6/18 Mark Brown <[email protected]>:
> > On Mon, Jun 18, 2012 at 07:55:59PM +0800, Axel Lin wrote:
> >
> >> Since this patch is target for 3.6, this is not required.
> >> The regulator core will set it automatically.
> >
> > Do we need to fix it for 3.5?
> No. In current Linus' tree, this driver does not use
> regulator_[is_enabled|get_voltage_sel]_regmap.
>

Yes, I understood. This fix is not required in 3.5.

If we use *_voltage_sel_regmap APIs then this regmap pointer is required
otherwise not required because regulator APIs passing proper regmap pointers.

Thanks for your clarification.

Regards
AnilKumar

2012-06-18 12:39:46

by AnilKumar, Chimata

[permalink] [raw]
Subject: RE: [PATCH RFT] regulator: tps65217: Convert to regulator_[is_enabled|get_voltage_sel]_regmap

Hi Axel,

Small correction in core file modification.

On Mon, Jun 18, 2012 at 17:25:59, Axel Lin wrote:
> > Basically regmap pointer is missed out in runtime configutaion data. Without this we
> > can see NULL pointer exception because config.regmap is NULL.
> >
> > + ? ? ? config.regmap = tps->regmap;
> >
> Since this patch is target for 3.6, this is not required.
> The regulator core will set it automatically.
> In drivers/regulator/core.c
> if (config->regmap)
> rdev->regmap = config->regmap;
> else
> rdev->regmap = dev_get_regmap(dev, NULL);
>

+ if (config->regmap)
+ rdev->regmap = config->regmap;
+ else
+ rdev->regmap = dev_get_regmap(dev->parent, NULL);

Regards
AnilKumar-

2012-06-18 12:45:17

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH RFT] regulator: tps65217: Convert to regulator_[is_enabled|get_voltage_sel]_regmap

On Mon, Jun 18, 2012 at 12:39:38PM +0000, AnilKumar, Chimata wrote:

> + if (config->regmap)
> + rdev->regmap = config->regmap;
> + else
> + rdev->regmap = dev_get_regmap(dev->parent, NULL);

No, this would be broken. We're specifically using the device we got
passed in. In this case the fact that the regmap is on the MFD means
that the driver does need to explicitly set the regmap. Or we should
have this be a multi-stage series of checks:

if (config->regmap)
rdev->regmap = config->regmap;
else if (dev_get_regmap(dev, NULL))
rdev->regmap = dev_get_regmap(dev, NULL);
else if (dev->parent)
rdev->regmap = dev_get_regmap(dev->parent, NULL);

which should cover the MFD case if there's no regmap on the child
without having to go through all the drivers doing it by hand.


Attachments:
(No filename) (790.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2012-06-18 12:56:48

by AnilKumar, Chimata

[permalink] [raw]
Subject: RE: [PATCH RFT] regulator: tps65217: Convert to regulator_[is_enabled|get_voltage_sel]_regmap

On Mon, Jun 18, 2012 at 18:15:14, Mark Brown wrote:
> On Mon, Jun 18, 2012 at 12:39:38PM +0000, AnilKumar, Chimata wrote:
>
> > + if (config->regmap)
> > + rdev->regmap = config->regmap;
> > + else
> > + rdev->regmap = dev_get_regmap(dev->parent, NULL);
>
> No, this would be broken. We're specifically using the device we got
> passed in. In this case the fact that the regmap is on the MFD means
> that the driver does need to explicitly set the regmap. Or we should
> have this be a multi-stage series of checks:
>
> if (config->regmap)
> rdev->regmap = config->regmap;
> else if (dev_get_regmap(dev, NULL))
> rdev->regmap = dev_get_regmap(dev, NULL);
> else if (dev->parent)
> rdev->regmap = dev_get_regmap(dev->parent, NULL);
>
> which should cover the MFD case if there's no regmap on the child
> without having to go through all the drivers doing it by hand.
>

I missed out !regmap case.

I think it is better to set in regulator it-self instead of these checks + calls,
this adds extra burden. So, regmap pointer set should be added if we are adding
regulator_[is_enabled|get_voltage_sel]_regmap API support to the driver.

Regards
AnilKumar

2012-06-18 13:09:15

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH RFT] regulator: tps65217: Convert to regulator_[is_enabled|get_voltage_sel]_regmap

On Mon, Jun 18, 2012 at 12:56:40PM +0000, AnilKumar, Chimata wrote:

Please fix the word wrapping in your mailer, it looks like it's wrapping
at 81 characters or something.

> I think it is better to set in regulator it-self instead of these checks + calls,
> this adds extra burden. So, regmap pointer set should be added if we are adding
> regulator_[is_enabled|get_voltage_sel]_regmap API support to the driver.

Yeah, the goal here is to ensure that the common case can be (as far as
possible) data rather than code so if there's lots of drivers doing the
same thing the core should do it for them.


Attachments:
(No filename) (603.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2012-06-18 13:14:05

by AnilKumar, Chimata

[permalink] [raw]
Subject: RE: [PATCH RFT] regulator: tps65217: Convert to regulator_[is_enabled|get_voltage_sel]_regmap

Hi Axel,

Thanks for the patch.

Tested-by: AnilKumar Ch <[email protected]>

On Wed, Jun 13, 2012 at 08:57:11, Axel Lin wrote:
> This patch converts .is_enabled and .get_voltage_sel to
> regulator_is_enabled_regmap and regulator_get_voltage_sel_regmap.
>
> For .enable, .disable, and .set_voltage_sel, the write protect level is either
> 1 or 2. So we cannot use regulator_[enable|disable|set_voltage_sel]_regmap.
>
> Now we store the enable reg/mask and vsel reg/mask in regulator_desc,
> so we can remove enable_mask, set_vout_reg, and set_vout_mask from
> struct tps_info.
>
> Signed-off-by: Axel Lin <[email protected]>
> ---
> Hi AnilKumar,
> Any chance to test this patch on your hardware?
>
> Thanks,
> Axel
>
> drivers/regulator/tps65217-regulator.c | 114 ++++++++++++--------------------
> include/linux/mfd/tps65217.h | 6 --
> 2 files changed, 42 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/regulator/tps65217-regulator.c b/drivers/regulator/tps65217-regulator.c
> index 9d371d2..f5fa05b 100644
> --- a/drivers/regulator/tps65217-regulator.c
> +++ b/drivers/regulator/tps65217-regulator.c
> @@ -26,7 +26,7 @@
> #include <linux/regulator/machine.h>
> #include <linux/mfd/tps65217.h>
>
> -#define TPS65217_REGULATOR(_name, _id, _ops, _n) \
> +#define TPS65217_REGULATOR(_name, _id, _ops, _n, _vr, _vm, _em) \
> { \
> .name = _name, \
> .id = _id, \
> @@ -34,9 +34,13 @@
> .n_voltages = _n, \
> .type = REGULATOR_VOLTAGE, \
> .owner = THIS_MODULE, \
> + .vsel_reg = _vr, \
> + .vsel_mask = _vm, \
> + .enable_reg = TPS65217_REG_ENABLE, \
> + .enable_mask = _em, \
> } \
>
> -#define TPS65217_INFO(_nm, _min, _max, _f1, _f2, _t, _n, _em, _vr, _vm) \
> +#define TPS65217_INFO(_nm, _min, _max, _f1, _f2, _t, _n)\
> { \
> .name = _nm, \
> .min_uV = _min, \
> @@ -45,9 +49,6 @@
> .uv_to_vsel = _f2, \
> .table = _t, \
> .table_len = _n, \
> - .enable_mask = _em, \
> - .set_vout_reg = _vr, \
> - .set_vout_mask = _vm, \
> }
>
> static const int LDO1_VSEL_table[] = {
> @@ -127,46 +128,21 @@ static int tps65217_uv_to_vsel2(int uV, unsigned int *vsel)
>
> static struct tps_info tps65217_pmic_regs[] = {
> TPS65217_INFO("DCDC1", 900000, 1800000, tps65217_vsel_to_uv1,
> - tps65217_uv_to_vsel1, NULL, 64, TPS65217_ENABLE_DC1_EN,
> - TPS65217_REG_DEFDCDC1, TPS65217_DEFDCDCX_DCDC_MASK),
> + tps65217_uv_to_vsel1, NULL, 64),
> TPS65217_INFO("DCDC2", 900000, 3300000, tps65217_vsel_to_uv1,
> - tps65217_uv_to_vsel1, NULL, 64, TPS65217_ENABLE_DC2_EN,
> - TPS65217_REG_DEFDCDC2, TPS65217_DEFDCDCX_DCDC_MASK),
> + tps65217_uv_to_vsel1, NULL, 64),
> TPS65217_INFO("DCDC3", 900000, 1500000, tps65217_vsel_to_uv1,
> - tps65217_uv_to_vsel1, NULL, 64, TPS65217_ENABLE_DC3_EN,
> - TPS65217_REG_DEFDCDC3, TPS65217_DEFDCDCX_DCDC_MASK),
> + tps65217_uv_to_vsel1, NULL, 64),
> TPS65217_INFO("LDO1", 1000000, 3300000, NULL, NULL, LDO1_VSEL_table,
> - 16, TPS65217_ENABLE_LDO1_EN, TPS65217_REG_DEFLDO1,
> - TPS65217_DEFLDO1_LDO1_MASK),
> + 16),
> TPS65217_INFO("LDO2", 900000, 3300000, tps65217_vsel_to_uv1,
> - tps65217_uv_to_vsel1, NULL, 64, TPS65217_ENABLE_LDO2_EN,
> - TPS65217_REG_DEFLDO2, TPS65217_DEFLDO2_LDO2_MASK),
> + tps65217_uv_to_vsel1, NULL, 64),
> TPS65217_INFO("LDO3", 1800000, 3300000, tps65217_vsel_to_uv2,
> - tps65217_uv_to_vsel2, NULL, 32,
> - TPS65217_ENABLE_LS1_EN | TPS65217_DEFLDO3_LDO3_EN,
> - TPS65217_REG_DEFLS1, TPS65217_DEFLDO3_LDO3_MASK),
> + tps65217_uv_to_vsel2, NULL, 32),
> TPS65217_INFO("LDO4", 1800000, 3300000, tps65217_vsel_to_uv2,
> - tps65217_uv_to_vsel2, NULL, 32,
> - TPS65217_ENABLE_LS2_EN | TPS65217_DEFLDO4_LDO4_EN,
> - TPS65217_REG_DEFLS2, TPS65217_DEFLDO4_LDO4_MASK),
> + tps65217_uv_to_vsel2, NULL, 32),
> };
>
> -static int tps65217_pmic_is_enabled(struct regulator_dev *dev)
> -{
> - int ret;
> - struct tps65217 *tps = rdev_get_drvdata(dev);
> - unsigned int data, rid = rdev_get_id(dev);
> -
> - if (rid < TPS65217_DCDC_1 || rid > TPS65217_LDO_4)
> - return -EINVAL;
> -
> - ret = tps65217_reg_read(tps, TPS65217_REG_ENABLE, &data);
> - if (ret)
> - return ret;
> -
> - return (data & tps->info[rid]->enable_mask) ? 1 : 0;
> -}
> -
> static int tps65217_pmic_enable(struct regulator_dev *dev)
> {
> struct tps65217 *tps = rdev_get_drvdata(dev);
> @@ -177,9 +153,8 @@ static int tps65217_pmic_enable(struct regulator_dev *dev)
>
> /* Enable the regulator and password protection is level 1 */
> return tps65217_set_bits(tps, TPS65217_REG_ENABLE,
> - tps->info[rid]->enable_mask,
> - tps->info[rid]->enable_mask,
> - TPS65217_PROTECT_L1);
> + dev->desc->enable_mask, dev->desc->enable_mask,
> + TPS65217_PROTECT_L1);
> }
>
> static int tps65217_pmic_disable(struct regulator_dev *dev)
> @@ -192,25 +167,7 @@ static int tps65217_pmic_disable(struct regulator_dev *dev)
>
> /* Disable the regulator and password protection is level 1 */
> return tps65217_clear_bits(tps, TPS65217_REG_ENABLE,
> - tps->info[rid]->enable_mask, TPS65217_PROTECT_L1);
> -}
> -
> -static int tps65217_pmic_get_voltage_sel(struct regulator_dev *dev)
> -{
> - int ret;
> - struct tps65217 *tps = rdev_get_drvdata(dev);
> - unsigned int selector, rid = rdev_get_id(dev);
> -
> - if (rid < TPS65217_DCDC_1 || rid > TPS65217_LDO_4)
> - return -EINVAL;
> -
> - ret = tps65217_reg_read(tps, tps->info[rid]->set_vout_reg, &selector);
> - if (ret)
> - return ret;
> -
> - selector &= tps->info[rid]->set_vout_mask;
> -
> - return selector;
> + dev->desc->enable_mask, TPS65217_PROTECT_L1);
> }
>
> static int tps65217_pmic_set_voltage_sel(struct regulator_dev *dev,
> @@ -221,8 +178,7 @@ static int tps65217_pmic_set_voltage_sel(struct regulator_dev *dev,
> unsigned int rid = rdev_get_id(dev);
>
> /* Set the voltage based on vsel value and write protect level is 2 */
> - ret = tps65217_set_bits(tps, tps->info[rid]->set_vout_reg,
> - tps->info[rid]->set_vout_mask,
> + ret = tps65217_set_bits(tps, dev->desc->vsel_reg, dev->desc->vsel_mask,
> selector, TPS65217_PROTECT_L2);
>
> /* Set GO bit for DCDCx to initiate voltage transistion */
> @@ -285,10 +241,10 @@ static int tps65217_pmic_list_voltage(struct regulator_dev *dev,
>
> /* Operations permitted on DCDCx, LDO2, LDO3 and LDO4 */
> static struct regulator_ops tps65217_pmic_ops = {
> - .is_enabled = tps65217_pmic_is_enabled,
> + .is_enabled = regulator_is_enabled_regmap,
> .enable = tps65217_pmic_enable,
> .disable = tps65217_pmic_disable,
> - .get_voltage_sel = tps65217_pmic_get_voltage_sel,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> .set_voltage_sel = tps65217_pmic_set_voltage_sel,
> .list_voltage = tps65217_pmic_list_voltage,
> .map_voltage = tps65217_pmic_map_voltage,
> @@ -296,22 +252,36 @@ static struct regulator_ops tps65217_pmic_ops = {
>
> /* Operations permitted on LDO1 */
> static struct regulator_ops tps65217_pmic_ldo1_ops = {
> - .is_enabled = tps65217_pmic_is_enabled,
> + .is_enabled = regulator_is_enabled_regmap,
> .enable = tps65217_pmic_enable,
> .disable = tps65217_pmic_disable,
> - .get_voltage_sel = tps65217_pmic_get_voltage_sel,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> .set_voltage_sel = tps65217_pmic_set_voltage_sel,
> .list_voltage = tps65217_pmic_list_voltage,
> };
>
> static const struct regulator_desc regulators[] = {
> - TPS65217_REGULATOR("DCDC1", TPS65217_DCDC_1, tps65217_pmic_ops, 64),
> - TPS65217_REGULATOR("DCDC2", TPS65217_DCDC_2, tps65217_pmic_ops, 64),
> - TPS65217_REGULATOR("DCDC3", TPS65217_DCDC_3, tps65217_pmic_ops, 64),
> - TPS65217_REGULATOR("LDO1", TPS65217_LDO_1, tps65217_pmic_ldo1_ops, 16),
> - TPS65217_REGULATOR("LDO2", TPS65217_LDO_2, tps65217_pmic_ops, 64),
> - TPS65217_REGULATOR("LDO3", TPS65217_LDO_3, tps65217_pmic_ops, 32),
> - TPS65217_REGULATOR("LDO4", TPS65217_LDO_4, tps65217_pmic_ops, 32),
> + TPS65217_REGULATOR("DCDC1", TPS65217_DCDC_1, tps65217_pmic_ops, 64,
> + TPS65217_REG_DEFDCDC1, TPS65217_DEFDCDCX_DCDC_MASK,
> + TPS65217_ENABLE_DC1_EN),
> + TPS65217_REGULATOR("DCDC2", TPS65217_DCDC_2, tps65217_pmic_ops, 64,
> + TPS65217_REG_DEFDCDC2, TPS65217_DEFDCDCX_DCDC_MASK,
> + TPS65217_ENABLE_DC2_EN),
> + TPS65217_REGULATOR("DCDC3", TPS65217_DCDC_3, tps65217_pmic_ops, 64,
> + TPS65217_REG_DEFDCDC3, TPS65217_DEFDCDCX_DCDC_MASK,
> + TPS65217_ENABLE_DC3_EN),
> + TPS65217_REGULATOR("LDO1", TPS65217_LDO_1, tps65217_pmic_ldo1_ops, 16,
> + TPS65217_REG_DEFLDO1, TPS65217_DEFLDO1_LDO1_MASK,
> + TPS65217_ENABLE_LDO1_EN),
> + TPS65217_REGULATOR("LDO2", TPS65217_LDO_2, tps65217_pmic_ops, 64,
> + TPS65217_REG_DEFLDO2, TPS65217_DEFLDO2_LDO2_MASK,
> + TPS65217_ENABLE_LDO2_EN),
> + TPS65217_REGULATOR("LDO3", TPS65217_LDO_3, tps65217_pmic_ops, 32,
> + TPS65217_REG_DEFLS1, TPS65217_DEFLDO3_LDO3_MASK,
> + TPS65217_ENABLE_LS1_EN | TPS65217_DEFLDO3_LDO3_EN),
> + TPS65217_REGULATOR("LDO4", TPS65217_LDO_4, tps65217_pmic_ops, 32,
> + TPS65217_REG_DEFLS2, TPS65217_DEFLDO4_LDO4_MASK,
> + TPS65217_ENABLE_LS2_EN | TPS65217_DEFLDO4_LDO4_EN),
> };
>
> static int __devinit tps65217_regulator_probe(struct platform_device *pdev)
> diff --git a/include/linux/mfd/tps65217.h b/include/linux/mfd/tps65217.h
> index e030ef9..4e035a4 100644
> --- a/include/linux/mfd/tps65217.h
> +++ b/include/linux/mfd/tps65217.h
> @@ -229,9 +229,6 @@ struct tps65217_board {
> * @uv_to_vsel: Function pointer to get selector from voltage
> * @table: Table for non-uniform voltage step-size
> * @table_len: Length of the voltage table
> - * @enable_mask: Regulator enable mask bits
> - * @set_vout_reg: Regulator output voltage set register
> - * @set_vout_mask: Regulator output voltage set mask
> *
> * This data is used to check the regualtor voltage limits while setting.
> */
> @@ -243,9 +240,6 @@ struct tps_info {
> int (*uv_to_vsel)(int uV, unsigned int *vsel);
> const int *table;
> unsigned int table_len;
> - unsigned int enable_mask;
> - unsigned int set_vout_reg;
> - unsigned int set_vout_mask;
> };
>
> /**
> --
> 1.7.9.5
>
>
>
>