[PATCH] rtc: add devm_rtc_device_{register,unregister}()

[PATCH] rtc: add devm_rtc_device_{register,unregister}()

[Jingoo Han]

These functios allows the driver core to automatically clean up
any allocation made by rtc drivers. Thus, it simplifies the error
paths.

Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
 drivers/rtc/class.c |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/rtc.h |    6 ++++
 2 files changed, 78 insertions(+), 0 deletions(-)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 9b742d3..bd3c8ca 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -259,6 +259,78 @@ void rtc_device_unregister(struct rtc_device *rtc)
 }
 EXPORT_SYMBOL_GPL(rtc_device_unregister);
 
+static void devm_rtc_device_release(struct device *dev, void *res)
+{
+	struct rtc_device *rtc = *(struct rtc_device **)res;
+
+	rtc_device_unregister(rtc);
+}
+
+static int devm_rtc_device_match(struct device *dev, void *res, void *data)
+{
+	struct rtc **r = res;
+	if (!r || !*r) {
+		WARN_ON(!r || !*r);
+		return 0;
+	}
+	return *r == data;
+}
+
+/**
+ * devm_rtc_device_register - resource managed rtc_device_register()
+ * @name: the name of the device
+ * @dev: the device to register
+ * @ops: the rtc operations structure
+ * @owner: the module owner
+ *
+ * Managed rtc_device_register(). The rtc_device returned from this function
+ * are automatically freed on driver detach. See rtc_device_register()
+ * for more information.
+ */
+
+struct rtc_device *devm_rtc_device_register(const char *name,
+					struct device *dev,
+					const struct rtc_class_ops *ops,
+					struct module *owner)
+{
+	struct rtc_device **ptr, *rtc;
+
+	ptr = devres_alloc(devm_rtc_device_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	rtc = rtc_device_register(name, dev, ops, owner);
+	if (!IS_ERR(rtc)) {
+		*ptr = rtc;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return rtc;
+}
+EXPORT_SYMBOL_GPL(devm_rtc_device_register);
+
+/**
+ * devm_rtc_device_unregister - resource managed devm_rtc_device_unregister()
+ * @dev: the device to unregister
+ * @rtc: the RTC class device to unregister
+ *
+ * Deallocated a rtc allocated with devm_rtc_device_register(). Normally this
+ * function will not need to be called and the resource management code will
+ * ensure that the resource is freed.
+ */
+void devm_rtc_device_unregister(struct device *dev, struct rtc_device *rtc)
+{
+	int rc;
+
+	rc = devres_release(dev, devm_rtc_device_release,
+				devm_rtc_device_match, rtc);
+	if (rc != 0)
+		WARN_ON(rc);
+}
+EXPORT_SYMBOL_GPL(devm_rtc_device_unregister);
+
 static int __init rtc_init(void)
 {
 	rtc_class = class_create(THIS_MODULE, "rtc");
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 580b24c..d955768 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -133,7 +133,13 @@ extern struct rtc_device *rtc_device_register(const char *name,
 					struct device *dev,
 					const struct rtc_class_ops *ops,
 					struct module *owner);
+extern struct rtc_device *devm_rtc_device_register(const char *name,
+					struct device *dev,
+					const struct rtc_class_ops *ops,
+					struct module *owner);
 extern void rtc_device_unregister(struct rtc_device *rtc);
+extern void devm_rtc_device_unregister(struct device *dev,
+					struct rtc_device *rtc);
 
 extern int rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm);
 extern int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm);
-- 
1.7.2.5

Re: [PATCH] rtc: add devm_rtc_device_{register,unregister}()

[Andrew Morton]

On Mon, 25 Feb 2013 16:35:53 +0900
Jingoo Han <jg1.han@samsung.com> wrote:

> These functios allows the driver core to automatically clean up
> any allocation made by rtc drivers. Thus, it simplifies the error
> paths.
> 
> ...
>
> @@ -259,6 +259,78 @@ void rtc_device_unregister(struct rtc_device *rtc)
>  }
>  EXPORT_SYMBOL_GPL(rtc_device_unregister);
>  
> +static void devm_rtc_device_release(struct device *dev, void *res)
> +{
> +	struct rtc_device *rtc = *(struct rtc_device **)res;
> +
> +	rtc_device_unregister(rtc);
> +}
> +
> +static int devm_rtc_device_match(struct device *dev, void *res, void *data)
> +{
> +	struct rtc **r = res;
> +	if (!r || !*r) {
> +		WARN_ON(!r || !*r);
> +		return 0;
> +	}

Use

	if (WARN_ON(!r || !*r))
		return 0;

However, if anyone ever reports this warning you will be sad because
you won't know which condition triggered it.  So a more useful approach
is

	if (WARN_ON(!r))
		return 0;
	if (WARN_ON(!*r))
		return 0;

> +	return *r == data;
> +}
> +
> +/**
> + * devm_rtc_device_register - resource managed rtc_device_register()
> + * @name: the name of the device
> + * @dev: the device to register
> + * @ops: the rtc operations structure
> + * @owner: the module owner
> + *
> + * Managed rtc_device_register(). The rtc_device returned from this function
> + * are automatically freed on driver detach. See rtc_device_register()
> + * for more information.
> + */

I think the name is inappropriate.  devm functions start with "devm"
and rtc functions start with "rtc" and this code is a part of rtc core,
not a part of devm core.  Hence it should be something like
"rtc_devm_device_register".

> +struct rtc_device *devm_rtc_device_register(const char *name,
> +					struct device *dev,
> +					const struct rtc_class_ops *ops,
> +					struct module *owner)
> +{
> +	struct rtc_device **ptr, *rtc;
> +
> +	ptr = devres_alloc(devm_rtc_device_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rtc = rtc_device_register(name, dev, ops, owner);
> +	if (!IS_ERR(rtc)) {
> +		*ptr = rtc;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return rtc;
> +}
> +EXPORT_SYMBOL_GPL(devm_rtc_device_register);

So this function will return an ERR_PTR on error.  Please add a
description of the return value into the kerneldoc.


> +/**
> + * devm_rtc_device_unregister - resource managed devm_rtc_device_unregister()
> + * @dev: the device to unregister
> + * @rtc: the RTC class device to unregister
> + *
> + * Deallocated a rtc allocated with devm_rtc_device_register(). Normally this
> + * function will not need to be called and the resource management code will
> + * ensure that the resource is freed.
> + */
> +void devm_rtc_device_unregister(struct device *dev, struct rtc_device *rtc)
> +{
> +	int rc;
> +
> +	rc = devres_release(dev, devm_rtc_device_release,
> +				devm_rtc_device_match, rtc);
> +	if (rc != 0)
> +		WARN_ON(rc);

	WARN_ON(!rc);

> +}
> +EXPORT_SYMBOL_GPL(devm_rtc_device_unregister);
> +
>
> ...
>

Re: [PATCH] rtc: add devm_rtc_device_{register,unregister}()

['Tejun Heo']

On Mon, Feb 25, 2013 at 04:35:53PM +0900, Jingoo Han wrote:
> +static void devm_rtc_device_release(struct device *dev, void *res)
> +{
> +	struct rtc_device *rtc = *(struct rtc_device **)res;
> +
> +	rtc_device_unregister(rtc);
> +}
> +
> +static int devm_rtc_device_match(struct device *dev, void *res, void *data)
> +{
> +	struct rtc **r = res;
> +	if (!r || !*r) {
> +		WARN_ON(!r || !*r);
> +		return 0;
> +	}

	if (WARN_ON(!r || !*r))
		return 0;

But why do we need this at all?  Other match functions don't need it.
Is something different about devm_rtc?

Thanks.

-- 
tejun

Re: [PATCH] rtc: add devm_rtc_device_{register,unregister}()

['Tejun Heo']

On Mon, Feb 25, 2013 at 02:29:57PM -0800, Andrew Morton wrote:
> > + * Managed rtc_device_register(). The rtc_device returned from this function
> > + * are automatically freed on driver detach. See rtc_device_register()
> > + * for more information.
> > + */
> 
> I think the name is inappropriate.  devm functions start with "devm"
> and rtc functions start with "rtc" and this code is a part of rtc core,
> not a part of devm core.  Hence it should be something like
> "rtc_devm_device_register".

I think devm_rtc_ is better.  All other devm interface functions put
devm_ prefix first and I think it's better to make devm_ more
pronounced as it denotes a somewhat subtle difference in behavior.

Thanks.

-- 
tejun

Re: [PATCH] rtc: add devm_rtc_device_{register,unregister}()

[Andrew Morton]

On Mon, 25 Feb 2013 15:33:45 -0800
"'Tejun Heo'" <tj@kernel.org> wrote:

> On Mon, Feb 25, 2013 at 02:29:57PM -0800, Andrew Morton wrote:
> > > + * Managed rtc_device_register(). The rtc_device returned from this function
> > > + * are automatically freed on driver detach. See rtc_device_register()
> > > + * for more information.
> > > + */
> > 
> > I think the name is inappropriate.  devm functions start with "devm"
> > and rtc functions start with "rtc" and this code is a part of rtc core,
> > not a part of devm core.  Hence it should be something like
> > "rtc_devm_device_register".
> 
> I think devm_rtc_ is better.  All other devm interface functions put
> devm_ prefix first

makes no sense.  Whatever.