[PATCH 1/2] regulator: core: add support to get VSEL register from hw driver

[PATCH 1/2] regulator: core: add support to get VSEL register from hw driver

[Laxman Dewangan]

Currently VSEL register is read from desc only if set_voltage_sel
callback is regulator_set_voltage_sel_regmap() but there are cases
where set_voltage_sel() is implemented inside the HW regulator driver
like MAX8973.

Add callback on the regulator ops to get the voltage selection
register address and mask from device regulator driver. Use this
new callback in regulator_get_hardware_vsel_register().

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/regulator/core.c         | 19 +++++++++++++------
 include/linux/regulator/driver.h |  7 +++++++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 443eaab..8c19bdf 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2392,14 +2392,20 @@ int regulator_get_hardware_vsel_register(struct regulator *regulator,
 {
 	struct regulator_dev *rdev = regulator->rdev;
 	const struct regulator_ops *ops = rdev->desc->ops;
+	int ret = 0;
 
-	if (ops->set_voltage_sel != regulator_set_voltage_sel_regmap)
+	if (ops->set_voltage_sel == regulator_set_voltage_sel_regmap) {
+		*vsel_reg = rdev->desc->vsel_reg;
+		*vsel_mask = rdev->desc->vsel_mask;
+	} else if (ops->get_voltage_sel_register) {
+		mutex_lock(&rdev->mutex);
+		ret = ops->get_voltage_sel_register(rdev, vsel_reg, vsel_mask);
+		mutex_unlock(&rdev->mutex);
+	} else {
 		return -EOPNOTSUPP;
+	}
 
-	 *vsel_reg = rdev->desc->vsel_reg;
-	 *vsel_mask = rdev->desc->vsel_mask;
-
-	 return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_get_hardware_vsel_register);
 
@@ -2422,7 +2428,8 @@ int regulator_list_hardware_vsel(struct regulator *regulator,
 
 	if (selector >= rdev->desc->n_voltages)
 		return -EINVAL;
-	if (ops->set_voltage_sel != regulator_set_voltage_sel_regmap)
+	if ((ops->set_voltage_sel != regulator_set_voltage_sel_regmap) &&
+		!ops->get_voltage_sel_register)
 		return -EOPNOTSUPP;
 
 	return selector;
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index fffa688..7b7433f 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -103,6 +103,9 @@ struct regulator_linear_range {
  * @set_bypass: Set the regulator in bypass mode.
  * @get_bypass: Get the regulator bypass mode state.
  *
+ * @get_voltage_sel_register: Get hardware voltage sel register address
+ *                    and its voltage mask.
+ *
  * @enable_time: Time taken for the regulator voltage output voltage to
  *               stabilise after being enabled, in microseconds.
  * @set_ramp_delay: Set the ramp delay for the regulator. The driver should
@@ -175,6 +178,10 @@ struct regulator_ops {
 	int (*set_bypass)(struct regulator_dev *dev, bool enable);
 	int (*get_bypass)(struct regulator_dev *dev, bool *enable);
 
+	/* Get HW VSEL register */
+	int (*get_voltage_sel_register)(struct regulator_dev *dev,
+			unsigned *vsel_reg, unsigned *vsel_mask);
+
 	/* the operations below are for configuration of regulator state when
 	 * its parent PMIC enters a global STANDBY/HIBERNATE state */
 
-- 
2.1.4

[PATCH 2/2] regulator: max8973: Implement callback to get vsel and mask

[Laxman Dewangan]

Implement callback API to get the current voltage selection
register and its voltage mask value.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/regulator/max8973-regulator.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/regulator/max8973-regulator.c b/drivers/regulator/max8973-regulator.c
index 00cf91c..b0f4f58 100644
--- a/drivers/regulator/max8973-regulator.c
+++ b/drivers/regulator/max8973-regulator.c
@@ -199,6 +199,16 @@ static int max8973_dcdc_set_voltage_sel(struct regulator_dev *rdev,
 	return 0;
 }
 
+static int max8973_dcdc_get_voltage_sel_register(struct regulator_dev *rdev,
+		unsigned *vsel_reg, unsigned *vsel_mask)
+{
+	struct max8973_chip *max = rdev_get_drvdata(rdev);
+
+	*vsel_reg = max->curr_vout_reg;
+	*vsel_mask = MAX8973_VOUT_MASK;
+	return 0;
+}
+
 static int max8973_dcdc_set_mode(struct regulator_dev *rdev, unsigned int mode)
 {
 	struct max8973_chip *max = rdev_get_drvdata(rdev);
@@ -247,6 +257,7 @@ static const struct regulator_ops max8973_dcdc_ops = {
 	.get_voltage_sel	= max8973_dcdc_get_voltage_sel,
 	.set_voltage_sel	= max8973_dcdc_set_voltage_sel,
 	.list_voltage		= regulator_list_voltage_linear,
+	.get_voltage_sel_register = max8973_dcdc_get_voltage_sel_register,
 	.set_mode		= max8973_dcdc_set_mode,
 	.get_mode		= max8973_dcdc_get_mode,
 };
-- 
2.1.4

Re: [PATCH 1/2] regulator: core: add support to get VSEL register from hw driver

[Mark Brown]

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

On Wed, May 27, 2015 at 08:10:20PM +0530, Laxman Dewangan wrote:

> Add callback on the regulator ops to get the voltage selection
> register address and mask from device regulator driver. Use this
> new callback in regulator_get_hardware_vsel_register().

It's not entirely clear to me that this is a good idea - the expected
use case for getting the vsel register is to hand it off to something
like a microcontroller for it to use but if the vsel register might
change at runtime then we also need infrastructure to synchronize those
changes with whatever is using the register.  How does your system keep
them in sync?

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

Re: [PATCH 1/2] regulator: core: add support to get VSEL register from hw driver

[Laxman Dewangan]

On Thursday 28 May 2015 07:22 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Wed, May 27, 2015 at 08:10:20PM +0530, Laxman Dewangan wrote:
>
>> Add callback on the regulator ops to get the voltage selection
>> register address and mask from device regulator driver. Use this
>> new callback in regulator_get_hardware_vsel_register().
> It's not entirely clear to me that this is a good idea - the expected
> use case for getting the vsel register is to hand it off to something
> like a microcontroller for it to use but if the vsel register might
> change at runtime then we also need infrastructure to synchronize those
> changes with whatever is using the register.  How does your system keep
> them in sync?
>

VOUT register address get changed based on DVS pin level. Say we have 
VOUT0 and VOUT1 register here for DVS pin state 0 and 1.
DVS pin can be used in two ways:
1. For dynamic voltage scaling and reducing the i2c command by using 
VOUT0 and VOUT1.
2. For sleep control like when rail is active, use the VOUT0 and when it 
is in sleep use the VOUT1 and control DVS for sleep entry/exit.
3. Some cases, DVS pin is tied to some level.

In our system, CPU-DFLL is another HW module which controls the CPU 
voltage based on CPU frequency request. It issues the i2c command to the 
device for voltage change when frequency get change. For this reason, 
CPU-DFLL need the VSEL register address. The CPU DVFS SW driver controls 
the DVS input pin to control sleep entry/exit and VOUT0 for voltage 
change. So on this case, we really donot need to change the VSEL 
register address for active state voltage change and use the DVS for 
sleep entry/exit.

The problem will be there when external controller use the DVS pin for 
DVFS and need two register address here. In this case, I think external 
controller need to get information from different mechanism rather than 
via regulator driver to avoid complication on regulator driver/framework.

Re: [PATCH 1/2] regulator: core: add support to get VSEL register from hw driver

[Mark Brown]

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

On Fri, May 29, 2015 at 11:21:36AM +0530, Laxman Dewangan wrote:

> In our system, CPU-DFLL is another HW module which controls the CPU voltage
> based on CPU frequency request. It issues the i2c command to the device for
> voltage change when frequency get change. For this reason, CPU-DFLL need the
> VSEL register address. The CPU DVFS SW driver controls the DVS input pin to
> control sleep entry/exit and VOUT0 for voltage change. So on this case, we
> really donot need to change the VSEL register address for active state
> voltage change and use the DVS for sleep entry/exit.

Sure, that's the normal use case for this stuff.

> The problem will be there when external controller use the DVS pin for DVFS
> and need two register address here. In this case, I think external
> controller need to get information from different mechanism rather than via
> regulator driver to avoid complication on regulator driver/framework.

Probably, it's definitely getting complicated (though in this case where
we only need one register we ought to be more able to manage it, not
that I have bright ideas right now).

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

Re: [PATCH 1/2] regulator: core: add support to get VSEL register from hw driver

[Laxman Dewangan]

On Friday 29 May 2015 03:19 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Fri, May 29, 2015 at 11:21:36AM +0530, Laxman Dewangan wrote:
>
>> really donot need to change the VSEL register address for active state
>> voltage change and use the DVS for sleep entry/exit.
> Sure, that's the normal use case for this stuff.

Currently, we are using this on drivers/clk/tegra/clk-dfll.c.

Re: [PATCH 1/2] regulator: core: add support to get VSEL register from hw driver

[Laxman Dewangan]

On Friday 29 May 2015 03:19 PM, Mark Brown wrote:
> The problem will be there when external controller use the DVS pin for DVFS
> and need two register address here. In this case, I think external
> controller need to get information from different mechanism rather than via
> regulator driver to avoid complication on regulator driver/framework.
> Probably, it's definitely getting complicated (though in this case where
> we only need one register we ought to be more able to manage it, not
> that I have bright ideas right now).
>
Hi Mark,
Are you accepting this patch? I am not much clear on the conclusion and 
hence my question is.

Is anything pending  from me to provide any information?

I have some more patches pending on MAX8973 for supporting MAX77621 
device and if this series is not fine, I like to go on next set of 
patches while we discussing this need.

Thanks,
Laxman

Re: [PATCH 1/2] regulator: core: add support to get VSEL register from hw driver

[Mark Brown]

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

On Fri, Jun 05, 2015 at 07:06:35PM +0530, Laxman Dewangan wrote:
> On Friday 29 May 2015 03:19 PM, Mark Brown wrote:
> >The problem will be there when external controller use the DVS pin for DVFS
> >and need two register address here. In this case, I think external
> >controller need to get information from different mechanism rather than via
> >regulator driver to avoid complication on regulator driver/framework.
> >Probably, it's definitely getting complicated (though in this case where
> >we only need one register we ought to be more able to manage it, not
> >that I have bright ideas right now).

> Are you accepting this patch? I am not much clear on the conclusion and
> hence my question is.

> Is anything pending  from me to provide any information?

I thought we agreed that this interface was a bad idea due to the need
to coordinate between the changes in register and the external device
using the value?

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