lkml.org 
[lkml]   [2009]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 2/2] als: add unique device-ids to the als device class
Hi Amit,

On Thu, 26 Nov 2009 14:06:26 +0200, Amit Kucheria wrote:
> Other devices classes such as hwmon and input class handle assignment of
> unique device-ids inside the core functions instead of pushing it out to
> individual drivers. This reduces code duplication and resulting bugs.
>
> Signed-off-by: Amit Kucheria <amit.kucheria@verdurent.com>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Jonathan Cameron <jic23@cam.ac.uk>
>
> ---
> drivers/als/als_sys.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/als/als_sys.c b/drivers/als/als_sys.c
> index e1d6395..aa15ad8 100644
> --- a/drivers/als/als_sys.c
> +++ b/drivers/als/als_sys.c
> @@ -26,22 +26,55 @@
> #include <linux/device.h>
> #include <linux/err.h>
> #include <linux/kdev_t.h>
> +#include <linux/idr.h>
>
> MODULE_AUTHOR("Zhang Rui <rui.zhang@intel.com>");
> MODULE_DESCRIPTION("Ambient Light Sensor sysfs/class support");
> MODULE_LICENSE("GPL");
>
> +#define ALS_ID_PREFIX "als"
> +#define ALS_ID_FORMAT ALS_ID_PREFIX "%d"
> +
> static struct class *als_class;
>
> +static DEFINE_IDR(als_idr);
> +static DEFINE_SPINLOCK(idr_lock);
> +
> /**
> * als_device_register - register a new Ambient Light Sensor class device
> * @parent: the device to register.
> *
> * Returns the pointer to the new device
> */
> -struct device *als_device_register(struct device *dev, char *name)
> +struct device *als_device_register(struct device *dev)

This is a public function but you forgot to update
include/linux/als_sys.h accordingly. This will let the build succeed
but crashes will happen at run time. Fix below.

> {
> - return device_create(als_class, dev, MKDEV(0, 0), NULL, name);
> + int id, err;
> + struct device *alsdev;
> +
> +again:
> + if (unlikely(idr_pre_get(&als_idr, GFP_KERNEL) == 0))
> + return ERR_PTR(-ENOMEM);
> +
> + spin_lock(&idr_lock);
> + err = idr_get_new(&als_idr, NULL, &id);
> + spin_unlock(&idr_lock);
> +
> + if (unlikely(err == -EAGAIN))
> + goto again;
> + else if (unlikely(err))
> + return ERR_PTR(err);
> +
> + id = id & MAX_ID_MASK;
> + alsdev = device_create(als_class, dev, MKDEV(0, 0), NULL,
> + ALS_ID_FORMAT, id);
> +
> + if (IS_ERR(alsdev)) {
> + spin_lock(&idr_lock);
> + idr_remove(&als_idr, id);
> + spin_unlock(&idr_lock);
> + }
> +
> + return alsdev;
> }
> EXPORT_SYMBOL(als_device_register);
>
> @@ -51,7 +84,16 @@ EXPORT_SYMBOL(als_device_register);
> */
> void als_device_unregister(struct device *dev)
> {
> - device_unregister(dev);
> + int id;
> +
> + if (likely(sscanf(dev_name(dev), ALS_ID_FORMAT, &id) == 1)) {
> + device_unregister(dev);
> + spin_lock(&idr_lock);
> + idr_remove(&als_idr, id);
> + spin_unlock(&idr_lock);
> + } else
> + dev_dbg(dev->parent,
> + "als_device_unregister() failed: bad class ID!\n");
> }
> EXPORT_SYMBOL(als_device_unregister);
>

Other than that I am very happy with this change, which kills 46 lines
of code in the tsl2550 driver (and virtually the same in every other
light sensor driver.) Please merge the following fix:

--- linux-2.6.32-rc8.orig/include/linux/als_sys.h 2009-11-26 15:32:38.000000000 +0100
+++ linux-2.6.32-rc8/include/linux/als_sys.h 2009-11-26 15:44:08.000000000 +0100
@@ -29,7 +29,7 @@
#define ALS_ILLUMINANCE_MIN 0
#define ALS_ILLUMINANCE_MAX -1

-struct device *als_device_register(struct device *dev, char *name);
+struct device *als_device_register(struct device *dev);
void als_device_unregister(struct device *dev);

#endif /* __ALS_SYS_H__ */

And then you can add:

Acked-by: Jean Delvare <khali@linux-fr.org>

That being said... If we want user-space to know what device is there,
we may want to still let drivers pass a name string to
als_device_register() and let the ALS core create a "name" sysfs
attribute returning the string in question. This would be much lighter
(for individual drivers) than the previous situation, as the string in
question would be a constant (e.g. "TSL2550".) Opinions?

--
Jean Delvare


\
 
 \ /
  Last update: 2009-11-26 16:09    [W:0.735 / U:1.384 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site