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

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.

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.

What about reset_control_get(struct device *, const char *, int flags)
to replace all those variants ?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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.

regards
Philipp

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