| Message ID | cover.1622008846.git.matti.vaittinen@fi.rohmeurope.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | gpio: gpio-regmap: Support few custom operations | expand |
On Wed, May 26, 2021 at 9:02 AM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > > 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/ > > 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, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Changelog v4: > - Convert also the existing users. > > Changelog v3: > - divide gpio_regmap into private part and part which contains > user-visible configurations. This should allow keeping the internal > data internal to gpio_regmap - while allowing the IC driver to re-use > configurations it has provided to gpio-regmap without a need of > storing them to private-data. Furthermore avoid implementing dummy > 'getter-functions' for regmap, driver-data, register details, > firmware node etc. > - change devm_add_action() to devm_add_action_or_reset() > - the bd71815 GPIO driver, completely drop private-data. > > Changelog v2: > - Add cover-letter > - Drop unnecessary checks for callback function validity > - drop driver_data setting function as it is likely to be a > race-condition-by-design > > --- > > Matti Vaittinen (3): > gpio: regmap: Support few IC specific operations > gpio: gpio-regmap: Use devm_add_action_or_reset() > gpio: bd71815: Use gpio-regmap > > drivers/gpio/Kconfig | 1 + > drivers/gpio/gpio-bd71815.c | 121 ++++----------- > drivers/gpio/gpio-regmap.c | 212 +++++++++++++++----------- > drivers/gpio/gpio-sl28cpld.c | 3 +- > drivers/pinctrl/bcm/pinctrl-bcm63xx.c | 8 +- > include/linux/gpio/regmap.h | 51 +++++-- > 6 files changed, 194 insertions(+), 202 deletions(-) > > > base-commit: c4681547bcce777daf576925a966ffa824edd09d > -- > 2.25.4 > > > -- > 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 to Simon Glass for the translation =]
Am 2021-05-26 10:42, schrieb Andy Shevchenko: > On Wed, May 26, 2021 at 9:02 AM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: >> >> 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/ >> >> 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? And it doesn't help you to call back into gpio-regmap once there are further methods provided by gpio-regmap. 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. As the user, you could already attach the config to the opaque data pointer and get the same result. 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? 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. I'm still not opposed to convert gpio-regmap into helpers as mentioned earlier. But until then, I'd really keep the "struct gpio_regmap *gpio" opaque pointer. -michael
On Wed, 2021-05-26 at 11:07 +0200, Michael Walle wrote: > Am 2021-05-26 10:42, schrieb Andy Shevchenko: > > On Wed, May 26, 2021 at 9:02 AM Matti Vaittinen > > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > 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/ > > > > > > 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
Am 2021-05-26 11:44, schrieb Matti Vaittinen: > On Wed, 2021-05-26 at 11:07 +0200, Michael Walle wrote: >> Am 2021-05-26 10:42, schrieb Andy Shevchenko: >> > On Wed, May 26, 2021 at 9:02 AM Matti Vaittinen >> > <matti.vaittinen@fi.rohmeurope.com> wrote: >> > > 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/ >> > > >> > > 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. Doing something depening on the offsets of some registers sounds like a hack to me. > 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. which also sounds like a hack, where one really should provide a driver priv to distiguish between different variant. > 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? Of course, but isn't your argument to have less boilerplate? ;) And again, I don't thing the config is the correct first parameter here for the callback. And it would be different from all the other subsystems in linux (as far as I know, please correct me if I'm wrong), which have "their" (sometimes opaque, sometimes not) pointer as the first argument. >> 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. I really don't find this solution "clean". > 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. Of course, your caller has to make sure it will allocate the memory and doesn't just allocate it on the stack. You're doing the same, just in gpio-regmap. >> 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. Well yes, thats just a matter of taste. Everyone is doing platform_get_drvdata(), for example. If you want to keep something internal you'd need accessor methods. > 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. Exporting the functions is just adding two lines in gpio/regmap.h. How can this be an argument for an overhead on the users? >> 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; Yes and I actually had that during my initial development, but decided against it, to decouple the information you'll need later and some you might just discard after probe. I'm afraid, but I really don't like having the gpio_regmap_config as the first parameter on callbacks, just because I think this is the wrong approach, so I vote against this change. I guess it is up to Linus to decide on this. Don't get me wrong, I'm all open for change, but there seems to be two equal approaches to your problem, which just depends on personal taste. -michael
On Wed, 2021-05-26 at 12:27 +0200, Michael Walle wrote: > Am 2021-05-26 11:44, schrieb Matti Vaittinen: > > On Wed, 2021-05-26 at 11:07 +0200, Michael Walle wrote: > > > Am 2021-05-26 10:42, schrieb Andy Shevchenko: > > > > On Wed, May 26, 2021 at 9:02 AM Matti Vaittinen > > > > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > > > 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/ > > > > > > > > > > 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. > > Doing something depening on the offsets of some registers sounds like > a hack to me. Hm. I an unsure I understood your comment. If the hardware requires setting a bit - then we must do it using the bit offset. What we win here is that we can get the register address from config w/o allocating driver data. > > 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. > > which also sounds like a hack, where one really should provide a > driver priv to distiguish between different variant. I disagree here. Storing a IC type flag in private data does not scale well when we have many variants and many 'entities' that depend on the variant. If we just have the IC type passed to call-back, then the call-back must have all variant details hard-coded. It scales better if we can just provide the correct value for the variant - not the variant type. > > 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? > > Of course, but isn't your argument to have less boilerplate? ;) Yes. We can provide the function "gpio_regmap_get_from_config()" which returns the pointer to struct gpio_regmap - _if ever needed_ instead of providing bunch of getter functions. (That would be driver_data, regmap and firmware node already now just to fulfill the needs to the bd71815 - which probably is one of the simplest controllers). So one potentially needed function vs three that would be needed already now. > And > again, I don't thing the config is the correct first parameter here > for the callback. And it would be different from all the other > subsystems in linux (as far as I know, please correct me if I'm > wrong), > which have "their" (sometimes opaque, sometimes not) pointer as the > first argument. I only know handful of subsystems - and most of those expose the internals. That's why I suggested it at first. Andy and you both thought exposing the gpio_regmap or gpio_chip to gpio_regmap users was a bad idea - hence I suggested this solution - which actually looks good to me :) > > > > 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. > > I really don't find this solution "clean". > > > 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. > > Of course, your caller has to make sure it will allocate the memory > and doesn't just allocate it on the stack. You're doing the same, > just in gpio-regmap. Point was that this way we are not necessarily permanently allocating the memory both in the gpio_regmap _and_ the IC driver. We are also not allowing IC driver to change config stored in gpio_regmap (well, without some tricks anyways). > > > > 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. > > Well yes, thats just a matter of taste. Everyone is doing > platform_get_drvdata(), for example. If you want to keep something > internal you'd need accessor methods. > > > 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. > > Exporting the functions is just adding two lines in gpio/regmap.h. > How > can > this be an argument for an overhead on the users? > > > > 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; > > Yes and I actually had that during my initial development, but > decided > against it, to decouple the information you'll need later and some > you might just discard after probe. > > I'm afraid, but I really don't like having the gpio_regmap_config as > the first parameter on callbacks, just because I think this is the > wrong approach, so I vote against this change. I guess it is up > to Linus to decide on this. > > Don't get me wrong, I'm all open for change, but there seems to be > two > equal approaches to your problem, which just depends on personal > taste. No problem Michael - we are all allowed to have our own opinions - even the wrong ones ;) Nah, really, the discussion is good. It rarely hurts the outcome. I think I've explained my view now too. Let's see what other think :) 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)
On Wed, May 26, 2021 at 8:02 AM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> 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, 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.
Yours,
Linus Walleij
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@fi.rohmeurope.com> 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)
On Thu, May 27, 2021 at 8:32 AM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > 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. Just do it the way the maintainer likes it I guess? Michael wrote the driver so do it in his fashion. > So > at the end of the day it's fair to go on in a way Michael and You find > easiest to maintain. What makes things easy for me to maintain is active and happy driver maintainers, so it is paramount that the file looks to Michael like something he wants to keep maintaining. This removes work from me and Bartosz. Maintainer style quirks are common, I have some myself (like never allowing __underscore_functions) we just adapt to their quirks and be good diplomats. Yours, Linus Walleij
On Fri, 2021-05-28 at 02:53 +0200, Linus Walleij wrote: > On Thu, May 27, 2021 at 8:32 AM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > 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. > > Just do it the way the maintainer likes it I guess? Michael wrote > the driver so do it in his fashion. This is fair game I'd say. If there is no compromise - then it should really be in a maintainer's fashion or not at all. > > > So > > at the end of the day it's fair to go on in a way Michael and You > > find > > easiest to maintain. > > What makes things easy for me to maintain is active and happy > driver maintainers, so it is paramount that the file looks to Michael > like something he wants to keep maintaining. This removes work > from me and Bartosz. I agree. When someone takes care of a driver, he should be happy with it. Period. And thanks to Michael for writing this driver and reviewing the patches. Reviewing is hard work. On the other hand, I don't enjoy writing code I am unhappy with either. And as this particular piece of code is not a paid task for me, I do this for fun. gpio-regmap is not mandatory for my drivers now. So, I'll just opt-out from this change. I'll happily use the gpio-regmap where it fits, when it fits. So, during the last cycle I promised to look the gpio-regmap usage on ROHM IC drivers: Currently it does not fit for the BD71815 as it lack support for init_valid_mask and set_config. It does not work for the BD71828 either due to lack of set_config and special handling of HALL_GPIO. Regarding the BD70528 - with my heart bleeding I just sent a set of patches to remove this IC's drivers completely. It seems the IC never really took off. The BD9571 was not authored by me and I don't have that IC - but at quick glance it seems that driver might work with gpio-regmap. Conversion would require some testing. > Maintainer style quirks are common, I have some myself (like > never allowing __underscore_functions) I like this. The Linux kernel community is well known for strict and enforced styling guide. I've also worked in an another pretty huge project, where every developer was pretty much allowed to use their own style. I think it resulted happier developers and definitely made code- readers much more careful ;) I'd claim that made people to pay more attention when reading code - you could _never_ safely assume. It was also somehow funny that at times one was able to recognize a file author just by looking the code :) Well, I would not suggest that to the Linux kernel - but I definitely like allowing few maintainer quirks here and there. And I am definitely hoping to see happy maintainers. Even though the __foo() for internal functions is the right thing to do (tm) ;) > we just adapt to their > quirks and be good diplomats. Or keep whingeing and acting up - depending on the person XD Keep up the good work you all :) Best Regards --Matti
On Fri, May 28, 2021 at 8:33 AM Vaittinen, Matti <Matti.Vaittinen@fi.rohmeurope.com> wrote: > [snip] > > > > What makes things easy for me to maintain is active and happy > > driver maintainers, so it is paramount that the file looks to Michael > > like something he wants to keep maintaining. This removes work > > from me and Bartosz. > > I agree. When someone takes care of a driver, he should be happy with > it. Period. And thanks to Michael for writing this driver and reviewing > the patches. Reviewing is hard work. > > On the other hand, I don't enjoy writing code I am unhappy with either. > And as this particular piece of code is not a paid task for me, I do > this for fun. gpio-regmap is not mandatory for my drivers now. So, I'll > just opt-out from this change. I'll happily use the gpio-regmap where > it fits, when it fits. > I take it that path 2/3 is still good to go? Bart [snip]
Hi Bartosz, On Fri, 2021-05-28 at 16:31 +0200, Bartosz Golaszewski wrote: > On Fri, May 28, 2021 at 8:33 AM Vaittinen, Matti > <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > [snip] > > > > What makes things easy for me to maintain is active and happy > > > driver maintainers, so it is paramount that the file looks to > > > Michael > > > like something he wants to keep maintaining. This removes work > > > from me and Bartosz. > > > > I agree. When someone takes care of a driver, he should be happy > > with > > it. Period. And thanks to Michael for writing this driver and > > reviewing > > the patches. Reviewing is hard work. > > > > On the other hand, I don't enjoy writing code I am unhappy with > > either. > > And as this particular piece of code is not a paid task for me, I > > do > > this for fun. gpio-regmap is not mandatory for my drivers now. So, > > I'll > > just opt-out from this change. I'll happily use the gpio-regmap > > where > > it fits, when it fits. > > > > I take it that path 2/3 is still good to go? I don't think it had explicit ack from Michael yet - but I think it was not objected either. I can respin it alone if needed but would help me if you just pick it from this series (assuming it's Ok for others). Best Regards Matti Vaittinen
Hi Matti, Am 2021-05-28 17:42, schrieb Vaittinen, Matti: > Hi Bartosz, > > On Fri, 2021-05-28 at 16:31 +0200, Bartosz Golaszewski wrote: >> On Fri, May 28, 2021 at 8:33 AM Vaittinen, Matti >> <Matti.Vaittinen@fi.rohmeurope.com> wrote: >> >> [snip] >> >> > > What makes things easy for me to maintain is active and happy >> > > driver maintainers, so it is paramount that the file looks to >> > > Michael >> > > like something he wants to keep maintaining. This removes work >> > > from me and Bartosz. >> > >> > I agree. When someone takes care of a driver, he should be happy >> > with >> > it. Period. And thanks to Michael for writing this driver and >> > reviewing >> > the patches. Reviewing is hard work. >> > >> > On the other hand, I don't enjoy writing code I am unhappy with >> > either. >> > And as this particular piece of code is not a paid task for me, I >> > do >> > this for fun. gpio-regmap is not mandatory for my drivers now. So, >> > I'll >> > just opt-out from this change. I'll happily use the gpio-regmap >> > where >> > it fits, when it fits. >> > >> >> I take it that path 2/3 is still good to go? > > I don't think it had explicit ack from Michael yet - but I think it was > not objected either. I can respin it alone if needed but would help me > if you just pick it from this series (assuming it's Ok for others). Just sent my R-b. I'd pick the removal of the gpio_regmap_set_drvdata(), too. If you're fine with it I'd submit a new patch with just that. -michael
On Fri, 2021-05-28 at 19:23 +0200, Michael Walle wrote: > Hi Matti, > > Am 2021-05-28 17:42, schrieb Vaittinen, Matti: > > Hi Bartosz, > > > > On Fri, 2021-05-28 at 16:31 +0200, Bartosz Golaszewski wrote: > > > On Fri, May 28, 2021 at 8:33 AM Vaittinen, Matti > > > <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > > > > > I take it that path 2/3 is still good to go? > > > > I don't think it had explicit ack from Michael yet - but I think it > > was > > not objected either. I can respin it alone if needed but would help > > me > > if you just pick it from this series (assuming it's Ok for others). > > Just sent my R-b. > > I'd pick the removal of the gpio_regmap_set_drvdata(), too. If you're > fine with it I'd submit a new patch with just that. Right. Besides the 2/3 might require the 1/3 to apply cleanly. I somehow thought I sent the devm_add_action_or_reset() as first change in the series. Feel free to rework the patches as the best fits your purposes :) Best Regards Matti