[PATCH v1 01/10] of: Rename "poweroff-source" property to "system-power-controller"

[PATCH v1 01/10] of: Rename "poweroff-source" property to "system-power-controller"

[Romain Perier]

As discussed on the mailing list, it makes more sense to rename this property
to "system-power-controller". Problem being that the word "source" usually tends
to be used for inputs and that is out of control of the OS. The poweroff
capability is an output which simply turns the system-power off. Also, this
property might be used by drivers which power-off the system and power back on
subsequent RTC alarms. This seems to suggest to remove "poweroff" from the
property name and to choose "system-power-controller" as the more generic name.
This patchs adds the required renaming changes and defines an helper function
which is compatible with both properties, the old one prefixed by a vendor name
and the new one without any prefix.

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 include/linux/of.h | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index 27b3ba1..c1ed2a5 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -867,14 +867,33 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
 extern int of_resolve_phandles(struct device_node *tree);
 
 /**
- * of_system_has_poweroff_source - Tells if poweroff-source is found for device_node
+ * of_is_system_power_controller - Tells if the property for controlling system
+ * power is found in 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");
+static inline bool of_is_system_power_controller(const struct device_node *np)
+{
+	struct property *pp;
+	unsigned long flags;
+	char *sep;
+	bool found = false;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	for_each_property_of_node(np, pp) {
+		if (of_prop_cmp(pp->name, "system-power-controller") == 0) {
+			found = true;
+			break;
+		}
+		sep = strchr(pp->name, ',');
+		if (sep && sep - pp->name && of_prop_cmp(sep + 1, "system-power-controller") == 0) {
+			found = true;
+			break;
+		}
+	}
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	return found;
 }
 
 #endif /* _LINUX_OF_H */
-- 
1.9.1

[PATCH v1 02/10] dt-bindings: Document the standard property "system-power-controller"

[Romain Perier]

This simply renames the previous documentation to something more generic and adds
updates according to last changes.

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

diff --git a/Documentation/devicetree/bindings/power/power-controller.txt b/Documentation/devicetree/bindings/power/power-controller.txt
new file mode 100644
index 0000000..e0a1abe
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/power-controller.txt
@@ -0,0 +1,18 @@
+* Generic system power control 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 this capability, which tells to the kernel
+how to switch off the system. The corresponding driver must have the standard
+property "system-power-controller" in its device node. This property marks the device as
+able to controller the system-power. In order to test if this property is found
+programmatically, use the helper function "of_is_system_power_controller" from
+of.h .
+
+Example:
+
+act8846: act8846@5 {
+	 compatible = "active-semi,act8846";
+	 status = "okay";
+	 system-power-controller;
+}
diff --git a/Documentation/devicetree/bindings/power/poweroff.txt b/Documentation/devicetree/bindings/power/poweroff.txt
deleted file mode 100644
index 845868b..0000000
--- a/Documentation/devicetree/bindings/power/poweroff.txt
+++ /dev/null
@@ -1,18 +0,0 @@
-* 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

Re: [PATCH v1 02/10] dt-bindings: Document the standard property "system-power-controller"

[Felipe Balbi]

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

On Mon, Oct 27, 2014 at 04:26:47PM +0000, Romain Perier wrote:
> This simply renames the previous documentation to something more generic and adds
> updates according to last changes.
> 
> Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/power/power-controller.txt     | 18 ++++++++++++++++++
>  Documentation/devicetree/bindings/power/poweroff.txt   | 18 ------------------
>  2 files changed, 18 insertions(+), 18 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/power/power-controller.txt
>  delete mode 100644 Documentation/devicetree/bindings/power/poweroff.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/power-controller.txt b/Documentation/devicetree/bindings/power/power-controller.txt
> new file mode 100644
> index 0000000..e0a1abe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/power-controller.txt
> @@ -0,0 +1,18 @@
> +* Generic system power control 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 this capability, which tells to the kernel
> +how to switch off the system. The corresponding driver must have the standard
> +property "system-power-controller" in its device node. This property marks the device as
> +able to controller the system-power. In order to test if this property is found
> +programmatically, use the helper function "of_is_system_power_controller" from
> +of.h .
> +
> +Example:
> +
> +act8846: act8846@5 {
> +	 compatible = "active-semi,act8846";
> +	 status = "okay";
> +	 system-power-controller;
> +}

there is no mention here that system-power-controller superseeds
poweroff-source and deprecates that.

-- 
balbi

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

Re: [PATCH v1 02/10] dt-bindings: Document the standard property "system-power-controller"

[Mark Brown]

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

On Mon, Oct 27, 2014 at 11:39:18AM -0500, Felipe Balbi wrote:
> On Mon, Oct 27, 2014 at 04:26:47PM +0000, Romain Perier wrote:

> > +act8846: act8846@5 {
> > +	 compatible = "active-semi,act8846";
> > +	 status = "okay";
> > +	 system-power-controller;
> > +}

> there is no mention here that system-power-controller superseeds
> poweroff-source and deprecates that.

Given that poweroff-source hasn't made it into a released kernel yet we
can probably just kill it off completely can't we?

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

Re: [PATCH v1 02/10] dt-bindings: Document the standard property "system-power-controller"

[Felipe Balbi]

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

On Mon, Oct 27, 2014 at 05:26:36PM +0000, Mark Brown wrote:
> On Mon, Oct 27, 2014 at 11:39:18AM -0500, Felipe Balbi wrote:
> > On Mon, Oct 27, 2014 at 04:26:47PM +0000, Romain Perier wrote:
> 
> > > +act8846: act8846@5 {
> > > +	 compatible = "active-semi,act8846";
> > > +	 status = "okay";
> > > +	 system-power-controller;
> > > +}
> 
> > there is no mention here that system-power-controller superseeds
> > poweroff-source and deprecates that.
> 
> Given that poweroff-source hasn't made it into a released kernel yet we
> can probably just kill it off completely can't we?

please do, then we will never have that commit in Linus' tree to give us
nightmares.

-- 
balbi

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

Re: [PATCH v1 02/10] dt-bindings: Document the standard property "system-power-controller"

[Mark Brown]

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

On Mon, Oct 27, 2014 at 12:30:25PM -0500, Felipe Balbi wrote:
> On Mon, Oct 27, 2014 at 05:26:36PM +0000, Mark Brown wrote:

> > Given that poweroff-source hasn't made it into a released kernel yet we
> > can probably just kill it off completely can't we?

> please do, then we will never have that commit in Linus' tree to give us
> nightmares.

Well, the commit isn't a particularly big deal (and got cross-merged
into another tree already) - if it's convenient to rebase it out we
probably should but it's also not the end of the world either so long as
it doesn't appear in a release.  Worst case people adopt it based on the
list discussions and we check for two properties which also isn't that
bad.

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

Re: [PATCH v1 02/10] dt-bindings: Document the standard property "system-power-controller"

[Romain Perier]

If this is possible, that's probably better to drop all these patches
(including poweroff-source) from "next", in this way it will be
possible to re-do things properly from scratch (with the new name).

If you have a look at linux-next the property "poweroff-source" is
already used by tps910 AND act8865, I don't like that (it is not used
yet by dts files). I mean, I don't really want to support
"vendor,system-power-controller", "system-power-controller" and
"poweroff-source" from "of_has_system_power_source" and do a lot of
unclean commits in order to don't break compatibility. Just keep
things simple.

What do you think ?


2014-10-27 18:57 GMT+01:00 Mark Brown <broonie@kernel.org>:
> On Mon, Oct 27, 2014 at 12:30:25PM -0500, Felipe Balbi wrote:
>> On Mon, Oct 27, 2014 at 05:26:36PM +0000, Mark Brown wrote:
>
>> > Given that poweroff-source hasn't made it into a released kernel yet we
>> > can probably just kill it off completely can't we?
>
>> please do, then we will never have that commit in Linus' tree to give us
>> nightmares.
>
> Well, the commit isn't a particularly big deal (and got cross-merged
> into another tree already) - if it's convenient to rebase it out we
> probably should but it's also not the end of the world either so long as
> it doesn't appear in a release.  Worst case people adopt it based on the
> list discussions and we check for two properties which also isn't that
> bad.

[PATCH v1 03/10] regulator: act8865: Use of_is_system_power_controller helper function

[Romain Perier]

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 drivers/regulator/act8865-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index 76301ed..435aba1 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -365,7 +365,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	if (of_system_has_poweroff_source(dev->of_node)) {
+	if (of_is_system_power_controller(dev->of_node)) {
 		if (!pm_power_off) {
 			act8865_i2c_client = client;
 			act8865->off_reg = off_reg;
-- 
1.9.1

Re: [PATCH v1 03/10] regulator: act8865: Use of_is_system_power_controller helper function

[Felipe Balbi]

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

On Mon, Oct 27, 2014 at 04:26:48PM +0000, Romain Perier wrote:
> Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/regulator/act8865-regulator.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
> index 76301ed..435aba1 100644
> --- a/drivers/regulator/act8865-regulator.c
> +++ b/drivers/regulator/act8865-regulator.c
> @@ -365,7 +365,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> -	if (of_system_has_poweroff_source(dev->of_node)) {
> +	if (of_is_system_power_controller(dev->of_node)) {

so building this driver is broken until this patch ? not very good
either.

Looks to me you should add of_is_system_power_controller without remove
of_system_has_poweroff_source() until all users of that have been
converted over.

-- 
balbi

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

Re: [PATCH v1 03/10] regulator: act8865: Use of_is_system_power_controller helper function

[Heiko Stübner]

Am Montag, 27. Oktober 2014, 11:40:22 schrieb Felipe Balbi:
> On Mon, Oct 27, 2014 at 04:26:48PM +0000, Romain Perier wrote:
> > Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > 
> >  drivers/regulator/act8865-regulator.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/regulator/act8865-regulator.c
> > b/drivers/regulator/act8865-regulator.c index 76301ed..435aba1 100644
> > --- a/drivers/regulator/act8865-regulator.c
> > +++ b/drivers/regulator/act8865-regulator.c
> > @@ -365,7 +365,7 @@ static int act8865_pmic_probe(struct i2c_client
> > *client,> 
> >  		return ret;
> >  	
> >  	}
> > 
> > -	if (of_system_has_poweroff_source(dev->of_node)) {
> > +	if (of_is_system_power_controller(dev->of_node)) {
> 
> so building this driver is broken until this patch ? not very good
> either.
> 
> Looks to me you should add of_is_system_power_controller without remove
> of_system_has_poweroff_source() until all users of that have been
> converted over.

or alternatively fold this change into patch1 to keep the renaming together

[PATCH v2 04/10] ARM: dts: rockchip: Enable power off in pmic for Radxa Rock

[Romain Perier]

Add "system-power-controller" property to act8846 node.
poweroff command is 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..a6ead3a 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>;
 
+		system-power-controller;
+
 		regulators {
 			vcc_ddr: REG1 {
 				regulator-name = "VCC_DDR";
-- 
1.9.1

[PATCH v1 05/10] dt-bindings: act8865: Update documentation about property system-power-controller

[Romain Perier]

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

diff --git a/Documentation/devicetree/bindings/regulator/act8865-regulator.txt b/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
index 01a5b07..45369d8 100644
--- a/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
@@ -6,8 +6,8 @@ Required properties:
 - 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 .
+- system-power-controller: Telling whether or not this pmic is controlling the
+  system power. See Documentation/devicetree/bindings/power/power-controller.txt.
 
 Any standard regulator properties can be used to configure the single regulator.
 
-- 
1.9.1

[PATCH v2 06/10] mfd: tps65910: Use the standard DT property system-power-controller

[Romain Perier]

No longer use custom property to define poweroff capability, use the standard
DT property instead.

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 drivers/mfd/tps65910.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index 7612d89..a7faff2 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -423,8 +423,7 @@ static struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
 
 	board_info->irq = client->irq;
 	board_info->irq_base = -1;
-	board_info->pm_off = of_property_read_bool(np,
-			"ti,system-power-controller");
+	board_info->pm_off = of_is_system_power_controller();
 
 	return board_info;
 }
-- 
1.9.1

Re: [PATCH v2 06/10] mfd: tps65910: Use the standard DT property system-power-controller

[Lucas Stach]

Am Montag, den 27.10.2014, 16:26 +0000 schrieb Romain Perier:
> No longer use custom property to define poweroff capability, use the standard
> DT property instead.
> 
> Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/mfd/tps65910.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
> index 7612d89..a7faff2 100644
> --- a/drivers/mfd/tps65910.c
> +++ b/drivers/mfd/tps65910.c
> @@ -423,8 +423,7 @@ static struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
>  
>  	board_info->irq = client->irq;
>  	board_info->irq_base = -1;
> -	board_info->pm_off = of_property_read_bool(np,
> -			"ti,system-power-controller");
> +	board_info->pm_off = of_is_system_power_controller();
>  
>  	return board_info;
>  }
You are breaking compatibility with older DTs here. This is not
acceptable.

You may change all in-tree DTs to use the new property and also patch
the driver to understand it, but you must make sure that the driver
still understands the old, custom property. And especially in this case
it isn't really hard to do.

Regards,
Lucas
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

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

Re: [PATCH v2 06/10] mfd: tps65910: Use the standard DT property system-power-controller

[Felipe Balbi]

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

On Mon, Oct 27, 2014 at 05:35:49PM +0100, Lucas Stach wrote:
> Am Montag, den 27.10.2014, 16:26 +0000 schrieb Romain Perier:
> > No longer use custom property to define poweroff capability, use the standard
> > DT property instead.
> > 
> > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > ---
> >  drivers/mfd/tps65910.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
> > index 7612d89..a7faff2 100644
> > --- a/drivers/mfd/tps65910.c
> > +++ b/drivers/mfd/tps65910.c
> > @@ -423,8 +423,7 @@ static struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
> >  
> >  	board_info->irq = client->irq;
> >  	board_info->irq_base = -1;
> > -	board_info->pm_off = of_property_read_bool(np,
> > -			"ti,system-power-controller");
> > +	board_info->pm_off = of_is_system_power_controller();
> >  
> >  	return board_info;
> >  }
> You are breaking compatibility with older DTs here. This is not
> acceptable.
> 
> You may change all in-tree DTs to use the new property and also patch
> the driver to understand it, but you must make sure that the driver
> still understands the old, custom property. And especially in this case
> it isn't really hard to do.

correct, it should be simple to hide that under
of_is_system_power_controller() itself.

-- 
balbi

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

Re: [PATCH v2 06/10] mfd: tps65910: Use the standard DT property system-power-controller

[Johan Hovold]

On Mon, Oct 27, 2014 at 11:41:53AM -0500, Felipe Balbi wrote:
> On Mon, Oct 27, 2014 at 05:35:49PM +0100, Lucas Stach wrote:
> > Am Montag, den 27.10.2014, 16:26 +0000 schrieb Romain Perier:
> > > No longer use custom property to define poweroff capability, use the standard
> > > DT property instead.
> > > 
> > > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > > ---
> > >  drivers/mfd/tps65910.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
> > > index 7612d89..a7faff2 100644
> > > --- a/drivers/mfd/tps65910.c
> > > +++ b/drivers/mfd/tps65910.c
> > > @@ -423,8 +423,7 @@ static struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
> > >  
> > >  	board_info->irq = client->irq;
> > >  	board_info->irq_base = -1;
> > > -	board_info->pm_off = of_property_read_bool(np,
> > > -			"ti,system-power-controller");
> > > +	board_info->pm_off = of_is_system_power_controller();
> > >  
> > >  	return board_info;
> > >  }
> > You are breaking compatibility with older DTs here. This is not
> > acceptable.
> > 
> > You may change all in-tree DTs to use the new property and also patch
> > the driver to understand it, but you must make sure that the driver
> > still understands the old, custom property. And especially in this case
> > it isn't really hard to do.
> 
> correct, it should be simple to hide that under
> of_is_system_power_controller() itself.

Guys, take a look at the code (PATCH 1/10). ;)

Johan

Re: [PATCH v2 06/10] mfd: tps65910: Use the standard DT property system-power-controller

[Romain Perier]

Please take a look at "PATCH v1 1/10" of this serie, read it
carefully... it's already handled ^^

2014-10-27 17:43 GMT+01:00 Johan Hovold <johan@kernel.org>:
> On Mon, Oct 27, 2014 at 11:41:53AM -0500, Felipe Balbi wrote:
>> On Mon, Oct 27, 2014 at 05:35:49PM +0100, Lucas Stach wrote:
>> > Am Montag, den 27.10.2014, 16:26 +0000 schrieb Romain Perier:
>> > > No longer use custom property to define poweroff capability, use the standard
>> > > DT property instead.
>> > >
>> > > Signed-off-by: Romain Perier <romain.perier@gmail.com>
>> > > ---
>> > >  drivers/mfd/tps65910.c | 3 +--
>> > >  1 file changed, 1 insertion(+), 2 deletions(-)
>> > >
>> > > diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
>> > > index 7612d89..a7faff2 100644
>> > > --- a/drivers/mfd/tps65910.c
>> > > +++ b/drivers/mfd/tps65910.c
>> > > @@ -423,8 +423,7 @@ static struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
>> > >
>> > >   board_info->irq = client->irq;
>> > >   board_info->irq_base = -1;
>> > > - board_info->pm_off = of_property_read_bool(np,
>> > > -                 "ti,system-power-controller");
>> > > + board_info->pm_off = of_is_system_power_controller();
>> > >
>> > >   return board_info;
>> > >  }
>> > You are breaking compatibility with older DTs here. This is not
>> > acceptable.
>> >
>> > You may change all in-tree DTs to use the new property and also patch
>> > the driver to understand it, but you must make sure that the driver
>> > still understands the old, custom property. And especially in this case
>> > it isn't really hard to do.
>>
>> correct, it should be simple to hide that under
>> of_is_system_power_controller() itself.
>
> Guys, take a look at the code (PATCH 1/10). ;)
>
> Johan

Re: [PATCH v2 06/10] mfd: tps65910: Use the standard DT property system-power-controller

[Heiko Stübner]

Am Montag, 27. Oktober 2014, 11:41:53 schrieb Felipe Balbi:
> On Mon, Oct 27, 2014 at 05:35:49PM +0100, Lucas Stach wrote:
> > Am Montag, den 27.10.2014, 16:26 +0000 schrieb Romain Perier:
> > > No longer use custom property to define poweroff capability, use the
> > > standard DT property instead.
> > > 
> > > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > > ---
> > > 
> > >  drivers/mfd/tps65910.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
> > > index 7612d89..a7faff2 100644
> > > --- a/drivers/mfd/tps65910.c
> > > +++ b/drivers/mfd/tps65910.c
> > > @@ -423,8 +423,7 @@ static struct tps65910_board
> > > *tps65910_parse_dt(struct i2c_client *client,> > 
> > >  	board_info->irq = client->irq;
> > >  	board_info->irq_base = -1;
> > > 
> > > -	board_info->pm_off = of_property_read_bool(np,
> > > -			"ti,system-power-controller");
> > > +	board_info->pm_off = of_is_system_power_controller();
> > > 
> > >  	return board_info;
> > >  
> > >  }
> > 
> > You are breaking compatibility with older DTs here. This is not
> > acceptable.
> > 
> > You may change all in-tree DTs to use the new property and also patch
> > the driver to understand it, but you must make sure that the driver
> > still understands the old, custom property. And especially in this case
> > it isn't really hard to do.
> 
> correct, it should be simple to hide that under
> of_is_system_power_controller() itself.

If I'm reading patch 1 correctly, it already does handle the generic "system-
power-controller", as well as any foo,system-power-controller properties.


Heiko

Re: [PATCH v2 06/10] mfd: tps65910: Use the standard DT property system-power-controller

[Felipe Balbi]

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

On Mon, Oct 27, 2014 at 05:49:34PM +0100, Heiko Stübner wrote:
> Am Montag, 27. Oktober 2014, 11:41:53 schrieb Felipe Balbi:
> > On Mon, Oct 27, 2014 at 05:35:49PM +0100, Lucas Stach wrote:
> > > Am Montag, den 27.10.2014, 16:26 +0000 schrieb Romain Perier:
> > > > No longer use custom property to define poweroff capability, use the
> > > > standard DT property instead.
> > > > 
> > > > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/mfd/tps65910.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
> > > > index 7612d89..a7faff2 100644
> > > > --- a/drivers/mfd/tps65910.c
> > > > +++ b/drivers/mfd/tps65910.c
> > > > @@ -423,8 +423,7 @@ static struct tps65910_board
> > > > *tps65910_parse_dt(struct i2c_client *client,> > 
> > > >  	board_info->irq = client->irq;
> > > >  	board_info->irq_base = -1;
> > > > 
> > > > -	board_info->pm_off = of_property_read_bool(np,
> > > > -			"ti,system-power-controller");
> > > > +	board_info->pm_off = of_is_system_power_controller();
> > > > 
> > > >  	return board_info;
> > > >  
> > > >  }
> > > 
> > > You are breaking compatibility with older DTs here. This is not
> > > acceptable.
> > > 
> > > You may change all in-tree DTs to use the new property and also patch
> > > the driver to understand it, but you must make sure that the driver
> > > still understands the old, custom property. And especially in this case
> > > it isn't really hard to do.
> > 
> > correct, it should be simple to hide that under
> > of_is_system_power_controller() itself.
> 
> If I'm reading patch 1 correctly, it already does handle the generic "system-
> power-controller", as well as any foo,system-power-controller properties.

that's very true, but it doesn't handle poweroff-source which, one way
or another, will be merged into Linus' tree creating a bisection point
where things won't work.

The best solution would be for those patches to be removed from
whichever tree they are, otherwise there will always be a commit in the
mainline kernel with that binding and, even if unlikely, there will
always be the possibility of people using that commit or a developer
having to bisect an issue only to find a few commits where things don't
even build.

-- 
balbi

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

Re: [PATCH v2 06/10] mfd: tps65910: Use the standard DT property system-power-controller

[Felipe Balbi]

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

On Mon, Oct 27, 2014 at 04:26:51PM +0000, Romain Perier wrote:
> No longer use custom property to define poweroff capability, use the standard
> DT property instead.
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  drivers/mfd/tps65910.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
> index 7612d89..a7faff2 100644
> --- a/drivers/mfd/tps65910.c
> +++ b/drivers/mfd/tps65910.c
> @@ -423,8 +423,7 @@ static struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
>  
>  	board_info->irq = client->irq;
>  	board_info->irq_base = -1;
> -	board_info->pm_off = of_property_read_bool(np,
> -			"ti,system-power-controller");
> +	board_info->pm_off = of_is_system_power_controller();

you didn't build-test this, did you ?

-- 
balbi

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

[PATCH v2 07/10] ARM: dts: tegra: Use standard power-off property in tps65911 for tegra30 apalis

[Romain Perier]

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 arch/arm/boot/dts/tegra30-apalis.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra30-apalis.dtsi b/arch/arm/boot/dts/tegra30-apalis.dtsi
index a5446cb..ced4436 100644
--- a/arch/arm/boot/dts/tegra30-apalis.dtsi
+++ b/arch/arm/boot/dts/tegra30-apalis.dtsi
@@ -412,7 +412,7 @@
 			#interrupt-cells = <2>;
 			interrupt-controller;
 
-			ti,system-power-controller;
+			system-power-controller;
 
 			#gpio-cells = <2>;
 			gpio-controller;
-- 
1.9.1

Re: [PATCH v2 07/10] ARM: dts: tegra: Use standard power-off property in tps65911 for tegra30 apalis

[Felipe Balbi]

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

On Mon, Oct 27, 2014 at 04:26:52PM +0000, Romain Perier wrote:
> Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  arch/arm/boot/dts/tegra30-apalis.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/tegra30-apalis.dtsi b/arch/arm/boot/dts/tegra30-apalis.dtsi
> index a5446cb..ced4436 100644
> --- a/arch/arm/boot/dts/tegra30-apalis.dtsi
> +++ b/arch/arm/boot/dts/tegra30-apalis.dtsi
> @@ -412,7 +412,7 @@
>  			#interrupt-cells = <2>;
>  			interrupt-controller;
>  
> -			ti,system-power-controller;
> +			system-power-controller;

this board is broken until this patch is applied.

-- 
balbi

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

Re: [PATCH v2 07/10] ARM: dts: tegra: Use standard power-off property in tps65911 for tegra30 apalis

[Alexandre Courbot]

On Tue, Oct 28, 2014 at 1:42 AM, Felipe Balbi <balbi@ti.com> wrote:
> On Mon, Oct 27, 2014 at 04:26:52PM +0000, Romain Perier wrote:
>> Signed-off-by: Romain Perier <romain.perier@gmail.com>
>> ---
>>  arch/arm/boot/dts/tegra30-apalis.dtsi | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/tegra30-apalis.dtsi b/arch/arm/boot/dts/tegra30-apalis.dtsi
>> index a5446cb..ced4436 100644
>> --- a/arch/arm/boot/dts/tegra30-apalis.dtsi
>> +++ b/arch/arm/boot/dts/tegra30-apalis.dtsi
>> @@ -412,7 +412,7 @@
>>                       #interrupt-cells = <2>;
>>                       interrupt-controller;
>>
>> -                     ti,system-power-controller;
>> +                     system-power-controller;
>
> this board is broken until this patch is applied.

It should not. Supporting a new property in the tps65911 is fine, but
the old property must keep being supported to preserve DT
compatibility. If a change removing support for
"ti,system-power-controller" has landed in -next, then it should be
removed.

Re: [PATCH v2 07/10] ARM: dts: tegra: Use standard power-off property in tps65911 for tegra30 apalis

[Romain Perier]

The old property is supported. See "[PATCH v1 1/10]" ^^
(of_is_system_power_controller supports both properties...)

2014-10-28 5:34 GMT+01:00 Alexandre Courbot <gnurou@gmail.com>:
> On Tue, Oct 28, 2014 at 1:42 AM, Felipe Balbi <balbi@ti.com> wrote:
>> On Mon, Oct 27, 2014 at 04:26:52PM +0000, Romain Perier wrote:
>>> Signed-off-by: Romain Perier <romain.perier@gmail.com>
>>> ---
>>>  arch/arm/boot/dts/tegra30-apalis.dtsi | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/tegra30-apalis.dtsi b/arch/arm/boot/dts/tegra30-apalis.dtsi
>>> index a5446cb..ced4436 100644
>>> --- a/arch/arm/boot/dts/tegra30-apalis.dtsi
>>> +++ b/arch/arm/boot/dts/tegra30-apalis.dtsi
>>> @@ -412,7 +412,7 @@
>>>                       #interrupt-cells = <2>;
>>>                       interrupt-controller;
>>>
>>> -                     ti,system-power-controller;
>>> +                     system-power-controller;
>>
>> this board is broken until this patch is applied.
>
> It should not. Supporting a new property in the tps65911 is fine, but
> the old property must keep being supported to preserve DT
> compatibility. If a change removing support for
> "ti,system-power-controller" has landed in -next, then it should be
> removed.

[PATCH v2 08/10] ARM: dts: tegra: Use standard power-off property in tps65911 for tegra30 beaver

[Romain Perier]

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 arch/arm/boot/dts/tegra30-beaver.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra30-beaver.dts b/arch/arm/boot/dts/tegra30-beaver.dts
index cee8f22..4abaf7f 100644
--- a/arch/arm/boot/dts/tegra30-beaver.dts
+++ b/arch/arm/boot/dts/tegra30-beaver.dts
@@ -188,7 +188,7 @@
 			#interrupt-cells = <2>;
 			interrupt-controller;
 
-			ti,system-power-controller;
+			system-power-controller;
 
 			#gpio-cells = <2>;
 			gpio-controller;
-- 
1.9.1

Re: [PATCH v2 08/10] ARM: dts: tegra: Use standard power-off property in tps65911 for tegra30 beaver

[Felipe Balbi]

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

On Mon, Oct 27, 2014 at 04:26:53PM +0000, Romain Perier wrote:
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  arch/arm/boot/dts/tegra30-beaver.dts | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/tegra30-beaver.dts b/arch/arm/boot/dts/tegra30-beaver.dts
> index cee8f22..4abaf7f 100644
> --- a/arch/arm/boot/dts/tegra30-beaver.dts
> +++ b/arch/arm/boot/dts/tegra30-beaver.dts
> @@ -188,7 +188,7 @@
>  			#interrupt-cells = <2>;
>  			interrupt-controller;
>  
> -			ti,system-power-controller;
> +			system-power-controller;

and this... likewise to all others. You should add regressions midway
through your series and you can't simply drop a DT binding which has
been shipped on stable kernels.

-- 
balbi

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

[PATCH v2 09/10] ARM: dts: tegra: Use standard power-off property in tps65911 for tegra30 cardhu

[Romain Perier]

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 arch/arm/boot/dts/tegra30-cardhu.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi
index 2063795..46b4ff1 100644
--- a/arch/arm/boot/dts/tegra30-cardhu.dtsi
+++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi
@@ -232,7 +232,7 @@
 			#interrupt-cells = <2>;
 			interrupt-controller;
 
-			ti,system-power-controller;
+			system-power-controller;
 
 			#gpio-cells = <2>;
 			gpio-controller;
-- 
1.9.1

[PATCH v2 10/10] ARM: dts: tegra: Use standard power-off property in tps65911 for tegra30 colibri

[Romain Perier]

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 arch/arm/boot/dts/tegra30-colibri.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra30-colibri.dtsi b/arch/arm/boot/dts/tegra30-colibri.dtsi
index c4ed1be..7833fc3 100644
--- a/arch/arm/boot/dts/tegra30-colibri.dtsi
+++ b/arch/arm/boot/dts/tegra30-colibri.dtsi
@@ -190,7 +190,7 @@
 			#interrupt-cells = <2>;
 			interrupt-controller;
 
-			ti,system-power-controller;
+			system-power-controller;
 
 			#gpio-cells = <2>;
 			gpio-controller;
-- 
1.9.1

Re: [PATCH v1 01/10] of: Rename "poweroff-source" property to "system-power-controller"

[Felipe Balbi]

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

On Mon, Oct 27, 2014 at 04:26:46PM +0000, Romain Perier wrote:
> As discussed on the mailing list, it makes more sense to rename this property
> to "system-power-controller". Problem being that the word "source" usually tends
> to be used for inputs and that is out of control of the OS. The poweroff
> capability is an output which simply turns the system-power off. Also, this
> property might be used by drivers which power-off the system and power back on
> subsequent RTC alarms. This seems to suggest to remove "poweroff" from the
> property name and to choose "system-power-controller" as the more generic name.
> This patchs adds the required renaming changes and defines an helper function
> which is compatible with both properties, the old one prefixed by a vendor name
> and the new one without any prefix.
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  include/linux/of.h | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 27b3ba1..c1ed2a5 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -867,14 +867,33 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
>  extern int of_resolve_phandles(struct device_node *tree);
>  
>  /**
> - * of_system_has_poweroff_source - Tells if poweroff-source is found for device_node
> + * of_is_system_power_controller - Tells if the property for controlling system
> + * power is found in 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");
> +static inline bool of_is_system_power_controller(const struct device_node *np)
> +{
> +	struct property *pp;
> +	unsigned long flags;
> +	char *sep;
> +	bool found = false;
> +
> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> +	for_each_property_of_node(np, pp) {
> +		if (of_prop_cmp(pp->name, "system-power-controller") == 0) {
> +			found = true;
> +			break;
> +		}
> +		sep = strchr(pp->name, ',');
> +		if (sep && sep - pp->name && of_prop_cmp(sep + 1, "system-power-controller") == 0) {
> +			found = true;
> +			break;
> +		}
> +	}
> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	return found;
>  }
>  
>  #endif /* _LINUX_OF_H */

I think you still need to support poweroff-source since it has been
released on a stable kernel. Perhaps add a warning message telling users
it's deprecated and asking them to switch over to
system-power-controller ? Still, simply removing it isn't very nice.

-- 
balbi

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

Re: [PATCH v1 01/10] of: Rename "poweroff-source" property to "system-power-controller"

[Johan Hovold]

On Mon, Oct 27, 2014 at 11:38:40AM -0500, Felipe Balbi wrote:
> On Mon, Oct 27, 2014 at 04:26:46PM +0000, Romain Perier wrote:
> > As discussed on the mailing list, it makes more sense to rename this property
> > to "system-power-controller". Problem being that the word "source" usually tends
> > to be used for inputs and that is out of control of the OS. The poweroff
> > capability is an output which simply turns the system-power off. Also, this
> > property might be used by drivers which power-off the system and power back on
> > subsequent RTC alarms. This seems to suggest to remove "poweroff" from the
> > property name and to choose "system-power-controller" as the more generic name.
> > This patchs adds the required renaming changes and defines an helper function
> > which is compatible with both properties, the old one prefixed by a vendor name
> > and the new one without any prefix.

> I think you still need to support poweroff-source since it has been
> released on a stable kernel. Perhaps add a warning message telling users
> it's deprecated and asking them to switch over to
> system-power-controller ? Still, simply removing it isn't very nice.

No, Romain sent a patch that replaced "<vendor>,system-power-controller"
with "poweroff-source". It's now in Mark's tree (for v3.19), and this
series "reverts" to the old name minus the vendor-prefix.

Johan

Re: [PATCH v1 01/10] of: Rename "poweroff-source" property to "system-power-controller"

[Felipe Balbi]

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

On Mon, Oct 27, 2014 at 05:41:03PM +0100, Johan Hovold wrote:
> On Mon, Oct 27, 2014 at 11:38:40AM -0500, Felipe Balbi wrote:
> > On Mon, Oct 27, 2014 at 04:26:46PM +0000, Romain Perier wrote:
> > > As discussed on the mailing list, it makes more sense to rename this property
> > > to "system-power-controller". Problem being that the word "source" usually tends
> > > to be used for inputs and that is out of control of the OS. The poweroff
> > > capability is an output which simply turns the system-power off. Also, this
> > > property might be used by drivers which power-off the system and power back on
> > > subsequent RTC alarms. This seems to suggest to remove "poweroff" from the
> > > property name and to choose "system-power-controller" as the more generic name.
> > > This patchs adds the required renaming changes and defines an helper function
> > > which is compatible with both properties, the old one prefixed by a vendor name
> > > and the new one without any prefix.
> 
> > I think you still need to support poweroff-source since it has been
> > released on a stable kernel. Perhaps add a warning message telling users
> > it's deprecated and asking them to switch over to
> > system-power-controller ? Still, simply removing it isn't very nice.
> 
> No, Romain sent a patch that replaced "<vendor>,system-power-controller"
> with "poweroff-source". It's now in Mark's tree (for v3.19), and this
> series "reverts" to the old name minus the vendor-prefix.

oh, so poweroff-source isn't in Linus' tree yet ? (/me goes grep)

Then it should be fine. My bad.

Many of the other comments are still valid because even though
poweroff-source isn't in mainline yet, this series still creates
bisection points which are broken. The best solution would be to drop
all those patches from Mark's tree. Read, not revert, drop.

-- 
balbi

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

Re: [PATCH v1 01/10] of: Rename "poweroff-source" property to "system-power-controller"

[Johan Hovold]

On Mon, Oct 27, 2014 at 11:47:41AM -0500, Felipe Balbi wrote:
> On Mon, Oct 27, 2014 at 05:41:03PM +0100, Johan Hovold wrote:
> > On Mon, Oct 27, 2014 at 11:38:40AM -0500, Felipe Balbi wrote:
> > > On Mon, Oct 27, 2014 at 04:26:46PM +0000, Romain Perier wrote:
> > > > As discussed on the mailing list, it makes more sense to rename this property
> > > > to "system-power-controller". Problem being that the word "source" usually tends
> > > > to be used for inputs and that is out of control of the OS. The poweroff
> > > > capability is an output which simply turns the system-power off. Also, this
> > > > property might be used by drivers which power-off the system and power back on
> > > > subsequent RTC alarms. This seems to suggest to remove "poweroff" from the
> > > > property name and to choose "system-power-controller" as the more generic name.
> > > > This patchs adds the required renaming changes and defines an helper function
> > > > which is compatible with both properties, the old one prefixed by a vendor name
> > > > and the new one without any prefix.
> > 
> > > I think you still need to support poweroff-source since it has been
> > > released on a stable kernel. Perhaps add a warning message telling users
> > > it's deprecated and asking them to switch over to
> > > system-power-controller ? Still, simply removing it isn't very nice.
> > 
> > No, Romain sent a patch that replaced "<vendor>,system-power-controller"
> > with "poweroff-source". It's now in Mark's tree (for v3.19), and this
> > series "reverts" to the old name minus the vendor-prefix.
> 
> oh, so poweroff-source isn't in Linus' tree yet ? (/me goes grep)
> 
> Then it should be fine. My bad.
> 
> Many of the other comments are still valid because even though
> poweroff-source isn't in mainline yet, this series still creates
> bisection points which are broken. The best solution would be to drop
> all those patches from Mark's tree. Read, not revert, drop.

I agree.

Johan

Re: [PATCH v1 01/10] of: Rename "poweroff-source" property to "system-power-controller"

[Heiko Stübner]

Am Montag, 27. Oktober 2014, 11:47:41 schrieb Felipe Balbi:
> On Mon, Oct 27, 2014 at 05:41:03PM +0100, Johan Hovold wrote:
> > On Mon, Oct 27, 2014 at 11:38:40AM -0500, Felipe Balbi wrote:
> > > On Mon, Oct 27, 2014 at 04:26:46PM +0000, Romain Perier wrote:
> > > > As discussed on the mailing list, it makes more sense to rename this
> > > > property to "system-power-controller". Problem being that the word
> > > > "source" usually tends to be used for inputs and that is out of
> > > > control of the OS. The poweroff capability is an output which simply
> > > > turns the system-power off. Also, this property might be used by
> > > > drivers which power-off the system and power back on subsequent RTC
> > > > alarms. This seems to suggest to remove "poweroff" from the property
> > > > name and to choose "system-power-controller" as the more generic
> > > > name. This patchs adds the required renaming changes and defines an
> > > > helper function which is compatible with both properties, the old one
> > > > prefixed by a vendor name and the new one without any prefix.
> > > 
> > > I think you still need to support poweroff-source since it has been
> > > released on a stable kernel. Perhaps add a warning message telling users
> > > it's deprecated and asking them to switch over to
> > > system-power-controller ? Still, simply removing it isn't very nice.
> > 
> > No, Romain sent a patch that replaced "<vendor>,system-power-controller"
> > with "poweroff-source". It's now in Mark's tree (for v3.19), and this
> > series "reverts" to the old name minus the vendor-prefix.
> 
> oh, so poweroff-source isn't in Linus' tree yet ? (/me goes grep)
> 
> Then it should be fine. My bad.
> 
> Many of the other comments are still valid because even though
> poweroff-source isn't in mainline yet, this series still creates
> bisection points which are broken. The best solution would be to drop
> all those patches from Mark's tree. Read, not revert, drop.

There have never been any users of the poweroff-source. The act8846 in the 
radxarock would have been the first, but I held off with the dts patch as the 
naming issue came up at the same time.

So I guess if Romain keeps the renaming together there shouldn't be any other 
bad bisection points?

Re: [PATCH v1 01/10] of: Rename "poweroff-source" property to "system-power-controller"

[Felipe Balbi]

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

On Mon, Oct 27, 2014 at 06:05:35PM +0100, Heiko Stübner wrote:
> Am Montag, 27. Oktober 2014, 11:47:41 schrieb Felipe Balbi:
> > On Mon, Oct 27, 2014 at 05:41:03PM +0100, Johan Hovold wrote:
> > > On Mon, Oct 27, 2014 at 11:38:40AM -0500, Felipe Balbi wrote:
> > > > On Mon, Oct 27, 2014 at 04:26:46PM +0000, Romain Perier wrote:
> > > > > As discussed on the mailing list, it makes more sense to rename this
> > > > > property to "system-power-controller". Problem being that the word
> > > > > "source" usually tends to be used for inputs and that is out of
> > > > > control of the OS. The poweroff capability is an output which simply
> > > > > turns the system-power off. Also, this property might be used by
> > > > > drivers which power-off the system and power back on subsequent RTC
> > > > > alarms. This seems to suggest to remove "poweroff" from the property
> > > > > name and to choose "system-power-controller" as the more generic
> > > > > name. This patchs adds the required renaming changes and defines an
> > > > > helper function which is compatible with both properties, the old one
> > > > > prefixed by a vendor name and the new one without any prefix.
> > > > 
> > > > I think you still need to support poweroff-source since it has been
> > > > released on a stable kernel. Perhaps add a warning message telling users
> > > > it's deprecated and asking them to switch over to
> > > > system-power-controller ? Still, simply removing it isn't very nice.
> > > 
> > > No, Romain sent a patch that replaced "<vendor>,system-power-controller"
> > > with "poweroff-source". It's now in Mark's tree (for v3.19), and this
> > > series "reverts" to the old name minus the vendor-prefix.
> > 
> > oh, so poweroff-source isn't in Linus' tree yet ? (/me goes grep)
> > 
> > Then it should be fine. My bad.
> > 
> > Many of the other comments are still valid because even though
> > poweroff-source isn't in mainline yet, this series still creates
> > bisection points which are broken. The best solution would be to drop
> > all those patches from Mark's tree. Read, not revert, drop.
> 
> There have never been any users of the poweroff-source. The act8846 in the 
> radxarock would have been the first, but I held off with the dts patch as the 
> naming issue came up at the same time.
> 
> So I guess if Romain keeps the renaming together there shouldn't be any other 
> bad bisection points?

Not build breaks, but there will always be the commit below:

commit a88f5c6deb2a44f694b01aac48231ec97059b26a
Author: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date:   Tue Oct 14 06:31:12 2014 +0000

    dt-bindings: Document the standard property "poweroff-source"
    
    Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
    Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

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;
+}

Even if for a small time frame, there will always be a commit where we
called "poweroff-source" a standard binding and, as such, as should
support it.

-- 
balbi

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

Re: [PATCH v1 01/10] of: Rename "poweroff-source" property to "system-power-controller"

[Johan Hovold]

On Mon, Oct 27, 2014 at 04:26:46PM +0000, Romain Perier wrote:
> As discussed on the mailing list, it makes more sense to rename this property
> to "system-power-controller". Problem being that the word "source" usually tends
> to be used for inputs and that is out of control of the OS. The poweroff
> capability is an output which simply turns the system-power off. Also, this
> property might be used by drivers which power-off the system and power back on
> subsequent RTC alarms. This seems to suggest to remove "poweroff" from the
> property name and to choose "system-power-controller" as the more generic name.
> This patchs adds the required renaming changes and defines an helper function
> which is compatible with both properties, the old one prefixed by a vendor name
> and the new one without any prefix.
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  include/linux/of.h | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 27b3ba1..c1ed2a5 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -867,14 +867,33 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
>  extern int of_resolve_phandles(struct device_node *tree);
>  
>  /**
> - * of_system_has_poweroff_source - Tells if poweroff-source is found for device_node
> + * of_is_system_power_controller - Tells if the property for controlling system
> + * power is found in 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");
> +static inline bool of_is_system_power_controller(const struct device_node *np)
> +{
> +	struct property *pp;
> +	unsigned long flags;
> +	char *sep;
> +	bool found = false;
> +
> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> +	for_each_property_of_node(np, pp) {
> +		if (of_prop_cmp(pp->name, "system-power-controller") == 0) {
> +			found = true;
> +			break;
> +		}
> +		sep = strchr(pp->name, ',');
> +		if (sep && sep - pp->name && of_prop_cmp(sep + 1, "system-power-controller") == 0) {
> +			found = true;
> +			break;
> +		}
> +	}
> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	return found;
>  }

You probably don't want this to be inline.

Johan

Re: [PATCH v1 01/10] of: Rename "poweroff-source" property to "system-power-controller"

[Romain Perier]

2014-10-27 18:16 GMT+01:00 Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> You probably don't want this to be inline.
>
Yeah this is what I thought, I agree, it's a bit big to be inline... I
will fix it, I just wait a bit to see what is decided for this
serie...