Re: [PATCH v2 1/3] gpio: regmap: Support few IC specific operations

["Vaittinen, Matti" <Matti.Vaittinen@xxxxxxxxxxxxxxxxx>]

On Fri, 2021-05-21 at 12:46 +0200, Michael Walle wrote:
> Am 2021-05-21 12:25, schrieb Vaittinen, Matti:
> > On Fri, 2021-05-21 at 12:19 +0200, Michael Walle wrote:
> > > Am 2021-05-21 12:09, schrieb Andy Shevchenko:
> > > > On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen
> > > > <matti.vaittinen@xxxxxxxxxxxxxxxxx> wrote:
> > > > > Changelog v2: (based on suggestions by Michael Walle)
> > > > >   - drop gpio_regmap_set_drvdata()
> > > > 
> > > > But why do we have gpio_regmap_get_drvdata() and why is it
> > > > different
> > > > now to the new member handling?
> > > 
> > > Eg. the reg_mask_xlate() callback is just passed a "struct
> > > gpio_regmap*".
> > > If someone needs to access private data there,
> > > gpio_regmap_get_drvdata()
> > > is used. At least that was its intention.
> > 
> > I would help the IC driver here too and just directly provide the
> > drvdata pointer as argument. I don't see much value in providing
> > the
> > regmap_gpio pointer as IC driver can not dereference it.
> 
> What is it with the "it's useless if one cannot dereference it"?
> You're
> also passing "struct regmap *" which you cannot dereference. It's an
> opaque pointer you need to pass to gpio_regmap to call a function
> there.
> 
> What is the problem with letting gpio_regmap derefence its internal
> data
> structure and return the value for you?
> 
> Adding the drvdata to reg_mask_xlate() highlights my former concern;
> you
> need to keep chaning the users to add another parameter. What if
> xlate()
> needs the regmap, too? Then you need to change it again. Granted this
> is
> a silly example, but you should get my point. It is by far more easy
> to

I don't think it's a silly example. 

What I am not liking here is always adding a dummy function for getting
a regmap_gpio members which are likely to be needed by all users.
(drvdata, regmap). It makes IC drivers more complicated when they need
to call a function just to get a member value. Sure this adds some
flexibility what comes to potential changes - but it only is beneficial
if the changes ever happen. Else these getters and setters are just
boilerplate.

> just add another new gpio_regmap_*(struct gpio_regmap *) function and
> you don't have to change existing users.

The regmap is not a problem as it is in any case coming to the gpio-
regamp from the IC driver. The IC driver can store regmap in drvdata.
(Sure it would be then stored in two places - gpio_regmap and drvdata).

> Also what if gpio_regmap provides some useful helper function in the
> future, it will likely need its internal data struct.

That is is a valid point. As I said, I just don't like the idea that
things which are likely to be used must be fetched via an API call.
Those should be provided as such. But yes, it does not mean that there
would never be case(s) where the gpio_regmap should not be used as a
handle.

Thanks for the patience while explaining me your point Michael :) I'll
ponder this for a lil while before creating a new version (probably at
next week).

Best Regards
	Matti Vaittinen