[RFC PATCH v2 1/4] regulator: Add helper function to get "poweroff-source" property

[Romain Perier]

Several drivers create their own devicetree property when they register
poweroff capabilities. This is for example the case for mfd, regulator
or power drivers which define "vendor,system-power-controller" property.
This patch adds support for a standard property "poweroff-source"
which marks the device as able to shutdown the system.

Signed-off-by: Romain Perier <romain.perier-***@public.gmane.org>
---
drivers/regulator/of_regulator.c | 12 ++++++++++++
include/linux/regulator/of_regulator.h | 6 ++++++
2 files changed, 18 insertions(+)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 7a51814..8b898e6 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -240,3 +240,15 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,

return init_data;
}
+
+/**
+ * is_system_poweroff_source - Tells if poweroff-source is found for device_node
+ * @np: Pointer to the given device_node
+ *
+ * return true if present false otherwise
+ */
+bool is_system_poweroff_source(const struct device_node *np)
+{
+ return of_property_read_bool(np, "poweroff-source");
+}
+EXPORT_SYMBOL_GPL(is_system_poweroff_source);
diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
index f921796..9d8fbb2 100644
--- a/include/linux/regulator/of_regulator.h
+++ b/include/linux/regulator/of_regulator.h
@@ -20,6 +20,7 @@ extern struct regulator_init_data
extern int of_regulator_match(struct device *dev, struct device_node *node,
struct of_regulator_match *matches,
unsigned int num_matches);
+extern bool is_system_poweroff_source(const struct device_node *np);
#else
static inline struct regulator_init_data
*of_get_regulator_init_data(struct device *dev,
@@ -35,6 +36,11 @@ static inline int of_regulator_match(struct device *dev,
{
return 0;
}
+
+static inline bool is_system_poweroff_source(const struct device_node *np)
+{
+ return false;
+}
#endif /* CONFIG_OF */

#endif /* __LINUX_OF_REG_H */

[Romain Perier]

When the property "poweroff-source" is found in the
devicetree, the function pm_power_off is defined. This function sends the
rights bit fields to the global off control register. shutdown/poweroff
commands are now supported for hardware components which use these PMU.

Signed-off-by: Romain Perier <***@gmail.com>
---
drivers/regulator/act8865-regulator.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index afd06f9..8bba591 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -61,6 +61,8 @@
#define ACT8846_REG12_VSET 0xa0
#define ACT8846_REG12_CTRL 0xa1
#define ACT8846_REG13_CTRL 0xb1
+#define ACT8846_GLB_OFF_CTRL 0xc3
+#define ACT8846_OFF_SYSMASK 0x18

/*
* ACT8865 Global Register Map.
@@ -84,6 +86,7 @@
#define ACT8865_LDO3_CTRL 0x61
#define ACT8865_LDO4_VSET 0x64
#define ACT8865_LDO4_CTRL 0x65
+#define ACT8865_MSTROFF 0x20

/*
* Field Definitions.
@@ -98,6 +101,8 @@

struct act8865 {
struct regmap *regmap;
+ int off_reg;
+ int off_mask;
};

static const struct regmap_config act8865_regmap_config = {
@@ -275,6 +280,16 @@ static struct regulator_init_data
return NULL;
}

+static struct i2c_client *act8865_i2c_client;
+static void act8865_power_off(void)
+{
+ struct act8865 *act8865;
+
+ act8865 = i2c_get_clientdata(act8865_i2c_client);
+ regmap_write(act8865->regmap, act8865->off_reg, act8865->off_mask);
+ while (1);
+}
+
static int act8865_pmic_probe(struct i2c_client *client,
const struct i2c_device_id *i2c_id)
{
@@ -285,6 +300,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
int i, ret, num_regulators;
struct act8865 *act8865;
unsigned long type;
+ int off_reg, off_mask;

pdata = dev_get_platdata(dev);

@@ -304,10 +320,14 @@ static int act8865_pmic_probe(struct i2c_client *client,
case ACT8846:
regulators = act8846_regulators;
num_regulators = ARRAY_SIZE(act8846_regulators);
+ off_reg = ACT8846_GLB_OFF_CTRL;
+ off_mask = ACT8846_OFF_SYSMASK;
break;
case ACT8865:
regulators = act8865_regulators;
num_regulators = ARRAY_SIZE(act8865_regulators);
+ off_reg = ACT8865_SYS_CTRL;
+ off_mask = ACT8865_MSTROFF;
break;
default:
dev_err(dev, "invalid device id %lu\n", type);
@@ -345,6 +365,14 @@ static int act8865_pmic_probe(struct i2c_client *client,
return ret;
}

+ if (dev->of_node && is_system_poweroff_source(dev->of_node) &&
+ !pm_power_off) {
+ act8865_i2c_client = client;
+ act8865->off_reg = off_reg;
+ act8865->off_mask = off_mask;
+ pm_power_off = act8865_power_off;
+ }
+
/* Finally register devices */
for (i = 0; i < num_regulators; i++) {
const struct regulator_desc *desc = &regulators[i];

[Mark Brown]

Post by Romain Perier
When the property "poweroff-source" is found in the
devicetree, the function pm_power_off is defined. This function sends the
rights bit fields to the global off control register. shutdown/poweroff
commands are now supported for hardware components which use these PMU.

Reviwed-by: Mark Brown <***@kernel.org>

but...

Post by Romain Perier
+ if (dev->of_node && is_system_poweroff_source(dev->of_node) &&
+ !pm_power_off) {
+ act8865_i2c_client = client;
+ act8865->off_reg = off_reg;
+ act8865->off_mask = off_mask;
+ pm_power_off = act8865_power_off;
+ }

Perhaps worth wrapping the dev->of_node check into the function? I
imagine the same pattern will be quite common.

Might also make sense to warn if we've got the property but we're not
setting pm_power_off - it's indicating that things aren't working as
expected, an error will help users figure out what's going wrong.

[Romain Perier]

Signed-off-by: Romain Perier <***@gmail.com>
---
Documentation/devicetree/bindings/regulator/act8865-regulator.txt | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/act8865-regulator.txt b/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
index 865614b..4d7f33e 100644
--- a/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
@@ -5,6 +5,10 @@ Required properties:
- compatible: "active-semi,act8846" or "active-semi,act8865"
- reg: I2C slave address

+Optional properties:
+- poweroff-source: Telling whether or not this pmic is controlling
+ the system power
+
Any standard regulator properties can be used to configure the single regulator.

The valid names for regulators are:

[Mark Brown]

Post by Romain Perier
---
Documentation/devicetree/bindings/regulator/act8865-regulator.txt | 4 ++++
1 file changed, 4 insertions(+)

Should this documentation be put in some central place (and perhaps
referenced from this binding) given that we're trying to make it a
standard property? Not sure where it'd fit though...

[PERIER Romain]

Mark,

I did not update the whole serie yet, so, this is the old version... :)
(my patch about act8865 too, as "is_system_poweroff_source" should be
prefixed by "of_get" as requested by Grant)
I noted all your remarks.

Thanks

Post by Mark Brown

Post by Romain Perier
---
Documentation/devicetree/bindings/regulator/act8865-regulator.txt | 4 ++++
1 file changed, 4 insertions(+)

Should this documentation be put in some central place (and perhaps
referenced from this binding) given that we're trying to make it a
standard property? Not sure where it'd fit though...

[Romain Perier]

Add "poweroff-source" property to act8846 node.
shutdown/poweroff commands are now handled for this board.

Signed-off-by: Romain Perier <***@gmail.com>
---
arch/arm/boot/dts/rk3188-radxarock.dts | 2 ++
1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/rk3188-radxarock.dts b/arch/arm/boot/dts/rk3188-radxarock.dts
index 15910c9..2affff0 100644
--- a/arch/arm/boot/dts/rk3188-radxarock.dts
+++ b/arch/arm/boot/dts/rk3188-radxarock.dts
@@ -141,6 +141,8 @@
pinctrl-names = "default";
pinctrl-0 = <&act8846_dvs0_ctl>;

+ poweroff-source;
+
regulators {
vcc_ddr: REG1 {
regulator-name = "VCC_DDR";

[PERIER Romain]

This is not the final location, I have no idea exactly where I might
put this helper function. This is why I ask you your opinion (and
also, this is why that's a proposal)

Post by Romain Perier
Several drivers create their own devicetree property when they register
poweroff capabilities. This is for example the case for mfd, regulator
or power drivers which define "vendor,system-power-controller" property.
This patch adds support for a standard property "poweroff-source"
which marks the device as able to shutdown the system.
---
drivers/regulator/of_regulator.c | 12 ++++++++++++
include/linux/regulator/of_regulator.h | 6 ++++++
2 files changed, 18 insertions(+)
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 7a51814..8b898e6 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -240,3 +240,15 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
return init_data;
}
+
+/**
+ * is_system_poweroff_source - Tells if poweroff-source is found for device_node
+ *
+ * return true if present false otherwise
+ */
+bool is_system_poweroff_source(const struct device_node *np)
+{
+ return of_property_read_bool(np, "poweroff-source");
+}
+EXPORT_SYMBOL_GPL(is_system_poweroff_source);
diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
index f921796..9d8fbb2 100644
--- a/include/linux/regulator/of_regulator.h
+++ b/include/linux/regulator/of_regulator.h
@@ -20,6 +20,7 @@ extern struct regulator_init_data
extern int of_regulator_match(struct device *dev, struct device_node *node,
struct of_regulator_match *matches,
unsigned int num_matches);
+extern bool is_system_poweroff_source(const struct device_node *np);
#else
static inline struct regulator_init_data
*of_get_regulator_init_data(struct device *dev,
@@ -35,6 +36,11 @@ static inline int of_regulator_match(struct device *dev,
{
return 0;
}
+
+static inline bool is_system_poweroff_source(const struct device_node *np)
+{
+ return false;
+}
#endif /* CONFIG_OF */
#endif /* __LINUX_OF_REG_H */
--
1.9.1

[Grant Likely]

Post by PERIER Romain
This is not the final location, I have no idea exactly where I might
put this helper function. This is why I ask you your opinion (and
also, this is why that's a proposal)

Post by Romain Perier
Several drivers create their own devicetree property when they register
poweroff capabilities. This is for example the case for mfd, regulator
or power drivers which define "vendor,system-power-controller" property.
This patch adds support for a standard property "poweroff-source"
which marks the device as able to shutdown the system.
---
drivers/regulator/of_regulator.c | 12 ++++++++++++
include/linux/regulator/of_regulator.h | 6 ++++++
2 files changed, 18 insertions(+)
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 7a51814..8b898e6 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -240,3 +240,15 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
return init_data;
}
+
+/**
+ * is_system_poweroff_source - Tells if poweroff-source is found for device_node
+ *
+ * return true if present false otherwise
+ */
+bool is_system_poweroff_source(const struct device_node *np)
+{
+ return of_property_read_bool(np, "poweroff-source");
+}
+EXPORT_SYMBOL_GPL(is_system_poweroff_source);

Hi Romain,

This is an extremely simple wrapper. It may be best to simply make it
a static inline. It is also generic enough that I don't have a problem
with it living in include/linux/of.h; but of_regulator.h would be fine
too.

Since this is a device tree specific function (it doesn't work with
ACPI or platform_data), you should prefix the function name with of_

g.


What I'm more concerned about is the semantics of the property. What
do the generic code paths gain by standardizing this property? Is it
expected that

Post by PERIER Romain

Post by Romain Perier
diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
index f921796..9d8fbb2 100644
--- a/include/linux/regulator/of_regulator.h
+++ b/include/linux/regulator/of_regulator.h
@@ -20,6 +20,7 @@ extern struct regulator_init_data
extern int of_regulator_match(struct device *dev, struct device_node *node,
struct of_regulator_match *matches,
unsigned int num_matches);
+extern bool is_system_poweroff_source(const struct device_node *np);
#else
static inline struct regulator_init_data
*of_get_regulator_init_data(struct device *dev,
@@ -35,6 +36,11 @@ static inline int of_regulator_match(struct device *dev,
{
return 0;
}
+
+static inline bool is_system_poweroff_source(const struct device_node *np)
+{
+ return false;
+}
#endif /* CONFIG_OF */
#endif /* __LINUX_OF_REG_H */
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Mark Brown]

Post by Grant Likely
This is an extremely simple wrapper. It may be best to simply make it
a static inline. It is also generic enough that I don't have a problem
with it living in include/linux/of.h; but of_regulator.h would be fine
too.

I think of.h might be a better idea - it's not something that needs to
be regulator specific, system power controllers could provide the same
functionality but may not offer any regulator control to Linux.

[PERIER Romain]

Post by Grant Likely
What I'm more concerned about is the semantics of the property. What
do the generic code paths gain by standardizing this property? Is it
expected that

We really need to come up with a standard property for this and document
it rather than continuing to add individual device specific properties
each time a driver adds poweroff capability,
all doing the same thing (a lot of regulators driver, mfd drivers, soc
specific drivers, power drivers already do that, that's very redudant)
. This is a simple unification logic.
About its name, I found my inspiration with "wakeup-source" which
marks an device as able to wakeup the system, poweroff capability is
exactly the same
except that the device will control the power of the system, so I
choose "poweroff-source". However, suggestions are welcome ;)

About of_regulator.c, I agree with Mark. poweroff capability is not
really specific to regulators, so it does not make sense to put the
helper there, imho.

Romain
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Grant Likely]

Post by PERIER Romain

Post by Grant Likely
What I'm more concerned about is the semantics of the property. What
do the generic code paths gain by standardizing this property? Is it
expected that

We really need to come up with a standard property for this and document
it rather than continuing to add individual device specific properties
each time a driver adds poweroff capability,
all doing the same thing (a lot of regulators driver, mfd drivers, soc
specific drivers, power drivers already do that, that's very redudant)
. This is a simple unification logic.

Yes, it's fine. I accidentally left that paragraph in when I sent my
reply. I started writing that concern, and then thought better of it.
The property is fine.

g.

[Heiko Stübner]

Hi Grant,

Post by Grant Likely
What I'm more concerned about is the semantics of the property. What
do the generic code paths gain by standardizing this property? Is it
expected that

[seems to be missing some text in the original mail]

We currently see an influx of system-power-controller variants. While in the
past it only was
ti,system-power-controller

there is now already
maxim,system-power-controller
rockchip,system-power-controller

Romain's work would introduce "active-semi,system-power-controller", I have a
"netronix,system-power-controller" sitting in some distant tree and there may
be more already waiting somewhere.

So in the worst case I'd think you could expect such a property for every
pmic-vendor in vendor-prefixes.txt ... as it seems to be a quite common use-
case these days to have the pmic handle system power on its own.


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Mark Brown]

Post by Romain Perier
Several drivers create their own devicetree property when they register
poweroff capabilities. This is for example the case for mfd, regulator

This seems fine from a code point of view but as discussed it's probably
better placed outside of the regulator API. Making it a static inline
doesn't seem like a bad idea either.