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@xxxxxxxxxxxxxxxxx> 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
