Re: [RFC PATCH v2 1/4] regulator: Add helper function to get "poweroff-source" property

Re: [RFC PATCH v2 1/4] regulator: Add helper function to get "poweroff-source" property

[PERIER Romain]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  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
>

[RFC PATCH v2 1/4] regulator: Add helper function to get "poweroff-source" property

[Romain Perier]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

[RFC PATCH v2 2/4] regulator: act8865: Add support to turn off all outputs

[Romain Perier]

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 <romain.perier@gmail.com>
---
 drivers/regulator/act8865-regulator.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index afd06f9..8bba591 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,14 @@ static int act8865_pmic_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	if (dev->of_node && is_system_poweroff_source(dev->of_node) &&
+		!pm_power_off) {
+		act8865_i2c_client = client;
+		act8865->off_reg = off_reg;
+		act8865->off_mask = off_mask;
+		pm_power_off = act8865_power_off;
+	}
+
 	/* Finally register devices */
 	for (i = 0; i < num_regulators; i++) {
 		const struct regulator_desc *desc = &regulators[i];
-- 
1.9.1

[RFC PATCH v1 3/4] ARM: dts: rockchip: Enable power off in pmic for Radxa Rock

[Romain Perier]

Add "poweroff-source" property to act8846 node.
shutdown/poweroff commands are now handled for this board.

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 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

[RFC PATCH v1 4/4] dt-bindings: Document the property poweroff-source for act8865 regulator

[Romain Perier]

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 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..4d7f33e 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
+
 Any standard regulator properties can be used to configure the single regulator.
 
 The valid names for regulators are:
-- 
1.9.1

Re: [RFC PATCH v2 1/4] regulator: Add helper function to get "poweroff-source" property

[Grant Likely]

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
>>

Re: [RFC PATCH v2 1/4] regulator: Add helper function to get "poweroff-source" property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 495 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH v2 1/4] regulator: Add helper function to get "poweroff-source" property

[PERIER Romain]

>
> 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 1/4] regulator: Add helper function to get "poweroff-source" property

[Heiko Stübner]

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

Re: [RFC PATCH v2 1/4] regulator: Add helper function to get "poweroff-source" property

[Grant Likely]

On Fri, Oct 10, 2014 at 1:29 PM, PERIER Romain <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.

Re: [RFC PATCH v2 1/4] regulator: Add helper function to get "poweroff-source" property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 392 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH v2 2/4] regulator: act8865: Add support to turn off all outputs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]

On Tue, Oct 07, 2014 at 07:45:02PM +0000, Romain Perier wrote:
> 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.

Reviwed-by: Mark Brown <broonie@kernel.org>

but...

> +	if (dev->of_node && is_system_poweroff_source(dev->of_node) &&
> +		!pm_power_off) {
> +		act8865_i2c_client = client;
> +		act8865->off_reg = off_reg;
> +		act8865->off_mask = off_mask;
> +		pm_power_off = act8865_power_off;
> +	}

Perhaps worth wrapping the dev->of_node check into the function?  I
imagine the same pattern will be quite common.

Might also make sense to warn if we've got the property but we're not
setting pm_power_off - it's indicating that things aren't working as
expected, an error will help users figure out what's going wrong.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH v1 4/4] dt-bindings: Document the property poweroff-source for act8865 regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 429 bytes --]

On Tue, Oct 07, 2014 at 07:45:04PM +0000, Romain Perier wrote:
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  Documentation/devicetree/bindings/regulator/act8865-regulator.txt | 4 ++++
>  1 file changed, 4 insertions(+)

Should this documentation be put in some central place (and perhaps
referenced from this binding) given that we're trying to make it a
standard property?  Not sure where it'd fit though...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH v1 4/4] dt-bindings: Document the property poweroff-source for act8865 regulator

[PERIER Romain]

Mark,

I did not update the whole serie yet, so, this is the old version... :)
(my patch about act8865 too, as "is_system_poweroff_source" should be
prefixed by "of_get" as requested by Grant)
I noted all your remarks.

Thanks

2014-10-13 15:17 GMT+02:00 Mark Brown <broonie@kernel.org>:
> On Tue, Oct 07, 2014 at 07:45:04PM +0000, Romain Perier wrote:
>> Signed-off-by: Romain Perier <romain.perier@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/regulator/act8865-regulator.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>
> Should this documentation be put in some central place (and perhaps
> referenced from this binding) given that we're trying to make it a
> standard property?  Not sure where it'd fit though...