Re: [PATCH 2/5] dt-bindings: mfd: da9062: add regulator voltage selection documentation

From: Marco Felsch
Date: Tue Oct 22 2019 - 06:22:36 EST

Next message: Michal Hocko: "Re: [RFC PATCH v2 10/16] mm,hwpoison: Rework soft offline for free pages"
Previous message: Peng Ma: "RE: [EXT] Re: [V5 1/2] dmaengine: fsl-dpaa2-qdma: Add the DPDMAI(Data Path DMA Interface) support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Linus, Bartosz,

can you have a look on this discussion? I wanna prepare a new version
and this is still open.

Regards,
Marco

On 19-10-02 15:45, Marco Felsch wrote:
> On 19-09-30 09:53, Adam Thomson wrote:
> > On 26 September 2019 15:39, Marco Felsch wrote:
> >
> > > On 19-09-26 14:04, Adam Thomson wrote:
> > > > On 26 September 2019 12:44, Marco Felsch wrote:
> > > >
> > > > > On 19-09-26 10:17, Adam Thomson wrote:
> > > > > > On 26 September 2019 09:10, Marco Felsch wrote:
> > > > > >
> > > > > > > On 19-09-25 16:18, Adam Thomson wrote:
> > > > > > > > On 25 September 2019 16:52, Marco Felsch wrote:
> > > > > > > >
> > > > > > > > > Hi Adam,
> > > > > > > > >
> > > > > > > > > On 19-09-24 09:23, Adam Thomson wrote:
> > > > > > > > > > On 17 September 2019 13:43, Marco Felsch wrote:
> > > > > > > > > >
> > > > > > > > > > > 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@xxxxxxxxxxxxxx>
> > > > > > > > > > > ---
> > > > > > > > > > > Documentation/devicetree/bindings/mfd/da9062.txt | 9
> > > +++++++++
> > > > > > > > > > > 1 file changed, 9 insertions(+)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt
> > > > > > > > > > > b/Documentation/devicetree/bindings/mfd/da9062.txt
> > > > > > > > > > > index edca653a5777..9d9820d8177d 100644
> > > > > > > > > > > --- a/Documentation/devicetree/bindings/mfd/da9062.txt
> > > > > > > > > > > +++ b/Documentation/devicetree/bindings/mfd/da9062.txt
> > > > > > > > > > > @@ -66,6 +66,15 @@ 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 : The GPIO reference which should be
> > > used
> > > > > by
> > > > > > > the
> > > > > > > > > > > + regulator to switch the voltage between active/suspend
> > > voltage
> > > > > > > settings.
> > > > > > > > > If
> > > > > > > > > > > + the signal is active the active-settings are applied else the
> > > suspend
> > > > > > > > > > > + settings are applied. Attention: Sharing the same gpio for other
> > > > > > > purposes
> > > > > > > > > > > + or across multiple regulators is possible but the gpio settings
> > > must
> > > > > be
> > > > > > > the
> > > > > > > > > > > + same. Also the gpio phandle must refer to to the dlg,da9062-
> > > gpio
> > > > > > > device
> > > > > > > > > > > + other gpios are not allowed and make no sense.
> > > > > > > > > > > +
> > > > > > > > > >
> > > > > > > > > > Should we not use the binding names that are defined in 'gpio-
> > > > > > > regulator.yaml'
> > > > > > > > > as
> > > > > > > > > > these seem to be generic and would probably serve the purpose
> > > here?
> > > > > > > > >
> > > > > > > > > Hm.. as the description says:
> > > > > > > > >
> > > > > > > > > 8<--------------------------------------------------
> > > > > > > > > gpios:
> > > > > > > > > description: Array of one or more GPIO pins used to select the
> > > > > > > > > regulator voltage/current listed in "states".
> > > > > > > > > 8<--------------------------------------------------
> > > > > > > > >
> > > > > > > > > But we don't have a "states" property and we can't select between
> > > > > > > > > voltage or current.
> > > > > > > >
> > > > > > > > Yes I think I was at cross purposes when I made this remark. The
> > > bindings
> > > > > there
> > > > > > > > describe the GPOs that are used to enable/disable and set
> > > voltage/current
> > > > > for
> > > > > > > > regulators and the supported voltage/current levels that can be
> > > configured
> > > > > in
> > > > > > > > this manner. What you're describing is the GPI for DA9061/2. If you look
> > > at
> > > > > > > > GPIO handling in existing regulator drivers I believe they all deal with
> > > > > external
> > > > > > > > GPOs that are configured to enable/disable and set voltage/current
> > > limits
> > > > > > > rather
> > > > > > > > than the GPI on the PMIC itself. That's why I'm thinking that the
> > > > > configurations
> > > > > > > > you're doing here should actually be in a pinctrl or GPIO driver.
> > > > > > >
> > > > > > > That's true, the common gpio bindings are from the view of the
> > > > > > > processor, e.g. which gpio must the processor drive to enable/switch the
> > > > > > > regualtor. So one reasone more to use a non-common binding.
> > > > > > >
> > > > > > > Please take a look on my other comment I made :) I don't use the
> > > > > > > gpio-alternative function. I use it as an input.
> > > > > >
> > > > > > I know in the datasheet this isn't marked as an alternate function specifically
> > > > > > but to me having regulator control by the chip's own GPI is an alternative
> > > > > > function for that GPIO pin, in the same way a specific pin can be used for
> > > > > > SYS_EN or Watchdog control. It's a dedicated purpose rather than being a
> > > > > normal
> > > > > > GPI.
> > > > >
> > > > > Nope, SYS_EN or Watchdog is a special/alternate function and not a
> > > > > normal input.
> > > >
> > > > Having spoken with our HW team there's essentially no real difference.
> > >
> > > So I don't have to configure the gpio to alternate to use it as SYS_EN?
> >
> > Yes you do, but the effect is much the same as manually configuring the GPIO as
> > input, just that the IC does it for you. The regulator control features could
> > well have been done in a similar manner. Guess that was a design choice.
> >
> > >
> > > > >
> > > > > > See the following as an example of what I'm suggesting:
> > > > > >
> > > > > >
> > > > >
> > > https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindin
> > > > > gs/pinctrl/pinctrl-palmas.txt
> > > > > >
> > > > > > You could then pass the pinctrl information to the regulator driver and use
> > > > > > that rather than having device specific bindings for this. That's at least my
> > > > > > current interpretation of this anyway.
> > > > >
> > > > > For me pinctrl decides which function should be assigned to a pin. So in
> > > > > our case this would be:
> > > > > - alternate
> > > > > - gpo
> > > > > - gpi
> > > > >
> > > > > In our use-case it is a gpi..
> > > >
> > > > It's not being used as a normal GPI as such. It's being used to enable/disable
> > > > the regulator so I disagree.
> > >
> > > This one is used as voltage-selection. What is a "normal" GPI in your
> > > point of view?
> >
> > With the voltage selection and enable/disable control the actual work of
> > handling the GPI state is all done internally in the IC. There is no control
> > required from SW other than setting of initial direction. For a normal GPI
>
> Thats correct and setting the direction and active levels can be done
> perfectly by the gpio-interface.
>
> > I would expect SW to be involved in the handling of that GPI state, for example
> > as part of a bit banging interface.
> >
> > >
> > > > >
> > > > > An other reason why pinctrl seems not be the right solution is that the
> > > > > regulator must be configured to use this gpi. This decision can't be
> > > > > made globally because each regulator can be configured differently.. For
> > > > > me its just a local gpio.
> > > >
> > > > You'd pass pinctrl information, via DT, to the regulator driver so it can set
> > > > accordingly. At least that's my take here, unless I'm missing something. The
> > > > regulator driver would be the consumer and could set the regulator control
> > > > accordingly.
> > >
> > > IMHO this is what I have done. I use the gpi so the regulator is the
> > > consumer. Since the gpi can be used by several regulators for voltage
> > > selection or enable/disable action this gpi is marked as shared. If I
> > > got you right than you would do something like for regulatorX.
> > >
> > > pinctrl-node {
> > >
> > > gpio2 {
> > > func = "vsel";
> > > }
> > > }
> > >
> > > But the gpi(o)2 can also be used to enable/disable a regulatorY if I
> > > understood the datasheet correctly. I other words:
> > >
> > >
> > >
> > > +--> Alternate function
> > > /
> > > ---+ +--> GPI ----> Edge detection ---> more processing
> > > | | |
> > > | | +-----> Regulator control
> > > | | |
> > > \__ __/ \__________ _______
> > > \/ \/
> > > pinctrl gpio
> > >
> > > This is how I understood the pinctrl use-case. I configure the pin as
> > > gpio and then the regulator driver consume a gpio.
> >
> > How I see it is that you configure the function through pinctrl as
> > 'regulator_switch' or 'regulator_vsel' (or whatever name is deemed sensible to
>
> The case is that it can be "muxed" for both at same time so there is no
> or instead it is a or/and. Depending on the design some regulators can
> be turned off and some should be switched upon that signal.
>
> > cover the two types of functionality) and then the pinctrl driver code would do
> > the work of requesting and configuring the relevant GPIO as input so it's no
> > longer available for use as something else (basically what you do in the
> > regulator driver right now).
>
> So by this I avoided the user-space to get this gpio and this seems fine
> to me. What a driver does with a gpio is up to the driver but the gpio
> shouldn't be reachable from the user-space.
>
> > I believe you can have more than one consumer of a pinctrl pin so it could be
> > provided to both regulator X and Y to indicate that this is the chosen
> > functionality of that pin and so the regulator can then be marked as being
> > controlled by that pin. Using pinctrl also would mean you're using standard
> > bindings as well rather than something which is device specific.
>
> That seems wrong to me because pinctrl should assign a dedicated function
> to that pin e.g. regulator_switch or regulator_vsel. But thats wrong as
> I pointed out above. On the other hand the gpio as the name says is
> general purpose and each regulator gives the meaning to it.
>
> > > > At the end of the day I'm not the gatekeeper here so I think Mark's input is
> > > > necessary as he will likely have a view on how this should be done. I appreciate
> > > > the work you've done here but I want to be sure we have a generic solution
> > > > as this would also apply to DA9063 and possibly other devices too.
> > >
> > > Why should this only apply to da9062 devices? IMHO this property can be
> > > used by any other dlg pmic as well if it is supported. Comments and suggestions
> > > are welcome so no worries ;)
> >
> > You're right. You can do the same for DA9063 and other devices potentially. I
> > would just like to make sure we take the right/agreed approach. Potentially
> > this could be used in non-Dialog products as well which have similar
> > functionality.
>
> Yeah, but I think we should consider about that after an other device
> appears with that funcitonality.
>
> > As I say, Mark is really the gatekeeper so his input is also key in this.
>
> That's right, I added Mark to To.
>
> Regards,
> Marco
>
> >
> > >
> > > Regards,
> > > Marco
> > >
> > > > Have added Mark to the 'To' in this e-mail thread so he might see it.
> > > > >
> > > > > Regards,
> > > > > Marco
> > > > >
> > > > > > >
> > > > > > > Regards,
> > > > > > > Marco
> > > > > > >
> > > > > > >
> > > > > > > > I'd be interested in hearing Mark's view on this though as he has far
> > > more
> > > > > > > > experience in this area than I do.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > > Marco
> > > > > > > > >
> > > > > > > > > > > - 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.
> > > > > > > > > > > --
> > > > > > > > > > > 2.20.1
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Pengutronix e.K. | |
> > > > > > > > > Industrial Linux Solutions | http://www.pengutronix.de/ |
> > > > > > > > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-
> > > 0
> > > > > |
> > > > > > > > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555
> > > |
> > > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Pengutronix e.K. | |
> > > > > > > Industrial Linux Solutions | http://www.pengutronix.de/ |
> > > > > > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> > > |
> > > > > > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> > > > > >
> > > > >
> > > > > --
> > > > > Pengutronix e.K. | |
> > > > > Industrial Linux Solutions | http://www.pengutronix.de/ |
> > > > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> > > > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> > > >
> > >
> > > --
> > > Pengutronix e.K. | |
> > > Industrial Linux Solutions | http://www.pengutronix.de/ |
> > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> >
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>
>

--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

Next message: Michal Hocko: "Re: [RFC PATCH v2 10/16] mm,hwpoison: Rework soft offline for free pages"
Previous message: Peng Ma: "RE: [EXT] Re: [V5 1/2] dmaengine: fsl-dpaa2-qdma: Add the DPDMAI(Data Path DMA Interface) support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]