[PATCH] regulator: adding disable options for regulator-always-on and regulator-boots-on

[PATCH] regulator: adding disable options for regulator-always-on and regulator-boots-on

[Hugh Kang]

From: "hugh.kang" <hugh.kang@lge.com>

If a regulator is set by always-on option, the regulator will be set forever.
For example, suppose LDO1 is set to always-on at RevA.dts with including of a.dtsi. After that
RevB.dts may wants to include the same a.dtsi but override the LDO1 always-on option. However,
currently there is no way to delete the always-on option, even when we change the LDO1 option value,
the always-on setting is still remains.

Signed-off-by: hugh.kang <hugh.kang@lge.com>
---
 drivers/regulator/of_regulator.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index ee5e67b..4178dbd 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -52,7 +52,13 @@ static void of_get_regulation_constraints(struct device_node *np,
 		constraints->valid_ops_mask |= REGULATOR_CHANGE_CURRENT;
 
 	constraints->boot_on = of_property_read_bool(np, "regulator-boot-on");
+	if (of_property_read_bool(np, "regulator-disable-boot-on"))
+		constraints->boot_on = false;
+
 	constraints->always_on = of_property_read_bool(np, "regulator-always-on");
+	if (of_property_read_bool(np, "regulator-disable-always-on"))
+		constraints->always_on = false;
+
 	if (!constraints->always_on) /* status change should be possible. */
 		constraints->valid_ops_mask |= REGULATOR_CHANGE_STATUS;
 
-- 
1.8.3.2

Re: [PATCH] regulator: adding disable options for regulator-always-on and regulator-boots-on

[Markus Pargmann]

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

Hi,

On Mon, Nov 03, 2014 at 10:26:43AM +0900, Hugh Kang wrote:
> From: "hugh.kang" <hugh.kang@lge.com>
> 
> If a regulator is set by always-on option, the regulator will be set forever.
> For example, suppose LDO1 is set to always-on at RevA.dts with including of a.dtsi. After that
> RevB.dts may wants to include the same a.dtsi but override the LDO1 always-on option. However,
> currently there is no way to delete the always-on option, even when we change the LDO1 option value,
> the always-on setting is still remains.

Why don't you split your devicetrees according to the real hardware and
set the always-on property only for revA?

Regards,

Markus

> 
> Signed-off-by: hugh.kang <hugh.kang@lge.com>
> ---
>  drivers/regulator/of_regulator.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
> index ee5e67b..4178dbd 100644
> --- a/drivers/regulator/of_regulator.c
> +++ b/drivers/regulator/of_regulator.c
> @@ -52,7 +52,13 @@ static void of_get_regulation_constraints(struct device_node *np,
>  		constraints->valid_ops_mask |= REGULATOR_CHANGE_CURRENT;
>  
>  	constraints->boot_on = of_property_read_bool(np, "regulator-boot-on");
> +	if (of_property_read_bool(np, "regulator-disable-boot-on"))
> +		constraints->boot_on = false;
> +
>  	constraints->always_on = of_property_read_bool(np, "regulator-always-on");
> +	if (of_property_read_bool(np, "regulator-disable-always-on"))
> +		constraints->always_on = false;
> +
>  	if (!constraints->always_on) /* status change should be possible. */
>  		constraints->valid_ops_mask |= REGULATOR_CHANGE_STATUS;
>  
> -- 
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH] regulator: adding disable options for regulator-always-on and regulator-boots-on

[Mark Brown]

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

On Mon, Nov 03, 2014 at 10:26:43AM +0900, Hugh Kang wrote:
> From: "hugh.kang" <hugh.kang@lge.com>
> 
> If a regulator is set by always-on option, the regulator will be set forever.
> For example, suppose LDO1 is set to always-on at RevA.dts with including of a.dtsi. After that
> RevB.dts may wants to include the same a.dtsi but override the LDO1 always-on option. However,
> currently there is no way to delete the always-on option, even when we change the LDO1 option value,
> the always-on setting is still remains.

This sounds like a problem with the way the DTSs have been written - I'd
expect the thing to do here is just to move the property to the rev A
DTS when rev B is created.  Why is that not the way forward?

Otherwise this is an issue which affects any boolean property in the DT
so if it's something we need to fix we should be fixing it in a generic
fashion that will work for other properties too.

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

Re: [PATCH] regulator: adding disable options for regulator-always-on and regulator-boots-on

[Hugh Kang]

Hello

On 2014년 11월 03일 21:03, Mark Brown wrote:
> On Mon, Nov 03, 2014 at 10:26:43AM +0900, Hugh Kang wrote:
>> From: "hugh.kang" <hugh.kang@lge.com>
>>
>> If a regulator is set by always-on option, the regulator will be set forever.
>> For example, suppose LDO1 is set to always-on at RevA.dts with including of a.dtsi. After that
>> RevB.dts may wants to include the same a.dtsi but override the LDO1 always-on option. However,
>> currently there is no way to delete the always-on option, even when we change the LDO1 option value,
>> the always-on setting is still remains.
> This sounds like a problem with the way the DTSs have been written - I'd
> expect the thing to do here is just to move the property to the rev A
> DTS when rev B is created.  Why is that not the way forward?
>
> Otherwise this is an issue which affects any boolean property in the DT
> so if it's something we need to fix we should be fixing it in a generic
> fashion that will work for other properties too.

I understand that I could make Rev.B with b.dtsi due to LDOs option is different. However, aim to use device tree is that making easy steps. Refer to dts option, if you set to be set status disabled option, the driver dose not probe when the system boot up. Even though, the system has different board revision exist. So dts have to be no corrupts any overlap situation.
I have mentions the simple example to change only one LDOs. However, if someone want to edit many regulators with similar configuration, he has to do lots of copy and paste works. Because he has to create new dts file. So I would suggest to make it as simple as I could.

Also, I have somehow agree with you that affects any boolean property in the DT.

Re: [PATCH] regulator: adding disable options for regulator-always-on and regulator-boots-on

[Krzysztof Kozłowski]

On 04.11.2014 09:20, Hugh Kang wrote:
> Hello
> 
> On 2014년 11월 03일 21:03, Mark Brown wrote:
>> On Mon, Nov 03, 2014 at 10:26:43AM +0900, Hugh Kang wrote:
>>> From: "hugh.kang" <hugh.kang@lge.com>
>>>
>>> If a regulator is set by always-on option, the regulator will be set forever.
>>> For example, suppose LDO1 is set to always-on at RevA.dts with including of a.dtsi. After that
>>> RevB.dts may wants to include the same a.dtsi but override the LDO1 always-on option. However,
>>> currently there is no way to delete the always-on option, even when we change the LDO1 option value,
>>> the always-on setting is still remains.
>> This sounds like a problem with the way the DTSs have been written - I'd
>> expect the thing to do here is just to move the property to the rev A
>> DTS when rev B is created.  Why is that not the way forward?
>>
>> Otherwise this is an issue which affects any boolean property in the DT
>> so if it's something we need to fix we should be fixing it in a generic
>> fashion that will work for other properties too.
> 
> I understand that I could make Rev.B with b.dtsi due to LDOs option is different. However, aim to use device tree is that making easy steps. Refer to dts option, if you set to be set status disabled option, the driver dose not probe when the system boot up. Even though, the system has different board revision exist. So dts have to be no corrupts any overlap situation.
> I have mentions the simple example to change only one LDOs. However, if someone want to edit many regulators with similar configuration, he has to do lots of copy and paste works. Because he has to create new dts file. So I would suggest to make it as simple as I could.
> 
> Also, I have somehow agree with you that affects any boolean property in the DT.

Now you propose to have a final DTS like:
$ scripts/dtc/dtc -I dtb -O dts
	reg {
		regulator-name = "vdd";
		regulator-min-microvolt = <850000>;
		regulator-max-microvolt = <1100000>;
		regulator-always-on;
		regulator-disable-always-on;
	};
Which one of "always-on" is more important? Later one could add
"regulator-enable-always-even-if-disable-always-on;" ...

The "status" property can be overridden. So maybe the new property
should behave the same way? Something like:
dts-common {
		regulator-status-on = "always-on";
		regulator-boot-on = "boot-on";
}
dst-rev-b {
		regulator-status-on = "default";
		regulator-boot-on = "default";
}

Best regards,
Krzysztof

Re: [PATCH] regulator: adding disable options for regulator-always-on and regulator-boots-on

[Mark Brown]

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

On Tue, Nov 04, 2014 at 05:20:11PM +0900, Hugh Kang wrote:

Please fix your mailer to word wrap within paragraphs.

> I understand that I could make Rev.B with b.dtsi due to LDOs option is
> different. However, aim to use device tree is that making easy steps.
> Refer to dts option, if you set to be set status disabled option, the
> driver dose not probe when the system boot up. Even though, the system
> has different board revision exist. So dts have to be no corrupts any
> overlap situation.

Right, nothing is going to be perfect but equally if we use this sort of
override property to do things it then becomes harder to read the DT for
the system since you can't trust that a property you see in one file
won't be overridden by another file somewhere else.  This can happen to
an extent already but normally not and it doesn't seem like a pattern we
want to encourage.

> I have mentions the simple example to change only one LDOs. However,
> if someone want to edit many regulators with similar configuration, he
> has to do lots of copy and paste works. Because he has to create new
> dts file. So I would suggest to make it as simple as I could.

Sure, but that's purely mechanical and very easy to review - you can do
a code motion patch to split the bits of DT out then start changing
things, and you can still keep the common paramters in the core file.

> Also, I have somehow agree with you that affects any boolean property in the DT.

This is a big one for me - it's going to be both time consuming and
complex to add this sort of thing for more properties and we do get
into complexity things if each individual property and its override
has to be handled by hand.

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

Re: [PATCH] regulator: adding disable options for regulator-always-on and regulator-boots-on

[Hugh Kang]

On 2014년 11월 05일 04:56, Mark Brown wrote:
> On Tue, Nov 04, 2014 at 05:20:11PM +0900, Hugh Kang wrote:
>
> Please fix your mailer to word wrap within paragraphs.
>
>> I understand that I could make Rev.B with b.dtsi due to LDOs option is
>> different. However, aim to use device tree is that making easy steps.
>> Refer to dts option, if you set to be set status disabled option, the
>> driver dose not probe when the system boot up. Even though, the system
>> has different board revision exist. So dts have to be no corrupts any
>> overlap situation.
> Right, nothing is going to be perfect but equally if we use this sort of
> override property to do things it then becomes harder to read the DT for
> the system since you can't trust that a property you see in one file
> won't be overridden by another file somewhere else.  This can happen to
> an extent already but normally not and it doesn't seem like a pattern we
> want to encourage.
>
>> I have mentions the simple example to change only one LDOs. However,
>> if someone want to edit many regulators with similar configuration, he
>> has to do lots of copy and paste works. Because he has to create new
>> dts file. So I would suggest to make it as simple as I could.
> Sure, but that's purely mechanical and very easy to review - you can do
> a code motion patch to split the bits of DT out then start changing
> things, and you can still keep the common paramters in the core file.
>
>> Also, I have somehow agree with you that affects any boolean property in the DT.
> This is a big one for me - it's going to be both time consuming and
> complex to add this sort of thing for more properties and we do get
> into complexity things if each individual property and its override
> has to be handled by hand.
Thank you for advising. I would take another way to apply it.
Also, I would like to say thanks to Krzysztof, giving me a advice.