[RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[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@gmail.com>
---
 include/linux/of.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index 6545e7a..27b3ba1 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -866,4 +866,15 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
 /* CONFIG_OF_RESOLVE api */
 extern int of_resolve_phandles(struct device_node *tree);
 
+/**
+ * of_system_has_poweroff_source - Tells if poweroff-source is found for device_node
+ * @np: Pointer to the given device_node
+ *
+ * return true if present false otherwise
+ */
+static inline bool of_system_has_poweroff_source(const struct device_node *np)
+{
+	return of_property_read_bool(np, "poweroff-source");
+}
+
 #endif /* _LINUX_OF_H */
-- 
1.9.1

[RFC PATCH v3 2/5] regulator: act8865: Add support to turn off all outputs

[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 <romain.perier@gmail.com>
---
 drivers/regulator/act8865-regulator.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index afd06f9..76301ed 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,17 @@ static int act8865_pmic_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	if (of_system_has_poweroff_source(dev->of_node)) {
+		if (!pm_power_off) {
+			act8865_i2c_client = client;
+			act8865->off_reg = off_reg;
+			act8865->off_mask = off_mask;
+			pm_power_off = act8865_power_off;
+		} else {
+			dev_err(dev, "Failed to set poweroff capability, already defined\n");
+		}
+	}
+
 	/* Finally register devices */
 	for (i = 0; i < num_regulators; i++) {
 		const struct regulator_desc *desc = &regulators[i];
-- 
1.9.1

[RFC PATCH v3 3/5] ARM: dts: rockchip: Enable power off in pmic for Radxa Rock

[Romain Perier]

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

Signed-off-by: Romain Perier <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";
-- 
1.9.1

[RFC PATCH v3 4/5] dt-bindings: Document the standard property "poweroff-source"

[Romain Perier]

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 Documentation/devicetree/bindings/power/poweroff.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/poweroff.txt

diff --git a/Documentation/devicetree/bindings/power/poweroff.txt b/Documentation/devicetree/bindings/power/poweroff.txt
new file mode 100644
index 0000000..845868b
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/poweroff.txt
@@ -0,0 +1,18 @@
+* Generic Poweroff capability
+
+Power-management integrated circuits or miscellaneous harware components are
+sometimes able to control the system power. The device driver associated to these
+components might needs to define poweroff capability, which tells to the kernel
+how to switch off the system. The corresponding driver must have the standard
+property "poweroff-source" in its device node. This property marks the device as
+able to shutdown the system. In order to test if this property is found
+programmatically, use the helper function "of_system_has_poweroff_source" from
+of.h .
+
+Example:
+
+act8846: act8846@5 {
+	 compatible = "active-semi,act8846";
+	 status = "okay";
+	 poweroff-source;
+}
-- 
1.9.1

[RFC PATCH v3 5/5] dt-bindings: Document the property poweroff-source for act8865 regulator

[Romain Perier]

Signed-off-by: Romain Perier <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..01a5b07 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. See Documentation/devicetree/bindings/power/poweroff.txt .
+
 Any standard regulator properties can be used to configure the single regulator.
 
 The valid names for regulators are:
-- 
1.9.1

Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[Grant Likely]

On Tue, 14 Oct 2014 06:31:09 +0000
, Romain Perier <romain.perier@gmail.com>
 wrote:
> 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@gmail.com>

Acked-by: Grant Likely <grant.likely@linaro.org>

And same for the documentation patches.

g.

> ---
>  include/linux/of.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 6545e7a..27b3ba1 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -866,4 +866,15 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
>  /* CONFIG_OF_RESOLVE api */
>  extern int of_resolve_phandles(struct device_node *tree);
>  
> +/**
> + * of_system_has_poweroff_source - Tells if poweroff-source is found for device_node
> + * @np: Pointer to the given device_node
> + *
> + * return true if present false otherwise
> + */
> +static inline bool of_system_has_poweroff_source(const struct device_node *np)
> +{
> +	return of_property_read_bool(np, "poweroff-source");
> +}
> +
>  #endif /* _LINUX_OF_H */
> -- 
> 1.9.1
> 

Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[Mark Brown]

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

On Wed, Oct 15, 2014 at 02:41:42PM +0200, Grant Likely wrote:
> , Romain Perier <romain.perier@gmail.com>
>  wrote:
> > 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@gmail.com>

> Acked-by: Grant Likely <grant.likely@linaro.org>

> And same for the documentation patches.

I guess I should apply these (except the DTS update) since the first
user that's being added is a regulator driver?

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

Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[Heiko Stübner]

Am Mittwoch, 15. Oktober 2014, 15:42:45 schrieb Mark Brown:
> On Wed, Oct 15, 2014 at 02:41:42PM +0200, Grant Likely wrote:
> > , Romain Perier <romain.perier@gmail.com>
> > 
> >  wrote:
> > > 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@gmail.com>
> > 
> > Acked-by: Grant Likely <grant.likely@linaro.org>
> > 
> > And same for the documentation patches.
> 
> I guess I should apply these (except the DTS update) since the first
> user that's being added is a regulator driver?

I'd think so.
In any case, I'll take the "ARM: dts: ..." patch if you take the others.


Heiko

Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[Mark Brown]

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

On Wed, Oct 15, 2014 at 03:56:03PM +0200, Heiko Stübner wrote:
> Am Mittwoch, 15. Oktober 2014, 15:42:45 schrieb Mark Brown:

> > I guess I should apply these (except the DTS update) since the first
> > user that's being added is a regulator driver?

> I'd think so.
> In any case, I'll take the "ARM: dts: ..." patch if you take the others.

Sounds like a plan.  I just tried applying and got some conflicts but
I'm guessing that this is due to changes that are landing in the merge
window so I'll try again once -rc1 is out.

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

Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[PERIER Romain]

Hi all,

I am just curious but where do you plan to merge this serie ? in
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git ?

You have conflicts because I use linux-next as base repository.


Thanks for your feebacks (all of you).
Romain

2014-10-15 16:03 GMT+02:00 Mark Brown <broonie@kernel.org>:
> On Wed, Oct 15, 2014 at 03:56:03PM +0200, Heiko Stübner wrote:
>> Am Mittwoch, 15. Oktober 2014, 15:42:45 schrieb Mark Brown:
>
>> > I guess I should apply these (except the DTS update) since the first
>> > user that's being added is a regulator driver?
>
>> I'd think so.
>> In any case, I'll take the "ARM: dts: ..." patch if you take the others.
>
> Sounds like a plan.  I just tried applying and got some conflicts but
> I'm guessing that this is due to changes that are landing in the merge
> window so I'll try again once -rc1 is out.

Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[Heiko Stübner]

Hi Romain,

Am Freitag, 17. Oktober 2014, 08:01:31 schrieb PERIER Romain:
> Hi all,
> 
> I am just curious but where do you plan to merge this serie ? in
> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git ?

The single dts patch will be going into my dts branch and the rest will go 
through Mark's regulator tree. As Mark said, he'll try to apply these again 
once 3.18-rc1 is released and if it still doesn't apply then, he'll probably 
ask you to rebase them onto the regulator tree [which will at the time include 
everything that is in -rc1].


Heiko



> 2014-10-15 16:03 GMT+02:00 Mark Brown <broonie@kernel.org>:
> > On Wed, Oct 15, 2014 at 03:56:03PM +0200, Heiko Stübner wrote:
> >> Am Mittwoch, 15. Oktober 2014, 15:42:45 schrieb Mark Brown:
> >> > I guess I should apply these (except the DTS update) since the first
> >> > user that's being added is a regulator driver?
> >> 
> >> I'd think so.
> >> In any case, I'll take the "ARM: dts: ..." patch if you take the others.
> > 
> > Sounds like a plan.  I just tried applying and got some conflicts but
> > I'm guessing that this is due to changes that are landing in the merge
> > window so I'll try again once -rc1 is out.

Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[PERIER Romain]

Hi Heiko,

Oh sure, no problem. It was just to understand better how things will happen ;)
If my patches needs to be rebased, will do, of course.

Have a nice day,
Romain

2014-10-17 8:06 GMT+02:00 Heiko Stübner <heiko@sntech.de>:
> Hi Romain,
>
> Am Freitag, 17. Oktober 2014, 08:01:31 schrieb PERIER Romain:
>> Hi all,
>>
>> I am just curious but where do you plan to merge this serie ? in
>> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git ?
>
> The single dts patch will be going into my dts branch and the rest will go
> through Mark's regulator tree. As Mark said, he'll try to apply these again
> once 3.18-rc1 is released and if it still doesn't apply then, he'll probably
> ask you to rebase them onto the regulator tree [which will at the time include
> everything that is in -rc1].
>
>
> Heiko
>
>
>
>> 2014-10-15 16:03 GMT+02:00 Mark Brown <broonie@kernel.org>:
>> > On Wed, Oct 15, 2014 at 03:56:03PM +0200, Heiko Stübner wrote:
>> >> Am Mittwoch, 15. Oktober 2014, 15:42:45 schrieb Mark Brown:
>> >> > I guess I should apply these (except the DTS update) since the first
>> >> > user that's being added is a regulator driver?
>> >>
>> >> I'd think so.
>> >> In any case, I'll take the "ARM: dts: ..." patch if you take the others.
>> >
>> > Sounds like a plan.  I just tried applying and got some conflicts but
>> > I'm guessing that this is due to changes that are landing in the merge
>> > window so I'll try again once -rc1 is out.
>

Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[PERIER Romain]

Hi all,

Do I need to rebase my patches onto the regulator tree or it's okay ?

Have a nice day,
Romain

2014-10-17 9:23 GMT+02:00 PERIER Romain <romain.perier@gmail.com>:
> Hi Heiko,
>
> Oh sure, no problem. It was just to understand better how things will happen ;)
> If my patches needs to be rebased, will do, of course.
>
> Have a nice day,
> Romain
>
> 2014-10-17 8:06 GMT+02:00 Heiko Stübner <heiko@sntech.de>:
>> Hi Romain,
>>
>> Am Freitag, 17. Oktober 2014, 08:01:31 schrieb PERIER Romain:
>>> Hi all,
>>>
>>> I am just curious but where do you plan to merge this serie ? in
>>> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git ?
>>
>> The single dts patch will be going into my dts branch and the rest will go
>> through Mark's regulator tree. As Mark said, he'll try to apply these again
>> once 3.18-rc1 is released and if it still doesn't apply then, he'll probably
>> ask you to rebase them onto the regulator tree [which will at the time include
>> everything that is in -rc1].
>>
>>
>> Heiko
>>
>>
>>
>>> 2014-10-15 16:03 GMT+02:00 Mark Brown <broonie@kernel.org>:
>>> > On Wed, Oct 15, 2014 at 03:56:03PM +0200, Heiko Stübner wrote:
>>> >> Am Mittwoch, 15. Oktober 2014, 15:42:45 schrieb Mark Brown:
>>> >> > I guess I should apply these (except the DTS update) since the first
>>> >> > user that's being added is a regulator driver?
>>> >>
>>> >> I'd think so.
>>> >> In any case, I'll take the "ARM: dts: ..." patch if you take the others.
>>> >
>>> > Sounds like a plan.  I just tried applying and got some conflicts but
>>> > I'm guessing that this is due to changes that are landing in the merge
>>> > window so I'll try again once -rc1 is out.
>>

Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[Mark Brown]

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

On Tue, Oct 14, 2014 at 06:31:09AM +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
> 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.

Applied everything except the DT update, thanks.

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

Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[Johan Hovold]

[ +CC: Guenter, Lee, linux-pm ]

On Tue, Oct 14, 2014 at 06:31:09AM +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
> or power drivers which define "vendor,system-power-controller" property.
> This patch adds support for a standard property "poweroff-source"

Shouldn't this property really be called "power-off-source" or even
"power-off-controller"?

The power-off handler call-chain infrastructure is about to be merged
and will be using power[-_ ]off (i.e. not "poweroff") consistently (at
least in its interface).

Furthermore, isn't "controller" as in "power-off-controller" more
appropriate than "source" in this case? We have wake-up sources, which
might appear analogous, but that really isn't the same thing.

I now this has already been merged to the regulator tree, but there's
still still time to fix this.

> which marks the device as able to shutdown the system.
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  include/linux/of.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 6545e7a..27b3ba1 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -866,4 +866,15 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
>  /* CONFIG_OF_RESOLVE api */
>  extern int of_resolve_phandles(struct device_node *tree);
>  
> +/**
> + * of_system_has_poweroff_source - Tells if poweroff-source is found for device_node
> + * @np: Pointer to the given device_node
> + *
> + * return true if present false otherwise
> + */
> +static inline bool of_system_has_poweroff_source(const struct device_node *np)

Why "system_has"? Shouldn't this be of_is_power_off_source (controller)?

> +{
> +	return of_property_read_bool(np, "poweroff-source");
> +}
> +
>  #endif /* _LINUX_OF_H */

Thanks,
Johan

Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[Romain Perier]

Hi Johan,

If that's still possible to do these changes, I am opened to suggestions.

2014-10-23 11:53 GMT+02:00 Johan Hovold <johan@kernel.org>:
> [ +CC: Guenter, Lee, linux-pm ]
>
> On Tue, Oct 14, 2014 at 06:31:09AM +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
>> or power drivers which define "vendor,system-power-controller" property.
>> This patch adds support for a standard property "poweroff-source"
>
> Shouldn't this property really be called "power-off-source" or even
> "power-off-controller"?
>
> The power-off handler call-chain infrastructure is about to be merged
> and will be using power[-_ ]off (i.e. not "poweroff") consistently (at
> least in its interface).

"poweroff" or "power-off", I don't care. If people prefer "power-off",
choose this name :)

>
> Furthermore, isn't "controller" as in "power-off-controller" more
> appropriate than "source" in this case? We have wake-up sources, which
> might appear analogous, but that really isn't the same thing.

As I said, the idea with "power-off-source" (or "poweroff-source",
that's not the point here) is to mark the device as able to poweroff
the system, like "wakeup-source" which marks the device as able to
wakeup the system.
This is why I chose this name, because it is quite similar to wakeup
property except that it is for handling power, so it did make sense to
me.

The question is: what is the advantage of the suffix "controller"
compared to "source" ?

>
> I now this has already been merged to the regulator tree, but there's
> still still time to fix this.
>
>> which marks the device as able to shutdown the system.
>>
>> Signed-off-by: Romain Perier <romain.perier@gmail.com>
>> ---
>>  include/linux/of.h | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index 6545e7a..27b3ba1 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -866,4 +866,15 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
>>  /* CONFIG_OF_RESOLVE api */
>>  extern int of_resolve_phandles(struct device_node *tree);
>>
>> +/**
>> + * of_system_has_poweroff_source - Tells if poweroff-source is found for device_node
>> + * @np: Pointer to the given device_node
>> + *
>> + * return true if present false otherwise
>> + */
>> +static inline bool of_system_has_poweroff_source(const struct device_node *np)
>
> Why "system_has"? Shouldn't this be of_is_power_off_source (controller)?

Note that the current custom vendor properties contain "system-" as prefix ;)

we have several possibilities:
- of_system_has_power_off_source()
- of_has_power_off_source()

We should either to use "has" or "is" as prefix because that's a
predicate function.
I would prefer "has" since it refers to a property inside a node :
this node "has" the corresponding property, so "is" is not a good
candidate.

Have a nice day,
Romain

Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[Johan Hovold]

[+CC: Felipe ]

On Sat, Oct 25, 2014 at 09:28:36AM +0200, Romain Perier wrote:
> Hi Johan,
> 
> If that's still possible to do these changes, I am opened to suggestions.

Before v3.18 comes out, we can always change it with a follow-up patch.

> 2014-10-23 11:53 GMT+02:00 Johan Hovold <johan@kernel.org>:
> > [ +CC: Guenter, Lee, linux-pm ]
> >
> > On Tue, Oct 14, 2014 at 06:31:09AM +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
> >> or power drivers which define "vendor,system-power-controller" property.
> >> This patch adds support for a standard property "poweroff-source"
> >
> > Shouldn't this property really be called "power-off-source" or even
> > "power-off-controller"?
> >
> > The power-off handler call-chain infrastructure is about to be merged
> > and will be using power[-_ ]off (i.e. not "poweroff") consistently (at
> > least in its interface).
> 
> "poweroff" or "power-off", I don't care. If people prefer "power-off",
> choose this name :)

Let's try to stick to power off (and power_off) then.

> > Furthermore, isn't "controller" as in "power-off-controller" more
> > appropriate than "source" in this case? We have wake-up sources, which
> > might appear analogous, but that really isn't the same thing.
> 
> As I said, the idea with "power-off-source" (or "poweroff-source",
> that's not the point here) is to mark the device as able to poweroff
> the system, like "wakeup-source" which marks the device as able to
> wakeup the system.
> This is why I chose this name, because it is quite similar to wakeup
> property except that it is for handling power, so it did make sense to
> me.
> 
> The question is: what is the advantage of the suffix "controller"
> compared to "source" ?

Yeah, I figured you had been inspired by the "wakeup-source" property.

The problem is that "source" tends to be used for inputs, for example,
wake-up source, interrupt source, entropy source, etc. Something that is
outside of the control of the OS. Contrary to for instance an output
which turns the system-power off.

> > I now this has already been merged to the regulator tree, but there's
> > still still time to fix this.
> >
> >> which marks the device as able to shutdown the system.
> >>
> >> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> >> ---
> >>  include/linux/of.h | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/include/linux/of.h b/include/linux/of.h
> >> index 6545e7a..27b3ba1 100644
> >> --- a/include/linux/of.h
> >> +++ b/include/linux/of.h
> >> @@ -866,4 +866,15 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
> >>  /* CONFIG_OF_RESOLVE api */
> >>  extern int of_resolve_phandles(struct device_node *tree);
> >>
> >> +/**
> >> + * of_system_has_poweroff_source - Tells if poweroff-source is found for device_node
> >> + * @np: Pointer to the given device_node
> >> + *
> >> + * return true if present false otherwise
> >> + */
> >> +static inline bool of_system_has_poweroff_source(const struct device_node *np)
> >
> > Why "system_has"? Shouldn't this be of_is_power_off_source (controller)?
> 
> Note that the current custom vendor properties contain "system-" as prefix ;)

Yes, but you dropped it. ;)

And it's not the system that has the property (e.g. "poweroff-source"),
it's the node (or the device it describes).
 
> we have several possibilities:
> - of_system_has_power_off_source()
> - of_has_power_off_source()
> 
> We should either to use "has" or "is" as prefix because that's a
> predicate function.
> I would prefer "has" since it refers to a property inside a node :
> this node "has" the corresponding property, so "is" is not a good
> candidate.

The boolean property in question describes a feature of the node
(device). Say the feature would be redness and call the property "red".
You would then generally ask whether the node *is red*, rather than
whether it has (the property) red (or has redness).

I'm actually inclined to just sticking to the current name
"system-power-controller" and just drop the vendor prefixes. Perhaps
your helper function can be used to parse both versions (i.e. with or
without a vendor prefix) as we will still need to support both.

I suggest you call that helper function

	of_is_system_power_controller(node)

or alternatively

	of_is_power_off_controller(node)

if that property name is preferred.

Note also that in at least one case (rtc-omap, patches in mm, see [1])
the property describes that the RTC is used to control an external PMIC,
which both allows us to power off the system *and* power back on again
on subsequent RTC alarms. This seems to suggest that the more generic
"system-power-controller" property name should be preferred.

Thanks,
Johan

[1] https://lkml.org/lkml/2014/10/21/631

Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[Heiko Stübner]

Am Samstag, 25. Oktober 2014, 10:37:33 schrieb Johan Hovold:
> [+CC: Felipe ]
> 
> On Sat, Oct 25, 2014 at 09:28:36AM +0200, Romain Perier wrote:
> > Hi Johan,
> > 
> > If that's still possible to do these changes, I am opened to suggestions.
> 
> Before v3.18 comes out, we can always change it with a follow-up patch.
> 
> > 2014-10-23 11:53 GMT+02:00 Johan Hovold <johan@kernel.org>:
> > > [ +CC: Guenter, Lee, linux-pm ]
> > > 
> > > On Tue, Oct 14, 2014 at 06:31:09AM +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
> > >> or power drivers which define "vendor,system-power-controller"
> > >> property.
> > >> This patch adds support for a standard property "poweroff-source"
> > > 
> > > Shouldn't this property really be called "power-off-source" or even
> > > "power-off-controller"?
> > > 
> > > The power-off handler call-chain infrastructure is about to be merged
> > > and will be using power[-_ ]off (i.e. not "poweroff") consistently (at
> > > least in its interface).
> > 
> > "poweroff" or "power-off", I don't care. If people prefer "power-off",
> > choose this name :)
> 
> Let's try to stick to power off (and power_off) then.
> 
> > > Furthermore, isn't "controller" as in "power-off-controller" more
> > > appropriate than "source" in this case? We have wake-up sources, which
> > > might appear analogous, but that really isn't the same thing.
> > 
> > As I said, the idea with "power-off-source" (or "poweroff-source",
> > that's not the point here) is to mark the device as able to poweroff
> > the system, like "wakeup-source" which marks the device as able to
> > wakeup the system.
> > This is why I chose this name, because it is quite similar to wakeup
> > property except that it is for handling power, so it did make sense to
> > me.
> > 
> > The question is: what is the advantage of the suffix "controller"
> > compared to "source" ?
> 
> Yeah, I figured you had been inspired by the "wakeup-source" property.
> 
> The problem is that "source" tends to be used for inputs, for example,
> wake-up source, interrupt source, entropy source, etc. Something that is
> outside of the control of the OS. Contrary to for instance an output
> which turns the system-power off.
> 
> > > I now this has already been merged to the regulator tree, but there's
> > > still still time to fix this.
> > > 
> > >> which marks the device as able to shutdown the system.
> > >> 
> > >> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > >> ---
> > >> 
> > >>  include/linux/of.h | 11 +++++++++++
> > >>  1 file changed, 11 insertions(+)
> > >> 
> > >> diff --git a/include/linux/of.h b/include/linux/of.h
> > >> index 6545e7a..27b3ba1 100644
> > >> --- a/include/linux/of.h
> > >> +++ b/include/linux/of.h
> > >> @@ -866,4 +866,15 @@ static inline int
> > >> of_changeset_update_property(struct of_changeset *ocs,> >> 
> > >>  /* CONFIG_OF_RESOLVE api */
> > >>  extern int of_resolve_phandles(struct device_node *tree);
> > >> 
> > >> +/**
> > >> + * of_system_has_poweroff_source - Tells if poweroff-source is found
> > >> for device_node + * @np: Pointer to the given device_node
> > >> + *
> > >> + * return true if present false otherwise
> > >> + */
> > >> +static inline bool of_system_has_poweroff_source(const struct
> > >> device_node *np)> > 
> > > Why "system_has"? Shouldn't this be of_is_power_off_source (controller)?
> > 
> > Note that the current custom vendor properties contain "system-" as prefix
> > ;)
> Yes, but you dropped it. ;)
> 
> And it's not the system that has the property (e.g. "poweroff-source"),
> it's the node (or the device it describes).
> 
> > we have several possibilities:
> > - of_system_has_power_off_source()
> > - of_has_power_off_source()
> > 
> > We should either to use "has" or "is" as prefix because that's a
> > predicate function.
> > I would prefer "has" since it refers to a property inside a node :
> > this node "has" the corresponding property, so "is" is not a good
> > candidate.
> 
> The boolean property in question describes a feature of the node
> (device). Say the feature would be redness and call the property "red".
> You would then generally ask whether the node *is red*, rather than
> whether it has (the property) red (or has redness).
> 
> I'm actually inclined to just sticking to the current name
> "system-power-controller" and just drop the vendor prefixes. Perhaps
> your helper function can be used to parse both versions (i.e. with or
> without a vendor prefix) as we will still need to support both.
> 
> I suggest you call that helper function
> 
> 	of_is_system_power_controller(node)
> 
> or alternatively
> 
> 	of_is_power_off_controller(node)
> 
> if that property name is preferred.
> 
> Note also that in at least one case (rtc-omap, patches in mm, see [1])
> the property describes that the RTC is used to control an external PMIC,
> which both allows us to power off the system *and* power back on again
> on subsequent RTC alarms. This seems to suggest that the more generic
> "system-power-controller" property name should be preferred.

just as sidenote, I'll hold off on applying patch3 (the dts change) then.

Romain, after you two (and maybe Mark) agree on the final naming of the 
property and function you'd need to
- send a followup patch against Marks tree, implementing the changes
- a new patch adding the property to the Radxa board


Heiko

Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[Romain Perier]

Johan,

You convinced me. I will add an helper function
"of_is_system_power_controller(node)" which is compatible with both
properties: with or without the vendor prefix (until everything switch
to the new one).
In this case , we can adapt all drivers without break compatibility
and in few months if we plan to remove this compability we will just
need to modify this helper function.

Heiko: will do, thanks your help. I will use the regulator tree as
based repository and then send fixes to Mark.

Romain



2014-10-26 12:53 GMT+01:00 Heiko Stübner <heiko@sntech.de>:
> Am Samstag, 25. Oktober 2014, 10:37:33 schrieb Johan Hovold:
>> [+CC: Felipe ]
>>
>> On Sat, Oct 25, 2014 at 09:28:36AM +0200, Romain Perier wrote:
>> > Hi Johan,
>> >
>> > If that's still possible to do these changes, I am opened to suggestions.
>>
>> Before v3.18 comes out, we can always change it with a follow-up patch.
>>
>> > 2014-10-23 11:53 GMT+02:00 Johan Hovold <johan@kernel.org>:
>> > > [ +CC: Guenter, Lee, linux-pm ]
>> > >
>> > > On Tue, Oct 14, 2014 at 06:31:09AM +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
>> > >> or power drivers which define "vendor,system-power-controller"
>> > >> property.
>> > >> This patch adds support for a standard property "poweroff-source"
>> > >
>> > > Shouldn't this property really be called "power-off-source" or even
>> > > "power-off-controller"?
>> > >
>> > > The power-off handler call-chain infrastructure is about to be merged
>> > > and will be using power[-_ ]off (i.e. not "poweroff") consistently (at
>> > > least in its interface).
>> >
>> > "poweroff" or "power-off", I don't care. If people prefer "power-off",
>> > choose this name :)
>>
>> Let's try to stick to power off (and power_off) then.
>>
>> > > Furthermore, isn't "controller" as in "power-off-controller" more
>> > > appropriate than "source" in this case? We have wake-up sources, which
>> > > might appear analogous, but that really isn't the same thing.
>> >
>> > As I said, the idea with "power-off-source" (or "poweroff-source",
>> > that's not the point here) is to mark the device as able to poweroff
>> > the system, like "wakeup-source" which marks the device as able to
>> > wakeup the system.
>> > This is why I chose this name, because it is quite similar to wakeup
>> > property except that it is for handling power, so it did make sense to
>> > me.
>> >
>> > The question is: what is the advantage of the suffix "controller"
>> > compared to "source" ?
>>
>> Yeah, I figured you had been inspired by the "wakeup-source" property.
>>
>> The problem is that "source" tends to be used for inputs, for example,
>> wake-up source, interrupt source, entropy source, etc. Something that is
>> outside of the control of the OS. Contrary to for instance an output
>> which turns the system-power off.
>>
>> > > I now this has already been merged to the regulator tree, but there's
>> > > still still time to fix this.
>> > >
>> > >> which marks the device as able to shutdown the system.
>> > >>
>> > >> Signed-off-by: Romain Perier <romain.perier@gmail.com>
>> > >> ---
>> > >>
>> > >>  include/linux/of.h | 11 +++++++++++
>> > >>  1 file changed, 11 insertions(+)
>> > >>
>> > >> diff --git a/include/linux/of.h b/include/linux/of.h
>> > >> index 6545e7a..27b3ba1 100644
>> > >> --- a/include/linux/of.h
>> > >> +++ b/include/linux/of.h
>> > >> @@ -866,4 +866,15 @@ static inline int
>> > >> of_changeset_update_property(struct of_changeset *ocs,> >>
>> > >>  /* CONFIG_OF_RESOLVE api */
>> > >>  extern int of_resolve_phandles(struct device_node *tree);
>> > >>
>> > >> +/**
>> > >> + * of_system_has_poweroff_source - Tells if poweroff-source is found
>> > >> for device_node + * @np: Pointer to the given device_node
>> > >> + *
>> > >> + * return true if present false otherwise
>> > >> + */
>> > >> +static inline bool of_system_has_poweroff_source(const struct
>> > >> device_node *np)> >
>> > > Why "system_has"? Shouldn't this be of_is_power_off_source (controller)?
>> >
>> > Note that the current custom vendor properties contain "system-" as prefix
>> > ;)
>> Yes, but you dropped it. ;)
>>
>> And it's not the system that has the property (e.g. "poweroff-source"),
>> it's the node (or the device it describes).
>>
>> > we have several possibilities:
>> > - of_system_has_power_off_source()
>> > - of_has_power_off_source()
>> >
>> > We should either to use "has" or "is" as prefix because that's a
>> > predicate function.
>> > I would prefer "has" since it refers to a property inside a node :
>> > this node "has" the corresponding property, so "is" is not a good
>> > candidate.
>>
>> The boolean property in question describes a feature of the node
>> (device). Say the feature would be redness and call the property "red".
>> You would then generally ask whether the node *is red*, rather than
>> whether it has (the property) red (or has redness).
>>
>> I'm actually inclined to just sticking to the current name
>> "system-power-controller" and just drop the vendor prefixes. Perhaps
>> your helper function can be used to parse both versions (i.e. with or
>> without a vendor prefix) as we will still need to support both.
>>
>> I suggest you call that helper function
>>
>>       of_is_system_power_controller(node)
>>
>> or alternatively
>>
>>       of_is_power_off_controller(node)
>>
>> if that property name is preferred.
>>
>> Note also that in at least one case (rtc-omap, patches in mm, see [1])
>> the property describes that the RTC is used to control an external PMIC,
>> which both allows us to power off the system *and* power back on again
>> on subsequent RTC alarms. This seems to suggest that the more generic
>> "system-power-controller" property name should be preferred.
>
> just as sidenote, I'll hold off on applying patch3 (the dts change) then.
>
> Romain, after you two (and maybe Mark) agree on the final naming of the
> property and function you'd need to
> - send a followup patch against Marks tree, implementing the changes
> - a new patch adding the property to the Radxa board
>
>
> Heiko