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 <[email protected]>
---
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)
+{
+ return of_property_read_bool(np, "poweroff-source");
+}
+
#endif /* _LINUX_OF_H */
--
1.9.1
When the property "poweroff-source" is found in the
devicetree, the function pm_power_off is defined. This function sends the
rights bit fields to the global off control register. shutdown/poweroff
commands are now supported for hardware components which use these PMU.
Signed-off-by: Romain Perier <[email protected]>
---
drivers/regulator/act8865-regulator.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index afd06f9..76301ed 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -61,6 +61,8 @@
#define ACT8846_REG12_VSET 0xa0
#define ACT8846_REG12_CTRL 0xa1
#define ACT8846_REG13_CTRL 0xb1
+#define ACT8846_GLB_OFF_CTRL 0xc3
+#define ACT8846_OFF_SYSMASK 0x18
/*
* ACT8865 Global Register Map.
@@ -84,6 +86,7 @@
#define ACT8865_LDO3_CTRL 0x61
#define ACT8865_LDO4_VSET 0x64
#define ACT8865_LDO4_CTRL 0x65
+#define ACT8865_MSTROFF 0x20
/*
* Field Definitions.
@@ -98,6 +101,8 @@
struct act8865 {
struct regmap *regmap;
+ int off_reg;
+ int off_mask;
};
static const struct regmap_config act8865_regmap_config = {
@@ -275,6 +280,16 @@ static struct regulator_init_data
return NULL;
}
+static struct i2c_client *act8865_i2c_client;
+static void act8865_power_off(void)
+{
+ struct act8865 *act8865;
+
+ act8865 = i2c_get_clientdata(act8865_i2c_client);
+ regmap_write(act8865->regmap, act8865->off_reg, act8865->off_mask);
+ while (1);
+}
+
static int act8865_pmic_probe(struct i2c_client *client,
const struct i2c_device_id *i2c_id)
{
@@ -285,6 +300,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
int i, ret, num_regulators;
struct act8865 *act8865;
unsigned long type;
+ int off_reg, off_mask;
pdata = dev_get_platdata(dev);
@@ -304,10 +320,14 @@ static int act8865_pmic_probe(struct i2c_client *client,
case ACT8846:
regulators = act8846_regulators;
num_regulators = ARRAY_SIZE(act8846_regulators);
+ off_reg = ACT8846_GLB_OFF_CTRL;
+ off_mask = ACT8846_OFF_SYSMASK;
break;
case ACT8865:
regulators = act8865_regulators;
num_regulators = ARRAY_SIZE(act8865_regulators);
+ off_reg = ACT8865_SYS_CTRL;
+ off_mask = ACT8865_MSTROFF;
break;
default:
dev_err(dev, "invalid device id %lu\n", type);
@@ -345,6 +365,17 @@ static int act8865_pmic_probe(struct i2c_client *client,
return ret;
}
+ if (of_system_has_poweroff_source(dev->of_node)) {
+ if (!pm_power_off) {
+ act8865_i2c_client = client;
+ act8865->off_reg = off_reg;
+ act8865->off_mask = off_mask;
+ pm_power_off = act8865_power_off;
+ } else {
+ dev_err(dev, "Failed to set poweroff capability, already defined\n");
+ }
+ }
+
/* Finally register devices */
for (i = 0; i < num_regulators; i++) {
const struct regulator_desc *desc = ®ulators[i];
--
1.9.1
Signed-off-by: Romain Perier <[email protected]>
---
Documentation/devicetree/bindings/regulator/act8865-regulator.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/regulator/act8865-regulator.txt b/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
index 865614b..01a5b07 100644
--- a/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
@@ -5,6 +5,10 @@ Required properties:
- compatible: "active-semi,act8846" or "active-semi,act8865"
- 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 .
+
Any standard regulator properties can be used to configure the single regulator.
The valid names for regulators are:
--
1.9.1
Signed-off-by: Romain Perier <[email protected]>
---
Documentation/devicetree/bindings/power/poweroff.txt | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/poweroff.txt
diff --git a/Documentation/devicetree/bindings/power/poweroff.txt b/Documentation/devicetree/bindings/power/poweroff.txt
new file mode 100644
index 0000000..845868b
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/poweroff.txt
@@ -0,0 +1,18 @@
+* 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;
+}
--
1.9.1
Add "poweroff-source" property to act8846 node.
shutdown/poweroff commands are now handled for this board.
Signed-off-by: Romain Perier <[email protected]>
---
arch/arm/boot/dts/rk3188-radxarock.dts | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/boot/dts/rk3188-radxarock.dts b/arch/arm/boot/dts/rk3188-radxarock.dts
index 15910c9..2affff0 100644
--- a/arch/arm/boot/dts/rk3188-radxarock.dts
+++ b/arch/arm/boot/dts/rk3188-radxarock.dts
@@ -141,6 +141,8 @@
pinctrl-names = "default";
pinctrl-0 = <&act8846_dvs0_ctl>;
+ poweroff-source;
+
regulators {
vcc_ddr: REG1 {
regulator-name = "VCC_DDR";
--
1.9.1
On Tue, 14 Oct 2014 06:31:09 +0000
, Romain Perier <[email protected]>
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"
> which marks the device as able to shutdown the system.
>
> Signed-off-by: Romain Perier <[email protected]>
Acked-by: Grant Likely <[email protected]>
And same for the documentation patches.
g.
> ---
> 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)
> +{
> + return of_property_read_bool(np, "poweroff-source");
> +}
> +
> #endif /* _LINUX_OF_H */
> --
> 1.9.1
>
On Wed, Oct 15, 2014 at 02:41:42PM +0200, Grant Likely wrote:
> , Romain Perier <[email protected]>
> 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"
> > which marks the device as able to shutdown the system.
> > Signed-off-by: Romain Perier <[email protected]>
> Acked-by: Grant Likely <[email protected]>
> And same for the documentation patches.
I guess I should apply these (except the DTS update) since the first
user that's being added is a regulator driver?
Am Mittwoch, 15. Oktober 2014, 15:42:45 schrieb Mark Brown:
> On Wed, Oct 15, 2014 at 02:41:42PM +0200, Grant Likely wrote:
> > , Romain Perier <[email protected]>
> >
> > 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"
> > > which marks the device as able to shutdown the system.
> > >
> > > Signed-off-by: Romain Perier <[email protected]>
> >
> > Acked-by: Grant Likely <[email protected]>
> >
> > And same for the documentation patches.
>
> I guess I should apply these (except the DTS update) since the first
> user that's being added is a regulator driver?
I'd think so.
In any case, I'll take the "ARM: dts: ..." patch if you take the others.
Heiko
On Wed, Oct 15, 2014 at 03:56:03PM +0200, Heiko St?bner wrote:
> Am Mittwoch, 15. Oktober 2014, 15:42:45 schrieb Mark Brown:
> > I guess I should apply these (except the DTS update) since the first
> > user that's being added is a regulator driver?
> I'd think so.
> In any case, I'll take the "ARM: dts: ..." patch if you take the others.
Sounds like a plan. I just tried applying and got some conflicts but
I'm guessing that this is due to changes that are landing in the merge
window so I'll try again once -rc1 is out.
Hi all,
I am just curious but where do you plan to merge this serie ? in
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git ?
You have conflicts because I use linux-next as base repository.
Thanks for your feebacks (all of you).
Romain
2014-10-15 16:03 GMT+02:00 Mark Brown <[email protected]>:
> On Wed, Oct 15, 2014 at 03:56:03PM +0200, Heiko Stübner wrote:
>> Am Mittwoch, 15. Oktober 2014, 15:42:45 schrieb Mark Brown:
>
>> > I guess I should apply these (except the DTS update) since the first
>> > user that's being added is a regulator driver?
>
>> I'd think so.
>> In any case, I'll take the "ARM: dts: ..." patch if you take the others.
>
> Sounds like a plan. I just tried applying and got some conflicts but
> I'm guessing that this is due to changes that are landing in the merge
> window so I'll try again once -rc1 is out.
Hi Romain,
Am Freitag, 17. Oktober 2014, 08:01:31 schrieb PERIER Romain:
> Hi all,
>
> I am just curious but where do you plan to merge this serie ? in
> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git ?
The single dts patch will be going into my dts branch and the rest will go
through Mark's regulator tree. As Mark said, he'll try to apply these again
once 3.18-rc1 is released and if it still doesn't apply then, he'll probably
ask you to rebase them onto the regulator tree [which will at the time include
everything that is in -rc1].
Heiko
> 2014-10-15 16:03 GMT+02:00 Mark Brown <[email protected]>:
> > On Wed, Oct 15, 2014 at 03:56:03PM +0200, Heiko St?bner wrote:
> >> Am Mittwoch, 15. Oktober 2014, 15:42:45 schrieb Mark Brown:
> >> > I guess I should apply these (except the DTS update) since the first
> >> > user that's being added is a regulator driver?
> >>
> >> I'd think so.
> >> In any case, I'll take the "ARM: dts: ..." patch if you take the others.
> >
> > Sounds like a plan. I just tried applying and got some conflicts but
> > I'm guessing that this is due to changes that are landing in the merge
> > window so I'll try again once -rc1 is out.
Hi Heiko,
Oh sure, no problem. It was just to understand better how things will happen ;)
If my patches needs to be rebased, will do, of course.
Have a nice day,
Romain
2014-10-17 8:06 GMT+02:00 Heiko Stübner <[email protected]>:
> Hi Romain,
>
> Am Freitag, 17. Oktober 2014, 08:01:31 schrieb PERIER Romain:
>> Hi all,
>>
>> I am just curious but where do you plan to merge this serie ? in
>> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git ?
>
> The single dts patch will be going into my dts branch and the rest will go
> through Mark's regulator tree. As Mark said, he'll try to apply these again
> once 3.18-rc1 is released and if it still doesn't apply then, he'll probably
> ask you to rebase them onto the regulator tree [which will at the time include
> everything that is in -rc1].
>
>
> Heiko
>
>
>
>> 2014-10-15 16:03 GMT+02:00 Mark Brown <[email protected]>:
>> > On Wed, Oct 15, 2014 at 03:56:03PM +0200, Heiko Stübner wrote:
>> >> Am Mittwoch, 15. Oktober 2014, 15:42:45 schrieb Mark Brown:
>> >> > I guess I should apply these (except the DTS update) since the first
>> >> > user that's being added is a regulator driver?
>> >>
>> >> I'd think so.
>> >> In any case, I'll take the "ARM: dts: ..." patch if you take the others.
>> >
>> > Sounds like a plan. I just tried applying and got some conflicts but
>> > I'm guessing that this is due to changes that are landing in the merge
>> > window so I'll try again once -rc1 is out.
>
Hi all,
Do I need to rebase my patches onto the regulator tree or it's okay ?
Have a nice day,
Romain
2014-10-17 9:23 GMT+02:00 PERIER Romain <[email protected]>:
> Hi Heiko,
>
> Oh sure, no problem. It was just to understand better how things will happen ;)
> If my patches needs to be rebased, will do, of course.
>
> Have a nice day,
> Romain
>
> 2014-10-17 8:06 GMT+02:00 Heiko Stübner <[email protected]>:
>> Hi Romain,
>>
>> Am Freitag, 17. Oktober 2014, 08:01:31 schrieb PERIER Romain:
>>> Hi all,
>>>
>>> I am just curious but where do you plan to merge this serie ? in
>>> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git ?
>>
>> The single dts patch will be going into my dts branch and the rest will go
>> through Mark's regulator tree. As Mark said, he'll try to apply these again
>> once 3.18-rc1 is released and if it still doesn't apply then, he'll probably
>> ask you to rebase them onto the regulator tree [which will at the time include
>> everything that is in -rc1].
>>
>>
>> Heiko
>>
>>
>>
>>> 2014-10-15 16:03 GMT+02:00 Mark Brown <[email protected]>:
>>> > On Wed, Oct 15, 2014 at 03:56:03PM +0200, Heiko Stübner wrote:
>>> >> Am Mittwoch, 15. Oktober 2014, 15:42:45 schrieb Mark Brown:
>>> >> > I guess I should apply these (except the DTS update) since the first
>>> >> > user that's being added is a regulator driver?
>>> >>
>>> >> I'd think so.
>>> >> In any case, I'll take the "ARM: dts: ..." patch if you take the others.
>>> >
>>> > Sounds like a plan. I just tried applying and got some conflicts but
>>> > I'm guessing that this is due to changes that are landing in the merge
>>> > window so I'll try again once -rc1 is out.
>>
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"
> which marks the device as able to shutdown the system.
Applied everything except the DT update, thanks.
[ +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).
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.
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 <[email protected]>
> ---
> 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)?
> +{
> + return of_property_read_bool(np, "poweroff-source");
> +}
> +
> #endif /* _LINUX_OF_H */
Thanks,
Johan
Hi Johan,
If that's still possible to do these changes, I am opened to suggestions.
2014-10-23 11:53 GMT+02:00 Johan Hovold <[email protected]>:
> [ +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 :)
>
> 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" ?
>
> 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 <[email protected]>
>> ---
>> 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 ;)
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.
Have a nice day,
Romain
[+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 <[email protected]>:
> > [ +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 <[email protected]>
> >> ---
> >> 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
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 <[email protected]>:
> > > [ +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 <[email protected]>
> > >> ---
> > >>
> > >> 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
Johan,
You convinced me. I will add an helper function
"of_is_system_power_controller(node)" which is compatible with both
properties: with or without the vendor prefix (until everything switch
to the new one).
In this case , we can adapt all drivers without break compatibility
and in few months if we plan to remove this compability we will just
need to modify this helper function.
Heiko: will do, thanks your help. I will use the regulator tree as
based repository and then send fixes to Mark.
Romain
2014-10-26 12:53 GMT+01:00 Heiko Stübner <[email protected]>:
> 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 <[email protected]>:
>> > > [ +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 <[email protected]>
>> > >> ---
>> > >>
>> > >> 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