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 <[email protected]>
---
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
This simply renames the previous documentation to something more generic and adds
updates according to last changes.
Signed-off-by: Romain Perier <[email protected]>
---
.../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
Add "system-power-controller" property to act8846 node.
poweroff command is now handled for this board.
Signed-off-by: Romain Perier <[email protected]>
---
arch/arm/boot/dts/rk3188-radxarock.dts | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/boot/dts/rk3188-radxarock.dts b/arch/arm/boot/dts/rk3188-radxarock.dts
index 15910c9..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
No longer use custom property to define poweroff capability, use the standard
DT property instead.
Signed-off-by: Romain Perier <[email protected]>
---
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
Signed-off-by: Romain Perier <[email protected]>
---
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
Signed-off-by: Romain Perier <[email protected]>
---
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
Signed-off-by: Romain Perier <[email protected]>
---
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
Signed-off-by: Romain Perier <[email protected]>
---
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
Signed-off-by: Romain Perier <[email protected]>
---
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
Signed-off-by: Romain Perier <[email protected]>
---
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
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 <[email protected]>
> ---
> 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/ |
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> .../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
On Mon, Oct 27, 2014 at 04:26:48PM +0000, Romain Perier wrote:
> Signed-off-by: Romain Perier <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
On Mon, Oct 27, 2014 at 04:26:52PM +0000, Romain Perier wrote:
> Signed-off-by: Romain Perier <[email protected]>
> ---
> 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
On Mon, Oct 27, 2014 at 04:26:53PM +0000, Romain Perier wrote:
> Signed-off-by: Romain Perier <[email protected]>
> ---
> 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
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
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 <[email protected]>
> > > ---
> > >
> > > 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
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 <[email protected]>
> > > ---
> > > 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
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
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 <[email protected]>
> > > > ---
> > > >
> > > > 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
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 <[email protected]>:
> 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 <[email protected]>
>> > > ---
>> > > 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
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 <[email protected]>
> > ---
> >
> > 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
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?
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
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 <[email protected]>
Date: Tue Oct 14 06:31:12 2014 +0000
dt-bindings: Document the standard property "poweroff-source"
Signed-off-by: Romain Perier <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
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
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 <[email protected]>
> ---
> 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
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?
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
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.
2014-10-27 18:16 GMT+01:00 Johan Hovold <[email protected]>:
> 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...
On Tue, Oct 28, 2014 at 1:42 AM, Felipe Balbi <[email protected]> wrote:
> On Mon, Oct 27, 2014 at 04:26:52PM +0000, Romain Perier wrote:
>> Signed-off-by: Romain Perier <[email protected]>
>> ---
>> 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.
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 <[email protected]>:
> On Tue, Oct 28, 2014 at 1:42 AM, Felipe Balbi <[email protected]> wrote:
>> On Mon, Oct 27, 2014 at 04:26:52PM +0000, Romain Perier wrote:
>>> Signed-off-by: Romain Perier <[email protected]>
>>> ---
>>> 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.
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 <[email protected]>:
> 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.