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

From: Michael Walle <michael@walle.cc>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: "Matti Vaittinen" <matti.vaittinen@fi.rohmeurope.com>,
	"Matti Vaittinen" <mazziesaccount@gmail.com>,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Bartosz Golaszewski" <bgolaszewski@baylibre.com>,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	bcm-kernel-feedback-list <bcm-kernel-feedback-list@broadcom.com>,
	"Jonas Gorski" <jonas.gorski@gmail.com>,
	"Álvaro Fernández Rojas" <noltari@gmail.com>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	linux-power <linux-power@fi.rohmeurope.com>,
	"linux-arm Mailing List" <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations
Date: Wed, 26 May 2021 11:07:51 +0200	[thread overview]
Message-ID: <2e201aabd9b42da9a2bdcb2f7504ec12@walle.cc> (raw)
In-Reply-To: <CAHp75VeHZg1DC76sg1F-=49SfVLNhf4pG7ArcXHxjU0nXZOpWw@mail.gmail.com>

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

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