[PATCH] regulator: tps65917: Fix SMPS enable/disable/is_enable

[PATCH] regulator: tps65917: Fix SMPS enable/disable/is_enable

[Nishanth Menon]

We use regmap regulator ops to enable/disable and check if regulator
is enabled for various SMPS. However, these depend on valid
enable_reg, enable_mask and enable_value in regulator descriptor.

So, similar to fix we did in commit 318dbb02b50c
("regulator: palmas: Fix SMPS enable/disable/is_enabled"), populate the
same for TPS65917 SMPS registration. LDO definitions are already in place.

Fixes: d6f83370ed97 ("regulator: palmas: Add tps65917 PMIC support")
Signed-off-by: Nishanth Menon <nm@ti.com>
---

Applies on:
	git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
branch: topic/palmas (4c0c9ca Merge remote-tracking branch 'regulator/fix/palmas' into regulator-palmas)

Note: Ignored the minor style check from checkpatch --strict as fixing
it would create an 80 char warning

 drivers/regulator/palmas-regulator.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c
index 7c8b441..c7aa1b1 100644
--- a/drivers/regulator/palmas-regulator.c
+++ b/drivers/regulator/palmas-regulator.c
@@ -1333,6 +1333,14 @@ static int tps65917_smps_registration(struct palmas_pmic *pmic,
 		pmic->current_reg_mode[id] = reg &
 				PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK;
 
+		pmic->desc[id].enable_reg =
+				PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
+					palmas_regs_info[id].ctrl_addr);
+		pmic->desc[id].enable_mask =
+				PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK;
+		/* set_mode overrides this value */
+		pmic->desc[id].enable_val = SMPS_CTRL_MODE_ON;
+
 		pmic->desc[id].type = REGULATOR_VOLTAGE;
 		pmic->desc[id].owner = THIS_MODULE;
 
-- 
1.7.9.5

Re: [PATCH] regulator: tps65917: Fix SMPS enable/disable/is_enable

[Keerthy]

Hello Nishanth,

On Friday 27 June 2014 12:01 AM, Nishanth Menon wrote:
> We use regmap regulator ops to enable/disable and check if regulator
> is enabled for various SMPS. However, these depend on valid
> enable_reg, enable_mask and enable_value in regulator descriptor.
>
> So, similar to fix we did in commit 318dbb02b50c
> ("regulator: palmas: Fix SMPS enable/disable/is_enabled"), populate the
> same for TPS65917 SMPS registration. LDO definitions are already in place.
>
> Fixes: d6f83370ed97 ("regulator: palmas: Add tps65917 PMIC support")
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>
> Applies on:
> 	git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
> branch: topic/palmas (4c0c9ca Merge remote-tracking branch 'regulator/fix/palmas' into regulator-palmas)
>
> Note: Ignored the minor style check from checkpatch --strict as fixing
> it would create an 80 char warning
>
>   drivers/regulator/palmas-regulator.c |    8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c
> index 7c8b441..c7aa1b1 100644
> --- a/drivers/regulator/palmas-regulator.c
> +++ b/drivers/regulator/palmas-regulator.c
> @@ -1333,6 +1333,14 @@ static int tps65917_smps_registration(struct palmas_pmic *pmic,
>   		pmic->current_reg_mode[id] = reg &
>   				PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK;
>   
> +		pmic->desc[id].enable_reg =
> +				PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
> +					palmas_regs_info[id].ctrl_addr);

This is wrong. Please change palmas_regs_info[id].ctrl_addr to
ddata->palmas_regs_info[id].ctrl_addr. The palmas_regs_info
should come from the driver data for specific instances as the regmap
is different for the different PMICs we support.

Once you make the above changes please feel free to add
Tested-by: Keerthy <j-keerthy@ti.com>.

> +		pmic->desc[id].enable_mask =
> +				PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK;
> +		/* set_mode overrides this value */
> +		pmic->desc[id].enable_val = SMPS_CTRL_MODE_ON;
> +
>   		pmic->desc[id].type = REGULATOR_VOLTAGE;
>   		pmic->desc[id].owner = THIS_MODULE;
>   

Regards,
Keerthy

Re: [PATCH] regulator: tps65917: Fix SMPS enable/disable/is_enable

[Nishanth Menon]

On 14:48-20140627, Keerthy wrote:
> Hello Nishanth,
> 
> On Friday 27 June 2014 12:01 AM, Nishanth Menon wrote:
> >We use regmap regulator ops to enable/disable and check if regulator
> >is enabled for various SMPS. However, these depend on valid
> >enable_reg, enable_mask and enable_value in regulator descriptor.
> >
> >So, similar to fix we did in commit 318dbb02b50c
> >("regulator: palmas: Fix SMPS enable/disable/is_enabled"), populate the
> >same for TPS65917 SMPS registration. LDO definitions are already in place.
> >
> >Fixes: d6f83370ed97 ("regulator: palmas: Add tps65917 PMIC support")
> >Signed-off-by: Nishanth Menon <nm@ti.com>
> >---
> >
> >Applies on:
> >	git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
> >branch: topic/palmas (4c0c9ca Merge remote-tracking branch 'regulator/fix/palmas' into regulator-palmas)
> >
> >Note: Ignored the minor style check from checkpatch --strict as fixing
> >it would create an 80 char warning
> >
> >  drivers/regulator/palmas-regulator.c |    8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> >diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c
> >index 7c8b441..c7aa1b1 100644
> >--- a/drivers/regulator/palmas-regulator.c
> >+++ b/drivers/regulator/palmas-regulator.c
> >@@ -1333,6 +1333,14 @@ static int tps65917_smps_registration(struct palmas_pmic *pmic,
> >  		pmic->current_reg_mode[id] = reg &
> >  				PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK;
> >+		pmic->desc[id].enable_reg =
> >+				PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
> >+					palmas_regs_info[id].ctrl_addr);
> 
> This is wrong. Please change palmas_regs_info[id].ctrl_addr to
> ddata->palmas_regs_info[id].ctrl_addr. The palmas_regs_info
> should come from the driver data for specific instances as the regmap
> is different for the different PMICs we support.
> 
> Once you make the above changes please feel free to add

Uggh.. just realized... since I was testing CPUFREQ with SMPS1 never
realized it. and searched elsewhere on topic/palmas branch as well..
Looks like we should make the following changes? since the register data
is now a parameter and it so happened that palmas_reg_info was mapping
back to a valid variable and the error is easily missed - so rename the
variable as well - I can break this up as proper series of patches for
topic/palmas - let me know what you think..


diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c
index 7c8b441..86fefdfb 100644
--- a/drivers/regulator/palmas-regulator.c
+++ b/drivers/regulator/palmas-regulator.c
@@ -41,7 +41,7 @@ static const struct regulator_linear_range smps_high_ranges[] = {
 	REGULATOR_LINEAR_RANGE(3300000, 0x7A, 0x7f, 0),
 };
 
-static struct regs_info palmas_regs_info[] = {
+static struct regs_info palmas_generic_regs_info[] = {
 	{
 		.name		= "SMPS12",
 		.sname		= "smps1-in",
@@ -1165,18 +1165,18 @@ static int palmas_smps_registration(struct palmas_pmic *pmic,
 			pmic->desc[id].ops = &palmas_ops_smps10;
 			pmic->desc[id].vsel_reg =
 					PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
-							PALMAS_SMPS10_CTRL);
+					        ddata->palmas_regs_info[id].ctrl_addr);
 			pmic->desc[id].vsel_mask = SMPS10_VSEL;
 			pmic->desc[id].enable_reg =
 					PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
-							PALMAS_SMPS10_CTRL);
+					        ddata->palmas_regs_info[id].ctrl_addr);
 			if (id == PALMAS_REG_SMPS10_OUT1)
 				pmic->desc[id].enable_mask = SMPS10_SWITCH_EN;
 			else
 				pmic->desc[id].enable_mask = SMPS10_BOOST_EN;
 			pmic->desc[id].bypass_reg =
 					PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
-							PALMAS_SMPS10_CTRL);
+					        ddata->palmas_regs_info[id].ctrl_addr);
 			pmic->desc[id].bypass_mask = SMPS10_BYPASS_EN;
 			pmic->desc[id].min_uV = 3750000;
 			pmic->desc[id].uV_step = 1250000;
@@ -1188,7 +1188,7 @@ static int palmas_smps_registration(struct palmas_pmic *pmic,
 			 * otherwise we error in probe with unsupportable
 			 * ranges. Read the current smps mode for later use.
 			 */
-			addr = palmas_regs_info[id].vsel_addr;
+			addr = ddata->palmas_regs_info[id].vsel_addr;
 			pmic->desc[id].n_linear_ranges = 3;
 
 			ret = palmas_smps_read(pmic->palmas, addr, &reg);
@@ -1209,12 +1209,12 @@ static int palmas_smps_registration(struct palmas_pmic *pmic,
 			pmic->desc[id].n_voltages = PALMAS_SMPS_NUM_VOLTAGES;
 			pmic->desc[id].vsel_reg =
 					PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
-						palmas_regs_info[id].vsel_addr);
+						ddata->palmas_regs_info[id].vsel_addr);
 			pmic->desc[id].vsel_mask =
 					PALMAS_SMPS12_VOLTAGE_VSEL_MASK;
 
 			/* Read the smps mode for later use. */
-			addr = palmas_regs_info[id].ctrl_addr;
+			addr = ddata->palmas_regs_info[id].ctrl_addr;
 			ret = palmas_smps_read(pmic->palmas, addr, &reg);
 			if (ret)
 				return ret;
@@ -1223,7 +1223,7 @@ static int palmas_smps_registration(struct palmas_pmic *pmic,
 
 			pmic->desc[id].enable_reg =
 					PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
-						palmas_regs_info[id].ctrl_addr);
+						ddata->palmas_regs_info[id].ctrl_addr);
 			pmic->desc[id].enable_mask =
 					PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK;
 			/* set_mode overrides this value */
@@ -1319,7 +1319,7 @@ static int tps65917_smps_registration(struct palmas_pmic *pmic,
 		pmic->desc[id].n_voltages = PALMAS_SMPS_NUM_VOLTAGES;
 		pmic->desc[id].vsel_reg =
 				PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
-						   tps65917_regs_info[id].vsel_addr);
+					        ddata->palmas_regs_info[id].vsel_addr);
 		pmic->desc[id].vsel_mask =
 				PALMAS_SMPS12_VOLTAGE_VSEL_MASK;
 
@@ -1414,7 +1414,7 @@ struct palmas_pmic_driver_data palmas_ddata = {
 	.ldo_begin = PALMAS_REG_LDO1,
 	.ldo_end = PALMAS_REG_LDOUSB,
 	.max_reg = PALMAS_NUM_REGS,
-	.palmas_regs_info = palmas_regs_info,
+	.palmas_regs_info = palmas_generic_regs_info,
 	.palmas_matches = palmas_matches,
 	.sleep_req_info = palma_sleep_req_info,
 	.smps_register = palmas_smps_registration,
-- 
Regards,
Nishanth Menon

Re: [PATCH] regulator: tps65917: Fix SMPS enable/disable/is_enable

[Keerthy]

Hi Nishanth,

On Friday 27 June 2014 10:45 PM, Nishanth Menon wrote:
> On 14:48-20140627, Keerthy wrote:
>> Hello Nishanth,
>>
>> On Friday 27 June 2014 12:01 AM, Nishanth Menon wrote:
>>> We use regmap regulator ops to enable/disable and check if regulator
>>> is enabled for various SMPS. However, these depend on valid
>>> enable_reg, enable_mask and enable_value in regulator descriptor.
>>>
>>> So, similar to fix we did in commit 318dbb02b50c
>>> ("regulator: palmas: Fix SMPS enable/disable/is_enabled"), populate the
>>> same for TPS65917 SMPS registration. LDO definitions are already in place.
>>>
>>> Fixes: d6f83370ed97 ("regulator: palmas: Add tps65917 PMIC support")
>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> ---
>>>
>>> Applies on:
>>> 	git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
>>> branch: topic/palmas (4c0c9ca Merge remote-tracking branch 'regulator/fix/palmas' into regulator-palmas)
>>>
>>> Note: Ignored the minor style check from checkpatch --strict as fixing
>>> it would create an 80 char warning
>>>
>>>   drivers/regulator/palmas-regulator.c |    8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c
>>> index 7c8b441..c7aa1b1 100644
>>> --- a/drivers/regulator/palmas-regulator.c
>>> +++ b/drivers/regulator/palmas-regulator.c
>>> @@ -1333,6 +1333,14 @@ static int tps65917_smps_registration(struct palmas_pmic *pmic,
>>>   		pmic->current_reg_mode[id] = reg &
>>>   				PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK;
>>> +		pmic->desc[id].enable_reg =
>>> +				PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
>>> +					palmas_regs_info[id].ctrl_addr);
>> This is wrong. Please change palmas_regs_info[id].ctrl_addr to
>> ddata->palmas_regs_info[id].ctrl_addr. The palmas_regs_info
>> should come from the driver data for specific instances as the regmap
>> is different for the different PMICs we support.
>>
>> Once you make the above changes please feel free to add
> Uggh.. just realized... since I was testing CPUFREQ with SMPS1 never
> realized it. and searched elsewhere on topic/palmas branch as well..
> Looks like we should make the following changes? since the register data
> is now a parameter and it so happened that palmas_reg_info was mapping
> back to a valid variable and the error is easily missed - so rename the
> variable as well - I can break this up as proper series of patches for
> topic/palmas - let me know what you think..
>
>
> diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c
> index 7c8b441..86fefdfb 100644
> --- a/drivers/regulator/palmas-regulator.c
> +++ b/drivers/regulator/palmas-regulator.c
> @@ -41,7 +41,7 @@ static const struct regulator_linear_range smps_high_ranges[] = {
>   	REGULATOR_LINEAR_RANGE(3300000, 0x7A, 0x7f, 0),
>   };
>   
> -static struct regs_info palmas_regs_info[] = {
> +static struct regs_info palmas_generic_regs_info[] = {


Good renaming but You need to make one more change i guess. Otherwise
there is a compilation break.

@@ -643,7 +643,8 @@
static int palmas_regulator_config_external(struct palmas *palmas, int id,
          struct palmas_reg_init *reg_init)
{
-    int sleep_id = palmas_regs_info[id].sleep_id;
+    struct palmas_pmic_driver_data *ddata = palmas->pmic_ddata;
+    int sleep_id = ddata->palmas_regs_info[id].sleep_id;
      int ret;

Including the above change.
Tested-by: Keerthy <j-keerthy@ti.com>.

>   	{
>   		.name		= "SMPS12",
>   		.sname		= "smps1-in",
> @@ -1165,18 +1165,18 @@ static int palmas_smps_registration(struct palmas_pmic *pmic,
>   			pmic->desc[id].ops = &palmas_ops_smps10;
>   			pmic->desc[id].vsel_reg =
>   					PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
> -							PALMAS_SMPS10_CTRL);
> +					        ddata->palmas_regs_info[id].ctrl_addr);
>   			pmic->desc[id].vsel_mask = SMPS10_VSEL;
>   			pmic->desc[id].enable_reg =
>   					PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
> -							PALMAS_SMPS10_CTRL);
> +					        ddata->palmas_regs_info[id].ctrl_addr);
>   			if (id == PALMAS_REG_SMPS10_OUT1)
>   				pmic->desc[id].enable_mask = SMPS10_SWITCH_EN;
>   			else
>   				pmic->desc[id].enable_mask = SMPS10_BOOST_EN;
>   			pmic->desc[id].bypass_reg =
>   					PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
> -							PALMAS_SMPS10_CTRL);
> +					        ddata->palmas_regs_info[id].ctrl_addr);
>   			pmic->desc[id].bypass_mask = SMPS10_BYPASS_EN;
>   			pmic->desc[id].min_uV = 3750000;
>   			pmic->desc[id].uV_step = 1250000;
> @@ -1188,7 +1188,7 @@ static int palmas_smps_registration(struct palmas_pmic *pmic,
>   			 * otherwise we error in probe with unsupportable
>   			 * ranges. Read the current smps mode for later use.
>   			 */
> -			addr = palmas_regs_info[id].vsel_addr;
> +			addr = ddata->palmas_regs_info[id].vsel_addr;
>   			pmic->desc[id].n_linear_ranges = 3;
>   
>   			ret = palmas_smps_read(pmic->palmas, addr, &reg);
> @@ -1209,12 +1209,12 @@ static int palmas_smps_registration(struct palmas_pmic *pmic,
>   			pmic->desc[id].n_voltages = PALMAS_SMPS_NUM_VOLTAGES;
>   			pmic->desc[id].vsel_reg =
>   					PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
> -						palmas_regs_info[id].vsel_addr);
> +						ddata->palmas_regs_info[id].vsel_addr);
>   			pmic->desc[id].vsel_mask =
>   					PALMAS_SMPS12_VOLTAGE_VSEL_MASK;
>   
>   			/* Read the smps mode for later use. */
> -			addr = palmas_regs_info[id].ctrl_addr;
> +			addr = ddata->palmas_regs_info[id].ctrl_addr;
>   			ret = palmas_smps_read(pmic->palmas, addr, &reg);
>   			if (ret)
>   				return ret;
> @@ -1223,7 +1223,7 @@ static int palmas_smps_registration(struct palmas_pmic *pmic,
>   
>   			pmic->desc[id].enable_reg =
>   					PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
> -						palmas_regs_info[id].ctrl_addr);
> +						ddata->palmas_regs_info[id].ctrl_addr);
>   			pmic->desc[id].enable_mask =
>   					PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK;
>   			/* set_mode overrides this value */
> @@ -1319,7 +1319,7 @@ static int tps65917_smps_registration(struct palmas_pmic *pmic,
>   		pmic->desc[id].n_voltages = PALMAS_SMPS_NUM_VOLTAGES;
>   		pmic->desc[id].vsel_reg =
>   				PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
> -						   tps65917_regs_info[id].vsel_addr);
> +					        ddata->palmas_regs_info[id].vsel_addr);
>   		pmic->desc[id].vsel_mask =
>   				PALMAS_SMPS12_VOLTAGE_VSEL_MASK;
>   
> @@ -1414,7 +1414,7 @@ struct palmas_pmic_driver_data palmas_ddata = {
>   	.ldo_begin = PALMAS_REG_LDO1,
>   	.ldo_end = PALMAS_REG_LDOUSB,
>   	.max_reg = PALMAS_NUM_REGS,
> -	.palmas_regs_info = palmas_regs_info,
> +	.palmas_regs_info = palmas_generic_regs_info,
>   	.palmas_matches = palmas_matches,
>   	.sleep_req_info = palma_sleep_req_info,
>   	.smps_register = palmas_smps_registration,

Regards,
Keerthy