Convert ->get_mode() and ->set_mode() hooks to read/write operating mode
from/to hardware instead of relying on driver memory.
While at it, map fixed-frequency PWM regulators to REGULATOR_MODE_FAST.
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/regulator/act8945a-regulator.c | 62 ++++++++++++++++++--------
1 file changed, 44 insertions(+), 18 deletions(-)
diff --git a/drivers/regulator/act8945a-regulator.c b/drivers/regulator/act8945a-regulator.c
index e26264529b74..bd54d76efcbc 100644
--- a/drivers/regulator/act8945a-regulator.c
+++ b/drivers/regulator/act8945a-regulator.c
@@ -65,12 +65,10 @@ enum {
ACT8945A_ID_LDO2,
ACT8945A_ID_LDO3,
ACT8945A_ID_LDO4,
- ACT8945A_ID_MAX,
};
struct act8945a_pmic {
struct regmap *regmap;
- u32 op_mode[ACT8945A_ID_MAX];
};
static const struct linear_range act8945a_voltage_ranges[] = {
@@ -142,6 +140,7 @@ static unsigned int act8945a_of_map_mode(unsigned int mode)
{
switch (mode) {
case ACT8945A_REGULATOR_MODE_FIXED:
+ return REGULATOR_MODE_FAST;
case ACT8945A_REGULATOR_MODE_NORMAL:
return REGULATOR_MODE_NORMAL;
case ACT8945A_REGULATOR_MODE_LOWPOWER:
@@ -153,10 +152,9 @@ static unsigned int act8945a_of_map_mode(unsigned int mode)
static int act8945a_set_mode(struct regulator_dev *rdev, unsigned int mode)
{
- struct act8945a_pmic *act8945a = rdev_get_drvdata(rdev);
struct regmap *regmap = rdev->regmap;
int id = rdev_get_id(rdev);
- int reg, ret, val = 0;
+ int reg, val = 0;
switch (id) {
case ACT8945A_ID_DCDC1:
@@ -185,36 +183,64 @@ static int act8945a_set_mode(struct regulator_dev *rdev, unsigned int mode)
}
switch (mode) {
- case REGULATOR_MODE_STANDBY:
- if (id > ACT8945A_ID_DCDC3)
- val = BIT(5);
- break;
+ case REGULATOR_MODE_FAST:
case REGULATOR_MODE_NORMAL:
if (id <= ACT8945A_ID_DCDC3)
val = BIT(5);
break;
+ case REGULATOR_MODE_STANDBY:
+ if (id > ACT8945A_ID_DCDC3)
+ val = BIT(5);
+ break;
default:
return -EINVAL;
}
- ret = regmap_update_bits(regmap, reg, BIT(5), val);
- if (ret)
- return ret;
-
- act8945a->op_mode[id] = mode;
-
- return 0;
+ return regmap_update_bits(regmap, reg, BIT(5), val);
}
static unsigned int act8945a_get_mode(struct regulator_dev *rdev)
{
- struct act8945a_pmic *act8945a = rdev_get_drvdata(rdev);
+ struct regmap *regmap = rdev->regmap;
int id = rdev_get_id(rdev);
+ int reg, ret, val = 0;
- if (id < ACT8945A_ID_DCDC1 || id >= ACT8945A_ID_MAX)
+ switch (id) {
+ case ACT8945A_ID_DCDC1:
+ reg = ACT8945A_DCDC1_CTRL;
+ break;
+ case ACT8945A_ID_DCDC2:
+ reg = ACT8945A_DCDC2_CTRL;
+ break;
+ case ACT8945A_ID_DCDC3:
+ reg = ACT8945A_DCDC3_CTRL;
+ break;
+ case ACT8945A_ID_LDO1:
+ reg = ACT8945A_LDO1_CTRL;
+ break;
+ case ACT8945A_ID_LDO2:
+ reg = ACT8945A_LDO2_CTRL;
+ break;
+ case ACT8945A_ID_LDO3:
+ reg = ACT8945A_LDO3_CTRL;
+ break;
+ case ACT8945A_ID_LDO4:
+ reg = ACT8945A_LDO4_CTRL;
+ break;
+ default:
return -EINVAL;
+ }
- return act8945a->op_mode[id];
+ ret = regmap_read(regmap, reg, &val);
+ if (ret)
+ return ret;
+
+ if (id <= ACT8945A_ID_DCDC3 && (val & BIT(5)))
+ return REGULATOR_MODE_FAST;
+ else if (id > ACT8945A_ID_DCDC3 && !(val & BIT(5)))
+ return REGULATOR_MODE_NORMAL;
+ else
+ return REGULATOR_MODE_STANDBY;
}
static const struct regulator_ops act8945a_ops = {
--
2.25.1
Remove struct act8945a_pmic, as we no longer need drvdata.
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/regulator/act8945a-regulator.c | 29 ++++++++------------------
1 file changed, 9 insertions(+), 20 deletions(-)
diff --git a/drivers/regulator/act8945a-regulator.c b/drivers/regulator/act8945a-regulator.c
index bd54d76efcbc..73504253acc9 100644
--- a/drivers/regulator/act8945a-regulator.c
+++ b/drivers/regulator/act8945a-regulator.c
@@ -67,10 +67,6 @@ enum {
ACT8945A_ID_LDO4,
};
-struct act8945a_pmic {
- struct regmap *regmap;
-};
-
static const struct linear_range act8945a_voltage_ranges[] = {
REGULATOR_LINEAR_RANGE(600000, 0, 23, 25000),
REGULATOR_LINEAR_RANGE(1200000, 24, 47, 50000),
@@ -301,17 +297,13 @@ static int act8945a_pmic_probe(struct platform_device *pdev)
{
struct regulator_config config = { };
const struct regulator_desc *regulators;
- struct act8945a_pmic *act8945a;
struct regulator_dev *rdev;
+ struct regmap *regmap;
int i, num_regulators;
bool voltage_select;
- act8945a = devm_kzalloc(&pdev->dev, sizeof(*act8945a), GFP_KERNEL);
- if (!act8945a)
- return -ENOMEM;
-
- act8945a->regmap = dev_get_regmap(pdev->dev.parent, NULL);
- if (!act8945a->regmap) {
+ regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!regmap) {
dev_err(&pdev->dev,
"could not retrieve regmap from parent device\n");
return -EINVAL;
@@ -330,7 +322,6 @@ static int act8945a_pmic_probe(struct platform_device *pdev)
config.dev = &pdev->dev;
config.dev->of_node = pdev->dev.parent->of_node;
- config.driver_data = act8945a;
for (i = 0; i < num_regulators; i++) {
rdev = devm_regulator_register(&pdev->dev, ®ulators[i],
&config);
@@ -342,33 +333,31 @@ static int act8945a_pmic_probe(struct platform_device *pdev)
}
}
- platform_set_drvdata(pdev, act8945a);
-
/* Unlock expert registers. */
- return regmap_write(act8945a->regmap, ACT8945A_SYS_UNLK_REGS, 0xef);
+ return regmap_write(regmap, ACT8945A_SYS_UNLK_REGS, 0xef);
}
-static int __maybe_unused act8945a_suspend(struct device *pdev)
+static int __maybe_unused act8945a_suspend(struct device *dev)
{
- struct act8945a_pmic *act8945a = dev_get_drvdata(pdev);
+ struct regmap *regmap = dev_get_regmap(dev->parent, NULL);
/*
* Ask the PMIC to enter the suspend mode on the next PWRHLD
* transition.
*/
- return regmap_write(act8945a->regmap, ACT8945A_SYS_CTRL, 0x42);
+ return regmap_write(regmap, ACT8945A_SYS_CTRL, 0x42);
}
static SIMPLE_DEV_PM_OPS(act8945a_pm, act8945a_suspend, NULL);
static void act8945a_pmic_shutdown(struct platform_device *pdev)
{
- struct act8945a_pmic *act8945a = platform_get_drvdata(pdev);
+ struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
/*
* Ask the PMIC to shutdown everything on the next PWRHLD transition.
*/
- regmap_write(act8945a->regmap, ACT8945A_SYS_CTRL, 0x0);
+ regmap_write(regmap, ACT8945A_SYS_CTRL, 0x0);
}
static struct platform_driver act8945a_pmic_driver = {
--
2.25.1
On Mon, Jun 05, 2023 at 04:18:29PM +0530, Raag Jadav wrote:
> Convert ->get_mode() and ->set_mode() hooks to read/write operating mode
> from/to hardware instead of relying on driver memory.
Why is this change being made - what is the benefit here?
> While at it, map fixed-frequency PWM regulators to REGULATOR_MODE_FAST.
Don't combine multiple changes into a single patch, this just makes
everything harder to review.
On Mon, Jun 05, 2023 at 01:16:33PM +0100, Mark Brown wrote:
> On Mon, Jun 05, 2023 at 04:18:29PM +0530, Raag Jadav wrote:
>
> > Convert ->get_mode() and ->set_mode() hooks to read/write operating mode
> > from/to hardware instead of relying on driver memory.
>
> Why is this change being made - what is the benefit here?
Original implementation uses drvdata to load/store
operating mode for the regulator.
This change doesn't really add anything new.
It is just to make sure that the driver is in sync
with the current state of affairs in the hardware
instead of relying on locally stored status in the memory.
> > While at it, map fixed-frequency PWM regulators to REGULATOR_MODE_FAST.
>
> Don't combine multiple changes into a single patch, this just makes
> everything harder to review.
Will split in v2.
Cheers,
Raag
On Mon, Jun 05, 2023 at 08:01:03PM +0530, Raag Jadav wrote:
> On Mon, Jun 05, 2023 at 01:16:33PM +0100, Mark Brown wrote:
> > On Mon, Jun 05, 2023 at 04:18:29PM +0530, Raag Jadav wrote:
> > > Convert ->get_mode() and ->set_mode() hooks to read/write operating mode
> > > from/to hardware instead of relying on driver memory.
> > Why is this change being made - what is the benefit here?
> Original implementation uses drvdata to load/store
> operating mode for the regulator.
> This change doesn't really add anything new.
> It is just to make sure that the driver is in sync
> with the current state of affairs in the hardware
> instead of relying on locally stored status in the memory.
It's also removing a cache so we need to talk to the hardware more often
which doesn't seem like a win. If there's a bootstapping problem then
shouldn't we just read once at startup? If there's no problem what's
the advantage?
On Mon, Jun 05, 2023 at 03:44:37PM +0100, Mark Brown wrote:
> On Mon, Jun 05, 2023 at 08:01:03PM +0530, Raag Jadav wrote:
> > On Mon, Jun 05, 2023 at 01:16:33PM +0100, Mark Brown wrote:
> > > On Mon, Jun 05, 2023 at 04:18:29PM +0530, Raag Jadav wrote:
>
> > > > Convert ->get_mode() and ->set_mode() hooks to read/write operating mode
> > > > from/to hardware instead of relying on driver memory.
>
> > > Why is this change being made - what is the benefit here?
>
> > Original implementation uses drvdata to load/store
> > operating mode for the regulator.
>
> > This change doesn't really add anything new.
> > It is just to make sure that the driver is in sync
> > with the current state of affairs in the hardware
> > instead of relying on locally stored status in the memory.
>
> It's also removing a cache so we need to talk to the hardware more often
> which doesn't seem like a win. If there's a bootstapping problem then
> shouldn't we just read once at startup? If there's no problem what's
> the advantage?
Well, there could be cases of access which are done
outside of driver context or hardware failure,
which are not really ideal but I've faced such problems
a while back, so just decided to share it.
You have the final call on this.
Cheers,
Raag
On Mon, Jun 05, 2023 at 09:46:14PM +0530, Raag Jadav wrote:
> On Mon, Jun 05, 2023 at 03:44:37PM +0100, Mark Brown wrote:
> > It's also removing a cache so we need to talk to the hardware more often
> > which doesn't seem like a win. If there's a bootstapping problem then
> > shouldn't we just read once at startup? If there's no problem what's
> > the advantage?
> Well, there could be cases of access which are done
> outside of driver context or hardware failure,
> which are not really ideal but I've faced such problems
> a while back, so just decided to share it.
If we have those then we've got a bigger problem, for example we might
switch to a lower power mode which can't support the load we're trying
to get the consumer to run.
On Mon, Jun 05, 2023 at 05:30:36PM +0100, Mark Brown wrote:
> On Mon, Jun 05, 2023 at 09:46:14PM +0530, Raag Jadav wrote:
> > On Mon, Jun 05, 2023 at 03:44:37PM +0100, Mark Brown wrote:
>
> > > It's also removing a cache so we need to talk to the hardware more often
> > > which doesn't seem like a win. If there's a bootstapping problem then
> > > shouldn't we just read once at startup? If there's no problem what's
> > > the advantage?
>
> > Well, there could be cases of access which are done
> > outside of driver context or hardware failure,
> > which are not really ideal but I've faced such problems
> > a while back, so just decided to share it.
>
> If we have those then we've got a bigger problem, for example we might
> switch to a lower power mode which can't support the load we're trying
> to get the consumer to run.
Yes, but in such cases it will atleast report whatever mode currently
set in the hardware and not the incorrect one stored locally in the memory,
and this way I think we have a better chance of finding out that
there indeed is a problem.
Cheers,
Raag
On Tue, Jun 06, 2023 at 12:30:07PM +0530, Raag Jadav wrote:
> On Mon, Jun 05, 2023 at 05:30:36PM +0100, Mark Brown wrote:
> > If we have those then we've got a bigger problem, for example we might
> > switch to a lower power mode which can't support the load we're trying
> > to get the consumer to run.
> Yes, but in such cases it will atleast report whatever mode currently
> set in the hardware and not the incorrect one stored locally in the memory,
> and this way I think we have a better chance of finding out that
> there indeed is a problem.
If this is a concern you should add something that verifies that the
mode hasn't changed underneath the driver, this won't actually detect
any corruption - it just might silently fix it.