[v3,3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
diff mbox series

Message ID 20191129172537.31410-4-m.felsch@pengutronix.de
State Changes Requested
Headers show
Series
  • DA9062 PMIC features
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Marco Felsch Nov. 29, 2019, 5:25 p.m. UTC
Add the documentation which describe the voltage selection gpio support.
This property can be applied to each subnode within the 'regulators'
node so each regulator can be configured differently.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Changelog:
v3:
- adapt binding description

 Documentation/devicetree/bindings/mfd/da9062.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Mark Brown Dec. 4, 2019, 1:46 p.m. UTC | #1
On Fri, Nov 29, 2019 at 06:25:34PM +0100, Marco Felsch wrote:

> +  Optional regulator device-specific properties:
> +  - dlg,vsel-sense-gpios : A GPIO reference to a local general purpose input,
> +    the datasheet calls it GPI. The regulator sense the input signal and select
> +    the active or suspend voltage settings. If the signal is active the
> +    active-settings are applied else the suspend-settings are applied.
> +    Attention: Sharing the same GPI for other purposes or across multiple
> +    regulators is possible but the polarity setting must equal.

I'm really confused by this.  As far as I understand it it seems
to be doing pinmuxing on the chip using the GPIO bindings which
is itself a bit odd and I don't see anything here that configures
whatever sets the state of the pins.  Don't we need another GPIO
to set the vsel-sense inputs on the PMIC?
Marco Felsch Dec. 10, 2019, 9:41 a.m. UTC | #2
Hi Mark,

On 19-12-04 13:46, Mark Brown wrote:
> On Fri, Nov 29, 2019 at 06:25:34PM +0100, Marco Felsch wrote:
> 
> > +  Optional regulator device-specific properties:
> > +  - dlg,vsel-sense-gpios : A GPIO reference to a local general purpose input,
> > +    the datasheet calls it GPI. The regulator sense the input signal and select
> > +    the active or suspend voltage settings. If the signal is active the
> > +    active-settings are applied else the suspend-settings are applied.
> > +    Attention: Sharing the same GPI for other purposes or across multiple
> > +    regulators is possible but the polarity setting must equal.
> 
> I'm really confused by this.  As far as I understand it it seems
> to be doing pinmuxing on the chip using the GPIO bindings which
> is itself a bit odd and I don't see anything here that configures
> whatever sets the state of the pins.  Don't we need another GPIO
> to set the vsel-sense inputs on the PMIC?

Yes the PMIC is very configurable and it took a while till I understand
it.. @Adam please correct me if I'm wrong.

The PMIC regulators regardless of the type: ldo or buck can be
simplified drawn as:



da9062-gpio               da9062-regulator
    
  +-------------------------------------------------------
  |                  PMIC
  |    
  > GPIO0            +--------------------------+
  |                  |         REGULATOR-0      |
  > GPIO1 -------+   |                          |
  |              +-- > vsel-in    voltage-a-out <
  > GPIO2        |   |                          |
  |              |   > enable-in  voltage-b-out <
  |              |   |                          |
  |              |   +--------------------------+
  |              |                              
  |              |   +--------------------------+                          
  |              |   |         REGULATOR-1      |                          
  |              |   |                          |                          
  |              +-- > vsel-in    voltage-a-out <                          
  |                  |                          |                          
  |                  > enable-in  voltage-b-out <
  |                  |                          |
  |                  +--------------------------+
  |

The 'vsel-in' and 'enable-in' regulator inputs must be routed to the
PMIC GPIOs which must be configured as input. If this is a pinmux in
your opinion, then yes we need to do that. IMHO it isn't a pinmux
because from the regulator point of view it is just a GPIO which comes
from our own gpio-dev (da9062-gpio). So the abstraction is vald. Anyway
I'm with you that this isn't the typical use-case.

Regards,
  Marco
Adam Thomson Dec. 11, 2019, 4:14 p.m. UTC | #3
On 10 December 2019 09:42, Marco Felsch wrote:

> Hi Mark,
> 
> On 19-12-04 13:46, Mark Brown wrote:
> > On Fri, Nov 29, 2019 at 06:25:34PM +0100, Marco Felsch wrote:
> >
> > > +  Optional regulator device-specific properties:
> > > +  - dlg,vsel-sense-gpios : A GPIO reference to a local general purpose input,
> > > +    the datasheet calls it GPI. The regulator sense the input signal and select
> > > +    the active or suspend voltage settings. If the signal is active the
> > > +    active-settings are applied else the suspend-settings are applied.
> > > +    Attention: Sharing the same GPI for other purposes or across multiple
> > > +    regulators is possible but the polarity setting must equal.
> >
> > I'm really confused by this.  As far as I understand it it seems
> > to be doing pinmuxing on the chip using the GPIO bindings which
> > is itself a bit odd and I don't see anything here that configures
> > whatever sets the state of the pins.  Don't we need another GPIO
> > to set the vsel-sense inputs on the PMIC?
> 
> Yes the PMIC is very configurable and it took a while till I understand
> it.. @Adam please correct me if I'm wrong.
> 
> The PMIC regulators regardless of the type: ldo or buck can be
> simplified drawn as:
> 
> 
> 
> da9062-gpio               da9062-regulator
> 
>   +-------------------------------------------------------
>   |                  PMIC
>   |
>   > GPIO0            +--------------------------+
>   |                  |         REGULATOR-0      |
>   > GPIO1 -------+   |                          |
>   |              +-- > vsel-in    voltage-a-out <
>   > GPIO2        |   |                          |
>   |              |   > enable-in  voltage-b-out <
>   |              |   |                          |
>   |              |   +--------------------------+
>   |              |
>   |              |   +--------------------------+
>   |              |   |         REGULATOR-1      |
>   |              |   |                          |
>   |              +-- > vsel-in    voltage-a-out <
>   |                  |                          |
>   |                  > enable-in  voltage-b-out <
>   |                  |                          |
>   |                  +--------------------------+
>   |
> 
> The 'vsel-in' and 'enable-in' regulator inputs must be routed to the
> PMIC GPIOs which must be configured as input. If this is a pinmux in
> your opinion, then yes we need to do that. IMHO it isn't a pinmux
> because from the regulator point of view it is just a GPIO which comes
> from our own gpio-dev (da9062-gpio). So the abstraction is vald. Anyway
> I'm with you that this isn't the typical use-case.

We've had this discussion before and to me it felt more like pinmux than GPIO
although I understand we're configuring the GPIO pin as input before then
configuring a regulator to take that specific internal GPIO as the control
signal. We're defining a specific role to this pin in HW rather than it being a
general software handled GPI so it feels like this would be neater under pinmux.
There does still need to be a mapping between that pin and the regulator which I
guess would be served by passing the pin to the regulator through generic pinmux
bindings and then in the regulator code you're simply just enabling the
regulator to be controlled from that pin. The HW lets you control multiple
regulators from the same input pin so there's a flexibility there to be
captured, as you mention.

> 
> Regards,
>   Marco
> 
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
Marco Felsch Dec. 11, 2019, 5:09 p.m. UTC | #4
Hi Adam,

On 19-12-11 16:14, Adam Thomson wrote:
> On 10 December 2019 09:42, Marco Felsch wrote:
> 
> > Hi Mark,
> > 
> > On 19-12-04 13:46, Mark Brown wrote:
> > > On Fri, Nov 29, 2019 at 06:25:34PM +0100, Marco Felsch wrote:
> > >
> > > > +  Optional regulator device-specific properties:
> > > > +  - dlg,vsel-sense-gpios : A GPIO reference to a local general purpose input,
> > > > +    the datasheet calls it GPI. The regulator sense the input signal and select
> > > > +    the active or suspend voltage settings. If the signal is active the
> > > > +    active-settings are applied else the suspend-settings are applied.
> > > > +    Attention: Sharing the same GPI for other purposes or across multiple
> > > > +    regulators is possible but the polarity setting must equal.
> > >
> > > I'm really confused by this.  As far as I understand it it seems
> > > to be doing pinmuxing on the chip using the GPIO bindings which
> > > is itself a bit odd and I don't see anything here that configures
> > > whatever sets the state of the pins.  Don't we need another GPIO
> > > to set the vsel-sense inputs on the PMIC?
> > 
> > Yes the PMIC is very configurable and it took a while till I understand
> > it.. @Adam please correct me if I'm wrong.
> > 
> > The PMIC regulators regardless of the type: ldo or buck can be
> > simplified drawn as:
> > 
> > 
> > 
> > da9062-gpio               da9062-regulator
> > 
> >   +-------------------------------------------------------
> >   |                  PMIC
> >   |
> >   > GPIO0            +--------------------------+
> >   |                  |         REGULATOR-0      |
> >   > GPIO1 -------+   |                          |
> >   |              +-- > vsel-in    voltage-a-out <
> >   > GPIO2        |   |                          |
> >   |              |   > enable-in  voltage-b-out <
> >   |              |   |                          |
> >   |              |   +--------------------------+
> >   |              |
> >   |              |   +--------------------------+
> >   |              |   |         REGULATOR-1      |
> >   |              |   |                          |
> >   |              +-- > vsel-in    voltage-a-out <
> >   |                  |                          |
> >   |                  > enable-in  voltage-b-out <
> >   |                  |                          |
> >   |                  +--------------------------+
> >   |
> > 
> > The 'vsel-in' and 'enable-in' regulator inputs must be routed to the
> > PMIC GPIOs which must be configured as input. If this is a pinmux in
> > your opinion, then yes we need to do that. IMHO it isn't a pinmux
> > because from the regulator point of view it is just a GPIO which comes
> > from our own gpio-dev (da9062-gpio). So the abstraction is vald. Anyway
> > I'm with you that this isn't the typical use-case.
> 
> We've had this discussion before and to me it felt more like pinmux than GPIO
> although I understand we're configuring the GPIO pin as input before then
> configuring a regulator to take that specific internal GPIO as the control
> signal. We're defining a specific role to this pin in HW rather than it being a
> general software handled GPI so it feels like this would be neater under pinmux.
> There does still need to be a mapping between that pin and the regulator which I
> guess would be served by passing the pin to the regulator through generic pinmux
> bindings and then in the regulator code you're simply just enabling the
> regulator to be controlled from that pin. The HW lets you control multiple
> regulators from the same input pin so there's a flexibility there to be
> captured, as you mention.

I know that we already had this discussion but the result was to wait
for the maintainers input. Since Linus is the pinctrl/gpio maintainer
and Mark the regulator maintainer we now have some input so we can move
forward. Linus made some comments on the dt-bindings and on the code but
he didn't pointed out that this usage is wrong. So I guessed it would be
fine for him. Mark did his first comments now and I explained the
current state..

I discussed it with a colleague again and he mentioned that pinctrl
should be named pinctrl instead it should be named padctrl. We don't
reconfigure the pad to a other function it is still a device general
purpose input pad. The hw-signal flow goes always trough the gpio block
so one argument more for my solution. Also we don't configure the "pad"
to be a vsel/ena-pin. The hw-pad can only be a gpio or has an alternate
function (WDKICK for GPIO0, Seq. SYS_EN for GPIO2, Seq. PWR_EN for GPIO4).
Instead we tell the regulator to use _this_ GPIO e.g. for voltage
selection so we go the other way around. My last argument why pinctrl
isn't the correct place is that the GPIO1 can be used for
regulator-0:vsel-in and for regulator-1:enable-in. So this pad would
have different states which is invalid IMHO.

Regards,
  Marco

> > Regards,
> >   Marco
> > 
> > --
> > Pengutronix e.K.                           |                             |
> > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>
Linus Walleij Dec. 12, 2019, 3:08 p.m. UTC | #5
On Wed, Dec 11, 2019 at 6:09 PM Marco Felsch <m.felsch@pengutronix.de> wrote:

> I discussed it with a colleague again and he mentioned that pinctrl
> should be named pinctrl instead it should be named padctrl.

Quoting Documentation/driver-api/pinctl.rst:

(...)
Definition of PIN:

- PINS are equal to pads, fingers, balls or whatever packaging input or
  output line you want to control and these are denoted by unsigned integers
  in the range 0..maxpin.
(...)

> We don't
> reconfigure the pad to a other function it is still a device general
> purpose input pad. The hw-signal flow goes always trough the gpio block
> so one argument more for my solution. Also we don't configure the "pad"
> to be a vsel/ena-pin. The hw-pad can only be a gpio or has an alternate
> function (WDKICK for GPIO0, Seq. SYS_EN for GPIO2, Seq. PWR_EN for GPIO4).
> Instead we tell the regulator to use _this_ GPIO e.g. for voltage
> selection so we go the other way around. My last argument why pinctrl
> isn't the correct place is that the GPIO1 can be used for
> regulator-0:vsel-in and for regulator-1:enable-in. So this pad would
> have different states which is invalid IMHO.

Yeah it is just one of these cases where the silicon designer pulled
a line of polysilicone over to the regulator enable signal and put a
switch on it and say "so you can also enable the regulator
with a signal from here", it can be used in parallel with anything
else, which is especially messy.

Special cases require special handling, since the electronic design
of this thing is a bit Rube Goldberg.

Yours,
Linus Walleij
Marco Felsch Dec. 12, 2019, 3:55 p.m. UTC | #6
Hi,

On 19-12-12 16:08, Linus Walleij wrote:
> On Wed, Dec 11, 2019 at 6:09 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
> 
> > I discussed it with a colleague again and he mentioned that pinctrl
> > should be named pinctrl instead it should be named padctrl.
> 
> Quoting Documentation/driver-api/pinctl.rst:
> 
> (...)
> Definition of PIN:
> 
> - PINS are equal to pads, fingers, balls or whatever packaging input or
>   output line you want to control and these are denoted by unsigned integers
>   in the range 0..maxpin.
> (...)

Okay there is the definition.

> > We don't
> > reconfigure the pad to a other function it is still a device general
> > purpose input pad. The hw-signal flow goes always trough the gpio block
> > so one argument more for my solution. Also we don't configure the "pad"
> > to be a vsel/ena-pin. The hw-pad can only be a gpio or has an alternate
> > function (WDKICK for GPIO0, Seq. SYS_EN for GPIO2, Seq. PWR_EN for GPIO4).
> > Instead we tell the regulator to use _this_ GPIO e.g. for voltage
> > selection so we go the other way around. My last argument why pinctrl
> > isn't the correct place is that the GPIO1 can be used for
> > regulator-0:vsel-in and for regulator-1:enable-in. So this pad would
> > have different states which is invalid IMHO.
> 
> Yeah it is just one of these cases where the silicon designer pulled
> a line of polysilicone over to the regulator enable signal and put a
> switch on it and say "so you can also enable the regulator
> with a signal from here", it can be used in parallel with anything
> else, which is especially messy.

I didn't say that the design isn't messy ;) I just wanna make the right
abstraction and IMHO this is the correct abstraction.

Regards,
  Marco

> Special cases require special handling, since the electronic design
> of this thing is a bit Rube Goldberg.
> 
> Yours,
> Linus Walleij
>
Mark Brown Dec. 12, 2019, 4:10 p.m. UTC | #7
On Wed, Dec 11, 2019 at 06:09:18PM +0100, Marco Felsch wrote:

> so one argument more for my solution. Also we don't configure the "pad"
> to be a vsel/ena-pin. The hw-pad can only be a gpio or has an alternate
> function (WDKICK for GPIO0, Seq. SYS_EN for GPIO2, Seq. PWR_EN for GPIO4).
> Instead we tell the regulator to use _this_ GPIO e.g. for voltage
> selection so we go the other way around. My last argument why pinctrl
> isn't the correct place is that the GPIO1 can be used for
> regulator-0:vsel-in and for regulator-1:enable-in. So this pad would
> have different states which is invalid IMHO.

Note that there's two bits to my concern - one is if we should be using
gpiolib or pinctrl, the other is what's driving the input (whichever API
it's configured through) which didn't seem to be mentioned.
Marco Felsch Dec. 12, 2019, 4:21 p.m. UTC | #8
On 19-12-12 16:10, Mark Brown wrote:
> On Wed, Dec 11, 2019 at 06:09:18PM +0100, Marco Felsch wrote:
> 
> > so one argument more for my solution. Also we don't configure the "pad"
> > to be a vsel/ena-pin. The hw-pad can only be a gpio or has an alternate
> > function (WDKICK for GPIO0, Seq. SYS_EN for GPIO2, Seq. PWR_EN for GPIO4).
> > Instead we tell the regulator to use _this_ GPIO e.g. for voltage
> > selection so we go the other way around. My last argument why pinctrl
> > isn't the correct place is that the GPIO1 can be used for
> > regulator-0:vsel-in and for regulator-1:enable-in. So this pad would
> > have different states which is invalid IMHO.
> 
> Note that there's two bits to my concern - one is if we should be using
> gpiolib or pinctrl, the other is what's driving the input (whichever API
> it's configured through) which didn't seem to be mentioned.

gpiolib or pinctrl:
I pointed out all my arguments above so I think it is time for Linus.

"... what's driving the input ..":
Sorry I didn't get you here. What did you mean? The input is driven by
the host. This can be any gpio line and in my case it is a gpio line
driven by the soc-hw during a suspend operation.

Regards,
  Marco
Mark Brown Dec. 12, 2019, 4:51 p.m. UTC | #9
On Thu, Dec 12, 2019 at 05:21:53PM +0100, Marco Felsch wrote:

> "... what's driving the input ..":
> Sorry I didn't get you here. What did you mean? The input is driven by
> the host. This can be any gpio line and in my case it is a gpio line
> driven by the soc-hw during a suspend operation.

Something needs to say what that thing is, especially if it's runtime
controllable.  In your case from the point of view of software there is
actually no enable control so we shouldn't be providing an enable
operation to the framework.
Marco Felsch Dec. 16, 2019, 8:55 a.m. UTC | #10
On 19-12-12 16:51, Mark Brown wrote:
> On Thu, Dec 12, 2019 at 05:21:53PM +0100, Marco Felsch wrote:
> 
> > "... what's driving the input ..":
> > Sorry I didn't get you here. What did you mean? The input is driven by
> > the host. This can be any gpio line and in my case it is a gpio line
> > driven by the soc-hw during a suspend operation.
> 
> Something needs to say what that thing is, especially if it's runtime
> controllable.  In your case from the point of view of software there is
> actually no enable control so we shouldn't be providing an enable
> operation to the framework.

The enabel control signal is always available, please check [1] table
63. There is a mux in front of the enable pin so:

             +-------------
 Seq. |\     |   Regulator
 GPI1 | \    |
 GPI2 | | -- > Enable
 GPI3 | /    |
      |/     .
             .
             .

Adam please correct me if this is wrong.

[1] https://www.dialog-semiconductor.com/sites/default/files/da9062_datasheet_3v6.pdf

Regards,
  Marco

> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Mark Brown Dec. 16, 2019, 11:44 a.m. UTC | #11
On Mon, Dec 16, 2019 at 09:55:25AM +0100, Marco Felsch wrote:
> On 19-12-12 16:51, Mark Brown wrote:

> > Something needs to say what that thing is, especially if it's runtime
> > controllable.  In your case from the point of view of software there is
> > actually no enable control so we shouldn't be providing an enable
> > operation to the framework.

> The enabel control signal is always available, please check [1] table
> 63. There is a mux in front of the enable pin so:

What I'm saying is that I think the binding needs to explicitly talk
about that since at the minute it's really confusing reading it as it
is, it sounds very much like it's trying to override that in a chip
specific fashion as using gpiolib and the GPIO bindings for pinmuxing is
really quite unusual.
Linus Walleij Dec. 16, 2019, 12:28 p.m. UTC | #12
On Thu, Dec 12, 2019 at 5:21 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
> On 19-12-12 16:10, Mark Brown wrote:

> > Note that there's two bits to my concern - one is if we should be using
> > gpiolib or pinctrl, the other is what's driving the input (whichever API
> > it's configured through) which didn't seem to be mentioned.
>
> gpiolib or pinctrl:
> I pointed out all my arguments above so I think it is time for Linus.

I think I've elaborated on this?

The API is fine with me, because this thing does some autonomous
control and I don't know any better way to share that same line
with the GPIO subsystem than to make this offset available to
the regulator driver.

Yours,
Linus Walleij
Adam Thomson Dec. 16, 2019, 4:32 p.m. UTC | #13
> -----Original Message-----
> From: Marco Felsch <m.felsch@pengutronix.de>
> Sent: 16 December 2019 08:55
> To: Mark Brown <broonie@kernel.org>
> Cc: devicetree@vger.kernel.org; Support Opensource
> <Support.Opensource@diasemi.com>; linux-aspeed@lists.ozlabs.org; linux-
> gpio@vger.kernel.org; andrew@aj.id.au; linus.walleij@linaro.org;
> lgirdwood@gmail.com; linux-kernel@vger.kernel.org;
> bgolaszewski@baylibre.com; robh+dt@kernel.org; joel@jms.id.au;
> kernel@pengutronix.de; Adam Thomson
> <Adam.Thomson.Opensource@diasemi.com>; lee.jones@linaro.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage
> selection documentation
> 
> On 19-12-12 16:51, Mark Brown wrote:
> > On Thu, Dec 12, 2019 at 05:21:53PM +0100, Marco Felsch wrote:
> >
> > > "... what's driving the input ..":
> > > Sorry I didn't get you here. What did you mean? The input is driven by
> > > the host. This can be any gpio line and in my case it is a gpio line
> > > driven by the soc-hw during a suspend operation.
> >
> > Something needs to say what that thing is, especially if it's runtime
> > controllable.  In your case from the point of view of software there is
> > actually no enable control so we shouldn't be providing an enable
> > operation to the framework.
> 
> The enabel control signal is always available, please check [1] table
> 63. There is a mux in front of the enable pin so:
> 
>              +-------------
>  Seq. |\     |   Regulator
>  GPI1 | \    |
>  GPI2 | | -- > Enable
>  GPI3 | /    |
>       |/     .
>              .
>              .
> 
> Adam please correct me if this is wrong.
> 
> [1] https://www.dialog-
> semiconductor.com/sites/default/files/da9062_datasheet_3v6.pdf
> 
> Regards,
>   Marco
> 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Adam Thomson Dec. 16, 2019, 4:32 p.m. UTC | #14
On 16 December 2019 08:55, Marco Felsch wrote:

> On 19-12-12 16:51, Mark Brown wrote:
> > On Thu, Dec 12, 2019 at 05:21:53PM +0100, Marco Felsch wrote:
> >
> > > "... what's driving the input ..":
> > > Sorry I didn't get you here. What did you mean? The input is driven by
> > > the host. This can be any gpio line and in my case it is a gpio line
> > > driven by the soc-hw during a suspend operation.
> >
> > Something needs to say what that thing is, especially if it's runtime
> > controllable.  In your case from the point of view of software there is
> > actually no enable control so we shouldn't be providing an enable
> > operation to the framework.
> 
> The enabel control signal is always available, please check [1] table
> 63. There is a mux in front of the enable pin so:
> 
>              +-------------
>  Seq. |\     |   Regulator
>  GPI1 | \    |
>  GPI2 | | -- > Enable
>  GPI3 | /    |
>       |/     .
>              .
>              .
> 
> Adam please correct me if this is wrong.

Yes the register can always be configured regardless of the associated pin
configuration, but if say GPIO1 was configured as a GPO but a regulator was
configured to use GPIO1 as its GPI control mechanism, the output signal from
GPIO1 would be ignored, the sequencer control would not have any effect and
you're simply left with manual I2C control. Really we shouldn't be getting into
that situation though. If a GPIO is to be used as a regulator control signal
then it should be marked as such and I don't think we should be able to use that
pin for anything other than regulator control.

> 
> [1] https://www.dialog-
> semiconductor.com/sites/default/files/da9062_datasheet_3v6.pdf
> 
> Regards,
>   Marco
> 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Marco Felsch Dec. 17, 2019, 7:35 a.m. UTC | #15
On 19-12-16 11:44, Mark Brown wrote:
> On Mon, Dec 16, 2019 at 09:55:25AM +0100, Marco Felsch wrote:
> > On 19-12-12 16:51, Mark Brown wrote:
> 
> > > Something needs to say what that thing is, especially if it's runtime
> > > controllable.  In your case from the point of view of software there is
> > > actually no enable control so we shouldn't be providing an enable
> > > operation to the framework.
> 
> > The enabel control signal is always available, please check [1] table
> > 63. There is a mux in front of the enable pin so:
> 
> What I'm saying is that I think the binding needs to explicitly talk
> about that since at the minute it's really confusing reading it as it
> is, it sounds very much like it's trying to override that in a chip
> specific fashion as using gpiolib and the GPIO bindings for pinmuxing is
> really quite unusual.

Hm.. I still think that we don't mux the pin to some special function.
It is still a gpio input pin and if we don't request the pin we could
read the input from user-space too and get a 'valid' value. Muxing would
happen if we change the pad to so called _alternate_ function. Anyway,
lets find a binding description:

name:
 - dlg,vsel-sense-gpios

IMHO this is very descriptive and needs no update.

description:
 - A GPIO reference to a local general purpose input, [1] calls it GPI.
   The DA9062 regulators can select between voltage-a/-b settings.
   Each regulator has a VBUCK*_GPI or VLDO*_GPI input to determine the
   active setting. In front of the VBUCK*_GPI/VLDO*_GPI input is a mux
   to select between different signal sources, valid sources are: the
   internal sequencer, GPI1, GPI2 and GPI3. See [1] table 63 for more
   information. Most the time the internal sequencer is fine but
   sometimes it is necessary to use the signal from the DA9062 GPI
   pads. This binding covers the second use case.
   Attention: Sharing the same GPI for other purposes or across multiple
   regulators is possible but the polarity setting must equal.

[1] https://www.dialog-semiconductor.com/sites/default/files/da9062_datasheet_3v6.pdf

Regards,
  Marco


> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Marco Felsch Dec. 17, 2019, 9 a.m. UTC | #16
On 19-12-16 16:32, Adam Thomson wrote:
> On 16 December 2019 08:55, Marco Felsch wrote:
> 
> > On 19-12-12 16:51, Mark Brown wrote:
> > > On Thu, Dec 12, 2019 at 05:21:53PM +0100, Marco Felsch wrote:
> > >
> > > > "... what's driving the input ..":
> > > > Sorry I didn't get you here. What did you mean? The input is driven by
> > > > the host. This can be any gpio line and in my case it is a gpio line
> > > > driven by the soc-hw during a suspend operation.
> > >
> > > Something needs to say what that thing is, especially if it's runtime
> > > controllable.  In your case from the point of view of software there is
> > > actually no enable control so we shouldn't be providing an enable
> > > operation to the framework.
> > 
> > The enabel control signal is always available, please check [1] table
> > 63. There is a mux in front of the enable pin so:
> > 
> >              +-------------
> >  Seq. |\     |   Regulator
> >  GPI1 | \    |
> >  GPI2 | | -- > Enable
> >  GPI3 | /    |
> >       |/     .
> >              .
> >              .
> > 
> > Adam please correct me if this is wrong.
> 
> Yes the register can always be configured regardless of the associated pin
> configuration, but if say GPIO1 was configured as a GPO but a regulator was
> configured to use GPIO1 as its GPI control mechanism, the output signal from
> GPIO1 would be ignored, the sequencer control would not have any effect and
> you're simply left with manual I2C control. Really we shouldn't be getting into
> that situation though. If a GPIO is to be used as a regulator control signal
> then it should be marked as such and I don't think we should be able to use that
> pin for anything other than regulator control.

I see, so we have to guarantee that the requested gpio is configured as
input. This can be done by:

  if (gpi->flags & FLAG_IS_OUT)
  	return -EINVAL;

Regards,
  Marco

> > 
> > [1] https://www.dialog-
> > semiconductor.com/sites/default/files/da9062_datasheet_3v6.pdf
> > 
> > Regards,
> >   Marco
> > 
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Marco Felsch Dec. 17, 2019, 9:12 a.m. UTC | #17
On 19-12-17 10:00, Marco Felsch wrote:
> On 19-12-16 16:32, Adam Thomson wrote:
> > On 16 December 2019 08:55, Marco Felsch wrote:
> > 
> > > On 19-12-12 16:51, Mark Brown wrote:
> > > > On Thu, Dec 12, 2019 at 05:21:53PM +0100, Marco Felsch wrote:
> > > >
> > > > > "... what's driving the input ..":
> > > > > Sorry I didn't get you here. What did you mean? The input is driven by
> > > > > the host. This can be any gpio line and in my case it is a gpio line
> > > > > driven by the soc-hw during a suspend operation.
> > > >
> > > > Something needs to say what that thing is, especially if it's runtime
> > > > controllable.  In your case from the point of view of software there is
> > > > actually no enable control so we shouldn't be providing an enable
> > > > operation to the framework.
> > > 
> > > The enabel control signal is always available, please check [1] table
> > > 63. There is a mux in front of the enable pin so:
> > > 
> > >              +-------------
> > >  Seq. |\     |   Regulator
> > >  GPI1 | \    |
> > >  GPI2 | | -- > Enable
> > >  GPI3 | /    |
> > >       |/     .
> > >              .
> > >              .
> > > 
> > > Adam please correct me if this is wrong.
> > 
> > Yes the register can always be configured regardless of the associated pin
> > configuration, but if say GPIO1 was configured as a GPO but a regulator was
> > configured to use GPIO1 as its GPI control mechanism, the output signal from
> > GPIO1 would be ignored, the sequencer control would not have any effect and
> > you're simply left with manual I2C control. Really we shouldn't be getting into
> > that situation though. If a GPIO is to be used as a regulator control signal
> > then it should be marked as such and I don't think we should be able to use that
> > pin for anything other than regulator control.
> 
> I see, so we have to guarantee that the requested gpio is configured as
> input. This can be done by:
> 
>   if (gpi->flags & FLAG_IS_OUT)
>   	return -EINVAL;

Sorry didn't noticed that the flags are only used internally. The
correct check must be:

	/* GPIO must be configured as input */
	if (!gpiod_get_direction(gpi)) {
		ret = -EINVAL;
		goto free;
	}

> 
> Regards,
>   Marco
> 
> > > 
> > > [1] https://www.dialog-
> > > semiconductor.com/sites/default/files/da9062_datasheet_3v6.pdf
> > > 
> > > Regards,
> > >   Marco
> > > 
> > > > _______________________________________________
> > > > linux-arm-kernel mailing list
> > > > linux-arm-kernel@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Adam Thomson Dec. 17, 2019, 9:53 a.m. UTC | #18
On 17 December 2019 09:01, Marco Felsch wrote:

> > > The enabel control signal is always available, please check [1] table
> > > 63. There is a mux in front of the enable pin so:
> > >
> > >              +-------------
> > >  Seq. |\     |   Regulator
> > >  GPI1 | \    |
> > >  GPI2 | | -- > Enable
> > >  GPI3 | /    |
> > >       |/     .
> > >              .
> > >              .
> > >
> > > Adam please correct me if this is wrong.
> >
> > Yes the register can always be configured regardless of the associated pin
> > configuration, but if say GPIO1 was configured as a GPO but a regulator was
> > configured to use GPIO1 as its GPI control mechanism, the output signal from
> > GPIO1 would be ignored, the sequencer control would not have any effect and
> > you're simply left with manual I2C control. Really we shouldn't be getting into
> > that situation though. If a GPIO is to be used as a regulator control signal
> > then it should be marked as such and I don't think we should be able to use that
> > pin for anything other than regulator control.
> 
> I see, so we have to guarantee that the requested gpio is configured as
> input. This can be done by:

This is one of the reasons I thought this was better suited to being done in the
pinctrl/pinmux side. If you configure the GPIO as for regulator control then
the code can automatically configure the GPIO for input. That doesn't then need
to be in the regulator driver.

But yes we wouldn't really want to configure a regulator to be controlled via a
GPI when it's configured as a GPO as it makes no sense.

> 
>   if (gpi->flags & FLAG_IS_OUT)
>   	return -EINVAL;
> 
> Regards,
>   Marco
Marco Felsch Dec. 17, 2019, 12:31 p.m. UTC | #19
On 19-12-17 09:53, Adam Thomson wrote:
> On 17 December 2019 09:01, Marco Felsch wrote:
> 
> > > > The enabel control signal is always available, please check [1] table
> > > > 63. There is a mux in front of the enable pin so:
> > > >
> > > >              +-------------
> > > >  Seq. |\     |   Regulator
> > > >  GPI1 | \    |
> > > >  GPI2 | | -- > Enable
> > > >  GPI3 | /    |
> > > >       |/     .
> > > >              .
> > > >              .
> > > >
> > > > Adam please correct me if this is wrong.
> > >
> > > Yes the register can always be configured regardless of the associated pin
> > > configuration, but if say GPIO1 was configured as a GPO but a regulator was
> > > configured to use GPIO1 as its GPI control mechanism, the output signal from
> > > GPIO1 would be ignored, the sequencer control would not have any effect and
> > > you're simply left with manual I2C control. Really we shouldn't be getting into
> > > that situation though. If a GPIO is to be used as a regulator control signal
> > > then it should be marked as such and I don't think we should be able to use that
> > > pin for anything other than regulator control.
> > 
> > I see, so we have to guarantee that the requested gpio is configured as
> > input. This can be done by:
> 
> This is one of the reasons I thought this was better suited to being done in the
> pinctrl/pinmux side. If you configure the GPIO as for regulator control then
> the code can automatically configure the GPIO for input. That doesn't then need
> to be in the regulator driver.

I still don't prefer that way.. pls check my arguments I already made
and I don't wanna repeat it again.

> But yes we wouldn't really want to configure a regulator to be controlled via a
> GPI when it's configured as a GPO as it makes no sense.

Okay, so the check is all we need to hardening the driver against wrong
usage :)

Regards,
  Marco

> 
> > 
> >   if (gpi->flags & FLAG_IS_OUT)
> >   	return -EINVAL;
> > 
> > Regards,
> >   Marco
>
Mark Brown Dec. 17, 2019, 12:58 p.m. UTC | #20
On Tue, Dec 17, 2019 at 08:35:33AM +0100, Marco Felsch wrote:
> On 19-12-16 11:44, Mark Brown wrote:

> > What I'm saying is that I think the binding needs to explicitly talk
> > about that since at the minute it's really confusing reading it as it
> > is, it sounds very much like it's trying to override that in a chip
> > specific fashion as using gpiolib and the GPIO bindings for pinmuxing is
> > really quite unusual.

> Hm.. I still think that we don't mux the pin to some special function.
> It is still a gpio input pin and if we don't request the pin we could
> read the input from user-space too and get a 'valid' value. Muxing would
> happen if we change the pad to so called _alternate_ function. Anyway,
> lets find a binding description:

I don't think any of this makes much difference from a user point of
view.

> IMHO this is very descriptive and needs no update.

> description:
>  - A GPIO reference to a local general purpose input, [1] calls it GPI.
>    The DA9062 regulators can select between voltage-a/-b settings.
>    Each regulator has a VBUCK*_GPI or VLDO*_GPI input to determine the
>    active setting. In front of the VBUCK*_GPI/VLDO*_GPI input is a mux
>    to select between different signal sources, valid sources are: the
>    internal sequencer, GPI1, GPI2 and GPI3. See [1] table 63 for more
>    information. Most the time the internal sequencer is fine but
>    sometimes it is necessary to use the signal from the DA9062 GPI
>    pads. This binding covers the second use case.
>    Attention: Sharing the same GPI for other purposes or across multiple
>    regulators is possible but the polarity setting must equal.

This doesn't say anything about how the GPIO input is expected to be
controlled, for voltage setting any runtime control would need to be
done by the driver and it sounds like that's all that can be controlled.
The way this reads I'd expect one use of this to be for fast voltage
setting for example (you could even combine that with suspend sequencing
using the internal sequencer if you mux back to the sequencer during
suspend).
Adam Thomson Dec. 17, 2019, 1:13 p.m. UTC | #21
On 17 December 2019 12:31, Marco Felsch wrote:

> On 19-12-17 09:53, Adam Thomson wrote:
> > On 17 December 2019 09:01, Marco Felsch wrote:
> >
> > > > > The enabel control signal is always available, please check [1] table
> > > > > 63. There is a mux in front of the enable pin so:
> > > > >
> > > > >              +-------------
> > > > >  Seq. |\     |   Regulator
> > > > >  GPI1 | \    |
> > > > >  GPI2 | | -- > Enable
> > > > >  GPI3 | /    |
> > > > >       |/     .
> > > > >              .
> > > > >              .
> > > > >
> > > > > Adam please correct me if this is wrong.
> > > >
> > > > Yes the register can always be configured regardless of the associated pin
> > > > configuration, but if say GPIO1 was configured as a GPO but a regulator was
> > > > configured to use GPIO1 as its GPI control mechanism, the output signal
> from
> > > > GPIO1 would be ignored, the sequencer control would not have any effect
> and
> > > > you're simply left with manual I2C control. Really we shouldn't be getting
> into
> > > > that situation though. If a GPIO is to be used as a regulator control signal
> > > > then it should be marked as such and I don't think we should be able to use
> that
> > > > pin for anything other than regulator control.
> > >
> > > I see, so we have to guarantee that the requested gpio is configured as
> > > input. This can be done by:
> >
> > This is one of the reasons I thought this was better suited to being done in the
> > pinctrl/pinmux side. If you configure the GPIO as for regulator control then
> > the code can automatically configure the GPIO for input. That doesn't then
> need
> > to be in the regulator driver.
> 
> I still don't prefer that way.. pls check my arguments I already made
> and I don't wanna repeat it again.

Yes, I read your arguments but still can't agree :)
Marco Felsch Jan. 7, 2020, 8:36 a.m. UTC | #22
Hi Mark,

On 19-12-17 12:58, Mark Brown wrote:
> On Tue, Dec 17, 2019 at 08:35:33AM +0100, Marco Felsch wrote:
> > On 19-12-16 11:44, Mark Brown wrote:
> 
> > > What I'm saying is that I think the binding needs to explicitly talk
> > > about that since at the minute it's really confusing reading it as it
> > > is, it sounds very much like it's trying to override that in a chip
> > > specific fashion as using gpiolib and the GPIO bindings for pinmuxing is
> > > really quite unusual.
> 
> > Hm.. I still think that we don't mux the pin to some special function.
> > It is still a gpio input pin and if we don't request the pin we could
> > read the input from user-space too and get a 'valid' value. Muxing would
> > happen if we change the pad to so called _alternate_ function. Anyway,
> > lets find a binding description:
> 
> I don't think any of this makes much difference from a user point of
> view.
> 
> > IMHO this is very descriptive and needs no update.
> 
> > description:
> >  - A GPIO reference to a local general purpose input, [1] calls it GPI.
> >    The DA9062 regulators can select between voltage-a/-b settings.
> >    Each regulator has a VBUCK*_GPI or VLDO*_GPI input to determine the
> >    active setting. In front of the VBUCK*_GPI/VLDO*_GPI input is a mux
> >    to select between different signal sources, valid sources are: the
> >    internal sequencer, GPI1, GPI2 and GPI3. See [1] table 63 for more
> >    information. Most the time the internal sequencer is fine but
> >    sometimes it is necessary to use the signal from the DA9062 GPI
> >    pads. This binding covers the second use case.
> >    Attention: Sharing the same GPI for other purposes or across multiple
> >    regulators is possible but the polarity setting must equal.
> 
> This doesn't say anything about how the GPIO input is expected to be
> controlled, for voltage setting any runtime control would need to be
> done by the driver and it sounds like that's all that can be controlled.
> The way this reads I'd expect one use of this to be for fast voltage
> setting for example (you could even combine that with suspend sequencing
> using the internal sequencer if you mux back to the sequencer during
> suspend).

The input signal is routed trough the da9062 gpio block to the
regualtors. You can't set any voltage value using a gpio instead you
decide which voltage setting is applied. The voltage values for
runtime/suspend comes from the dt-data. No it's not just a fast
switching option imagine the system suspend case where the cpu and soc
voltage can be reduced to a very low value. Older soc's like the imx6
signaling this state by a hard wired gpio line because the soc and
cpu cores don't work properly on such low voltage values. This is
my use case and I can't use the sequencer.

Regards,
  Marco
Mark Brown Jan. 7, 2020, 1:09 p.m. UTC | #23
On Tue, Jan 07, 2020 at 09:36:54AM +0100, Marco Felsch wrote:
> On 19-12-17 12:58, Mark Brown wrote:

> > This doesn't say anything about how the GPIO input is expected to be
> > controlled, for voltage setting any runtime control would need to be
> > done by the driver and it sounds like that's all that can be controlled.
> > The way this reads I'd expect one use of this to be for fast voltage
> > setting for example (you could even combine that with suspend sequencing
> > using the internal sequencer if you mux back to the sequencer during
> > suspend).

> The input signal is routed trough the da9062 gpio block to the
> regualtors. You can't set any voltage value using a gpio instead you
> decide which voltage setting is applied. The voltage values for
> runtime/suspend comes from the dt-data. No it's not just a fast
> switching option imagine the system suspend case where the cpu and soc
> voltage can be reduced to a very low value. Older soc's like the imx6
> signaling this state by a hard wired gpio line because the soc and
> cpu cores don't work properly on such low voltage values. This is
> my use case and I can't use the sequencer.

My point is that I can't tell any of this from the description.
Marco Felsch Jan. 7, 2020, 1:38 p.m. UTC | #24
On 20-01-07 13:09, Mark Brown wrote:
> On Tue, Jan 07, 2020 at 09:36:54AM +0100, Marco Felsch wrote:
> > On 19-12-17 12:58, Mark Brown wrote:
> 
> > > This doesn't say anything about how the GPIO input is expected to be
> > > controlled, for voltage setting any runtime control would need to be
> > > done by the driver and it sounds like that's all that can be controlled.
> > > The way this reads I'd expect one use of this to be for fast voltage
> > > setting for example (you could even combine that with suspend sequencing
> > > using the internal sequencer if you mux back to the sequencer during
> > > suspend).
> 
> > The input signal is routed trough the da9062 gpio block to the
> > regualtors. You can't set any voltage value using a gpio instead you
> > decide which voltage setting is applied. The voltage values for
> > runtime/suspend comes from the dt-data. No it's not just a fast
> > switching option imagine the system suspend case where the cpu and soc
> > voltage can be reduced to a very low value. Older soc's like the imx6
> > signaling this state by a hard wired gpio line because the soc and
> > cpu cores don't work properly on such low voltage values. This is
> > my use case and I can't use the sequencer.
> 
> My point is that I can't tell any of this from the description.

Therefore I want to discuss the dt-binding documentation with you and
the others to get this done. Is the above description better to
understand the dt-binding?

Regards,
  Marco
Mark Brown Jan. 14, 2020, 3:43 p.m. UTC | #25
On Tue, Jan 07, 2020 at 02:38:11PM +0100, Marco Felsch wrote:
> On 20-01-07 13:09, Mark Brown wrote:
> > On Tue, Jan 07, 2020 at 09:36:54AM +0100, Marco Felsch wrote:

> > > The input signal is routed trough the da9062 gpio block to the
> > > regualtors. You can't set any voltage value using a gpio instead you
> > > decide which voltage setting is applied. The voltage values for
> > > runtime/suspend comes from the dt-data. No it's not just a fast
> > > switching option imagine the system suspend case where the cpu and soc
> > > voltage can be reduced to a very low value. Older soc's like the imx6
> > > signaling this state by a hard wired gpio line because the soc and
> > > cpu cores don't work properly on such low voltage values. This is
> > > my use case and I can't use the sequencer.

> > My point is that I can't tell any of this from the description.

> Therefore I want to discuss the dt-binding documentation with you and
> the others to get this done. Is the above description better to
> understand the dt-binding?

That text really doesn't feel like text that'd be idiomatic
directly in a binding document but some of those ideas probably
do need to be in the text I think.

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt b/Documentation/devicetree/bindings/mfd/da9062.txt
index edca653a5777..b9cccd4c3f76 100644
--- a/Documentation/devicetree/bindings/mfd/da9062.txt
+++ b/Documentation/devicetree/bindings/mfd/da9062.txt
@@ -66,6 +66,14 @@  Sub-nodes:
   details of individual regulator device can be found in:
   Documentation/devicetree/bindings/regulator/regulator.txt
 
+  Optional regulator device-specific properties:
+  - dlg,vsel-sense-gpios : A GPIO reference to a local general purpose input,
+    the datasheet calls it GPI. The regulator sense the input signal and select
+    the active or suspend voltage settings. If the signal is active the
+    active-settings are applied else the suspend-settings are applied.
+    Attention: Sharing the same GPI for other purposes or across multiple
+    regulators is possible but the polarity setting must equal.
+
 - rtc : This node defines settings required for the Real-Time Clock associated
   with the DA9062. There are currently no entries in this binding, however
   compatible = "dlg,da9062-rtc" should be added if a node is created.