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

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

[Axel Lin]

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 <axel.lin@gmail.com>
---
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

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

[AnilKumar, Chimata]

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 <axel.lin@gmail.com>
> ---
> Hi AnilKumar,
> Any chance to test this patch on your hardware?

I will give the status in a day or two.

Regards
AnilKumar

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 202 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[AnilKumar, Chimata]

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 <axel.lin@gmail.com>
> > ---
> > 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

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

[Axel Lin]

> 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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 198 bytes --]

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?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[AnilKumar, Chimata]

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

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

[Axel Lin]

2012/6/18 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> 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

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

[AnilKumar, Chimata]

Hi Axel,

On Mon, Jun 18, 2012 at 17:36:14, Axel Lin wrote:
> 2012/6/18 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> > 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

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

[AnilKumar, Chimata]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 790 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[AnilKumar, Chimata]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[AnilKumar, Chimata]

Hi Axel,

Thanks for the patch.

Tested-by: AnilKumar Ch <anilkumar@ti.com>

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 <axel.lin@gmail.com>
> ---
> 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
> 
> 
> 
> 

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

[AnilKumar, Chimata]

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.
> 

Mark,

Currently this is not properly taken care, is there are any specific
reasons for this?

Thanks
AnilKumar

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

[Mark Brown]

On Fri, Sep 07, 2012 at 02:19:59PM +0000, AnilKumar, Chimata wrote:

> Currently this is not properly taken care, is there are any specific
> reasons for this?

You didn't send a patch.

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

[AnilKumar, Chimata]

On Fri, Sep 07, 2012 at 19:51:51, Mark Brown wrote:
> On Fri, Sep 07, 2012 at 02:19:59PM +0000, AnilKumar, Chimata wrote:
> 
> > Currently this is not properly taken care, is there are any specific
> > reasons for this?
> 
> You didn't send a patch.
> 

Its there, above to my comment
https://lkml.org/lkml/2012/6/18/308

Thanks
AnilKumar

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

[Mark Brown]

On Fri, Sep 07, 2012 at 02:26:53PM +0000, AnilKumar, Chimata wrote:
> On Fri, Sep 07, 2012 at 19:51:51, Mark Brown wrote:

> > You didn't send a patch.

> Its there, above to my comment
> https://lkml.org/lkml/2012/6/18/308

Right, but my reply was saying we should do something different so I was
expecting an update which implemented one of the suggestions.

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

[AnilKumar, Chimata]

On Fri, Sep 07, 2012 at 20:00:59, Mark Brown wrote:
> On Fri, Sep 07, 2012 at 02:26:53PM +0000, AnilKumar, Chimata wrote:
> > On Fri, Sep 07, 2012 at 19:51:51, Mark Brown wrote:
> 
> > > You didn't send a patch.
> 
> > Its there, above to my comment
> > https://lkml.org/lkml/2012/6/18/308
> 
> Right, but my reply was saying we should do something different so I was
> expecting an update which implemented one of the suggestions.

I will send the formal patch.

Thanks
AnilKumar