Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
On pią, 2014-10-31 at 10:32 +, Mark Brown wrote: On Fri, Oct 31, 2014 at 08:51:38AM +0100, Krzysztof Kozlowski wrote: However new DT style parsing (regulator_of_get_init_data()) does the basic parsing stuff and this removes a lot of code from driver. The driver no longer parses all regulaotrs but the regulator core does it. Unfortunately regulator core does not parse custom bindings (like such GPIOs) so I would have to iterate once again through all regulators just to find gpios property. We could always add a callback for the driver to handle any custom properties... one of the advantages of an OS like Linux is that we can improve the core code. I thought about this - adding a callback, called on each child in regulator_of_get_init_data(). However the reason behind this callback is to parse GPIO and set config.ena_gpio. However in that context the regulator_config is const so the callback cannot change it. Unless it casts it to non-const... which isn't what we want I think. So now I wonder whether adding generic bindings for ena_gpio make sense. These would look like bindings for fixed-regulator (with ena- prefix). Unfortunately this would duplicate a little the ena_gpio in regulator_config... but to me it seems more appropriate. What do you think about adding generic bindings for ena_gpio? Best regards, Krzysztof -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
On Mon, Nov 03, 2014 at 01:07:02PM +0100, Krzysztof Kozlowski wrote: On pią, 2014-10-31 at 10:32 +, Mark Brown wrote: We could always add a callback for the driver to handle any custom properties... one of the advantages of an OS like Linux is that we can improve the core code. I thought about this - adding a callback, called on each child in regulator_of_get_init_data(). However the reason behind this callback is to parse GPIO and set config.ena_gpio. However in that context the regulator_config is const so the callback cannot change it. Unless it casts it to non-const... which isn't what we want I think. So now I wonder whether adding generic bindings for ena_gpio make sense. These would look like bindings for fixed-regulator (with ena- prefix). Unfortunately this would duplicate a little the ena_gpio in regulator_config... but to me it seems more appropriate. What do you think about adding generic bindings for ena_gpio? Well, if you only want this for enable GPIO control (sorry, I'm really not reading a lot of these long threads when it looks like there'll be a resubmit anyway) we can always add a way for drivers to specify the name of a property to look at easily enough. signature.asc Description: Digital signature
Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
On pią, 2014-10-31 at 12:31 +0900, Alexandre Courbot wrote:
On Fri, Oct 31, 2014 at 12:03 AM, Krzysztof Kozlowski
k.kozlow...@samsung.com wrote:
On czw, 2014-10-30 at 22:56 +0900, Alexandre Courbot wrote:
Hi, and thanks for bringing this issue to us!
On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
javier.marti...@collabora.co.uk wrote:
[adding Linus and Alexandre to the cc list]
Hello Krzysztof,
On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
Hello Krzysztof,
On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
@@ -85,6 +91,9 @@ struct max77686_data {
struct max77686_regulator_data *regulators;
int num_regulators;
+ /* Array of size num_regulators with GPIOs for external
control. */
+ int *ext_control_gpio;
+
The integer-based GPIO API is deprecated in favor of the
descriptor-based GPIO
interface (Documentation/gpio/consumer.txt). Could you please use
the later?
Sure, I can. Please have in mind that regulator core still accepts
old
GPIO so I will have to use desc_to_gpio(). That should work... and
should be future-ready.
It seems I was too hasty... I think usage of the new gpiod API implies
completely different bindings.
The gpiod_get() gets GPIO from a device level, not from given sub-node
pointer. This means that you cannot have DTS like this:
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpy2 0 0;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpk0 2 0;
};
I could put GPIOs in device node:
max77686_pmic@09 {
compatible = maxim,max77686;
interrupt-parent = gpx0;
interrupts = 7 0;
reg = 0x09;
#clock-cells = 1;
ldo21-gpio = gpy2 0 0;
ldo22-gpio = gpk0 2 0;
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
This would work but I don't like it. The properties of a regulator are
above the node configuring that regulator.
Any ideas?
Continuing talking to myself... I found another problem - GPIO cannot be
requested more than once (-EBUSY). In case of this driver (and board:
Trats2) one GPIO is connected to regulators. The legacy GPIO API and
regulator core handle this.
With new GPIO API I would have to implement some additional steps in
such case...
So there are 2 issues:
1. Cannot put GPIO property in regulator node.
For this problem you will probably want to use the
dev(m)_get_named_gpiod_from_child() function from the following patch:
https://lkml.org/lkml/2014/10/6/529
It should reach -next soon now.
Thanks! Probably I would switch to top level gpios property anyway
(other reasons) but it would be valuable in some cases to specify them
per child node.
Mmm, but doesn't it make more sense to have them in the child nodes?
Yes, it makes more sense. Using old way of parsing regulators from DT it
was straightforward.
However new DT style parsing (regulator_of_get_init_data()) does the
basic parsing stuff and this removes a lot of code from driver. The
driver no longer parses all regulaotrs but the regulator core does it.
Unfortunately regulator core does not parse custom bindings (like such
GPIOs) so I would have to iterate once again through all regulators just
to find gpios property.
It is simpler just to do something like (5 regulators can be controlled
by GPIO):
max77686_pmic_dt_parse_gpio_control(struct platform_device *pdev, *gpio)
{
gpio[MAX77686_LDO20] = of_get_named_gpio(np, ldo20-gpios, 0);
gpio[MAX77686_LDO21] = of_get_named_gpio(np, ldo21-gpios, 0);
gpio[MAX77686_LDO22] = of_get_named_gpio(np, ldo22-gpios, 0);
gpio[MAX77686_BUCK8] = of_get_named_gpio(np, buck8-gpios, 0);
gpio[MAX77686_BUCK9] = of_get_named_gpio(np, buck9-gpios, 0);
}
Besides if the bindings of this driver have already been published,
I'm afraid you will have to maintain backward compability.
These are new bindings. Driver exists but I am adding new
Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
On Fri, Oct 31, 2014 at 08:51:38AM +0100, Krzysztof Kozlowski wrote: However new DT style parsing (regulator_of_get_init_data()) does the basic parsing stuff and this removes a lot of code from driver. The driver no longer parses all regulaotrs but the regulator core does it. Unfortunately regulator core does not parse custom bindings (like such GPIOs) so I would have to iterate once again through all regulators just to find gpios property. We could always add a callback for the driver to handle any custom properties... one of the advantages of an OS like Linux is that we can improve the core code. signature.asc Description: Digital signature
Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
On pią, 2014-10-31 at 10:32 +, Mark Brown wrote: On Fri, Oct 31, 2014 at 08:51:38AM +0100, Krzysztof Kozlowski wrote: However new DT style parsing (regulator_of_get_init_data()) does the basic parsing stuff and this removes a lot of code from driver. The driver no longer parses all regulaotrs but the regulator core does it. Unfortunately regulator core does not parse custom bindings (like such GPIOs) so I would have to iterate once again through all regulators just to find gpios property. We could always add a callback for the driver to handle any custom properties... one of the advantages of an OS like Linux is that we can improve the core code. Then I'll add it. Mark, what device should be assigned to config.dev during registration of regulators? The regulator driver's device or its parent (MFD main driver)? Various drivers do this differently. Best regards, Krzysztof -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
On Fri, Oct 31, 2014 at 12:45:47PM +0100, Krzysztof Kozlowski wrote: Then I'll add it. Mark, what device should be assigned to config.dev during registration of regulators? The regulator driver's device or its parent (MFD main driver)? Various drivers do this differently. Normally the parent device. signature.asc Description: Digital signature
Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
On Fri, Oct 31, 2014 at 4:51 PM, Krzysztof Kozlowski
k.kozlow...@samsung.com wrote:
On pią, 2014-10-31 at 12:31 +0900, Alexandre Courbot wrote:
On Fri, Oct 31, 2014 at 12:03 AM, Krzysztof Kozlowski
k.kozlow...@samsung.com wrote:
On czw, 2014-10-30 at 22:56 +0900, Alexandre Courbot wrote:
Hi, and thanks for bringing this issue to us!
On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
javier.marti...@collabora.co.uk wrote:
[adding Linus and Alexandre to the cc list]
Hello Krzysztof,
On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
Hello Krzysztof,
On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
@@ -85,6 +91,9 @@ struct max77686_data {
struct max77686_regulator_data *regulators;
int num_regulators;
+ /* Array of size num_regulators with GPIOs for external
control. */
+ int *ext_control_gpio;
+
The integer-based GPIO API is deprecated in favor of the
descriptor-based GPIO
interface (Documentation/gpio/consumer.txt). Could you please use
the later?
Sure, I can. Please have in mind that regulator core still accepts
old
GPIO so I will have to use desc_to_gpio(). That should work... and
should be future-ready.
It seems I was too hasty... I think usage of the new gpiod API implies
completely different bindings.
The gpiod_get() gets GPIO from a device level, not from given sub-node
pointer. This means that you cannot have DTS like this:
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpy2 0 0;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpk0 2 0;
};
I could put GPIOs in device node:
max77686_pmic@09 {
compatible = maxim,max77686;
interrupt-parent = gpx0;
interrupts = 7 0;
reg = 0x09;
#clock-cells = 1;
ldo21-gpio = gpy2 0 0;
ldo22-gpio = gpk0 2 0;
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
This would work but I don't like it. The properties of a regulator are
above the node configuring that regulator.
Any ideas?
Continuing talking to myself... I found another problem - GPIO cannot
be
requested more than once (-EBUSY). In case of this driver (and board:
Trats2) one GPIO is connected to regulators. The legacy GPIO API and
regulator core handle this.
With new GPIO API I would have to implement some additional steps in
such case...
So there are 2 issues:
1. Cannot put GPIO property in regulator node.
For this problem you will probably want to use the
dev(m)_get_named_gpiod_from_child() function from the following patch:
https://lkml.org/lkml/2014/10/6/529
It should reach -next soon now.
Thanks! Probably I would switch to top level gpios property anyway
(other reasons) but it would be valuable in some cases to specify them
per child node.
Mmm, but doesn't it make more sense to have them in the child nodes?
Yes, it makes more sense. Using old way of parsing regulators from DT it
was straightforward.
However new DT style parsing (regulator_of_get_init_data()) does the
basic parsing stuff and this removes a lot of code from driver. The
driver no longer parses all regulaotrs but the regulator core does it.
Unfortunately regulator core does not parse custom bindings (like such
GPIOs) so I would have to iterate once again through all regulators just
to find gpios property.
It is simpler just to do something like (5 regulators can be controlled
by GPIO):
max77686_pmic_dt_parse_gpio_control(struct platform_device *pdev, *gpio)
{
gpio[MAX77686_LDO20] = of_get_named_gpio(np, ldo20-gpios, 0);
gpio[MAX77686_LDO21] = of_get_named_gpio(np, ldo21-gpios, 0);
gpio[MAX77686_LDO22] = of_get_named_gpio(np, ldo22-gpios, 0);
gpio[MAX77686_BUCK8] = of_get_named_gpio(np, buck8-gpios, 0);
gpio[MAX77686_BUCK9] = of_get_named_gpio(np, buck9-gpios, 0);
}
It is simpler from the driver's perspective, but if I understand
correctly DT bindings are not
Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
Hi, and thanks for bringing this issue to us!
On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
javier.marti...@collabora.co.uk wrote:
[adding Linus and Alexandre to the cc list]
Hello Krzysztof,
On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
Hello Krzysztof,
On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
@@ -85,6 +91,9 @@ struct max77686_data {
struct max77686_regulator_data *regulators;
int num_regulators;
+ /* Array of size num_regulators with GPIOs for external
control. */
+ int *ext_control_gpio;
+
The integer-based GPIO API is deprecated in favor of the
descriptor-based GPIO
interface (Documentation/gpio/consumer.txt). Could you please use the
later?
Sure, I can. Please have in mind that regulator core still accepts old
GPIO so I will have to use desc_to_gpio(). That should work... and
should be future-ready.
It seems I was too hasty... I think usage of the new gpiod API implies
completely different bindings.
The gpiod_get() gets GPIO from a device level, not from given sub-node
pointer. This means that you cannot have DTS like this:
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpy2 0 0;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpk0 2 0;
};
I could put GPIOs in device node:
max77686_pmic@09 {
compatible = maxim,max77686;
interrupt-parent = gpx0;
interrupts = 7 0;
reg = 0x09;
#clock-cells = 1;
ldo21-gpio = gpy2 0 0;
ldo22-gpio = gpk0 2 0;
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
This would work but I don't like it. The properties of a regulator are
above the node configuring that regulator.
Any ideas?
Continuing talking to myself... I found another problem - GPIO cannot be
requested more than once (-EBUSY). In case of this driver (and board:
Trats2) one GPIO is connected to regulators. The legacy GPIO API and
regulator core handle this.
With new GPIO API I would have to implement some additional steps in
such case...
So there are 2 issues:
1. Cannot put GPIO property in regulator node.
For this problem you will probably want to use the
dev(m)_get_named_gpiod_from_child() function from the following patch:
https://lkml.org/lkml/2014/10/6/529
It should reach -next soon now.
2. Cannot request some GPIO more than once.
We have been confronted to this problem with the regulator core as well:
http://marc.info/?l=linux-arm-kernelm=140417649119733w=1
I have a draft patch that allows GPIOs to be requested by several
clients. What prevented me from submitting it was that I wanted to
make sure the different requested configurations were compatible, but
maybe I am overthinking this. There are also a couple of other patches
that this depends on (like removal of the big descs array), so I don't
think it will be available too soon, sadly.
So maybe your best shot for now is to keep using the integer API, as
much as I hate it. Once we become able to request the same GPIO
several times, you should be good to switch to descriptors. Sorry this
has not been done faster.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
On czw, 2014-10-30 at 22:56 +0900, Alexandre Courbot wrote:
Hi, and thanks for bringing this issue to us!
On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
javier.marti...@collabora.co.uk wrote:
[adding Linus and Alexandre to the cc list]
Hello Krzysztof,
On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
Hello Krzysztof,
On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
@@ -85,6 +91,9 @@ struct max77686_data {
struct max77686_regulator_data *regulators;
int num_regulators;
+ /* Array of size num_regulators with GPIOs for external
control. */
+ int *ext_control_gpio;
+
The integer-based GPIO API is deprecated in favor of the
descriptor-based GPIO
interface (Documentation/gpio/consumer.txt). Could you please use the
later?
Sure, I can. Please have in mind that regulator core still accepts old
GPIO so I will have to use desc_to_gpio(). That should work... and
should be future-ready.
It seems I was too hasty... I think usage of the new gpiod API implies
completely different bindings.
The gpiod_get() gets GPIO from a device level, not from given sub-node
pointer. This means that you cannot have DTS like this:
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpy2 0 0;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpk0 2 0;
};
I could put GPIOs in device node:
max77686_pmic@09 {
compatible = maxim,max77686;
interrupt-parent = gpx0;
interrupts = 7 0;
reg = 0x09;
#clock-cells = 1;
ldo21-gpio = gpy2 0 0;
ldo22-gpio = gpk0 2 0;
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
This would work but I don't like it. The properties of a regulator are
above the node configuring that regulator.
Any ideas?
Continuing talking to myself... I found another problem - GPIO cannot be
requested more than once (-EBUSY). In case of this driver (and board:
Trats2) one GPIO is connected to regulators. The legacy GPIO API and
regulator core handle this.
With new GPIO API I would have to implement some additional steps in
such case...
So there are 2 issues:
1. Cannot put GPIO property in regulator node.
For this problem you will probably want to use the
dev(m)_get_named_gpiod_from_child() function from the following patch:
https://lkml.org/lkml/2014/10/6/529
It should reach -next soon now.
Thanks! Probably I would switch to top level gpios property anyway
(other reasons) but it would be valuable in some cases to specify them
per child node.
2. Cannot request some GPIO more than once.
We have been confronted to this problem with the regulator core as well:
http://marc.info/?l=linux-arm-kernelm=140417649119733w=1
I have a draft patch that allows GPIOs to be requested by several
clients. What prevented me from submitting it was that I wanted to
make sure the different requested configurations were compatible, but
maybe I am overthinking this. There are also a couple of other patches
that this depends on (like removal of the big descs array), so I don't
think it will be available too soon, sadly.
Shouldn't be the nature of get()/put() interface to allow multiple
requests? To me it was a kind of intuitive that I could do another
devm_gpiod_get() for the same gpio. But then it hit me with EBUSY :).
So maybe your best shot for now is to keep using the integer API, as
much as I hate it. Once we become able to request the same GPIO
several times, you should be good to switch to descriptors. Sorry this
has not been done faster.
I'll do it legacy way but I'll try to use bindings gpiolib-safe. This
way future transition in the driver should not affect bindings.
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
On Fri, Oct 31, 2014 at 12:03 AM, Krzysztof Kozlowski
k.kozlow...@samsung.com wrote:
On czw, 2014-10-30 at 22:56 +0900, Alexandre Courbot wrote:
Hi, and thanks for bringing this issue to us!
On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
javier.marti...@collabora.co.uk wrote:
[adding Linus and Alexandre to the cc list]
Hello Krzysztof,
On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
Hello Krzysztof,
On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
@@ -85,6 +91,9 @@ struct max77686_data {
struct max77686_regulator_data *regulators;
int num_regulators;
+ /* Array of size num_regulators with GPIOs for external
control. */
+ int *ext_control_gpio;
+
The integer-based GPIO API is deprecated in favor of the
descriptor-based GPIO
interface (Documentation/gpio/consumer.txt). Could you please use
the later?
Sure, I can. Please have in mind that regulator core still accepts old
GPIO so I will have to use desc_to_gpio(). That should work... and
should be future-ready.
It seems I was too hasty... I think usage of the new gpiod API implies
completely different bindings.
The gpiod_get() gets GPIO from a device level, not from given sub-node
pointer. This means that you cannot have DTS like this:
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpy2 0 0;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpk0 2 0;
};
I could put GPIOs in device node:
max77686_pmic@09 {
compatible = maxim,max77686;
interrupt-parent = gpx0;
interrupts = 7 0;
reg = 0x09;
#clock-cells = 1;
ldo21-gpio = gpy2 0 0;
ldo22-gpio = gpk0 2 0;
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
This would work but I don't like it. The properties of a regulator are
above the node configuring that regulator.
Any ideas?
Continuing talking to myself... I found another problem - GPIO cannot be
requested more than once (-EBUSY). In case of this driver (and board:
Trats2) one GPIO is connected to regulators. The legacy GPIO API and
regulator core handle this.
With new GPIO API I would have to implement some additional steps in
such case...
So there are 2 issues:
1. Cannot put GPIO property in regulator node.
For this problem you will probably want to use the
dev(m)_get_named_gpiod_from_child() function from the following patch:
https://lkml.org/lkml/2014/10/6/529
It should reach -next soon now.
Thanks! Probably I would switch to top level gpios property anyway
(other reasons) but it would be valuable in some cases to specify them
per child node.
Mmm, but doesn't it make more sense to have them in the child nodes?
Besides if the bindings of this driver have already been published,
I'm afraid you will have to maintain backward compability.
2. Cannot request some GPIO more than once.
We have been confronted to this problem with the regulator core as well:
http://marc.info/?l=linux-arm-kernelm=140417649119733w=1
I have a draft patch that allows GPIOs to be requested by several
clients. What prevented me from submitting it was that I wanted to
make sure the different requested configurations were compatible, but
maybe I am overthinking this. There are also a couple of other patches
that this depends on (like removal of the big descs array), so I don't
think it will be available too soon, sadly.
Shouldn't be the nature of get()/put() interface to allow multiple
requests?
Only if it makes sense for the resource to be requested multiple
times. GPIOs are kind of a difficult case here. Consumers could ask
for e.g. conflicting directions. But for cases where it should work I
agree that multiple consumers would be welcome.
To me it was a kind of intuitive that I could do another
devm_gpiod_get() for the same gpio. But then it hit me with EBUSY :).
So maybe your best shot for now is to keep using the integer API, as
much as I hate it. Once we become able to request the same GPIO
several times, you should
Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
Hello Krzysztof,
On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
@@ -85,6 +91,9 @@ struct max77686_data {
struct max77686_regulator_data *regulators;
int num_regulators;
+ /* Array of size num_regulators with GPIOs for external
control. */
+ int *ext_control_gpio;
+
The integer-based GPIO API is deprecated in favor of the descriptor-based
GPIO
interface (Documentation/gpio/consumer.txt). Could you please use the
later?
Sure, I can. Please have in mind that regulator core still accepts old
GPIO so I will have to use desc_to_gpio(). That should work... and
should be future-ready.
It seems I was too hasty... I think usage of the new gpiod API implies
completely different bindings.
The gpiod_get() gets GPIO from a device level, not from given sub-node
pointer. This means that you cannot have DTS like this:
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpy2 0 0;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpk0 2 0;
};
I could put GPIOs in device node:
max77686_pmic@09 {
compatible = maxim,max77686;
interrupt-parent = gpx0;
interrupts = 7 0;
reg = 0x09;
#clock-cells = 1;
ldo21-gpio = gpy2 0 0;
ldo22-gpio = gpk0 2 0;
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
This would work but I don't like it. The properties of a regulator are
above the node configuring that regulator.
Any ideas?
Continuing talking to myself... I found another problem - GPIO cannot be
requested more than once (-EBUSY). In case of this driver (and board:
Trats2) one GPIO is connected to regulators. The legacy GPIO API and
regulator core handle this.
With new GPIO API I would have to implement some additional steps in
such case...
So there are 2 issues:
1. Cannot put GPIO property in regulator node.
2. Cannot request some GPIO more than once.
I'm going back to legacy API for now.
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
[adding Linus and Alexandre to the cc list]
Hello Krzysztof,
On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
Hello Krzysztof,
On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
@@ -85,6 +91,9 @@ struct max77686_data {
struct max77686_regulator_data *regulators;
int num_regulators;
+ /* Array of size num_regulators with GPIOs for external
control. */
+ int *ext_control_gpio;
+
The integer-based GPIO API is deprecated in favor of the
descriptor-based GPIO
interface (Documentation/gpio/consumer.txt). Could you please use the
later?
Sure, I can. Please have in mind that regulator core still accepts old
GPIO so I will have to use desc_to_gpio(). That should work... and
should be future-ready.
It seems I was too hasty... I think usage of the new gpiod API implies
completely different bindings.
The gpiod_get() gets GPIO from a device level, not from given sub-node
pointer. This means that you cannot have DTS like this:
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpy2 0 0;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpk0 2 0;
};
I could put GPIOs in device node:
max77686_pmic@09 {
compatible = maxim,max77686;
interrupt-parent = gpx0;
interrupts = 7 0;
reg = 0x09;
#clock-cells = 1;
ldo21-gpio = gpy2 0 0;
ldo22-gpio = gpk0 2 0;
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
This would work but I don't like it. The properties of a regulator are
above the node configuring that regulator.
Any ideas?
Continuing talking to myself... I found another problem - GPIO cannot be
requested more than once (-EBUSY). In case of this driver (and board:
Trats2) one GPIO is connected to regulators. The legacy GPIO API and
regulator core handle this.
With new GPIO API I would have to implement some additional steps in
such case...
So there are 2 issues:
1. Cannot put GPIO property in regulator node.
2. Cannot request some GPIO more than once.
I'm going back to legacy API for now.
I've added the GPIO maintainers as cc so hopefully they can comment on this.
Best regards,
Krzysztof
Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
Hello Krzysztof,
On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
@@ -85,6 +91,9 @@ struct max77686_data {
struct max77686_regulator_data *regulators;
int num_regulators;
+ /* Array of size num_regulators with GPIOs for external control. */
+ int *ext_control_gpio;
+
The integer-based GPIO API is deprecated in favor of the descriptor-based GPIO
interface (Documentation/gpio/consumer.txt). Could you please use the later?
Sure, I can. Please have in mind that regulator core still accepts old
GPIO so I will have to use desc_to_gpio(). That should work... and
should be future-ready.
Thanks for feedback,
Krzysztof
Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
Hello Krzysztof,
On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
@@ -85,6 +91,9 @@ struct max77686_data {
struct max77686_regulator_data *regulators;
int num_regulators;
+ /* Array of size num_regulators with GPIOs for external control. */
+ int *ext_control_gpio;
+
The integer-based GPIO API is deprecated in favor of the descriptor-based
GPIO
interface (Documentation/gpio/consumer.txt). Could you please use the later?
Sure, I can. Please have in mind that regulator core still accepts old
GPIO so I will have to use desc_to_gpio(). That should work... and
should be future-ready.
It seems I was too hasty... I think usage of the new gpiod API implies
completely different bindings.
The gpiod_get() gets GPIO from a device level, not from given sub-node
pointer. This means that you cannot have DTS like this:
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpy2 0 0;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
ec-gpio = gpk0 2 0;
};
I could put GPIOs in device node:
max77686_pmic@09 {
compatible = maxim,max77686;
interrupt-parent = gpx0;
interrupts = 7 0;
reg = 0x09;
#clock-cells = 1;
ldo21-gpio = gpy2 0 0;
ldo22-gpio = gpk0 2 0;
ldo21_reg: ldo21 {
regulator-compatible = LDO21;
regulator-name = VTF_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
ldo22_reg: ldo22 {
regulator-compatible = LDO22;
regulator-name = VMEM_VDD_2.8V;
regulator-min-microvolt = 280;
regulator-max-microvolt = 280;
};
This would work but I don't like it. The properties of a regulator are
above the node configuring that regulator.
Any ideas?
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/8] regulator: max77686: Add external GPIO control
Add control over GPIO for regulators supporting this: LDO20, LDO21,
LDO22, buck8 and buck9.
This is needed for proper (and full) configuration of the Maxim 77686
PMIC without creating redundant 'regulator-fixed' entries.
Signed-off-by: Krzysztof Kozlowski k.kozlow...@samsung.com
---
drivers/regulator/max77686.c | 101 ---
1 file changed, 96 insertions(+), 5 deletions(-)
diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index e5738c363f07..c54a8603f5b2 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -26,6 +26,7 @@
#include linux/bug.h
#include linux/err.h
#include linux/gpio.h
+#include linux/of_gpio.h
#include linux/slab.h
#include linux/platform_device.h
#include linux/regulator/driver.h
@@ -46,6 +47,11 @@
#define MAX77686_DVS_UVSTEP12500
/*
+ * Value for configuring buck[89] and LDO{20,21,22} as external control.
+ * It is the same as 'off' for other regulators.
+ */
+#define MAX77686_EXT_CONTROL 0x0
+/*
* Values used for configuring LDOs and bucks.
* Forcing low power mode: LDO1, 3-5, 9, 13, 17-26
*/
@@ -85,6 +91,9 @@ struct max77686_data {
struct max77686_regulator_data *regulators;
int num_regulators;
+ /* Array of size num_regulators with GPIOs for external control. */
+ int *ext_control_gpio;
+
unsigned int opmode[MAX77686_REGULATORS];
};
@@ -102,6 +111,28 @@ static unsigned int max77686_get_opmode_shift(int id)
}
}
+/*
+ * When regulator is configured for external control then it
+ * replaces normal mode. Any change from low power mode to normal
+ * should actually change to external control.
+ * Map normal mode to proper value for such regulators.
+ */
+static int max77686_map_normal_mode(struct max77686_data *max77686, int id)
+{
+ switch (id) {
+ case MAX77686_BUCK8:
+ case MAX77686_BUCK9:
+ if (gpio_is_valid(max77686-ext_control_gpio[id]))
+ return MAX77686_EXT_CONTROL;
+
+ case MAX77686_LDO20 ... MAX77686_LDO22:
+ if (gpio_is_valid(max77686-ext_control_gpio[id]))
+ return MAX77686_EXT_CONTROL;
+ }
+
+ return MAX77686_NORMAL;
+}
+
/* Some BUCKs and LDOs supports Normal[ON/OFF] mode during suspend */
static int max77686_set_suspend_disable(struct regulator_dev *rdev)
{
@@ -138,7 +169,7 @@ static int max77686_set_suspend_mode(struct regulator_dev
*rdev,
val = MAX77686_LDO_LOWPOWER_PWRREQ;
break;
case REGULATOR_MODE_NORMAL: /* ON in Normal Mode */
- val = MAX77686_NORMAL;
+ val = max77686_map_normal_mode(max77686, id);
break;
default:
pr_warn(%s: regulator_suspend_mode : 0x%x not supported\n,
@@ -162,7 +193,7 @@ static int max77686_ldo_set_suspend_mode(struct
regulator_dev *rdev,
{
unsigned int val;
struct max77686_data *max77686 = rdev_get_drvdata(rdev);
- int ret;
+ int ret, id = rdev_get_id(rdev);
switch (mode) {
case REGULATOR_MODE_STANDBY:/* switch off */
@@ -172,7 +203,7 @@ static int max77686_ldo_set_suspend_mode(struct
regulator_dev *rdev,
val = MAX77686_LDO_LOWPOWER_PWRREQ;
break;
case REGULATOR_MODE_NORMAL: /* ON in Normal Mode */
- val = MAX77686_NORMAL;
+ val = max77686_map_normal_mode(max77686, id);
break;
default:
pr_warn(%s: regulator_suspend_mode : 0x%x not supported\n,
@@ -186,7 +217,7 @@ static int max77686_ldo_set_suspend_mode(struct
regulator_dev *rdev,
if (ret)
return ret;
- max77686-opmode[rdev_get_id(rdev)] = val;
+ max77686-opmode[id] = val;
return 0;
}
@@ -199,7 +230,7 @@ static int max77686_enable(struct regulator_dev *rdev)
shift = max77686_get_opmode_shift(id);
if (max77686-opmode[id] == MAX77686_OFF_PWRREQ)
- max77686-opmode[id] = MAX77686_NORMAL;
+ max77686-opmode[id] = max77686_map_normal_mode(max77686, id);
return regmap_update_bits(rdev-regmap, rdev-desc-enable_reg,
rdev-desc-enable_mask,
@@ -433,6 +464,42 @@ static const struct regulator_desc regulators[] = {
regulator_desc_buck(9),
};
+static int max77686_enable_ext_control(struct regulator_dev *rdev)
+{
+ return regmap_update_bits(rdev-regmap, rdev-desc-enable_reg,
+ rdev-desc-enable_mask,
+ MAX77686_EXT_CONTROL);
+}
+
+static void max77686_pmic_dt_parse_ext_control_gpio(struct platform_device
*pdev,
+ struct max77686_regulator_data *rdata,
+ struct max77686_data *max77686)
+{
+ int *gpio = max77686-ext_control_gpio;
+ unsigned int i;
+
Re: [PATCH 6/8] regulator: max77686: Add external GPIO control
Hello Krzysztof,
On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
@@ -85,6 +91,9 @@ struct max77686_data {
struct max77686_regulator_data *regulators;
int num_regulators;
+ /* Array of size num_regulators with GPIOs for external control. */
+ int *ext_control_gpio;
+
The integer-based GPIO API is deprecated in favor of the descriptor-based GPIO
interface (Documentation/gpio/consumer.txt). Could you please use the later?
Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
