On Thu, Jul 20, 2017 at 5:55 AM, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> 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
