Re: [RFC v3 PATCH 1/5] of: Add standard property for poweroff capability

[Heiko Stübner]

Am Samstag, 25. Oktober 2014, 10:37:33 schrieb Johan Hovold:
> [+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.

just as sidenote, I'll hold off on applying patch3 (the dts change) then.

Romain, after you two (and maybe Mark) agree on the final naming of the 
property and function you'd need to
- send a followup patch against Marks tree, implementing the changes
- a new patch adding the property to the Radxa board


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/