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

[Michael Walle <michael@walle.cc>]

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.

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 kind of fail to see your point Andy. Or yes, I know exposing the
> gpio_chip to user allows much more flexibility. But what are the
> options? What would a driver developer do when his HW does almost fir
> the standard regmap_gpio - but not just quite? Say that for example the
> changing of gpio direction requires some odd additional register access
> - but other than that the regmap_gpio operations like setting/getting
> the value, IRQ options etc. fitted the regmap_gpio logic.
> 
> If he can not override this one function - then he will need to write
> wholly new GPIO driver without re-using any of the regmap-gpio stuff.
> You know, if one can't use regmap-gpio, he's likely to use the already
> exposed gpio_chip anyways. I'd say this is much more of a pain to
> maintain. Or maybe you add another work-around option in the
> gpio_regmap_config to indicate this (and every other) oddball HW -
> which eventually leads to a mess.

Agreed, if possible, I'd not like to see options just for one
obscure HW.

> 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?

-michael