| Message ID | 20191129172537.31410-4-m.felsch@pengutronix.de |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | DA9062 PMIC features | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
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?
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
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 |
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 | >
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
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 >
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.
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
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.
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
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.
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
> -----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
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
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
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 >
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 >
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
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 >
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).
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 :)
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
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.
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
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.
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.
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(+)