Re: regmap-gpio: Support set_config and other not quite so standard ICs?

["Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>]

On Fri, 2021-05-14 at 22:34 +0200, Michael Walle wrote:
> Am 2021-05-11 05:59, schrieb Matti Vaittinen:
> > Morning Andy,
> > 
> > On Mon, 2021-05-10 at 19:54 +0300, Andy Shevchenko wrote:
> > > On Mon, May 10, 2021 at 4:41 PM Matti Vaittinen
> > > <matti.vaittinen@fi.rohmeurope.com> wrote:
> > > > Hi Linus, All,
> > > > 
> > > > On Thu, 2021-03-25 at 12:32 +0200, Matti Vaittinen wrote:
> > > > > On Thu, 2021-03-25 at 10:35 +0100, Linus Walleij wrote:
> > > > 
> > > > snip
> > > > 
> > > > > > It could potentially (like the other Rohm GPIO MFD PMIC
> > > > > > drivers)
> > > > > > make some use of the gpio regmap library, but we have some
> > > > > > pending changes for that so look into it after the next
> > > > > > merge
> > > > > > window.
> > > > > > 
> > > > > > I.e. for your TODO: look at the GPIO_REGMAP helper.
> > > > > 
> > > > > I just took a quick peek at gpio_regmap and it looks pretty
> > > > > good
> > > > > to
> > > > > me!
> > > > > 
> > > > > Any particular reason why gpio_regmap is not just part of
> > > > > gpio_chip?
> > > > > I
> > > > > guess providing the 'gpio_regmap_direction_*()',
> > > > > 'gpio_regmap_get()',
> > > > > 'gpio_regmap_set()' as exported helpers and leaving calling
> > > > > the
> > > > > (devm_)gpiochip_add_data() to IC driver would have allowed
> > > > > more
> > > > > flexibility. Drivers could then use the gpio_regamap features
> > > > > which
> > > > > fit
> > > > > the IC (by providing pointers to helper functions in
> > > > > gpio_chip) -
> > > > > and
> > > > > handle potential oddball-features by using pointers to some
> > > > > customized
> > > > > functions in gpio_chip.
> > > > 
> > > > So, v5.13-rc1 is out. I started wondering the gpio_regamap -
> > > > and
> > > > same
> > > > question persists. Why hiding the gpio_chip from gpio_regmap
> > > > users?
> > > 
> > > In general to me this sounds like opening a window for
> > > non-controllable changes vs. controllable. Besides that, struct
> > > gpio_chip has more than a few callbacks. On top of that, opening
> > > this
> > > wide window means you won't be able to stop or refactoring become
> > > a
> > > burden. I would be on the stricter side here.
> 
> I tend to agree with Andy. Keep in mind that gpio-regmap was intended
> to catch all the simple and similar controllers.
> 
> That being said, I'd still like to see new users. I've had a look
> at existing drivers myself some time ago and determined that there
> are quirks here and there which prevent porting that driver to
> gpio-regmap, see for example gpio-mpc8xxx.c, there is a workaround
> for some specific SoC which caches some values in the driver.

Yes. In reality we have pretty limited amount of HW which has no quirks
or special things.

> If we make this gpio-regmap more like a library where users can
> just pick the functions they need, I fear that in the end it is
> nearly impossible to change such a function because you'll always
> break one or another user. But that is just a gut feeling.

I am proposing we keep these exported helpers as simple as possible.
When we allow users to use only functions that fit their HW - and write
own functions for HW requiring quirks - then we can indeed catch all
simple and similar controllers - and simple and similar controllers
only. This should allow keeping these functions clean and well defined
in the end :) This should also mean easier to change.

> > But this is all just my thinking - I'm kind of a "bystander" here
> > and
> > that's why I asked for opinions. Thanks for sharing yours, Andy. I
> > do
> > appreciate all the help and discussion.
> > 
> > > > 3) The last option would be adding pointer to regmap_gpio to
> > > > gpio_chip
> > > > - and exporting the regmap_gpio functions as helpers - leaving
> > > > the
> > > > gpio
> > > > registration to be done by the IC driver. That would allow IC
> > > > driver to
> > > > use the regmap_gpio helpers which suit the IC and write own
> > > > functions
> > > > for rest of the stuff.
> > 
> > I was trying to describe here the approach that has been taken in
> > use
> > at the regulator subsystem - which has used the regmap helpers for
> > quite a while. I think that approach is scaling quite Ok even for
> > strange HW.
> 
> Do you have any pointers?
> 

I'm not sure how familiar you are with the regulators but:
The exported helpers are in

drivers/regulator/helpers.c

These helpers can change/list voltages, enable/disable reguators etc.
based on the hardware (register) descritpion given at regulator
registration phase in struct regulator_desc (at
include/regulator/driver.h)

Each driver can fill the description according to it's own HW and use
the provided helpers where suitable (by giving the ops at regulator
registration, see struct regulator_ops, also at driver.h). Or, if the
provided helpers are not useful, user can just write own functions.

If this was brought to GPIO world, it would seem like the gpio_regmap
was not separate GPIO-driver but collection of exported helpers which
use the HW description information embedded in GPIO core.

Let's open the ROHM regulator drivers (which I happen to know :]) as an
example of a driver which uses both the helpers as such, and for some
cases own functions:

bd70528-regulator.c (ramp-delay is not handled by helpers, but it
probably could now when we added ramp-delay helper.)
bd71815-regulator.c (bucks 1 & 2  have special voltage-configuration
cases)
The upstream driver for BD71828 uses only standard helpers - but that's
just because it does not expose all HW features. Vendor driver provides
a 'run-level' specific control options - and can't use standard
functions for all operations.
bd718x7-regulator.c: The regulators require special handling when
voltage is changed (if regulators are enabled). And for some BD71837
bucks this is not allowed at all because change may cause voltage
spikes. So yes - mixture of standard ops and own code is again needed.

So, when looking at this which is kind of an analogy - All of the ROHM
regulator drivers use generic helper code - which saves code and helps
avoiding bugs - but all of them also need(ed) some own code to provide
proper support. If the regulator framework had used as strict policy as
gpio_regmap - then none of the ROHM regulators could use the standard
framework code. I believe pretty many other drivers have same kind of
mixtures. And still, the helpers code has been modified quite a bit
during the last three years I've followed it.

I've spent last 3 years writing mostly the PMIC code - so maybe this
explains my view on the gpio_regmap design :]

It may be the GPIO HW is not _as_ bad with the quirks - but as you
propably have found out already, quite many GPIO drivers have some
quirks which do not fit the gpio_regmap even if some parts would...
This is exactly the point I am addressing here :)

Best Regards
	Matti Vaittinen


> -michael