poweroff-source DT property renaming
Subsystems (6)
cover poweroff-source DT property renaming
The goal of this serie is to rename the property "poweroff-source" to "system-power-controller" and to fix things incrementally.
Changes and explanations since v1:
- The first patch defines "of_is_system_power_controller" which is compatible with both "vendor,system-power-controller" and "system-power-controller" properties. Also, It keeps the old helper function of_system_has_poweroff_source for source compatibility until everything is renamed (in this way, bisections are not broken and change is made "atomically" between each commit)
Note: the property "poweroff-source" itself is not used in dts files yet. Before this patch tps65910 was broken due to missing backward compatibility with "vendor,system-power-controller". As the old helper uses the new one, it works again.
- act8865 and tps65910 are ported to the new helper function
- The last commit removes the olf helper which was only used for source compatibility
Romain Perier (4):
of: Rename "poweroff-source" property to "system-power-controller"
regulator: act8865: Convert poweroff-source DT property to
system-power-controller
mfd: tps65910: Convert poweroff-source DT property to
system-power-controller
of: Remove of_system_has_poweroff_source helper function
.../devicetree/bindings/power/power-controller.txt | 18 ++++++++++++ .../devicetree/bindings/power/poweroff.txt | 18 ------------ .../bindings/regulator/act8865-regulator.txt | 4 +-- drivers/mfd/tps65910.c | 2 +- drivers/of/base.c | 34 ++++++++++++++++++++++ drivers/regulator/act8865-regulator.c | 2 +- include/linux/of.h | 12 ++------ 7 files changed, 58 insertions(+), 32 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/power-controller.txt delete mode 100644 Documentation/devicetree/bindings/power/poweroff.txt
Signature · 1 line
1.9.1
PATCH v2 1/4 of: Rename "poweroff-source" property to "system-power-controller"
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.
--- .../devicetree/bindings/power/power-controller.txt | 18 ++++++++++++ .../devicetree/bindings/power/poweroff.txt | 18 ------------ drivers/of/base.c | 34 ++++++++++++++++++++++ include/linux/of.h | 10 ++----- 4 files changed, 55 insertions(+), 25 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/power-controller.txt delete mode 100644 Documentation/devicetree/bindings/power/poweroff.txt
Documentation/devicetree/bindings/power/power-controller.txt new file +18 -0
@@ -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; +}
Documentation/devicetree/bindings/power/poweroff.txt deleted +0 -18
@@ -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; -}
drivers/of/base.c +34 -0
@@ -2260,3 +2260,37 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)return of_get_next_parent (np); }EXPORT_SYMBOL (of_graph_get_remote_port); + +/** +* 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 +*/ +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 ; + } +/* Backward compatibility with previous property "vendor,system-power-controller", +* we just check that an non-empty vendor-prefix exists here +*/ + 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; +} +EXPORT_SYMBOL (of_is_system_power_controller);
include/linux/of.h +3 -7
@@ -910,15 +910,11 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,/* CONFIG_OF_RESOLVE api */ extern int of_resolve_phandles (struct device_node *tree); -/** -* of_system_has_poweroff_source - Tells if poweroff-source is found for device_node -* @np: Pointer to the given device_node -* -* return true if present false otherwise -*/ +bool of_is_system_power_controller (const struct device_node *np); +static inline bool of_system_has_poweroff_source (const struct device_node *np) { -return of_property_read_bool (np,"poweroff-source" ); +return of_is_system_power_controller (np); }#endif /* _LINUX_OF_H */
Signature · 1 line
1.9.1
Re: [PATCH v2 1/4] of: Rename "poweroff-source" property to "system-power-controller"
Johan: as you are one of the people who requested the renaming, any suggestions ?
Romain
2014-10-29 8:35 GMT+01:00 Romain Perier <[email protected]>:
Quoted 137 lines
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]>--- .../devicetree/bindings/power/power-controller.txt | 18 ++++++++++++ .../devicetree/bindings/power/poweroff.txt | 18 ------------ drivers/of/base.c | 34 ++++++++++++++++++++++ include/linux/of.h | 10 ++----- 4 files changed, 55 insertions(+), 25 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/power-controller.txt delete mode 100644 Documentation/devicetree/bindings/power/poweroff.txtDocumentation/devicetree/bindings/power/power-controller.txt new file +18
-0@@ -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; +}Documentation/devicetree/bindings/power/poweroff.txt deleted +0
-18@@ -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; -}drivers/of/base.c +34
-0@@ -2260,3 +2260,37 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)return of_get_next_parent (np); }EXPORT_SYMBOL (of_graph_get_remote_port); + +/** +* 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 +*/ +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 ; + } +/* Backward compatibility with previous property "vendor,system-power-controller", +* we just check that an non-empty vendor-prefix exists here +*/ + 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; +} +EXPORT_SYMBOL (of_is_system_power_controller);include/linux/of.h +3
-8@@ -910,15 +910,11 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,/* CONFIG_OF_RESOLVE api */ extern int of_resolve_phandles (struct device_node *tree); -/** -* of_system_has_poweroff_source - Tells if poweroff-source is found for device_node -* @np: Pointer to the given device_node -* -* return true if present false otherwise -*/ +bool of_is_system_power_controller (const struct device_node *np); +static inline bool of_system_has_poweroff_source (const struct device_node *np) { -return of_property_read_bool (np,"poweroff-source" ); +return of_is_system_power_controller (np); }#endif /* _LINUX_OF_H */ --1.9.1
Re: [PATCH v2 1/4] of: Rename "poweroff-source" property to "system-power-controller"
Hi,
ping all
Romain
2014-10-30 11:08 GMT+01:00 Romain Perier <[email protected]>:
Quoted 143 lines
Johan: as you are one of the people who requested the renaming, any suggestions ?
Romain
2014-10-29 8:35 GMT+01:00 Romain Perier <[email protected]>:
Quoted 137 lines
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]>--- .../devicetree/bindings/power/power-controller.txt | 18 ++++++++++++ .../devicetree/bindings/power/poweroff.txt | 18 ------------ drivers/of/base.c | 34 ++++++++++++++++++++++ include/linux/of.h | 10 ++----- 4 files changed, 55 insertions(+), 25 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/power-controller.txt delete mode 100644 Documentation/devicetree/bindings/power/poweroff.txtDocumentation/devicetree/bindings/power/power-controller.txt new file +18
-0@@ -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; +}Documentation/devicetree/bindings/power/poweroff.txt deleted +0
-18@@ -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; -}drivers/of/base.c +34
-0@@ -2260,3 +2260,37 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)return of_get_next_parent (np); }EXPORT_SYMBOL (of_graph_get_remote_port); + +/** +* 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 +*/ +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 ; + } +/* Backward compatibility with previous property "vendor,system-power-controller", +* we just check that an non-empty vendor-prefix exists here +*/ + 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; +} +EXPORT_SYMBOL (of_is_system_power_controller);include/linux/of.h +3
-8@@ -910,15 +910,11 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,/* CONFIG_OF_RESOLVE api */ extern int of_resolve_phandles (struct device_node *tree); -/** -* of_system_has_poweroff_source - Tells if poweroff-source is found for device_node -* @np: Pointer to the given device_node -* -* return true if present false otherwise -*/ +bool of_is_system_power_controller (const struct device_node *np); +static inline bool of_system_has_poweroff_source (const struct device_node *np) { -return of_property_read_bool (np,"poweroff-source" ); +return of_is_system_power_controller (np); }#endif /* _LINUX_OF_H */ --1.9.1
Re: [PATCH v2 1/4] of: Rename "poweroff-source" property to "system-power-controller"
In reply to Romain PerierQuoted 2 lines · Romain Perier
As discussed on the mailing list, it makes more sense to rename this property to "system-power-controller".
Please also refer to the commit in the regulator tree renaming it to "poweroff-source", and that this in effect is a revert to the old name but without the vendor prefix.
Quoted 11 lines
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]>
First of all, always run your patches through checpatch.pl before submitting. There's a few warnings there for you to fix.
Quoted 15 lines
--- .../devicetree/bindings/power/power-controller.txt | 18 ++++++++++++ .../devicetree/bindings/power/poweroff.txt | 18 ------------ drivers/of/base.c | 34 ++++++++++++++++++++++ include/linux/of.h | 10 ++----- 4 files changed, 55 insertions(+), 25 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/power-controller.txt delete mode 100644 Documentation/devicetree/bindings/power/poweroff.txtDocumentation/devicetree/bindings/power/power-controller.txt new file +0
-0
Documentation/devicetree/bindings/power/power-controller.txt line 4
Quoted 4 lines
Documentation/devicetree/bindings/power/power-controller.txt new file +4
-0@@ -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
s/to/with/
Documentation/devicetree/bindings/power/power-controller.txt line 5
Quoted 1 line
Documentation/devicetree/bindings/power/power-controller.txt new file +1
-0+components might needs to define this capability,
s/needs/need/
Quoted 1 line
which tells to the kernel
s/to the/the/
Quoted 1 line
howto switch off the system.
this should be something like "that it can be used to switch off...".
Quoted 1 line
The corresponding driver must have the standard
It's the device that has the property, not the driver.
Documentation/devicetree/bindings/power/power-controller.txt line 8
Quoted 2 lines
Documentation/devicetree/bindings/power/power-controller.txt new file +2
-0+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
s/controller/control/
s/system-power/system power/
Quoted 66 lines
Documentation/devicetree/bindings/power/power-controller.txt new file +10
-0+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; +}Documentation/devicetree/bindings/power/poweroff.txt deleted +0
-18@@ -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; -}drivers/of/base.c +24
-0@@ -2260,3 +2260,37 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)return of_get_next_parent (np); }EXPORT_SYMBOL (of_graph_get_remote_port); + +/** +* 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 +*/ +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 ; + } +/* Backward compatibility with previous property "vendor,system-power-controller", +* we just check that an non-empty vendor-prefix exists here +*/
drivers/of/base.c struct device_node *of_graph_get_remote_port(const struct device_node *node) · line 2296
Quoted 10 lines
drivers/of/base.c +10
-0+ 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; +} +EXPORT_SYMBOL (of_is_system_power_controller);
I think this is the wrong approach. This way any driver will recognise the old deprecated vendor prefixes. But not only those -- also misspelled vendor prefixes.
Keep it simple and only parse the new property name (inline in the header file).
If we need to support the old property names with vendor prefix (we have dropped vendor prefixes in the past), then you could add a second helper
of_is_system_power_controller_compat(np, compat_propname)
where you pass in the whole old property name (e.g. "ti,system-power-controller") and use that name as a fall back.
This way it will be clear which drivers are still supporting the deprecated property names, and we can make sure that no new ones will.
Quoted 13 lines
include/linux/of.h +0
-5@@ -910,15 +910,11 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,/* CONFIG_OF_RESOLVE api */ extern int of_resolve_phandles (struct device_node *tree); -/** -* of_system_has_poweroff_source - Tells if poweroff-source is found for device_node -* @np: Pointer to the given device_node -* -* return true if present false otherwise
include/linux/of.h static inline int of_changeset_update_property(struct of_changeset *ocs, · line 920
Quoted 10 lines
include/linux/of.h +3
-2-*/ +bool of_is_system_power_controller (const struct device_node *np); +static inline bool of_system_has_poweroff_source (const struct device_node *np) { -return of_property_read_bool (np,"poweroff-source" ); +return of_is_system_power_controller (np); }#endif /* _LINUX_OF_H */
Johan
Re: [PATCH v2 1/4] of: Rename "poweroff-source" property to "system-power-controller"
In reply to Romain Perier[ Resend with lkml, arm, Felipe on CC -- why were these dropped from CC? ]
Quoted 2 lines · Romain Perier
As discussed on the mailing list, it makes more sense to rename this property to "system-power-controller".
Please also refer to the commit in the regulator tree renaming it to "poweroff-source", and that this in effect is a revert to the old name but without the vendor prefix.
Quoted 11 lines
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]>
First of all, always run your patches through checkpatch.pl before submitting. There's a few warnings there for you to fix.
Quoted 15 lines
--- .../devicetree/bindings/power/power-controller.txt | 18 ++++++++++++ .../devicetree/bindings/power/poweroff.txt | 18 ------------ drivers/of/base.c | 34 ++++++++++++++++++++++ include/linux/of.h | 10 ++----- 4 files changed, 55 insertions(+), 25 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/power-controller.txt delete mode 100644 Documentation/devicetree/bindings/power/poweroff.txtDocumentation/devicetree/bindings/power/power-controller.txt new file +0
-0
Documentation/devicetree/bindings/power/power-controller.txt line 4
Quoted 4 lines
Documentation/devicetree/bindings/power/power-controller.txt new file +4
-0@@ -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
s/to/with/
Documentation/devicetree/bindings/power/power-controller.txt line 5
Quoted 1 line
Documentation/devicetree/bindings/power/power-controller.txt new file +1
-0+components might needs to define this capability,
s/needs/need/
Quoted 1 line
which tells to the kernel
s/to the/the/
Quoted 1 line
howto switch off the system.
this should be something like "that it can be used to switch off...".
Quoted 1 line
The corresponding driver must have the standard
It's the device that has the property, not the driver.
Documentation/devicetree/bindings/power/power-controller.txt line 8
Quoted 2 lines
Documentation/devicetree/bindings/power/power-controller.txt new file +2
-0+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
s/controller/control/
s/system-power/system power/
Quoted 66 lines
Documentation/devicetree/bindings/power/power-controller.txt new file +10
-0+is found programmatically, use the helper function "of_is_system_power_controller" +from of.h . + +Example: + +act8846: act8846 at 5 { + compatible = "active-semi,act8846"; + status = "okay"; + system-power-controller; +}Documentation/devicetree/bindings/power/poweroff.txt deleted +0
-18@@ -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 at 5 { - compatible = "active-semi,act8846"; - status = "okay"; - poweroff-source; -}drivers/of/base.c +24
-0@@ -2260,3 +2260,37 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)return of_get_next_parent (np); }EXPORT_SYMBOL (of_graph_get_remote_port); + +/** +* 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 +*/ +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 ; + } +/* Backward compatibility with previous property "vendor,system-power-controller", +* we just check that an non-empty vendor-prefix exists here +*/
drivers/of/base.c struct device_node *of_graph_get_remote_port(const struct device_node *node) · line 2296
Quoted 10 lines
drivers/of/base.c +10
-0+ 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; +} +EXPORT_SYMBOL (of_is_system_power_controller);
I think this is the wrong approach. This way any driver will recognise the old deprecated vendor prefixes. But not only those -- also misspelled vendor prefixes.
Keep it simple and only parse the new property name (inline in the header file).
If we need to support the old property names with vendor prefix (we have dropped vendor prefixes in the past), then you could add a second helper
of_is_system_power_controller_compat(np, compat_propname)
where you pass in the whole old property name (e.g. "ti,system-power-controller") and use that name as a fall back.
This way it will be clear which drivers are still supporting the deprecated property names, and we can make sure that no new ones will.
Quoted 13 lines
include/linux/of.h +0
-5@@ -910,15 +910,11 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,/* CONFIG_OF_RESOLVE api */ extern int of_resolve_phandles (struct device_node *tree); -/** -* of_system_has_poweroff_source - Tells if poweroff-source is found for device_node -* @np: Pointer to the given device_node -* -* return true if present false otherwise
include/linux/of.h static inline int of_changeset_update_property(struct of_changeset *ocs, · line 920
Quoted 10 lines
include/linux/of.h +3
-2-*/ +bool of_is_system_power_controller (const struct device_node *np); +static inline bool of_system_has_poweroff_source (const struct device_node *np) { -return of_property_read_bool (np,"poweroff-source" ); +return of_is_system_power_controller (np); }#endif /* _LINUX_OF_H */
Johan
Re: [PATCH v2 1/4] of: Rename "poweroff-source" property to "system-power-controller"
2014-11-05 11:08 GMT+01:00 Johan Hovold <[email protected]>:
Quoted 9 lines
[ Resend with lkml, arm, Felipe on CC -- why were these dropped from CC? ]
Quoted 2 lines · Romain Perier
As discussed on the mailing list, it makes more sense to rename this property to "system-power-controller".
Please also refer to the commit in the regulator tree renaming it to "poweroff-source", and that this in effect is a revert to the old name but without the vendor prefix.
Ok
Quoted 11 lines
Quoted 11 lines
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.
Quoted 4 lines
Quoted 1 line
Signed-off-by Romain Perier <[email protected]>First of all, always run your patches through checkpatch.pl before submitting. There's a few warnings there for you to fix.
Ok, noted.
Quoted 11 lines
I think this is the wrong approach. This way any driver will recognise the old deprecated vendor prefixes. But not only those -- also misspelled vendor prefixes.
Keep it simple and only parse the new property name (inline in the header file).
If we need to support the old property names with vendor prefix (we have dropped vendor prefixes in the past), then you could add a second helper
Quoted 8 lines
of_is_system_power_controller_compat(np, compat_propname)where you pass in the whole old property name (e.g. "ti,system-power-controller") and use that name as a fall back.
This way it will be clear which drivers are still supporting the deprecated property names, and we can make sure that no new ones will.
I just want to be sure, do we need to keep backward compatibility or not ? Previous series did not contain backward compatibility and it was like "The Nursery Chainsaw Massacre" and created a lot of drama... People's opinion are welcome.
Re: [PATCH v2 1/4] of: Rename "poweroff-source" property to "system-power-controller"
In reply to Romain PerierQuoted 11 lines · 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.
Quoted 4 lines
Signed-off-by Romain Perier <[email protected]>--- .../devicetree/bindings/power/power-controller.txt | 18 ++++++++++++ .../devicetree/bindings/power/poweroff.txt | 18 ------------
Tip: use -M when using git diff or git send-email to track renames. The documentation should show up only as a rename, not separate add and remove files.
Quoted 12 lines
drivers/of/base.c | 34 ++++++++++++++++++++++ include/linux/of.h | 10 ++----- 4 files changed, 55 insertions(+), 25 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/power-controller.txt delete mode 100644 Documentation/devicetree/bindings/power/poweroff.txtDocumentation/devicetree/bindings/power/power-controller.txt new file +0
-0
Documentation/devicetree/bindings/power/power-controller.txt line 10
Quoted 10 lines
Documentation/devicetree/bindings/power/power-controller.txt new file +10
-0@@ -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 .
You've reflowed the text which makes it harder for git to recognise that this is a renamed file. Typically you would put the rename into a separate patch to cut down on the noise for reviewers.
Quoted 60 lines
Documentation/devicetree/bindings/power/power-controller.txt new file +8
-0+ +Example: + +act8846: act8846@5 { + compatible = "active-semi,act8846"; + status = "okay"; + system-power-controller; +}Documentation/devicetree/bindings/power/poweroff.txt deleted +0
-18@@ -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; -}drivers/of/base.c +20
-0@@ -2260,3 +2260,37 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)return of_get_next_parent (np); }EXPORT_SYMBOL (of_graph_get_remote_port); + +/** +* 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 +*/ +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 ;
drivers/of/base.c struct device_node *of_graph_get_remote_port(const struct device_node *node) · line 2292
Quoted 10 lines
drivers/of/base.c +10
-0+ } +/* Backward compatibility with previous property "vendor,system-power-controller", +* we just check that an non-empty vendor-prefix exists here +*/ + sep =strchr (pp->name,',' ); +if (sep && sep - pp->name &&of_prop_cmp (sep +1 ,"system-power-controller" ) ==0 ) { + found = true; +break ; + } + }
Far too complicated. Keep the code simple. The following should be sufficient:
{
if (of_property_read_bool("system-power-controller"))
return true;
return of_property_read_bool("ti,system-power-controller");
}It doesn't need to be a catch-all for every possible vendor string. Only put in the ones that have actually been seen in the wild. If this needs to be merged right away as a regression fix for tps65910, then put this change, and *only* this change into a separate patch without renaming the function and submit that for merging.
Quoted 15 lines
drivers/of/base.c +4
-0+raw_spin_unlock_irqrestore (&devtree_lock, flags); +return found; +} +EXPORT_SYMBOL (of_is_system_power_controller);include/linux/of.h +0
-3@@ -910,15 +910,11 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,/* CONFIG_OF_RESOLVE api */ extern int of_resolve_phandles (struct device_node *tree); -/** -* of_system_has_poweroff_source - Tells if poweroff-source is found for device_node -* @np: Pointer to the given device_node
include/linux/of.h static inline int of_changeset_update_property(struct of_changeset *ocs, · line 918
Quoted 10 lines
include/linux/of.h +3
-4-* -* return true if present false otherwise -*/ +bool of_is_system_power_controller (const struct device_node *np); +static inline bool of_system_has_poweroff_source (const struct device_node *np) { -return of_property_read_bool (np,"poweroff-source" ); +return of_is_system_power_controller (np); }
Instead of this, just change all of the callers to the new interface in the same patch. It doesn't matter if the patch covers multiple subsystems, the affected maintainers will work it out between us.
g.
PATCH v2 2/4 regulator: act8865: Convert poweroff-source DT property to system-power-controller
In reply to Romain Perier--- Documentation/devicetree/bindings/regulator/act8865-regulator.txt | 4 ++-- drivers/regulator/act8865-regulator.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/regulator/act8865-regulator.txt +2 -2
@@ -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.
drivers/regulator/act8865-regulator.c +1 -1
@@ -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;
Signature · 1 line
1.9.1
PATCH v2 3/4 mfd: tps65910: Convert poweroff-source DT property to system-power-controller
In reply to Romain Perier--- drivers/mfd/tps65910.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mfd/tps65910.c +1 -1
@@ -423,7 +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_system_has_poweroff_source (np); + board_info->pm_off =of_is_system_power_controller (np);return board_info; }
Signature · 1 line
1.9.1
PATCH 4/4 of: Remove of_system_has_poweroff_source helper function
In reply to Romain PerierIt was only used as backward compatibility to avoid broken bisections, until all dependent drivers use the new helper function.
--- include/linux/of.h | 4 ---- 1 file changed, 4 deletions(-)
include/linux/of.h +0 -4
@@ -912,9 +912,5 @@ extern int of_resolve_phandles(struct device_node *tree);bool of_is_system_power_controller (const struct device_node *np); -static inline bool of_system_has_poweroff_source (const struct device_node *np) -{ -return of_is_system_power_controller (np); -}#endif /* _LINUX_OF_H */
Signature · 1 line
1.9.1
+CC Felipe and [email protected] , as both were interested by some parts of the previous serie (sorry when I tried to reduce the CC list, I forgot you)
2014-10-29 8:35 GMT+01:00 Romain Perier <[email protected]>:
Quoted 42 lines
The goal of this serie is to rename the property "poweroff-source" to "system-power-controller" and to fix things incrementally.
Changes and explanations since v1:
- The first patch defines "
of_is_system_power_controller" which is compatible with both "vendor,system-power-controller" and "system-power-controller" properties. Also, It keeps the old helper functionof_system_has_poweroff_sourcefor source compatibility until everything is renamed (in this way, bisections are not broken and change is made "atomically" between each commit)Note: the property "poweroff-source" itself is not used in dts files yet. Before this patch tps65910 was broken due to missing backward compatibility with "vendor,system-power-controller". As the old helper uses the new one, it works again.
- act8865 and tps65910 are ported to the new helper function
- The last commit removes the olf helper which was only used for source compatibility
Romain Perier (4):
of: Rename "poweroff-source" property to "system-power-controller"
regulator: act8865: Convert poweroff-source DT property to
system-power-controller
mfd: tps65910: Convert poweroff-source DT property to
system-power-controller
of: Removeof_system_has_poweroff_sourcehelper function.../devicetree/bindings/power/power-controller.txt | 18 ++++++++++++ .../devicetree/bindings/power/poweroff.txt | 18 ------------ .../bindings/regulator/act8865-regulator.txt | 4 +-- drivers/mfd/tps65910.c | 2 +- drivers/of/base.c | 34 ++++++++++++++++++++++ drivers/regulator/act8865-regulator.c | 2 +- include/linux/of.h | 12 ++------ 7 files changed, 58 insertions(+), 32 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/power-controller.txt delete mode 100644 Documentation/devicetree/bindings/power/poweroff.txt--
1.9.1
Quoted 2 lines · Romain Perier
The goal of this serie is to rename the property "poweroff-source" to "system-power-controller" and to fix things incrementally.
Please mention (here and in the commit message) that this is a rename back to the old and fairly established property name, but without the vendor prefix.
Essentially, it is a revert of a commit that is only in the regulator tree.
Quoted 8 lines
Changes and explanations since v1:
- The first patch defines "
of_is_system_power_controller" which is compatible with both "vendor,system-power-controller" and "system-power-controller" properties. Also, It keeps the old helper functionof_system_has_poweroff_sourcefor source compatibility until everything is renamed (in this way, bisections are not broken and change is made "atomically" between each commit)
Quoted 4 lines
Note: the property "poweroff-source" itself is not used in dts files yet. Before this patch tps65910 was broken due to missing backward compatibility with "vendor,system-power-controller". As the old helper uses the new one, it works again.
What patch broke tps65910? Please refer to it using commit id and summary (in parenthesis).
Is that commit currently only in the regulator tree? If so, that could be an argument for not doing this change incrementally (but that's not my decision to make).
Quoted 5 lines
- act8865 and tps65910 are ported to the new helper function
- The last commit removes the olf helper which was only used for source compatibility
I think you should squash the last three patches (i.e. update the drivers and remove the helper in one patch).
Quoted 7 lines
Romain Perier (4):
of: Rename "poweroff-source" property to "system-power-controller"
regulator: act8865: Convert poweroff-source DT property to
system-power-controller
mfd: tps65910: Convert poweroff-source DT property to
system-power-controller
of: Removeof_system_has_poweroff_sourcehelper function
Please do not mix patch revisions in your Subject lines when submitting an updated series. Update the revision number for the series and all patches in it even if a single patch happens to be unchanged (or new as patch 4/4).
Johan
(did not include all in previous reply)
Hi Johan,
2014-11-05 10:39 GMT+01:00 Johan Hovold <[email protected]>:
Quoted 3 lines
Please mention (here and in the commit message) that this is a rename back to the old and fairly established property name, but without the vendor prefix.
Ok will do.
Quoted 3 lines
Essentially, it is a revert of a commit that is only in the regulator tree.
In regulator and mfd tree, afaik.
Quoted 9 lines
Quoted 9 lines
Changes and explanations since v1:
- The first patch defines "
of_is_system_power_controller" which is compatible with both "vendor,system-power-controller" and "system-power-controller" properties. Also, It keeps the old helper functionof_system_has_poweroff_sourcefor source compatibility until everything is renamed (in this way, bisections are not broken and change is made "atomically" between each commit)
Quoted 7 lines
Quoted 4 lines
Note: the property "poweroff-source" itself is not used in dts files yet. Before this patch tps65910 was broken due to missing backward compatibility with "vendor,system-power-controller". As the old helper uses the new one, it works again.
What patch broke tps65910? Please refer to it using commit id and summary (in parenthesis).
commit 645d10214b2867d26dd97cafb43a3a8b3ec1da66 (mfd: tps65910:
Convert ti,system-power-controller DT property to poweroff-source)
It breaks backward compatibility with existing dts files, the driver itself is not broken.
Quoted 4 lines
Is that commit currently only in the regulator tree? If so, that could be an argument for not doing this change incrementally (but that's not my decision to make).
As I said, in mfd and regulator tree. But it might have been cross-merged somewhere... no ? (and not in "next" yet)
Quoted 2 lines
I think you should squash the last three patches (i.e. update the drivers and remove the helper in one patch).
If it does not create drama and does not cause compat issues, I also think that this is the simpler solution.
Quoted 9 lines
Quoted 8 lines
Romain Perier (4):
of: Rename "poweroff-source" property to "system-power-controller"
regulator: act8865: Convert poweroff-source DT property to
system-power-controller
mfd: tps65910: Convert poweroff-source DT property to
system-power-controller
of: Removeof_system_has_poweroff_sourcehelper function
Quoted 4 lines
Please do not mix patch revisions in your Subject lines when submitting an updated series. Update the revision number for the series and all patches in it even if a single patch happens to be unchanged (or new as patch 4/4).
Arf, did not know... sorry ^^ . I will try my best next time.
Thanks for your feedbacks,
Romain
2014-11-05 10:39 GMT+01:00 Johan Hovold <[email protected]>:
Quoted 54 lines · Romain Perier
Quoted 2 lines
The goal of this serie is to rename the property "poweroff-source" to "system-power-controller" and to fix things incrementally.
Please mention (here and in the commit message) that this is a rename back to the old and fairly established property name, but without the vendor prefix.
Essentially, it is a revert of a commit that is only in the regulator tree.
Quoted 8 lines
Changes and explanations since v1:
- The first patch defines "
of_is_system_power_controller" which is compatible with both "vendor,system-power-controller" and "system-power-controller" properties. Also, It keeps the old helper functionof_system_has_poweroff_sourcefor source compatibility until everything is renamed (in this way, bisections are not broken and change is made "atomically" between each commit)Quoted 4 lines
Note: the property "poweroff-source" itself is not used in dts files yet. Before this patch tps65910 was broken due to missing backward compatibility with "vendor,system-power-controller". As the old helper uses the new one, it works again.
What patch broke tps65910? Please refer to it using commit id and summary (in parenthesis).
Is that commit currently only in the regulator tree? If so, that could be an argument for not doing this change incrementally (but that's not my decision to make).
Quoted 5 lines
- act8865 and tps65910 are ported to the new helper function
- The last commit removes the olf helper which was only used for source compatibility
I think you should squash the last three patches (i.e. update the drivers and remove the helper in one patch).
Quoted 7 lines
Romain Perier (4):
of: Rename "poweroff-source" property to "system-power-controller"
regulator: act8865: Convert poweroff-source DT property to
system-power-controller
mfd: tps65910: Convert poweroff-source DT property to
system-power-controller
of: Removeof_system_has_poweroff_sourcehelper functionPlease do not mix patch revisions in your Subject lines when submitting an updated series. Update the revision number for the series and all patches in it even if a single patch happens to be unchanged (or new as patch 4/4).
Johan