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@xxxxxxxxxxxxx>
> Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
> Cc: Jonathan Cameron <jic23@xxxxxxxxx>
>
> ---
> 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@xxxxxxxxx>");
> 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@xxxxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
