| Message ID | 008c01ce13bf$8d2cd550$a7867ff0$%han@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
Hello, On Tue, Feb 26, 2013 at 10:21:06AM +0900, Jingoo Han wrote: > 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> Looks okay to me but > +static int devm_rtc_device_match(struct device *dev, void *res, void *data) > +{ > + struct rtc **r = res; > + if (WARN_ON(!r || !*r)) > + return 0; > + > + return *r == data; > +} again, why do we need the WARN_ON() at all given that other devm_*_match() don't do that and the only way to get NULL there would be bug in devm_rtc*() code itself rather than its users? Thanks.
On Mon, Feb 25, 2013 at 05:23:17PM -0800, 'Tejun Heo' wrote: > again, why do we need the WARN_ON() at all given that other > devm_*_match() don't do that and the only way to get NULL there would > be bug in devm_rtc*() code itself rather than its users? Ughh... I see that you probably copied the chunk from clk / regulator implementation. Let's please not copy those. Input validation via WARN_ON()s is nice when the interface is taking input from outside or the code in question is complex / fragile. Here, the code involved is like 20 lines and self-contained. Thanks.
On Tuesday, February 26, 2013 10:30 AM, Tejun Heo wrote: > > On Mon, Feb 25, 2013 at 05:23:17PM -0800, 'Tejun Heo' wrote: > > again, why do we need the WARN_ON() at all given that other > > devm_*_match() don't do that and the only way to get NULL there would > > be bug in devm_rtc*() code itself rather than its users? > > Ughh... I see that you probably copied the chunk from clk / regulator > implementation. Let's please not copy those. Input validation via > WARN_ON()s is nice when the interface is taking input from outside or > the code in question is complex / fragile. Here, the code involved is > like 20 lines and self-contained. OK, I see. Thank you for your comment. I will send v3 patch soon :) Best regards, Jingoo Han > > Thanks. > > -- > tejun
On Tue, Feb 26, 2013 at 10:21:06AM +0900, Jingoo Han wrote: > +/** > + * 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) Why do you need a separate function? You can add a flag to struct rtc_device so it knows whether it is devm-managed or not and behave accordingly. And then you can do #define devm_rtc_device_unregister rtc_device_unregister Thanks.
On Wednesday, February 27, 2013 7:33 AM, Dmitry Torokhov wrote: > > On Tue, Feb 26, 2013 at 10:21:06AM +0900, Jingoo Han wrote: > > +/** > > + * 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) > > Why do you need a separate function? You can add a flag to struct rtc_device > so it knows whether it is devm-managed or not and behave accordingly. > And then you can do > > #define devm_rtc_device_unregister rtc_device_unregister Um, I don't prefer that way using additional a flag. Also, most of other devm_* functions are using a separate functions such as devm_pwm_put(), devm_regulator_put(),devm_usb_put_phy(), etc. > > Thanks. > > -- > Dmitry
diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c index 9b742d3..31496bc 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 (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 + * + * @return a struct rtc on success, or an ERR_PTR on error + * + * 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); + 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);
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> --- Changes since v1: - Simplified 'if' statements using WARN_ON() - Added a description of the return value of devm_rtc_device_register() drivers/rtc/class.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/rtc.h | 6 ++++ 2 files changed, 78 insertions(+), 0 deletions(-)