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