Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations

[Matti Vaittinen]

On Thu, 2021-05-27 at 00:46 +0200, Linus Walleij wrote:
> On Wed, May 26, 2021 at 8:02 AM Matti Vaittinen
> <matti.vaittinen@xxxxxxxxxxxxxxxxx> wrote:
> 
> > Support providing some IC specific operations at gpio_regmap
> > registration.
> 
> I see there is some discussion around the abstractions here.
> 
> I can only say how we designed gpio-mmio.c (CONFIG_GPIO_GENERIC).
> 
> It was designed for GPIO controllers with 8, 16 or 32 bits of GPIO,
> each stuffed in a consecutive bit in a set of registers. We later
> amended it to deal with bigendian as well, and 64 bit registers,
> and some quirks around the registers (like just readable etc).
> 
> But that's it. For anything more complex we have opted for
> users to write their own drivers with elaborate code.
> 
> As library it can sometimes be combined with an irqchip,
> if the interrupts are simple.
> 
> But overall: each GPIO needs to be a single bit, not 2 not 3
> not in every 7th register etc.
> 
> I would not try to turn gpio regmap into a Rube Goldberg Machine
> panacea-fit-all for all kinds of register and bit layouts, 

This is exactly the point of this patch series. The goal is that
complex hardware is handled by IC specific drivers. But at the same
time, hardware which has 'standard parts' and 'complex parts' could re-
use the well-defined gpio_regmap parts instead of duplicating that code
in IC driver. As far as I understand we do agree with Michael about the
benefits of this overall idea. Current patch only allows IC specific
bits for init_valid_mask and set_config - but I personally see no
problem in expanding this if needed. Place to add more callbacks would
be there - whether to add something or not can then be evaluated case
by case.

I think that the disagreement boils down to few styling issues - and
one more pragmatic one. And only what comes to how we allow
implementing the IC specific call-backs for these more complex HW
specific cases. "Styling" boils down to providing getter-functions for
well-defined gpio_regmap properties like regmap, device and fwnode
pointers Vs. exposing these in a well-defined structure as function
parameters.

The more pragmatic question is how the IC specific bits are provided to
the callbacks. My approach would be allowing IC drivers to use the
existing regmap_gpio config structure as I think in many cases that
would be enough and IC drivers could omit the driver_data, like gpio-
bd71815.c could do. I guess I could go through some of the existing
drivers and see if others would benefit from this too. The other option
I see is to force the IC driver to always allocate drvdata if it needs
any HW information in the callbacks - no matter if this is same
information it has already passed to gpio-regmap in the config. 

> it's nice to
> be able to combine it with an interrupt chip or pin controller if
> those
> functions are also simple, like the set/get registers.
> 
> Any too bold ambitions will be hard to maintain, I think.

In my eyes maintaining drivers which all allocate own set of private
data gets much more messy than maintaining set of drivers most of which
use same predefined config struct and only allocate drvdata if really
needed. After that being said - I am not the one maintaining this :] So
at the end of the day it's fair to go on in a way Michael and You find
easiest to maintain. It is just my view that this series would be the
way to allow many of the IC drivers using regmap to enjoy the benefits
of the gpio-regmap - while still maintaining the formal structure. 

Please, let me know if you think I should see how much this would drop
the code from some of the existing GPIO drivers. It makes no sense to
invest more time in this if others are 100% against it.

Best Regards
	Matti Vaittinen

--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)