| Message ID | 20171002120854.5212-1-quentin.schulz@free-electrons.com |
|---|---|
| Headers | show |
| Series | add pinmuxing support for pins in AXP209 and AXP813 PMICs | expand |
On Mon, Oct 02, 2017 at 12:08:43PM +0000, Quentin Schulz wrote: > To prepare the driver for the upcoming pinctrl features, move the GPIO > driver AXP209 from GPIO to pinctrl subsystem. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Thanks! Maxime
On Mon, Oct 02, 2017 at 12:08:46PM +0000, Quentin Schulz wrote: > To prepare for patches that will add support for a new PMIC that has a > different GPIO input status register, add a gpio_status_offset within > axp20x_pctl structure and use it. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Thanks! Maxime
On Mon, Oct 02, 2017 at 12:08:48PM +0000, Quentin Schulz wrote: > As GPIO/pinctrl driver now supports AXP813, add a cell for it. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Thanks! Maxime
On Mon, Oct 02, 2017 at 12:08:49PM +0000, Quentin Schulz wrote: > This board has an AXP813 PMIC so let's include its dtsi. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > --- > arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts > index 723641f56a74..4f4db07ca19f 100644 > --- a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts > +++ b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts > @@ -146,7 +146,7 @@ > &r_rsb { > status = "okay"; > > - axp813: pmic@3a3 { > + axp81x: pmic@3a3 { Aren't we going to have an axp813 DTSI? What's your plan here Maxime
On Mon, Oct 02, 2017 at 12:08:54PM +0000, Quentin Schulz wrote: > On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively) > ldo_io0 and ldo_io1. > > Let's add the pinctrl properties to the said regulators. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > --- > arch/arm/boot/dts/axp81x.dtsi | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/arch/arm/boot/dts/axp81x.dtsi b/arch/arm/boot/dts/axp81x.dtsi > index f90f257130d5..099b0ddc1bbb 100644 > --- a/arch/arm/boot/dts/axp81x.dtsi > +++ b/arch/arm/boot/dts/axp81x.dtsi > @@ -52,6 +52,16 @@ > compatible = "x-powers,axp813-gpio"; > gpio-controller; > #gpio-cells = <2>; > + > + gpio0_ldo: gpio0_ldo { > + pins = "GPIO0"; > + function = "ldo"; > + }; > + > + gpio1_ldo: gpio1_ldo { > + pins = "GPIO1"; > + function = "ldo"; > + }; The node names are not supposed to contain any hyphens. Thanks! Maxime
On Tue, Oct 3, 2017 at 4:42 AM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Mon, Oct 02, 2017 at 12:08:54PM +0000, Quentin Schulz wrote: >> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively) >> ldo_io0 and ldo_io1. >> >> Let's add the pinctrl properties to the said regulators. >> >> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> >> --- >> arch/arm/boot/dts/axp81x.dtsi | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/arch/arm/boot/dts/axp81x.dtsi b/arch/arm/boot/dts/axp81x.dtsi >> index f90f257130d5..099b0ddc1bbb 100644 >> --- a/arch/arm/boot/dts/axp81x.dtsi >> +++ b/arch/arm/boot/dts/axp81x.dtsi >> @@ -52,6 +52,16 @@ >> compatible = "x-powers,axp813-gpio"; >> gpio-controller; >> #gpio-cells = <2>; >> + >> + gpio0_ldo: gpio0_ldo { >> + pins = "GPIO0"; >> + function = "ldo"; >> + }; >> + >> + gpio1_ldo: gpio1_ldo { >> + pins = "GPIO1"; >> + function = "ldo"; >> + }; > > The node names are not supposed to contain any hyphens. Hmm, I was under the impression that hyphens were preferred in node names, and a warning would be added to dtc later on. I might be wrong though. ChenYu -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 3, 2017 at 4:18 AM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Mon, Oct 02, 2017 at 12:08:43PM +0000, Quentin Schulz wrote: >> To prepare the driver for the upcoming pinctrl features, move the GPIO >> driver AXP209 from GPIO to pinctrl subsystem. >> >> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > > Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Acked-by: Chen-Yu Tsai <wens@csie.org> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 3, 2017 at 4:38 AM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Mon, Oct 02, 2017 at 12:08:46PM +0000, Quentin Schulz wrote: >> To prepare for patches that will add support for a new PMIC that has a >> different GPIO input status register, add a gpio_status_offset within >> axp20x_pctl structure and use it. >> >> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > > Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Acked-by: Chen-Yu Tsai <wens@csie.org> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 03, 2017 at 10:06:29AM +0800, Chen-Yu Tsai wrote: > On Tue, Oct 3, 2017 at 4:42 AM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > On Mon, Oct 02, 2017 at 12:08:54PM +0000, Quentin Schulz wrote: > >> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively) > >> ldo_io0 and ldo_io1. > >> > >> Let's add the pinctrl properties to the said regulators. > >> > >> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > >> --- > >> arch/arm/boot/dts/axp81x.dtsi | 14 ++++++++++++++ > >> 1 file changed, 14 insertions(+) > >> > >> diff --git a/arch/arm/boot/dts/axp81x.dtsi b/arch/arm/boot/dts/axp81x.dtsi > >> index f90f257130d5..099b0ddc1bbb 100644 > >> --- a/arch/arm/boot/dts/axp81x.dtsi > >> +++ b/arch/arm/boot/dts/axp81x.dtsi > >> @@ -52,6 +52,16 @@ > >> compatible = "x-powers,axp813-gpio"; > >> gpio-controller; > >> #gpio-cells = <2>; > >> + > >> + gpio0_ldo: gpio0_ldo { > >> + pins = "GPIO0"; > >> + function = "ldo"; > >> + }; > >> + > >> + gpio1_ldo: gpio1_ldo { > >> + pins = "GPIO1"; > >> + function = "ldo"; > >> + }; > > > > The node names are not supposed to contain any hyphens. > > Hmm, I was under the impression that hyphens were preferred in > node names, and a warning would be added to dtc later on. > I might be wrong though. I think there's a terminology issue here. "-" is a hyphen or minus sign. "_" is an underscore. Underscores are not supposed to be used for node names, instead hyphens are preferred. I think Maxime means "underscore". Here's the list from dtc/checks.c: #define LOWERCASE "abcdefghijklmnopqrstuvwxyz" #define UPPERCASE "ABCDEFGHIJKLMNOPQRSTUVWXYZ" #define DIGITS "0123456789" #define PROPNODECHARS LOWERCASE UPPERCASE DIGITS ",._+*#?-" #define PROPNODECHARSSTRICT LOWERCASE UPPERCASE DIGITS ",-" If strict mode is enabled, use of any of "._+#?" in the node name will produce a warning.
On Mon, Oct 2, 2017 at 2:08 PM, Quentin Schulz <quentin.schulz@free-electrons.com> wrote: > On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively) > ldo_io0 and ldo_io1. (...) > + gpio0_ldo: gpio0_ldo { > + pins = "GPIO0"; > + function = "ldo"; > + }; (...) > + pinctrl-names = "default"; > + pinctrl-0 = <&gpio0_ldo>; > /* Disable by default to avoid conflicts with GPIO */ > status = "disabled"; So this is still by default disabled, but you make the default mode something called "ldo". And I think that is to be understood as a low-dropout regulator? So is the idea that this should be represented as a regulator in the end? Then I think the state name should not be "default" rather something like "regulator" and "default" should be the GPIO mode, as I guess something like that exists. Activating a regulator using pin control "default" mode is not very pretty. It would probably be unintuitive and end up wasting power because people will get confused about what is going on. Instead, call this state "regulator" and when using, in Linux create a regulator device that set the pin into "regulator" state to start using it as a LDO, and "default" to deactivate it as LDO, if that is how the usage is intended. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 03, 2017 at 09:18:37AM +0000, Russell King - ARM Linux wrote: > On Tue, Oct 03, 2017 at 10:06:29AM +0800, Chen-Yu Tsai wrote: > > On Tue, Oct 3, 2017 at 4:42 AM, Maxime Ripard > > <maxime.ripard@free-electrons.com> wrote: > > > On Mon, Oct 02, 2017 at 12:08:54PM +0000, Quentin Schulz wrote: > > >> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively) > > >> ldo_io0 and ldo_io1. > > >> > > >> Let's add the pinctrl properties to the said regulators. > > >> > > >> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > > >> --- > > >> arch/arm/boot/dts/axp81x.dtsi | 14 ++++++++++++++ > > >> 1 file changed, 14 insertions(+) > > >> > > >> diff --git a/arch/arm/boot/dts/axp81x.dtsi b/arch/arm/boot/dts/axp81x.dtsi > > >> index f90f257130d5..099b0ddc1bbb 100644 > > >> --- a/arch/arm/boot/dts/axp81x.dtsi > > >> +++ b/arch/arm/boot/dts/axp81x.dtsi > > >> @@ -52,6 +52,16 @@ > > >> compatible = "x-powers,axp813-gpio"; > > >> gpio-controller; > > >> #gpio-cells = <2>; > > >> + > > >> + gpio0_ldo: gpio0_ldo { > > >> + pins = "GPIO0"; > > >> + function = "ldo"; > > >> + }; > > >> + > > >> + gpio1_ldo: gpio1_ldo { > > >> + pins = "GPIO1"; > > >> + function = "ldo"; > > >> + }; > > > > > > The node names are not supposed to contain any hyphens. > > > > Hmm, I was under the impression that hyphens were preferred in > > node names, and a warning would be added to dtc later on. > > I might be wrong though. > > I think there's a terminology issue here. > > "-" is a hyphen or minus sign. > "_" is an underscore. > > Underscores are not supposed to be used for node names, instead hyphens > are preferred. I think Maxime means "underscore". > > Here's the list from dtc/checks.c: > > #define LOWERCASE "abcdefghijklmnopqrstuvwxyz" > #define UPPERCASE "ABCDEFGHIJKLMNOPQRSTUVWXYZ" > #define DIGITS "0123456789" > #define PROPNODECHARS LOWERCASE UPPERCASE DIGITS ",._+*#?-" > #define PROPNODECHARSSTRICT LOWERCASE UPPERCASE DIGITS ",-" > > If strict mode is enabled, use of any of "._+#?" in the node name will > produce a warning. Right, sorry, I meant underscores.. Maxime
Hi Linus, On Tue, Oct 03, 2017 at 09:27:17AM +0000, Linus Walleij wrote: > On Mon, Oct 2, 2017 at 2:08 PM, Quentin Schulz > <quentin.schulz@free-electrons.com> wrote: > > > On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively) > > ldo_io0 and ldo_io1. > (...) > > + gpio0_ldo: gpio0_ldo { > > + pins = "GPIO0"; > > + function = "ldo"; > > + }; > (...) > > + pinctrl-names = "default"; > > + pinctrl-0 = <&gpio0_ldo>; > > /* Disable by default to avoid conflicts with GPIO */ > > status = "disabled"; > > So this is still by default disabled, but you make the default > mode something called "ldo". > > And I think that is to be understood as a low-dropout regulator? > > So is the idea that this should be represented as a regulator > in the end? > > Then I think the state name should not be "default" rather > something like "regulator" and "default" should be the GPIO > mode, as I guess something like that exists. > > Activating a regulator using pin control "default" mode is > not very pretty. It would probably be unintuitive and end > up wasting power because people will get confused about > what is going on. That's not really it. The PMIC has pins that can be muxed either to (regular) GPIOs, an ADC or to an LDO regulator. This is just muxing, the regulator will be enabled and disabled separately through another register. If it wasn't the case, it would indeed be very messy. Maxime
On Tue, Oct 3, 2017 at 10:47 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Hi Linus, > > On Tue, Oct 03, 2017 at 09:27:17AM +0000, Linus Walleij wrote: >> On Mon, Oct 2, 2017 at 2:08 PM, Quentin Schulz >> <quentin.schulz@free-electrons.com> wrote: >> >> > On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively) >> > ldo_io0 and ldo_io1. >> (...) >> > + gpio0_ldo: gpio0_ldo { >> > + pins = "GPIO0"; >> > + function = "ldo"; >> > + }; >> (...) >> > + pinctrl-names = "default"; >> > + pinctrl-0 = <&gpio0_ldo>; >> > /* Disable by default to avoid conflicts with GPIO */ >> > status = "disabled"; >> >> So this is still by default disabled, but you make the default >> mode something called "ldo". >> >> And I think that is to be understood as a low-dropout regulator? >> >> So is the idea that this should be represented as a regulator >> in the end? >> >> Then I think the state name should not be "default" rather >> something like "regulator" and "default" should be the GPIO >> mode, as I guess something like that exists. >> >> Activating a regulator using pin control "default" mode is >> not very pretty. It would probably be unintuitive and end >> up wasting power because people will get confused about >> what is going on. > > That's not really it. The PMIC has pins that can be muxed either to > (regular) GPIOs, an ADC or to an LDO regulator. > > This is just muxing, the regulator will be enabled and disabled > separately through another register. If it wasn't the case, it would > indeed be very messy. No. Actually they are controlled in the same register, so it is very messy. The muxing options are: - 0: drive low - 1: drive high - 2: input with interrupt triggering - 3: LDO on - 4: LDO off - 5~7: floating (or ADC) ChenYu -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Chen-Yu, Linus, On 03/10/2017 17:08, Chen-Yu Tsai wrote: > On Tue, Oct 3, 2017 at 10:47 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: >> Hi Linus, >> >> On Tue, Oct 03, 2017 at 09:27:17AM +0000, Linus Walleij wrote: >>> On Mon, Oct 2, 2017 at 2:08 PM, Quentin Schulz >>> <quentin.schulz@free-electrons.com> wrote: >>> >>>> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively) >>>> ldo_io0 and ldo_io1. >>> (...) >>>> + gpio0_ldo: gpio0_ldo { >>>> + pins = "GPIO0"; >>>> + function = "ldo"; >>>> + }; >>> (...) >>>> + pinctrl-names = "default"; >>>> + pinctrl-0 = <&gpio0_ldo>; >>>> /* Disable by default to avoid conflicts with GPIO */ >>>> status = "disabled"; >>> >>> So this is still by default disabled, but you make the default >>> mode something called "ldo". >>> >>> And I think that is to be understood as a low-dropout regulator? >>> >>> So is the idea that this should be represented as a regulator >>> in the end? >>> >>> Then I think the state name should not be "default" rather >>> something like "regulator" and "default" should be the GPIO >>> mode, as I guess something like that exists. >>> >>> Activating a regulator using pin control "default" mode is >>> not very pretty. It would probably be unintuitive and end >>> up wasting power because people will get confused about >>> what is going on. >> >> That's not really it. The PMIC has pins that can be muxed either to >> (regular) GPIOs, an ADC or to an LDO regulator. >> >> This is just muxing, the regulator will be enabled and disabled >> separately through another register. If it wasn't the case, it would >> indeed be very messy. > > No. Actually they are controlled in the same register, so it is > very messy. The muxing options are: > > - 0: drive low > - 1: drive high > - 2: input with interrupt triggering > - 3: LDO on > - 4: LDO off > - 5~7: floating (or ADC) > Just to be a little more precise, - 0: drive low - 1: drive high - 2: input with interrupt triggering - 3: LDO on - 4: LDO off - 5~7: floating (or ADC) for AXP813, and - 0: drive low - 1: drive high - 2: input with interrupt triggering - 3: LDO on - 4: ADC - 5~7: floating for AXP209. So I think what you suggested Linus is not really relevant here as the regulator framework will take care of disabling the regulator when needed (for AXP813 via the ldo_off "muxing" selected by the regulator framework). However, there is no LDO off bit for AXP209 and the LDO can't be set to 0V in any other register. What's done now in the regulator driver for AXP209 is to select the floating "muxing" for the pin when wanting to disable the regulator. So I guess that's a way to handle it. Should we do it another way? Thanks for raising the issue, I frankly haven't thought about that at all. I have to send a v4 to update the support for AXP813 (basically setting ADC muxing to 0x5 instead of 0x4, for AXP813) as I misread the muxing register description when adding support for it. Thanks, Quentin
On Mon, Oct 2, 2017 at 2:08 PM, Quentin Schulz <quentin.schulz@free-electrons.com> wrote: > The AXP209 and AXP813 PMICs have several pins (respectively 3 and 2) that can > be used either as GPIOs or for other purposes (ADC or LDO here). > > We already have a GPIO driver for the GPIO use of those pins on the AXP209. > Let's "upgrade" this driver to support all the functions these pins can have. > > Then we add support to this driver for the AXP813 which is slighlty different > (basically a different offset in a register and one less pin). > > I suggest patches 1 to 6 go through Linus's tree and 7 to 12 via Maxime or > Chen-Yu's tree. > > This version of the patchset is based on Chen-Yu's patchset for AXP813/818 > regulators[1]. > > v3: This is starting to look really good, but I see there are still comments. Keep the series going, we merge once Maxime and Chen-Yu are happy. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 4, 2017 at 3:35 PM, Quentin Schulz <quentin.schulz@free-electrons.com> wrote: > Hi Chen-Yu, Linus, > > On 03/10/2017 17:08, Chen-Yu Tsai wrote: >> On Tue, Oct 3, 2017 at 10:47 PM, Maxime Ripard >> <maxime.ripard@free-electrons.com> wrote: >>> Hi Linus, >>> >>> On Tue, Oct 03, 2017 at 09:27:17AM +0000, Linus Walleij wrote: >>>> On Mon, Oct 2, 2017 at 2:08 PM, Quentin Schulz >>>> <quentin.schulz@free-electrons.com> wrote: >>>> >>>>> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively) >>>>> ldo_io0 and ldo_io1. >>>> (...) >>>>> + gpio0_ldo: gpio0_ldo { >>>>> + pins = "GPIO0"; >>>>> + function = "ldo"; >>>>> + }; >>>> (...) >>>>> + pinctrl-names = "default"; >>>>> + pinctrl-0 = <&gpio0_ldo>; >>>>> /* Disable by default to avoid conflicts with GPIO */ >>>>> status = "disabled"; >>>> >>>> So this is still by default disabled, but you make the default >>>> mode something called "ldo". >>>> >>>> And I think that is to be understood as a low-dropout regulator? >>>> >>>> So is the idea that this should be represented as a regulator >>>> in the end? >>>> >>>> Then I think the state name should not be "default" rather >>>> something like "regulator" and "default" should be the GPIO >>>> mode, as I guess something like that exists. >>>> >>>> Activating a regulator using pin control "default" mode is >>>> not very pretty. It would probably be unintuitive and end >>>> up wasting power because people will get confused about >>>> what is going on. >>> >>> That's not really it. The PMIC has pins that can be muxed either to >>> (regular) GPIOs, an ADC or to an LDO regulator. >>> >>> This is just muxing, the regulator will be enabled and disabled >>> separately through another register. If it wasn't the case, it would >>> indeed be very messy. >> >> No. Actually they are controlled in the same register, so it is >> very messy. The muxing options are: >> >> - 0: drive low >> - 1: drive high >> - 2: input with interrupt triggering >> - 3: LDO on >> - 4: LDO off >> - 5~7: floating (or ADC) >> > > Just to be a little more precise, > - 0: drive low > - 1: drive high > - 2: input with interrupt triggering > - 3: LDO on > - 4: LDO off > - 5~7: floating (or ADC) > > for AXP813, and > - 0: drive low > - 1: drive high > - 2: input with interrupt triggering > - 3: LDO on > - 4: ADC > - 5~7: floating > > for AXP209. > > So I think what you suggested Linus is not really relevant here as the > regulator framework will take care of disabling the regulator when > needed (for AXP813 via the ldo_off "muxing" selected by the regulator > framework). Linus is suggesting that we use (switching between) pinctrl states to control the regulator, as opposed to overriding the register value directly. That would be nice, as both subsystems would have the same idea of what's actually happening in the hardware. As Linus mentioned, having the LDO on or off as the default pinctrl state is not pretty. It also means as soon as the device is brought up, the regulator state gets overridden. That would not work well for regulators that have/want the "always-on" or "boot-on" properties. > However, there is no LDO off bit for AXP209 and the LDO can't be set to > 0V in any other register. What's done now in the regulator driver for > AXP209 is to select the floating "muxing" for the pin when wanting to > disable the regulator. So I guess that's a way to handle it. Should we > do it another way? I can't think of one. :) You could have an actual pinmux option for "floating". If there's no "LDO off" state, the regulator driver could use the "floating" state instead. ChenYu > Thanks for raising the issue, I frankly haven't thought about that at all. > > I have to send a v4 to update the support for AXP813 (basically setting > ADC muxing to 0x5 instead of 0x4, for AXP813) as I misread the muxing > register description when adding support for it. > > Thanks, > Quentin > -- > Quentin Schulz, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 4, 2017 at 9:35 AM, Quentin Schulz <quentin.schulz@free-electrons.com> wrote: > Just to be a little more precise, > - 0: drive low > - 1: drive high > - 2: input with interrupt triggering > - 3: LDO on > - 4: LDO off > - 5~7: floating (or ADC) > > for AXP813, and > - 0: drive low > - 1: drive high > - 2: input with interrupt triggering > - 3: LDO on > - 4: ADC > - 5~7: floating Fair enough, it's mux modes that the pin supports, no big surprises. > So I think what you suggested Linus is not really relevant here as the > regulator framework will take care of disabling the regulator when > needed (for AXP813 via the ldo_off "muxing" selected by the regulator > framework). I think I see why I got confused. The point is that your mode for setting it to "LDO on" should have the pin control state connected to the relevant device. It should be connected to the regulator and nothing else, so if there is a fixed regulator or whatever in the device tree, it should have pinctrl-0 and pinctrl-names = ".."; here is is for some obscure reason connected to the GPIO controller (!) instead, and the actual consumer of this state is NOT the GPIO controller, but quite obviously the regulator, so put the pinctrl business in that regulator node instead. "default" mode is OK on a regulator, as that can be expected to make the pin precisely a regulator pin. Forget my ramblings about a "regulator" state. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 10, 2017 at 03:09:11AM +0000, Chen-Yu Tsai wrote: > On Wed, Oct 4, 2017 at 3:35 PM, Quentin Schulz > <quentin.schulz@free-electrons.com> wrote: > > Hi Chen-Yu, Linus, > > > > On 03/10/2017 17:08, Chen-Yu Tsai wrote: > >> On Tue, Oct 3, 2017 at 10:47 PM, Maxime Ripard > >> <maxime.ripard@free-electrons.com> wrote: > >>> Hi Linus, > >>> > >>> On Tue, Oct 03, 2017 at 09:27:17AM +0000, Linus Walleij wrote: > >>>> On Mon, Oct 2, 2017 at 2:08 PM, Quentin Schulz > >>>> <quentin.schulz@free-electrons.com> wrote: > >>>> > >>>>> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively) > >>>>> ldo_io0 and ldo_io1. > >>>> (...) > >>>>> + gpio0_ldo: gpio0_ldo { > >>>>> + pins = "GPIO0"; > >>>>> + function = "ldo"; > >>>>> + }; > >>>> (...) > >>>>> + pinctrl-names = "default"; > >>>>> + pinctrl-0 = <&gpio0_ldo>; > >>>>> /* Disable by default to avoid conflicts with GPIO */ > >>>>> status = "disabled"; > >>>> > >>>> So this is still by default disabled, but you make the default > >>>> mode something called "ldo". > >>>> > >>>> And I think that is to be understood as a low-dropout regulator? > >>>> > >>>> So is the idea that this should be represented as a regulator > >>>> in the end? > >>>> > >>>> Then I think the state name should not be "default" rather > >>>> something like "regulator" and "default" should be the GPIO > >>>> mode, as I guess something like that exists. > >>>> > >>>> Activating a regulator using pin control "default" mode is > >>>> not very pretty. It would probably be unintuitive and end > >>>> up wasting power because people will get confused about > >>>> what is going on. > >>> > >>> That's not really it. The PMIC has pins that can be muxed either to > >>> (regular) GPIOs, an ADC or to an LDO regulator. > >>> > >>> This is just muxing, the regulator will be enabled and disabled > >>> separately through another register. If it wasn't the case, it would > >>> indeed be very messy. > >> > >> No. Actually they are controlled in the same register, so it is > >> very messy. The muxing options are: > >> > >> - 0: drive low > >> - 1: drive high > >> - 2: input with interrupt triggering > >> - 3: LDO on > >> - 4: LDO off > >> - 5~7: floating (or ADC) > >> > > > > Just to be a little more precise, > > - 0: drive low > > - 1: drive high > > - 2: input with interrupt triggering > > - 3: LDO on > > - 4: LDO off > > - 5~7: floating (or ADC) > > > > for AXP813, and > > - 0: drive low > > - 1: drive high > > - 2: input with interrupt triggering > > - 3: LDO on > > - 4: ADC > > - 5~7: floating > > > > for AXP209. > > > > So I think what you suggested Linus is not really relevant here as the > > regulator framework will take care of disabling the regulator when > > needed (for AXP813 via the ldo_off "muxing" selected by the regulator > > framework). > > Linus is suggesting that we use (switching between) pinctrl states to > control the regulator, as opposed to overriding the register value > directly. That would be nice, as both subsystems would have the same > idea of what's actually happening in the hardware. > > As Linus mentioned, having the LDO on or off as the default pinctrl state > is not pretty. It also means as soon as the device is brought up, the > regulator state gets overridden. That would not work well for regulators > that have/want the "always-on" or "boot-on" properties. What about not enforcing any muxing state when we want to mux to the "ldo" function? We just leave it to whatever value it is, that way we keep it under the regulator framework's control, and we don't disrupt anything when the pin is requested. Maxime
On Wed, Oct 11, 2017 at 2:00 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > What about not enforcing any muxing state when we want to mux to the > "ldo" function? We just leave it to whatever value it is, that way we > keep it under the regulator framework's control, and we don't disrupt > anything when the pin is requested. In a way since setting the bits one way means "LDO on" and another setting means "LDO off" those bits should be handled by the regulator framework when used as a regulator, not pin control. So I would say yes. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 12, 2017 at 3:09 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Oct 11, 2017 at 2:00 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > >> What about not enforcing any muxing state when we want to mux to the >> "ldo" function? We just leave it to whatever value it is, that way we >> keep it under the regulator framework's control, and we don't disrupt >> anything when the pin is requested. > > In a way since setting the bits one way means "LDO on" and another > setting means "LDO off" those bits should be handled by the > regulator framework when used as a regulator, not pin control. > > So I would say yes. I agree. That would be the best solution. ChenYu -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html