Nouveau - Jul 2017 - [PATCH 000/102] Convert drivers to explicit reset API

On Thu, Jul 20, 2017 at 5:55 AM, Philipp Zabel <p.zabel at pengutronix.de>
wrote:
> Hi Thomas,
>
> On Thu, 2017-07-20 at 12:36 +0200, Thomas Petazzoni wrote:
>> Hello,
>>
>> On Thu, 20 Jul 2017 11:36:55 +0200, Philipp Zabel wrote:
>>
>> > > I don't know if it has been discussed in the past, so
forgive me if it
>> > > has been. Have you considered adding a "int flags"
argument to the
>> > > existing reset_control_get_*() functions, rather than
introducing
>> > > separate exclusive variants ?
>> > >
>> > > Indeed, with a "int flags" argument you could in
the future add more
>> > > variants/behaviors without actually multiplying the number of
>> > > functions. Something like the "flags" argument for
request_irq() for
>> > > example.
>> >
>> > I can't find the discussion right now, but I remember we had
talked
>> > about this in the past.
>> > Behind the scenes, all the inline API functions already call
common
>> > entry points with flags (well, currently separate bool parameters
for
>> > shared and optional).
>> > One reason against exposing those as an int flags in the user
facing API
>> > is the possibility to accidentally provide a wrong value.
>>
>> This is a quite strange argument. You could also accidentally use the
>> wrong variant of the function, just like you could use the wrong flag.
>
> You can't accidentally use no flag at all or a completely bogus value
> with the "plethora of inline functions" variant.
>
>> Once again, the next time you have another parameter for those reset
>> functions, beyond the exclusive/shared variant, you will multiply again
>> by two the number of functions ? You already have the  exclusive/shared
>> and optional/mandatory variants, so 4 variants. When you'll add a
new
>> parameter, you'll have 8 variants. Doesn't seem really good.
>
> I'd rather avoid adding more variants, if possible. The complexity
> increases regardless of whether the API is expressed as a bunch of
> functions or as a single function with a bunch of flags.
>
>> What about reset_control_get(struct device *, const char *, int flags)
>> to replace all those variants ?
>
> While I like how this looks, unfortunately (devm_)reset_control_get
> already exists without the flags, so we can't change to that with a
> gentle transition.
This was done for gpiod_get() and its flags argument with horrifying
#define-ry, which thankfully was completely hidden from users.

-- 
Dmitry

On Thu, Jul 20, 2017 at 10:46 PM, Dmitry Torokhov
<dmitry.torokhov at gmail.com> wrote:
> On Thu, Jul 20, 2017 at 5:55 AM, Philipp Zabel <p.zabel at
pengutronix.de> wrote:
>>> What about reset_control_get(struct device *, const char *, int
flags)
>>> to replace all those variants ?
>>
>> While I like how this looks, unfortunately (devm_)reset_control_get
>> already exists without the flags, so we can't change to that with a
>> gentle transition.
>
> This was done for gpiod_get() and its flags argument with horrifying
> #define-ry, which thankfully was completely hidden from users.
For your reference:

commit bae48da237fcedd7ad09569025483b988635efb7
"gpiolib: add gpiod_get() and gpiod_put() functions"

commit 39b2bbe3d715cf5013b5c48695ccdd25bd3bf120
"gpio: add flags argument to gpiod_get*() functions"

commit 0dbc8b7afef6e4fddcfebcbacbeb269a0a3b06d5
"gpio: move varargs hack outside #ifdef GPIOLIB"

commit b17d1bf16cc72a374a48d748940f700009d40ff4
"gpio: make flags mandatory for gpiod_get functions"

Retrospectively ... was that really a good idea... it was a LOT
of trouble to add a flag, maybe it had been better to try and
just slam all users in a single go.

But it worked.

Yours,
Linus Walleij

On Sun, 2017-07-23 at 20:41 +0200, Linus Walleij wrote:
> On Thu, Jul 20, 2017 at 10:46 PM, Dmitry Torokhov
> <dmitry.torokhov at gmail.com> wrote:
> > On Thu, Jul 20, 2017 at 5:55 AM, Philipp Zabel <p.zabel at
pengutronix.de> wrote:
> 
> >>> What about reset_control_get(struct device *, const char *,
int flags)
> >>> to replace all those variants ?
> >>
> >> While I like how this looks, unfortunately
(devm_)reset_control_get
> >> already exists without the flags, so we can't change to that
with a
> >> gentle transition.
> >
> > This was done for gpiod_get() and its flags argument with horrifying
> > #define-ry, which thankfully was completely hidden from users.
> 
> For your reference:
> 
> commit bae48da237fcedd7ad09569025483b988635efb7
> "gpiolib: add gpiod_get() and gpiod_put() functions"
> 
> commit 39b2bbe3d715cf5013b5c48695ccdd25bd3bf120
> "gpio: add flags argument to gpiod_get*() functions"
> 
> commit 0dbc8b7afef6e4fddcfebcbacbeb269a0a3b06d5
> "gpio: move varargs hack outside #ifdef GPIOLIB"
> 
> commit b17d1bf16cc72a374a48d748940f700009d40ff4
> "gpio: make flags mandatory for gpiod_get functions"
> 
> Retrospectively ... was that really a good idea... it was a LOT
> of trouble to add a flag, maybe it had been better to try and
> just slam all users in a single go.
> 
> But it worked.
Thanks for the hint and the references. It seems this turned out okay,
but I wouldn't dare to introduce such macro horror^Wmagic.
I'd rather have all users converted to the _exclusive/_shared function
calls and maybe then replace the internal __reset_control_get with
Thomas' suggestion.

regards
Philipp