| Message ID | 1414422226-10948-7-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 = <2800000>; regulator-max-microvolt = <2800000>; ec-gpio = <&gpy2 0 0>; }; ldo22_reg: ldo22 { regulator-compatible = "LDO22"; regulator-name = "VMEM_VDD_2.8V"; regulator-min-microvolt = <2800000>; regulator-max-microvolt = <2800000>; 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 = <2800000>; regulator-max-microvolt = <2800000>; }; ldo22_reg: ldo22 { regulator-compatible = "LDO22"; regulator-name = "VMEM_VDD_2.8V"; regulator-min-microvolt = <2800000>; regulator-max-microvolt = <2800000>; }; 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 = <2800000>; > regulator-max-microvolt = <2800000>; > ec-gpio = <&gpy2 0 0>; > }; > > ldo22_reg: ldo22 { > regulator-compatible = "LDO22"; > regulator-name = "VMEM_VDD_2.8V"; > regulator-min-microvolt = <2800000>; > regulator-max-microvolt = <2800000>; > 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 = <2800000>; > regulator-max-microvolt = <2800000>; > }; > > ldo22_reg: ldo22 { > regulator-compatible = "LDO22"; > regulator-name = "VMEM_VDD_2.8V"; > regulator-min-microvolt = <2800000>; > regulator-max-microvolt = <2800000>; > }; > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[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 = <2800000>; >> regulator-max-microvolt = <2800000>; >> ec-gpio = <&gpy2 0 0>; >> }; >> >> ldo22_reg: ldo22 { >> regulator-compatible = "LDO22"; >> regulator-name = "VMEM_VDD_2.8V"; >> regulator-min-microvolt = <2800000>; >> regulator-max-microvolt = <2800000>; >> 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 = <2800000>; >> regulator-max-microvolt = <2800000>; >> }; >> >> ldo22_reg: ldo22 { >> regulator-compatible = "LDO22"; >> regulator-name = "VMEM_VDD_2.8V"; >> regulator-min-microvolt = <2800000>; >> regulator-max-microvolt = <2800000>; >> }; >> >> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, and thanks for bringing this issue to us! On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas <javier.martinez@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 = <2800000>; >>> regulator-max-microvolt = <2800000>; >>> ec-gpio = <&gpy2 0 0>; >>> }; >>> >>> ldo22_reg: ldo22 { >>> regulator-compatible = "LDO22"; >>> regulator-name = "VMEM_VDD_2.8V"; >>> regulator-min-microvolt = <2800000>; >>> regulator-max-microvolt = <2800000>; >>> 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 = <2800000>; >>> regulator-max-microvolt = <2800000>; >>> }; >>> >>> ldo22_reg: ldo22 { >>> regulator-compatible = "LDO22"; >>> regulator-name = "VMEM_VDD_2.8V"; >>> regulator-min-microvolt = <2800000>; >>> regulator-max-microvolt = <2800000>; >>> }; >>> >>> 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-kernel&m=140417649119733&w=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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.martinez@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 = <2800000>; > >>> regulator-max-microvolt = <2800000>; > >>> ec-gpio = <&gpy2 0 0>; > >>> }; > >>> > >>> ldo22_reg: ldo22 { > >>> regulator-compatible = "LDO22"; > >>> regulator-name = "VMEM_VDD_2.8V"; > >>> regulator-min-microvolt = <2800000>; > >>> regulator-max-microvolt = <2800000>; > >>> 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 = <2800000>; > >>> regulator-max-microvolt = <2800000>; > >>> }; > >>> > >>> ldo22_reg: ldo22 { > >>> regulator-compatible = "LDO22"; > >>> regulator-name = "VMEM_VDD_2.8V"; > >>> regulator-min-microvolt = <2800000>; > >>> regulator-max-microvolt = <2800000>; > >>> }; > >>> > >>> 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-kernel&m=140417649119733&w=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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 31, 2014 at 12:03 AM, Krzysztof Kozlowski <k.kozlowski@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.martinez@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 = <2800000>; >> >>> regulator-max-microvolt = <2800000>; >> >>> ec-gpio = <&gpy2 0 0>; >> >>> }; >> >>> >> >>> ldo22_reg: ldo22 { >> >>> regulator-compatible = "LDO22"; >> >>> regulator-name = "VMEM_VDD_2.8V"; >> >>> regulator-min-microvolt = <2800000>; >> >>> regulator-max-microvolt = <2800000>; >> >>> 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 = <2800000>; >> >>> regulator-max-microvolt = <2800000>; >> >>> }; >> >>> >> >>> ldo22_reg: ldo22 { >> >>> regulator-compatible = "LDO22"; >> >>> regulator-name = "VMEM_VDD_2.8V"; >> >>> regulator-min-microvolt = <2800000>; >> >>> regulator-max-microvolt = <2800000>; >> >>> }; >> >>> >> >>> 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-kernel&m=140417649119733&w=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 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. For DT bindings, please refer to these brand-new instructions: https://lkml.org/lkml/2014/10/29/98 Personally I think having the GPIO phandle in the child node would be the most intuitive. You will also have to use the "-gpios" suffix, no "-gpio", if you can still change the bindings. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On pi?, 2014-10-31 at 12:31 +0900, Alexandre Courbot wrote: > On Fri, Oct 31, 2014 at 12:03 AM, Krzysztof Kozlowski > <k.kozlowski@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.martinez@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 = <2800000>; > >> >>> regulator-max-microvolt = <2800000>; > >> >>> ec-gpio = <&gpy2 0 0>; > >> >>> }; > >> >>> > >> >>> ldo22_reg: ldo22 { > >> >>> regulator-compatible = "LDO22"; > >> >>> regulator-name = "VMEM_VDD_2.8V"; > >> >>> regulator-min-microvolt = <2800000>; > >> >>> regulator-max-microvolt = <2800000>; > >> >>> 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 = <2800000>; > >> >>> regulator-max-microvolt = <2800000>; > >> >>> }; > >> >>> > >> >>> ldo22_reg: ldo22 { > >> >>> regulator-compatible = "LDO22"; > >> >>> regulator-name = "VMEM_VDD_2.8V"; > >> >>> regulator-min-microvolt = <2800000>; > >> >>> regulator-max-microvolt = <2800000>; > >> >>> }; > >> >>> > >> >>> 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 functionality: the "GPIO enable control". > >> > >> >> 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-kernel&m=140417649119733&w=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. Right, I haven't thought about conflicts. > 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 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. > > For DT bindings, please refer to these brand-new instructions: > > https://lkml.org/lkml/2014/10/29/98 > > Personally I think having the GPIO phandle in the child node would > be the most intuitive. You will also have to use the "-gpios" suffix, no > "-gpio", if you can still change the bindings. Thanks! I'll adjust to new style. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 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.
On pi?, 2014-10-31 at 10:32 +0000, 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
On Fri, Oct 31, 2014 at 4:51 PM, Krzysztof Kozlowski <k.kozlowski@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.kozlowski@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.martinez@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 = <2800000>; >> >> >>> regulator-max-microvolt = <2800000>; >> >> >>> ec-gpio = <&gpy2 0 0>; >> >> >>> }; >> >> >>> >> >> >>> ldo22_reg: ldo22 { >> >> >>> regulator-compatible = "LDO22"; >> >> >>> regulator-name = "VMEM_VDD_2.8V"; >> >> >>> regulator-min-microvolt = <2800000>; >> >> >>> regulator-max-microvolt = <2800000>; >> >> >>> 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 = <2800000>; >> >> >>> regulator-max-microvolt = <2800000>; >> >> >>> }; >> >> >>> >> >> >>> ldo22_reg: ldo22 { >> >> >>> regulator-compatible = "LDO22"; >> >> >>> regulator-name = "VMEM_VDD_2.8V"; >> >> >>> regulator-min-microvolt = <2800000>; >> >> >>> regulator-max-microvolt = <2800000>; >> >> >>> }; >> >> >>> >> >> >>> 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 supposed to be adapted to make life easier for a particular implementation. If the driver needs to make an additional pass into the child nodes, then so be it, as long as the nodes describe the hardware accurately and in a way that is easy to understand. You can always adapt the driver core to handle your use-case better, but once DT bindings are published, they are set in stone. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On pi?, 2014-10-31 at 10:32 +0000, 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 03, 2014 at 01:07:02PM +0100, Krzysztof Kozlowski wrote: > On pi?, 2014-10-31 at 10:32 +0000, 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.
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_UVSTEP 12500 /* + * 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; + unsigned int valid_regulators[5] = { MAX77686_LDO20, MAX77686_LDO21, + MAX77686_LDO22, MAX77686_BUCK8, MAX77686_BUCK9 }; + + /* + * 0 is a valid GPIO so initialize all GPIOs to negative value + * to indicate that external control won't be used for this regulator. + */ + for (i = 0; i < max77686->num_regulators; i++) + max77686->ext_control_gpio[i] = -EINVAL; + + for (i = 0; i < ARRAY_SIZE(valid_regulators); i++) { + unsigned int reg = valid_regulators[i]; + + if (!rdata[reg].initdata || !rdata[reg].of_node) + continue; + + gpio[reg] = of_get_named_gpio(rdata[reg].of_node, "gpio", 0); + if (gpio_is_valid(gpio[reg])) + dev_dbg(&pdev->dev, "Using GPIO %d for ext-control over %d/%s\n", + gpio[reg], reg, rdata[reg].of_node->name); + } +} + static int max77686_pmic_dt_parse(struct platform_device *pdev, struct max77686_data *max77686) { @@ -457,6 +524,14 @@ static int max77686_pmic_dt_parse(struct platform_device *pdev, return -ENOMEM; } + max77686->ext_control_gpio = devm_kmalloc_array(&pdev->dev, + max77686->num_regulators, + sizeof(*max77686->ext_control_gpio), GFP_KERNEL); + if (!max77686->ext_control_gpio) { + of_node_put(regulators_np); + return -ENOMEM; + } + for (i = 0; i < max77686->num_regulators; i++) { rmatch.name = regulators[i].name; rmatch.init_data = NULL; @@ -466,7 +541,9 @@ static int max77686_pmic_dt_parse(struct platform_device *pdev, rdata[i].of_node = rmatch.of_node; } + max77686_pmic_dt_parse_ext_control_gpio(pdev, rdata, max77686); max77686->regulators = rdata; + of_node_put(regulators_np); return 0; @@ -499,6 +576,7 @@ static int max77686_pmic_probe(struct platform_device *pdev) config.dev = &pdev->dev; config.regmap = iodev->regmap; config.driver_data = max77686; + config.ena_gpio_flags = GPIOF_OUT_INIT_HIGH; platform_set_drvdata(pdev, max77686); for (i = 0; i < MAX77686_REGULATORS; i++) { @@ -506,6 +584,8 @@ static int max77686_pmic_probe(struct platform_device *pdev) config.init_data = max77686->regulators[i].initdata; config.of_node = max77686->regulators[i].of_node; + config.ena_gpio = max77686->ext_control_gpio[i]; + config.ena_gpio_initialized = true; max77686->opmode[i] = MAX77686_NORMAL; @@ -516,6 +596,17 @@ static int max77686_pmic_probe(struct platform_device *pdev) "regulator init failed for %d\n", i); return PTR_ERR(rdev); } + + if (gpio_is_valid(config.ena_gpio)) { + ret = max77686_enable_ext_control(rdev); + + if (ret < 0) { + dev_err(&pdev->dev, + "regulator enable ext control failed for %d\n", + i); + return ret; + } + } } return 0;
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.kozlowski@samsung.com> --- drivers/regulator/max77686.c | 101 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 96 insertions(+), 5 deletions(-)