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 |
