Re: regmap-gpio: Support set_config and other not quite so standard ICs?

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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.

> Current IF makes it very hard (impossible?) for driver to override any
> of the regmap_gpio functions (or provide own alternatives) for cases
> which do not fit the generic regmap_gpio model.
>
> My first obstacle is providing gpio_chip.set_config for BD71815.
>
> 1) I guess the method fitting current design would be adding drive-mode
> register/mask(s) to the gpio_regmap_config. Certainly doable - but I
> have a bad feeling of this approach. I am afraid this leads to bloating
> the gpio_regmap_config with all kinds of IC specific workarounds (when
> HW designers have invented new cool control registers setups) - or then
> just not using the regmap_gpio for any ICs which have any quirks - even
> if 90% of regmap_gpio logic would fit...
>
> 2) Other possibility is allowing IC driver to provide function pointers
> for some operations (in my case for example for the set_config) - if
> the default operation the regmap_gpio provides does not fit the IC.
> This would require the regmap_gpio to be visible to IC drivers so that
> IC drivers can access the regmap, device & register information - or
> some way to convert the gpio_chip pointer to IC specific private data
> pointer. Doable but still slightly bloat.
>
> 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'd like to hear opinions - should I draft some changes according to
> these proposals (which one, 1,2,3 or something else?) - or as this all
> been already discussed and am I just missing something?
>
> Best Regards
>         Matti Vaittinen



-- 
With Best Regards,
Andy Shevchenko