On Wed, 2021-05-26 at 11:07 +0200, Michael Walle wrote:

Am 2021-05-26 10:42, schrieb Andy Shevchenko:

quoted

On Wed, May 26, 2021 at 9:02 AM Matti Vaittinen
[off-list ref] wrote:

quoted

Support providing some IC specific operations at gpio_regmap 
registration.

Implementation of few GPIO related functionalities are likely to
be
very IC specific. For example the pin-configuration registers and
the
pin validity checks. Allow IC driver to provide IC specific
functions
which gpio-regmap can utilize for these IC specific
configurations.
This should help broaden the gpio-regmap IC coverage without the
need
of exposing the registered gpio_chip or struct gpio_regmap to IC 
drivers.

The set_config and init_valid_mask are used by ROHM BD71815 GPIO 
driver.
Convert the BD71815 GPIO driver to use gpio-regmap and get rid of
some
code. Rest of the ROHM GPIO drivers are to be reworked after the
mechanism of adding IC specific functions is settled.

Some preliminary discussion can be seen here:
https://lore.kernel.org/linux-gpio/c4faac648d3e0c7f3dcb50f7e24c8b322e8c6974.camel@fi.rohmeurope.com/ (local)

I did also prepare change where the getters for drvdata and
regmap
are used. It can also work - but it does not scale quite as well
if (when) IC drivers need some register information to do custom
operations. Interested people can see the:
https://github.com/M-Vaittinen/linux/commits/gpio-regmap-getters
for comparison.

Entire series looks good to me,

Sorry, for being late to this. I got sidetracked.

TBH, I don't like the we have the config struct in the callbacks. Why
would you need all this information in the callback?

I believe there will be cases when the register information is needed
in callbacks. I don't know the GPIO controllers in details so that I
could give you an real-word example. I guess other people on the list
know the usual GPIO quirks far better than I do. I however have seen
bunch of hardware - and usually each IC has _some_ strange stuff. I
would be surprized if there weren't any cases where the one operation
"toggle X" would not require access to another register which is used
to control "feature Y" - and usually only once in a blue moon. Purely
imaginatory example could be that in order to change direction to
input, one would need to ensure some bit in a output configuration
register is cleared. Then it would be beneficial to have the register
description in call-back.

Or, if we look at the pinctrl-bcm63xx.c - another imaginatory case - we
would get another HW variant with different BCM63XX_BANK_GPIOS value.
Now the IC would not need to store the correct BCM63XX_BANK_GPIOS in
driver data for the xlate-callback - it could directly read the
ngpio_per_reg from config.

As I said, these cases are imaginatory - I don't know the GPIO
controllers well enough to give real-world examples - but I am positive
there are such.

 And it doesn't
help you to call back into gpio-regmap once there are further methods
provided by gpio-regmap.

If we later need this we can use container_of(), right?

Either we hide away the internals completely (which I still prefer!)
or
we open up the gpio_regmap struct. But this is somewhere in between. 

Yes. And I think this is the simplest and cleanest solution which still
provides decent amount of protection, while cuts off the boilerplate.
Additionally this does not add any extra structures because IC drivers
already know the config. Some gpio_regmap internals (like gpio_chip)
can still be kept internal - while config (which in any case is
populated by the IC driver) is public.

As
the user, you could already attach the config to the opaque data
pointer
and get the same result.

Actually no. This would require user to permanently store the config in
memory which would either duplicate the config or give IC driver a
pointer to gpio_regmap internals. This solution still gives pointer to
gpio_regmap config - but at least we can set it const in function
parameters.

I don't see how the following is an overhead:

int gpio_regmap_callback(struct gpio_regmap *gpio, ..)
{
     struct regmap *regmap = gpio_regmap_get_regmap(gpio);
     struct driver_priv *data = gpio_regmap_get_drvdata(gpio);
     ...
}

It doesn't clutter anything, there is just a small runtime overhead
(is
it?). Again this let you keep adding stuff in the future without
changing any users. So what are the drawbacks of this?

It still is overhead. Additionally, I dislike mixing function calls
with declarations - I know that's probably just my personal preference
though. And what is not shown here is the need to declare, define and
export these functions from gpio_regmap. And this is really just
unnecessary boilerplate to me.

Also I'd like to keep the duplication of the "struct gpio_regmap" 
members
and the config members. The gpio_regmap_config is just a struct so
the _register won't get cluttered with arguments.

The config (as passed from IC driver at register) is dublication. We
do:
gpio->config = *config;

It's just not all meld in the same level.

Best Regards
	Matti Vaittinen




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel