| Message ID | 1412711104-15902-1-git-send-email-romain.perier@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
This is not the final location, I have no idea exactly where I might put this helper function. This is why I ask you your opinion (and also, this is why that's a proposal) 2014-10-07 21:45 GMT+02:00 Romain Perier <romain.perier@gmail.com>: > 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" > which marks the device as able to shutdown the system. > > Signed-off-by: Romain Perier <romain.perier@gmail.com> > --- > drivers/regulator/of_regulator.c | 12 ++++++++++++ > include/linux/regulator/of_regulator.h | 6 ++++++ > 2 files changed, 18 insertions(+) > > diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c > index 7a51814..8b898e6 100644 > --- a/drivers/regulator/of_regulator.c > +++ b/drivers/regulator/of_regulator.c > @@ -240,3 +240,15 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev, > > return init_data; > } > + > +/** > + * is_system_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 is_system_poweroff_source(const struct device_node *np) > +{ > + return of_property_read_bool(np, "poweroff-source"); > +} > +EXPORT_SYMBOL_GPL(is_system_poweroff_source); > diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h > index f921796..9d8fbb2 100644 > --- a/include/linux/regulator/of_regulator.h > +++ b/include/linux/regulator/of_regulator.h > @@ -20,6 +20,7 @@ extern struct regulator_init_data > extern int of_regulator_match(struct device *dev, struct device_node *node, > struct of_regulator_match *matches, > unsigned int num_matches); > +extern bool is_system_poweroff_source(const struct device_node *np); > #else > static inline struct regulator_init_data > *of_get_regulator_init_data(struct device *dev, > @@ -35,6 +36,11 @@ static inline int of_regulator_match(struct device *dev, > { > return 0; > } > + > +static inline bool is_system_poweroff_source(const struct device_node *np) > +{ > + return false; > +} > #endif /* CONFIG_OF */ > > #endif /* __LINUX_OF_REG_H */ > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 7, 2014 at 8:43 PM, PERIER Romain <romain.perier@gmail.com> wrote: > This is not the final location, I have no idea exactly where I might > put this helper function. This is why I ask you your opinion (and > also, this is why that's a proposal) > > 2014-10-07 21:45 GMT+02:00 Romain Perier <romain.perier@gmail.com>: >> 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" >> which marks the device as able to shutdown the system. >> >> Signed-off-by: Romain Perier <romain.perier@gmail.com> >> --- >> drivers/regulator/of_regulator.c | 12 ++++++++++++ >> include/linux/regulator/of_regulator.h | 6 ++++++ >> 2 files changed, 18 insertions(+) >> >> diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c >> index 7a51814..8b898e6 100644 >> --- a/drivers/regulator/of_regulator.c >> +++ b/drivers/regulator/of_regulator.c >> @@ -240,3 +240,15 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev, >> >> return init_data; >> } >> + >> +/** >> + * is_system_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 is_system_poweroff_source(const struct device_node *np) >> +{ >> + return of_property_read_bool(np, "poweroff-source"); >> +} >> +EXPORT_SYMBOL_GPL(is_system_poweroff_source); Hi Romain, This is an extremely simple wrapper. It may be best to simply make it a static inline. It is also generic enough that I don't have a problem with it living in include/linux/of.h; but of_regulator.h would be fine too. Since this is a device tree specific function (it doesn't work with ACPI or platform_data), you should prefix the function name with of_ g. What I'm more concerned about is the semantics of the property. What do the generic code paths gain by standardizing this property? Is it expected that >> diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h >> index f921796..9d8fbb2 100644 >> --- a/include/linux/regulator/of_regulator.h >> +++ b/include/linux/regulator/of_regulator.h >> @@ -20,6 +20,7 @@ extern struct regulator_init_data >> extern int of_regulator_match(struct device *dev, struct device_node *node, >> struct of_regulator_match *matches, >> unsigned int num_matches); >> +extern bool is_system_poweroff_source(const struct device_node *np); >> #else >> static inline struct regulator_init_data >> *of_get_regulator_init_data(struct device *dev, >> @@ -35,6 +36,11 @@ static inline int of_regulator_match(struct device *dev, >> { >> return 0; >> } >> + >> +static inline bool is_system_poweroff_source(const struct device_node *np) >> +{ >> + return false; >> +} >> #endif /* CONFIG_OF */ >> >> #endif /* __LINUX_OF_REG_H */ >> -- >> 1.9.1 >> -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 10, 2014 at 12:47:08PM +0100, Grant Likely wrote: > This is an extremely simple wrapper. It may be best to simply make it > a static inline. It is also generic enough that I don't have a problem > with it living in include/linux/of.h; but of_regulator.h would be fine > too. I think of.h might be a better idea - it's not something that needs to be regulator specific, system power controllers could provide the same functionality but may not offer any regulator control to Linux.
> > What I'm more concerned about is the semantics of the property. What > do the generic code paths gain by standardizing this property? Is it > expected that We really need to come up with a standard property for this and document it rather than continuing to add individual device specific properties each time a driver adds poweroff capability, all doing the same thing (a lot of regulators driver, mfd drivers, soc specific drivers, power drivers already do that, that's very redudant) . This is a simple unification logic. About its name, I found my inspiration with "wakeup-source" which marks an device as able to wakeup the system, poweroff capability is exactly the same except that the device will control the power of the system, so I choose "poweroff-source". However, suggestions are welcome ;) About of_regulator.c, I agree with Mark. poweroff capability is not really specific to regulators, so it does not make sense to put the helper there, imho. Romain -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Grant, Am Freitag, 10. Oktober 2014, 12:47:08 schrieb Grant Likely: > What I'm more concerned about is the semantics of the property. What > do the generic code paths gain by standardizing this property? Is it > expected that [seems to be missing some text in the original mail] We currently see an influx of system-power-controller variants. While in the past it only was ti,system-power-controller there is now already maxim,system-power-controller rockchip,system-power-controller Romain's work would introduce "active-semi,system-power-controller", I have a "netronix,system-power-controller" sitting in some distant tree and there may be more already waiting somewhere. So in the worst case I'd think you could expect such a property for every pmic-vendor in vendor-prefixes.txt ... as it seems to be a quite common use- case these days to have the pmic handle system power on its own. Heiko -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 10, 2014 at 1:29 PM, PERIER Romain <romain.perier@gmail.com> wrote: >> >> What I'm more concerned about is the semantics of the property. What >> do the generic code paths gain by standardizing this property? Is it >> expected that > > We really need to come up with a standard property for this and document > it rather than continuing to add individual device specific properties > each time a driver adds poweroff capability, > all doing the same thing (a lot of regulators driver, mfd drivers, soc > specific drivers, power drivers already do that, that's very redudant) > . This is a simple unification logic. Yes, it's fine. I accidentally left that paragraph in when I sent my reply. I started writing that concern, and then thought better of it. The property is fine. g. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 07, 2014 at 07:45:01PM +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 This seems fine from a code point of view but as discussed it's probably better placed outside of the regulator API. Making it a static inline doesn't seem like a bad idea either.
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c index 7a51814..8b898e6 100644 --- a/drivers/regulator/of_regulator.c +++ b/drivers/regulator/of_regulator.c @@ -240,3 +240,15 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev, return init_data; } + +/** + * is_system_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 is_system_poweroff_source(const struct device_node *np) +{ + return of_property_read_bool(np, "poweroff-source"); +} +EXPORT_SYMBOL_GPL(is_system_poweroff_source); diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h index f921796..9d8fbb2 100644 --- a/include/linux/regulator/of_regulator.h +++ b/include/linux/regulator/of_regulator.h @@ -20,6 +20,7 @@ extern struct regulator_init_data extern int of_regulator_match(struct device *dev, struct device_node *node, struct of_regulator_match *matches, unsigned int num_matches); +extern bool is_system_poweroff_source(const struct device_node *np); #else static inline struct regulator_init_data *of_get_regulator_init_data(struct device *dev, @@ -35,6 +36,11 @@ static inline int of_regulator_match(struct device *dev, { return 0; } + +static inline bool is_system_poweroff_source(const struct device_node *np) +{ + return false; +} #endif /* CONFIG_OF */ #endif /* __LINUX_OF_REG_H */
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" which marks the device as able to shutdown the system. Signed-off-by: Romain Perier <romain.perier@gmail.com> --- drivers/regulator/of_regulator.c | 12 ++++++++++++ include/linux/regulator/of_regulator.h | 6 ++++++ 2 files changed, 18 insertions(+)