Re: [PATCH] drivers: Remove unused devm_*_put functions

[Josh Triplett <josh@joshtriplett.org>]

On Mon, Jan 06, 2014 at 04:48:39PM +0900, Alexandre Courbot wrote:
> On Sat, Jan 4, 2014 at 5:22 AM, Josh Triplett <josh@joshtriplett.org> wrote:
> > On Fri, Jan 03, 2014 at 08:07:22PM +0900, Alexandre Courbot wrote:
> >> On Thu, Jan 2, 2014 at 10:07 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> >> > On Sat, Dec 21, 2013 at 11:49 AM, Rashika Kheria
> >> > <rashika.kheria@gmail.com> wrote:
> >> >> -/**
> >> >> - * devm_gpiod_put - Resource-managed gpiod_put()
> >> >> - * @desc:      GPIO descriptor to dispose of
> >> >> - *
> >> >> - * Dispose of a GPIO descriptor obtained with devm_gpiod_get() or
> >> >> - * devm_gpiod_get_index(). Normally this function will not be called as the GPIO
> >> >> - * will be disposed of by the resource management code.
> >> >> - */
> >> >> -void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
> >> >> -{
> >> >> -       WARN_ON(devres_release(dev, devm_gpiod_release, devm_gpiod_match,
> >> >> -               &desc));
> >> >> -}
> >> >> -EXPORT_SYMBOL(devm_gpiod_put);
> >> >
> >> > Alexandre what do you think about this? Can we think of a scenario
> >> > where explicit garbage collection is going to be needed or should we
> >> > remove this for now?
> >>
> >> Sorry for the delayed reply, I quickly saw the patch and then the
> >> holidays got in the way. :)
> >>
> >> I think all these functions should be kept. It is true that they are
> >> seldomly used, and that the purpose of devm is to garbage-collect
> >> resources upon driver removal, but they might (actually, probably
> >> will) become needed by someone at some point in the future. One
> >> example I can think of is two drivers that collaborate to share the
> >> same GPIO line. If they acquire the GPIO through devm_gpiod_get() they
> >> will need devm_gpiod_put() to release it so the other driver can
> >> acquire it.
> >>
> >> On a more general note, devm_clk_put(), devm_regulator_put(),
> >> devm_pinctrl_put() and probably others devm_*_put() functions are
> >> actively used in the kernel, to  support the idea that a devm removal
> >> function makes sense. That not all the subsystem-provided functions
> >> are used by mainline drivers does not necessary mean they should be
> >> removed, especially if they serve a purpose. We should keep our APIs
> >> consistent and future-proof, not to mention out-of-tree drivers that
> >> may use them.
> >>
> >> So this patch is a nack as far as I'm concerned, not only the GPIO
> >> part, but the whole of it.
> >
> > As far as I can tell, the few calls to the devm_*_put functions I can
> > see look like unnecessary calls as part of driver/device
> > uninitialization, and could be removed either directly or with minor
> > reworking.  That there are only 1-2 calls to each in the entire kernel
> > is also quite telling.
> >
> > In any case, it's disappointing to have a pile of unused functions in
> > the kernel on the theory that they *might* be needed; it's not like it'd
> > be hard to retrieve them from git if they're ever needed.
> 
> My motivation is mainly API consistency ; and these functions are
> small enough to not worry too much about them IMHO. If we *really*
> want to get rid of them when they are unneeded, how about defining
> them as static inline in the corresponding header file? Since they are
> short and seldomly used (when used at all), this may be an acceptable
> compromise.

That seems quite reasonable.

> > However, if you're insistent on keeping them, it'd be easy enough to
> > provide patches to include an appropriate header with prototypes for
> > them instead, which would also eliminate the warning this patch series
> > set out to eliminate.
> 
> The patch did not mention any warning, unless I missed something.
> Would be nice to explicitly mention it in the commit message.

This patch and the others like it are seeking to eliminate
-Wmissing-prototypes warnings from GCC and -Wdecl warnings from Sparse,
caused by having functions that are neither static nor prototyped in
advance.

- Josh Triplett