| Message ID | 1414427215-14380-1-git-send-email-romain.perier@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 27, 2014 at 04:26:46PM +0000, Romain Perier wrote: > As discussed on the mailing list, it makes more sense to rename this property > to "system-power-controller". Problem being that the word "source" usually tends > to be used for inputs and that is out of control of the OS. The poweroff > capability is an output which simply turns the system-power off. Also, this > property might be used by drivers which power-off the system and power back on > subsequent RTC alarms. This seems to suggest to remove "poweroff" from the > property name and to choose "system-power-controller" as the more generic name. > This patchs adds the required renaming changes and defines an helper function > which is compatible with both properties, the old one prefixed by a vendor name > and the new one without any prefix. > > Signed-off-by: Romain Perier <romain.perier@gmail.com> > --- > include/linux/of.h | 27 +++++++++++++++++++++++---- > 1 file changed, 23 insertions(+), 4 deletions(-) > > diff --git a/include/linux/of.h b/include/linux/of.h > index 27b3ba1..c1ed2a5 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -867,14 +867,33 @@ static inline int of_changeset_update_property(struct of_changeset *ocs, > extern int of_resolve_phandles(struct device_node *tree); > > /** > - * of_system_has_poweroff_source - Tells if poweroff-source is found for device_node > + * of_is_system_power_controller - Tells if the property for controlling system > + * power is found in device_node. > * @np: Pointer to the given device_node > * > * return true if present false otherwise > */ > -static inline bool of_system_has_poweroff_source(const struct device_node *np) > -{ > - return of_property_read_bool(np, "poweroff-source"); > +static inline bool of_is_system_power_controller(const struct device_node *np) > +{ > + struct property *pp; > + unsigned long flags; > + char *sep; > + bool found = false; > + > + raw_spin_lock_irqsave(&devtree_lock, flags); > + for_each_property_of_node(np, pp) { > + if (of_prop_cmp(pp->name, "system-power-controller") == 0) { > + found = true; > + break; > + } > + sep = strchr(pp->name, ','); > + if (sep && sep - pp->name && of_prop_cmp(sep + 1, "system-power-controller") == 0) { > + found = true; > + break; > + } > + } > + raw_spin_unlock_irqrestore(&devtree_lock, flags); > + return found; > } > > #endif /* _LINUX_OF_H */ I think you still need to support poweroff-source since it has been released on a stable kernel. Perhaps add a warning message telling users it's deprecated and asking them to switch over to system-power-controller ? Still, simply removing it isn't very nice.
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
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.
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
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 04:26:46PM +0000, Romain Perier wrote: > As discussed on the mailing list, it makes more sense to rename this property > to "system-power-controller". Problem being that the word "source" usually tends > to be used for inputs and that is out of control of the OS. The poweroff > capability is an output which simply turns the system-power off. Also, this > property might be used by drivers which power-off the system and power back on > subsequent RTC alarms. This seems to suggest to remove "poweroff" from the > property name and to choose "system-power-controller" as the more generic name. > This patchs adds the required renaming changes and defines an helper function > which is compatible with both properties, the old one prefixed by a vendor name > and the new one without any prefix. > > Signed-off-by: Romain Perier <romain.perier@gmail.com> > --- > include/linux/of.h | 27 +++++++++++++++++++++++---- > 1 file changed, 23 insertions(+), 4 deletions(-) > > diff --git a/include/linux/of.h b/include/linux/of.h > index 27b3ba1..c1ed2a5 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -867,14 +867,33 @@ static inline int of_changeset_update_property(struct of_changeset *ocs, > extern int of_resolve_phandles(struct device_node *tree); > > /** > - * of_system_has_poweroff_source - Tells if poweroff-source is found for device_node > + * of_is_system_power_controller - Tells if the property for controlling system > + * power is found in device_node. > * @np: Pointer to the given device_node > * > * return true if present false otherwise > */ > -static inline bool of_system_has_poweroff_source(const struct device_node *np) > -{ > - return of_property_read_bool(np, "poweroff-source"); > +static inline bool of_is_system_power_controller(const struct device_node *np) > +{ > + struct property *pp; > + unsigned long flags; > + char *sep; > + bool found = false; > + > + raw_spin_lock_irqsave(&devtree_lock, flags); > + for_each_property_of_node(np, pp) { > + if (of_prop_cmp(pp->name, "system-power-controller") == 0) { > + found = true; > + break; > + } > + sep = strchr(pp->name, ','); > + if (sep && sep - pp->name && of_prop_cmp(sep + 1, "system-power-controller") == 0) { > + found = true; > + break; > + } > + } > + raw_spin_unlock_irqrestore(&devtree_lock, flags); > + return found; > } You probably don't want this to be inline. Johan
2014-10-27 18:16 GMT+01:00 Johan Hovold <johan@kernel.org>: > You probably don't want this to be inline. > Yeah this is what I thought, I agree, it's a bit big to be inline... I will fix it, I just wait a bit to see what is decided for this serie...
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 */
As discussed on the mailing list, it makes more sense to rename this property to "system-power-controller". Problem being that the word "source" usually tends to be used for inputs and that is out of control of the OS. The poweroff capability is an output which simply turns the system-power off. Also, this property might be used by drivers which power-off the system and power back on subsequent RTC alarms. This seems to suggest to remove "poweroff" from the property name and to choose "system-power-controller" as the more generic name. This patchs adds the required renaming changes and defines an helper function which is compatible with both properties, the old one prefixed by a vendor name and the new one without any prefix. Signed-off-by: Romain Perier <romain.perier@gmail.com> --- include/linux/of.h | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)