Re: [PATCH] pwm: add devm_pwm_get() and devm_pwm_put()

[Alex Courbot]

On Wed 01 Aug 2012 05:04:53 PM JST, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Wed, Aug 01, 2012 at 04:37:09PM +0900, Alexandre Courbot wrote:
> > Add resource managed variants of pwm_get() and pwm_put() for
> > convenience. Code is largely inspired by the equivalent devm functions
> > of the regulator framework.
> >
> > Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
> > ---
> >   Documentation/driver-model/devres.txt |  4 +++
> >   Documentation/pwm.txt                 |  5 +--
> >   drivers/pwm/core.c                    | 57 +++++++++++++++++++++++++++++++++++
> >   include/linux/pwm.h                   |  3 ++
> >   4 files changed, 67 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
> > index 950856b..43cff70 100644
> > --- a/Documentation/driver-model/devres.txt
> > +++ b/Documentation/driver-model/devres.txt
> > @@ -284,3 +284,7 @@ CLOCK
> >   PINCTRL
> >     devm_pinctrl_get()
> >     devm_pinctrl_put()
> > +
> > +PWM
> > +  devm_pwm_get()
> > +  devm_pwm_put()
> > diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
> > index 554290e..4719c12 100644
> > --- a/Documentation/pwm.txt
> > +++ b/Documentation/pwm.txt
> > @@ -35,8 +35,9 @@ Using PWMs
> >   Legacy users can request a PWM device using pwm_request() and free it
> >   after usage with pwm_free().
> >
> > -New users should use the pwm_get() function and pass to it the consumer
> > -device or a consumer name. pwm_put() is used to free the PWM device.
> > +New users should use the pwm_get() or devm_pwm_get() function and pass to it the
> > +consumer device or a consumer name. pwm_put() or devm_pwm_put() is used to free
> > +the PWM device.
>
> I think I'd prefer the original text with an additional line mentioning
> the managed variants.
>
> >   After being requested a PWM has to be configured using:
> >
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index ecb7690..5a5e19b 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -624,6 +624,63 @@ out:
> >   }
> >   EXPORT_SYMBOL_GPL(pwm_put);
> >
> > +static void devm_pwm_release(struct device *dev, void *res)
> > +{
> > +	pwm_put(*(struct pwm_device **)res);
> > +}
> > +
> > +/**
> > + * devm_pwm_get() - Resource managed pwm_get()
>
> This is missing parameter descriptions here.

This is because the documentation for this function just refers to 
pwm_get(), and the parameters are the same. I think it's better to avoid 
duplicating documentation as much as possible as this makes two 
maintenance points instead of one.

> > + *
> > + * This works are pwm_get() but the acquired pwm will automatically be released
>
> "works like"? Also "acquired PWM" or "acquired PWM device".

Ooops, yes. I really should proof-read what I write. Twice.

> > + * on driver detach. See pwm_get() for more details.
> > + */
> > +struct pwm_device *devm_pwm_get(struct device *dev, const char *consumer)
> > +{
> > +	struct pwm_device **ptr, *pwm;
> > +
> > +	ptr = devres_alloc(devm_pwm_release, sizeof(**ptr), GFP_KERNEL);
> > +	if (!ptr)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	pwm = pwm_get(dev, consumer);
> > +	if (!IS_ERR(pwm)) {
> > +		*ptr = pwm;
> > +		devres_add(dev, ptr);
> > +	} else {
> > +		devres_free(ptr);
> > +	}
> > +
> > +	return pwm;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_pwm_get);
> > +
> > +static int devm_pwm_match(struct device *dev, void *res, void *data)
> > +{
> > +	struct pwm_device **p = res;
> > +
> > +	if (WARN_ON(!p || !*p))
> > +		return 0;
> > +
> > +	return *p == data;
> > +}
> > +
> > +/**
> > + * devm_pwm_put() - Resource managed pwm_put()
> > + *
> > + * Releases a pwm previously allocated using devm_pwm_get. Calling this function
> > + * is usually not needed as the devm-allocated pwm will automatically be
> > + * released on driver detach.
> > + */
>
> Same comments as for devm_pwm_get().
>
> > +void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
> > +{
> > +	int ret;
> > +
> > +	ret = devres_release(dev, devm_pwm_release, devm_pwm_match, pwm);
> > +	WARN_ON(ret);
> > +}
> > +EXPORT_SYMBOL_GPL(devm_pwm_put);
> > +
> >   #ifdef CONFIG_DEBUG_FS
> >   static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
> >   {
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index 21d076c..af9c39a 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -123,7 +123,10 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
> >   					 const char *label);
> >
> >   struct pwm_device *pwm_get(struct device *dev, const char *consumer);
> > +struct pwm_device *devm_pwm_get(struct device *dev, const char *consumer);
> > +
> >   void pwm_put(struct pwm_device *pwm);
> > +void devm_pwm_put(struct device *dev, struct pwm_device *pwm);
>
> Can the managed variants be grouped together, please?

Sure.

Thanks,
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/