2014-10-07 19:42:17

by Romain Perier

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

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


2014-10-07 19:42:20

by Romain Perier

[permalink] [raw]
Subject: [RFC PATCH v2 2/4] regulator: act8865: Add support to turn off all outputs

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 = &regulators[i];
--
1.9.1

2014-10-07 19:42:23

by Romain Perier

[permalink] [raw]
Subject: [RFC PATCH v1 4/4] dt-bindings: Document the property poweroff-source for act8865 regulator

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

2014-10-07 19:42:49

by Romain Perier

[permalink] [raw]
Subject: [RFC PATCH v1 3/4] ARM: dts: rockchip: Enable power off in pmic for Radxa Rock

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

2014-10-07 19:43:49

by Romain Perier

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

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
>

2014-10-10 11:47:34

by Grant Likely

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

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
>>

2014-10-10 11:54:48

by Mark Brown

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

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.


Attachments:
(No filename) (495.00 B)
signature.asc (473.00 B)
Digital signature
Download all attachments

2014-10-10 12:29:06

by Romain Perier

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

>
> 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

2014-10-10 12:33:45

by Heiko Stübner

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

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

2014-10-11 13:52:23

by Grant Likely

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

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.

2014-10-13 13:13:36

by Mark Brown

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

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.


Attachments:
(No filename) (392.00 B)
signature.asc (473.00 B)
Digital signature
Download all attachments

2014-10-13 13:17:25

by Mark Brown

[permalink] [raw]
Subject: Re: [RFC PATCH v2 2/4] regulator: act8865: Add support to turn off all outputs

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.


Attachments:
(No filename) (953.00 B)
signature.asc (473.00 B)
Digital signature
Download all attachments

2014-10-13 13:17:48

by Mark Brown

[permalink] [raw]
Subject: Re: [RFC PATCH v1 4/4] dt-bindings: Document the property poweroff-source for act8865 regulator

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...


Attachments:
(No filename) (429.00 B)
signature.asc (473.00 B)
Digital signature
Download all attachments

2014-10-13 13:54:26

by Romain Perier

[permalink] [raw]
Subject: Re: [RFC PATCH v1 4/4] dt-bindings: Document the property poweroff-source for act8865 regulator

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...