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 <[email protected]>
---
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 */
--
1.9.1
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 <[email protected]>
---
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 = ®ulators[i];
--
1.9.1
Signed-off-by: Romain Perier <[email protected]>
---
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:
--
1.9.1
Add "poweroff-source" property to act8846 node.
shutdown/poweroff commands are now handled for this board.
Signed-off-by: Romain Perier <[email protected]>
---
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";
--
1.9.1
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)
2014-10-07 21:45 GMT+02:00 Romain Perier <[email protected]>:
> 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 <[email protected]>
> ---
> 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 */
> --
> 1.9.1
>
On Tue, Oct 7, 2014 at 8:43 PM, PERIER Romain <[email protected]> wrote:
> 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)
>
> 2014-10-07 21:45 GMT+02:00 Romain Perier <[email protected]>:
>> 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 <[email protected]>
>> ---
>> 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);
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
>> 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
>>
On Fri, Oct 10, 2014 at 12:47:08PM +0100, Grant Likely wrote:
> 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.
>
> 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
Hi Grant,
Am Freitag, 10. Oktober 2014, 12:47:08 schrieb 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
On Fri, Oct 10, 2014 at 1:29 PM, PERIER Romain <[email protected]> wrote:
>>
>> 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.
On Tue, Oct 07, 2014 at 07:45:01PM +0000, Romain Perier wrote:
> 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.
On Tue, Oct 07, 2014 at 07:45:02PM +0000, Romain Perier wrote:
> 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 <[email protected]>
but...
> + 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.
On Tue, Oct 07, 2014 at 07:45:04PM +0000, Romain Perier wrote:
> Signed-off-by: Romain Perier <[email protected]>
> ---
> 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...
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
2014-10-13 15:17 GMT+02:00 Mark Brown <[email protected]>:
> On Tue, Oct 07, 2014 at 07:45:04PM +0000, Romain Perier wrote:
>> Signed-off-by: Romain Perier <[email protected]>
>> ---
>> 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...