[PATCH 0/2] Introduding the Ambient Light Sensor (ALS) class

[PATCH 0/2] Introduding the Ambient Light Sensor (ALS) class

[Amit Kucheria]

Introduce a new class of devices to handle ambient light sensors. Currently
only one sysfs inteface, 'illuminance' is introduced. More will be added as
drivers are ported to use this new class.

Amit Kucheria (1):
  als: add unique device-ids to the als device class

Zhang Rui (1):
  introduce ALS sysfs class

 Documentation/ABI/testing/sysfs-class-als |    9 ++
 MAINTAINERS                               |    6 ++
 drivers/Kconfig                           |    2 +
 drivers/Makefile                          |    1 +
 drivers/als/Kconfig                       |   10 +++
 drivers/als/Makefile                      |    5 +
 drivers/als/als_sys.c                     |  116 +++++++++++++++++++++++++++++
 include/linux/als_sys.h                   |   35 +++++++++
 8 files changed, 184 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-als
 create mode 100644 drivers/als/Kconfig
 create mode 100644 drivers/als/Makefile
 create mode 100644 drivers/als/als_sys.c
 create mode 100644 include/linux/als_sys.h

[PATCH 1/2] introduce ALS sysfs class

[Amit Kucheria]

From: Zhang Rui <rui.zhang@intel.com>

This is a refresh of the ALS sysfs class driver.

ALS sysfs class device provides a standard sysfs interface
for Ambient Light Sensor devices.

Only one sysfs I/F is introduced currently.
/sys/class/als/xxx/illuminance:
	indicates the amount of light incident upon a specified surface area.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
Acked-by: Amit Kucheria <amit.kucheria@verdurent.com>
---
 Documentation/ABI/testing/sysfs-class-als |    9 ++++
 MAINTAINERS                               |    6 ++
 drivers/Kconfig                           |    2 +
 drivers/Makefile                          |    1 +
 drivers/als/Kconfig                       |   10 ++++
 drivers/als/Makefile                      |    5 ++
 drivers/als/als_sys.c                     |   74 +++++++++++++++++++++++++++++
 include/linux/als_sys.h                   |   35 ++++++++++++++
 8 files changed, 142 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-als
 create mode 100644 drivers/als/Kconfig
 create mode 100644 drivers/als/Makefile
 create mode 100644 drivers/als/als_sys.c
 create mode 100644 include/linux/als_sys.h

diff --git a/Documentation/ABI/testing/sysfs-class-als b/Documentation/ABI/testing/sysfs-class-als
new file mode 100644
index 0000000..d3b33f3
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-als
@@ -0,0 +1,9 @@
+What:		/sys/class/als/.../illuminance
+Date:		Sep. 2009
+KernelVersion:	2.6.32
+Contact:	Zhang Rui <rui.zhang@intel.com>
+Description:	Current Ambient Light Illuminance reported by
+		native ALS driver
+		Unit: lux (lumens per square meter)
+		RO
+
diff --git a/MAINTAINERS b/MAINTAINERS
index c824b4d..0894a1c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -409,6 +409,12 @@ S:	Maintained for 2.4; PCI support for 2.6.
 L:	linux-alpha@vger.kernel.org
 F:	arch/alpha/
 
+AMBIENT LIGHT SENSOR
+M:	Zhang Rui <rui.zhang@intel.com>
+S:	Supported
+F:	include/linux/als_sys.h
+F:	drivers/als/
+
 AMD GEODE CS5536 USB DEVICE CONTROLLER DRIVER
 M:	Thomas Dahlmann <dahlmann.thomas@arcor.de>
 L:	linux-geode@lists.infradead.org (moderated for non-subscribers)
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 48bbdbe..67cf884 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -62,6 +62,8 @@ source "drivers/power/Kconfig"
 
 source "drivers/hwmon/Kconfig"
 
+source "drivers/als/Kconfig"
+
 source "drivers/thermal/Kconfig"
 
 source "drivers/watchdog/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 6ee53c7..ecb6d5d 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_PPS)		+= pps/
 obj-$(CONFIG_W1)		+= w1/
 obj-$(CONFIG_POWER_SUPPLY)	+= power/
 obj-$(CONFIG_HWMON)		+= hwmon/
+obj-$(CONFIG_ALS)		+= als/
 obj-$(CONFIG_THERMAL)		+= thermal/
 obj-$(CONFIG_WATCHDOG)		+= watchdog/
 obj-$(CONFIG_PHONE)		+= telephony/
diff --git a/drivers/als/Kconfig b/drivers/als/Kconfig
new file mode 100644
index 0000000..200c52b
--- /dev/null
+++ b/drivers/als/Kconfig
@@ -0,0 +1,10 @@
+#
+# Ambient Light Sensor sysfs device configuration
+#
+
+menuconfig ALS
+	tristate "Ambient Light Sensor sysfs device"
+	help
+	  This framework provides a generic sysfs I/F for Ambient Light
+	  Sensor devices.
+	  If you want this support, you should say Y or M here.
diff --git a/drivers/als/Makefile b/drivers/als/Makefile
new file mode 100644
index 0000000..a527197
--- /dev/null
+++ b/drivers/als/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for sensor chip drivers.
+#
+
+obj-$(CONFIG_ALS)		+= als_sys.o
diff --git a/drivers/als/als_sys.c b/drivers/als/als_sys.c
new file mode 100644
index 0000000..e1d6395
--- /dev/null
+++ b/drivers/als/als_sys.c
@@ -0,0 +1,74 @@
+/*
+ *  als_sys.c - Ambient Light Sensor Sysfs support.
+ *
+ *  Copyright (C) 2009 Intel Corp
+ *  Copyright (C) 2009 Zhang Rui <rui.zhang@intel.com>
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kdev_t.h>
+
+MODULE_AUTHOR("Zhang Rui <rui.zhang@intel.com>");
+MODULE_DESCRIPTION("Ambient Light Sensor sysfs/class support");
+MODULE_LICENSE("GPL");
+
+static struct class *als_class;
+
+/**
+ * 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)
+{
+	return device_create(als_class, dev, MKDEV(0, 0), NULL, name);
+}
+EXPORT_SYMBOL(als_device_register);
+
+/**
+ * als_device_unregister - removes the registered ALS class device
+ * @dev:	the class device to destroy.
+ */
+void als_device_unregister(struct device *dev)
+{
+	device_unregister(dev);
+}
+EXPORT_SYMBOL(als_device_unregister);
+
+static int __init als_init(void)
+{
+	als_class = class_create(THIS_MODULE, "als");
+	if (IS_ERR(als_class)) {
+		printk(KERN_ERR "als_sys.c: couldn't create sysfs class\n");
+		return PTR_ERR(als_class);
+	}
+	return 0;
+}
+
+static void __exit als_exit(void)
+{
+	class_destroy(als_class);
+}
+
+subsys_initcall(als_init);
+module_exit(als_exit);
diff --git a/include/linux/als_sys.h b/include/linux/als_sys.h
new file mode 100644
index 0000000..500f300
--- /dev/null
+++ b/include/linux/als_sys.h
@@ -0,0 +1,35 @@
+/*
+ *  als_sys.h
+ *
+ *  Copyright (C) 2009  Intel Corp
+ *  Copyright (C) 2009  Zhang Rui <rui.zhang@intel.com>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#ifndef __ALS_SYS_H__
+#define __ALS_SYS_H__
+
+#include <linux/device.h>
+
+#define ALS_ILLUMINANCE_MIN 0
+#define ALS_ILLUMINANCE_MAX -1
+
+struct device *als_device_register(struct device *dev, char *name);
+void als_device_unregister(struct device *dev);
+
+#endif /* __ALS_SYS_H__ */
-- 
1.6.3.3

[PATCH 2/2] als: add unique device-ids to the als device class

[Amit Kucheria]

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)
 {
-	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);
 
-- 
1.6.3.3

Re: [PATCH 1/2] introduce ALS sysfs class

[Jonathan Cameron]

Hi Amit,

Sorry, NAK from me for this. We still need to get the registration
code switched to handling allocation of numbers etc in here rather
than in the drivers. If needed I can propose a patch to do that but
it will Saturday at the earliest before I get to it.

For references on this see for example Jean's comments on the tsl2550
port http://lkml.org/lkml/2009/10/10/127
and also the thread leading to
http://lkml.org/lkml/2009/11/10/63

Everything else is fine.

Jonathan
> 
> This is a refresh of the ALS sysfs class driver.
> 
> ALS sysfs class device provides a standard sysfs interface
> for Ambient Light Sensor devices.
> 
> Only one sysfs I/F is introduced currently.
> /sys/class/als/xxx/illuminance:
> 	indicates the amount of light incident upon a specified surface area.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> Acked-by: Amit Kucheria <amit.kucheria@verdurent.com>
> ---
>  Documentation/ABI/testing/sysfs-class-als |    9 ++++
>  MAINTAINERS                               |    6 ++
>  drivers/Kconfig                           |    2 +
>  drivers/Makefile                          |    1 +
>  drivers/als/Kconfig                       |   10 ++++
>  drivers/als/Makefile                      |    5 ++
>  drivers/als/als_sys.c                     |   74 +++++++++++++++++++++++++++++
>  include/linux/als_sys.h                   |   35 ++++++++++++++
>  8 files changed, 142 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-als
>  create mode 100644 drivers/als/Kconfig
>  create mode 100644 drivers/als/Makefile
>  create mode 100644 drivers/als/als_sys.c
>  create mode 100644 include/linux/als_sys.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-als b/Documentation/ABI/testing/sysfs-class-als
> new file mode 100644
> index 0000000..d3b33f3
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-als
> @@ -0,0 +1,9 @@
> +What:		/sys/class/als/.../illuminance
> +Date:		Sep. 2009
> +KernelVersion:	2.6.32
> +Contact:	Zhang Rui <rui.zhang@intel.com>
> +Description:	Current Ambient Light Illuminance reported by
> +		native ALS driver
> +		Unit: lux (lumens per square meter)
> +		RO
> +
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c824b4d..0894a1c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -409,6 +409,12 @@ S:	Maintained for 2.4; PCI support for 2.6.
>  L:	linux-alpha@vger.kernel.org
>  F:	arch/alpha/
>  
> +AMBIENT LIGHT SENSOR
> +M:	Zhang Rui <rui.zhang@intel.com>
> +S:	Supported
> +F:	include/linux/als_sys.h
> +F:	drivers/als/
> +
>  AMD GEODE CS5536 USB DEVICE CONTROLLER DRIVER
>  M:	Thomas Dahlmann <dahlmann.thomas@arcor.de>
>  L:	linux-geode@lists.infradead.org (moderated for non-subscribers)
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 48bbdbe..67cf884 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -62,6 +62,8 @@ source "drivers/power/Kconfig"
>  
>  source "drivers/hwmon/Kconfig"
>  
> +source "drivers/als/Kconfig"
> +
>  source "drivers/thermal/Kconfig"
>  
>  source "drivers/watchdog/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 6ee53c7..ecb6d5d 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -77,6 +77,7 @@ obj-$(CONFIG_PPS)		+= pps/
>  obj-$(CONFIG_W1)		+= w1/
>  obj-$(CONFIG_POWER_SUPPLY)	+= power/
>  obj-$(CONFIG_HWMON)		+= hwmon/
> +obj-$(CONFIG_ALS)		+= als/
>  obj-$(CONFIG_THERMAL)		+= thermal/
>  obj-$(CONFIG_WATCHDOG)		+= watchdog/
>  obj-$(CONFIG_PHONE)		+= telephony/
> diff --git a/drivers/als/Kconfig b/drivers/als/Kconfig
> new file mode 100644
> index 0000000..200c52b
> --- /dev/null
> +++ b/drivers/als/Kconfig
> @@ -0,0 +1,10 @@
> +#
> +# Ambient Light Sensor sysfs device configuration
> +#
> +
> +menuconfig ALS
> +	tristate "Ambient Light Sensor sysfs device"
> +	help
> +	  This framework provides a generic sysfs I/F for Ambient Light
> +	  Sensor devices.
> +	  If you want this support, you should say Y or M here.
> diff --git a/drivers/als/Makefile b/drivers/als/Makefile
> new file mode 100644
> index 0000000..a527197
> --- /dev/null
> +++ b/drivers/als/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for sensor chip drivers.
> +#
> +
> +obj-$(CONFIG_ALS)		+= als_sys.o
> diff --git a/drivers/als/als_sys.c b/drivers/als/als_sys.c
> new file mode 100644
> index 0000000..e1d6395
> --- /dev/null
> +++ b/drivers/als/als_sys.c
> @@ -0,0 +1,74 @@
> +/*
> + *  als_sys.c - Ambient Light Sensor Sysfs support.
> + *
> + *  Copyright (C) 2009 Intel Corp
> + *  Copyright (C) 2009 Zhang Rui <rui.zhang@intel.com>
> + *
> + *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kdev_t.h>
> +
> +MODULE_AUTHOR("Zhang Rui <rui.zhang@intel.com>");
> +MODULE_DESCRIPTION("Ambient Light Sensor sysfs/class support");
> +MODULE_LICENSE("GPL");
> +
> +static struct class *als_class;
> +
> +/**
> + * 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)
> +{
> +	return device_create(als_class, dev, MKDEV(0, 0), NULL, name);
> +}
> +EXPORT_SYMBOL(als_device_register);
> +
> +/**
> + * als_device_unregister - removes the registered ALS class device
> + * @dev:	the class device to destroy.
> + */
> +void als_device_unregister(struct device *dev)
> +{
> +	device_unregister(dev);
> +}
> +EXPORT_SYMBOL(als_device_unregister);
> +
> +static int __init als_init(void)
> +{
> +	als_class = class_create(THIS_MODULE, "als");
> +	if (IS_ERR(als_class)) {
> +		printk(KERN_ERR "als_sys.c: couldn't create sysfs class\n");
> +		return PTR_ERR(als_class);
> +	}
> +	return 0;
> +}
> +
> +static void __exit als_exit(void)
> +{
> +	class_destroy(als_class);
> +}
> +
> +subsys_initcall(als_init);
> +module_exit(als_exit);
> diff --git a/include/linux/als_sys.h b/include/linux/als_sys.h
> new file mode 100644
> index 0000000..500f300
> --- /dev/null
> +++ b/include/linux/als_sys.h
> @@ -0,0 +1,35 @@
> +/*
> + *  als_sys.h
> + *
> + *  Copyright (C) 2009  Intel Corp
> + *  Copyright (C) 2009  Zhang Rui <rui.zhang@intel.com>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +#ifndef __ALS_SYS_H__
> +#define __ALS_SYS_H__
> +
> +#include <linux/device.h>
> +
> +#define ALS_ILLUMINANCE_MIN 0
> +#define ALS_ILLUMINANCE_MAX -1
> +
> +struct device *als_device_register(struct device *dev, char *name);
> +void als_device_unregister(struct device *dev);
> +
> +#endif /* __ALS_SYS_H__ */

Re: [PATCH 1/2] introduce ALS sysfs class

[Jonathan Cameron]

Sorry scratch this comment. I've now actually read the second patch!

Gah, not enough coffee this morning.

Jonathan
> Hi Amit,
> 
> Sorry, NAK from me for this. We still need to get the registration
> code switched to handling allocation of numbers etc in here rather
> than in the drivers. If needed I can propose a patch to do that but
> it will Saturday at the earliest before I get to it.
> 
> For references on this see for example Jean's comments on the tsl2550
> port http://lkml.org/lkml/2009/10/10/127
> and also the thread leading to
> http://lkml.org/lkml/2009/11/10/63
> 
> Everything else is fine.
> 
> Jonathan
>> This is a refresh of the ALS sysfs class driver.
>>
>> ALS sysfs class device provides a standard sysfs interface
>> for Ambient Light Sensor devices.
>>
>> Only one sysfs I/F is introduced currently.
>> /sys/class/als/xxx/illuminance:
>> 	indicates the amount of light incident upon a specified surface area.
>>
>> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>> Acked-by: Amit Kucheria <amit.kucheria@verdurent.com>
>> ---
>>  Documentation/ABI/testing/sysfs-class-als |    9 ++++
>>  MAINTAINERS                               |    6 ++
>>  drivers/Kconfig                           |    2 +
>>  drivers/Makefile                          |    1 +
>>  drivers/als/Kconfig                       |   10 ++++
>>  drivers/als/Makefile                      |    5 ++
>>  drivers/als/als_sys.c                     |   74 +++++++++++++++++++++++++++++
>>  include/linux/als_sys.h                   |   35 ++++++++++++++
>>  8 files changed, 142 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/ABI/testing/sysfs-class-als
>>  create mode 100644 drivers/als/Kconfig
>>  create mode 100644 drivers/als/Makefile
>>  create mode 100644 drivers/als/als_sys.c
>>  create mode 100644 include/linux/als_sys.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-als b/Documentation/ABI/testing/sysfs-class-als
>> new file mode 100644
>> index 0000000..d3b33f3
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-als
>> @@ -0,0 +1,9 @@
>> +What:		/sys/class/als/.../illuminance
>> +Date:		Sep. 2009
>> +KernelVersion:	2.6.32
>> +Contact:	Zhang Rui <rui.zhang@intel.com>
>> +Description:	Current Ambient Light Illuminance reported by
>> +		native ALS driver
>> +		Unit: lux (lumens per square meter)
>> +		RO
>> +
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c824b4d..0894a1c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -409,6 +409,12 @@ S:	Maintained for 2.4; PCI support for 2.6.
>>  L:	linux-alpha@vger.kernel.org
>>  F:	arch/alpha/
>>  
>> +AMBIENT LIGHT SENSOR
>> +M:	Zhang Rui <rui.zhang@intel.com>
>> +S:	Supported
>> +F:	include/linux/als_sys.h
>> +F:	drivers/als/
>> +
>>  AMD GEODE CS5536 USB DEVICE CONTROLLER DRIVER
>>  M:	Thomas Dahlmann <dahlmann.thomas@arcor.de>
>>  L:	linux-geode@lists.infradead.org (moderated for non-subscribers)
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index 48bbdbe..67cf884 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -62,6 +62,8 @@ source "drivers/power/Kconfig"
>>  
>>  source "drivers/hwmon/Kconfig"
>>  
>> +source "drivers/als/Kconfig"
>> +
>>  source "drivers/thermal/Kconfig"
>>  
>>  source "drivers/watchdog/Kconfig"
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 6ee53c7..ecb6d5d 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -77,6 +77,7 @@ obj-$(CONFIG_PPS)		+= pps/
>>  obj-$(CONFIG_W1)		+= w1/
>>  obj-$(CONFIG_POWER_SUPPLY)	+= power/
>>  obj-$(CONFIG_HWMON)		+= hwmon/
>> +obj-$(CONFIG_ALS)		+= als/
>>  obj-$(CONFIG_THERMAL)		+= thermal/
>>  obj-$(CONFIG_WATCHDOG)		+= watchdog/
>>  obj-$(CONFIG_PHONE)		+= telephony/
>> diff --git a/drivers/als/Kconfig b/drivers/als/Kconfig
>> new file mode 100644
>> index 0000000..200c52b
>> --- /dev/null
>> +++ b/drivers/als/Kconfig
>> @@ -0,0 +1,10 @@
>> +#
>> +# Ambient Light Sensor sysfs device configuration
>> +#
>> +
>> +menuconfig ALS
>> +	tristate "Ambient Light Sensor sysfs device"
>> +	help
>> +	  This framework provides a generic sysfs I/F for Ambient Light
>> +	  Sensor devices.
>> +	  If you want this support, you should say Y or M here.
>> diff --git a/drivers/als/Makefile b/drivers/als/Makefile
>> new file mode 100644
>> index 0000000..a527197
>> --- /dev/null
>> +++ b/drivers/als/Makefile
>> @@ -0,0 +1,5 @@
>> +#
>> +# Makefile for sensor chip drivers.
>> +#
>> +
>> +obj-$(CONFIG_ALS)		+= als_sys.o
>> diff --git a/drivers/als/als_sys.c b/drivers/als/als_sys.c
>> new file mode 100644
>> index 0000000..e1d6395
>> --- /dev/null
>> +++ b/drivers/als/als_sys.c
>> @@ -0,0 +1,74 @@
>> +/*
>> + *  als_sys.c - Ambient Light Sensor Sysfs support.
>> + *
>> + *  Copyright (C) 2009 Intel Corp
>> + *  Copyright (C) 2009 Zhang Rui <rui.zhang@intel.com>
>> + *
>> + *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + *
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation; version 2 of the License.
>> + *
>> + *  This program is distributed in the hope that it will be useful, but
>> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + *  General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License along
>> + *  with this program; if not, write to the Free Software Foundation, Inc.,
>> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/kdev_t.h>
>> +
>> +MODULE_AUTHOR("Zhang Rui <rui.zhang@intel.com>");
>> +MODULE_DESCRIPTION("Ambient Light Sensor sysfs/class support");
>> +MODULE_LICENSE("GPL");
>> +
>> +static struct class *als_class;
>> +
>> +/**
>> + * 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)
>> +{
>> +	return device_create(als_class, dev, MKDEV(0, 0), NULL, name);
>> +}
>> +EXPORT_SYMBOL(als_device_register);
>> +
>> +/**
>> + * als_device_unregister - removes the registered ALS class device
>> + * @dev:	the class device to destroy.
>> + */
>> +void als_device_unregister(struct device *dev)
>> +{
>> +	device_unregister(dev);
>> +}
>> +EXPORT_SYMBOL(als_device_unregister);
>> +
>> +static int __init als_init(void)
>> +{
>> +	als_class = class_create(THIS_MODULE, "als");
>> +	if (IS_ERR(als_class)) {
>> +		printk(KERN_ERR "als_sys.c: couldn't create sysfs class\n");
>> +		return PTR_ERR(als_class);
>> +	}
>> +	return 0;
>> +}
>> +
>> +static void __exit als_exit(void)
>> +{
>> +	class_destroy(als_class);
>> +}
>> +
>> +subsys_initcall(als_init);
>> +module_exit(als_exit);
>> diff --git a/include/linux/als_sys.h b/include/linux/als_sys.h
>> new file mode 100644
>> index 0000000..500f300
>> --- /dev/null
>> +++ b/include/linux/als_sys.h
>> @@ -0,0 +1,35 @@
>> +/*
>> + *  als_sys.h
>> + *
>> + *  Copyright (C) 2009  Intel Corp
>> + *  Copyright (C) 2009  Zhang Rui <rui.zhang@intel.com>
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation; version 2 of the License.
>> + *
>> + *  This program is distributed in the hope that it will be useful, but
>> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + *  General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License along
>> + *  with this program; if not, write to the Free Software Foundation, Inc.,
>> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + */
>> +
>> +#ifndef __ALS_SYS_H__
>> +#define __ALS_SYS_H__
>> +
>> +#include <linux/device.h>
>> +
>> +#define ALS_ILLUMINANCE_MIN 0
>> +#define ALS_ILLUMINANCE_MAX -1
>> +
>> +struct device *als_device_register(struct device *dev, char *name);
>> +void als_device_unregister(struct device *dev);
>> +
>> +#endif /* __ALS_SYS_H__ */
> 
> 

Re: [PATCH 2/2] als: add unique device-ids to the als device class

[Jean Delvare]

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

Re: [PATCH 2/2] als: add unique device-ids to the als device class

[Jonathan Cameron]

Jean Delvare wrote:
> 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?
> 
Makes sense given we want all drivers to support some form of identification.
We could do it by stating they will all have that attribute, but given it's constant
will save repetition to put it in the driver. Conversely it might complicate the handling
of subsequent attribute_groups so I'd probably favour adding relevant documentation lines
and leaving it up to the drivers to implement this attribute.

Thus we'd require (within reason) all drivers to have illuminance0 and name.

Hence,
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>

Thanks for doing this!

Jonathan

Re: [PATCH 2/2] als: add unique device-ids to the als device class

[Greg KH]

On Thu, Nov 26, 2009 at 05:19:13PM +0000, Jonathan Cameron wrote:
> > 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?
> > 
> Makes sense given we want all drivers to support some form of identification.
> We could do it by stating they will all have that attribute, but given it's constant
> will save repetition to put it in the driver. Conversely it might complicate the handling
> of subsequent attribute_groups so I'd probably favour adding relevant documentation lines
> and leaving it up to the drivers to implement this attribute.
> 
> Thus we'd require (within reason) all drivers to have illuminance0 and name.

Why have a name attribute when you can just use the name of the device
itself instead?  Isn't that what it is there for?

confused,

greg k-h

Re: [PATCH 2/2] als: add unique device-ids to the als device class

[Jonathan Cameron]

Greg KH wrote:
> On Thu, Nov 26, 2009 at 05:19:13PM +0000, Jonathan Cameron wrote:
>>> 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?
>>>
>> Makes sense given we want all drivers to support some form of identification.
>> We could do it by stating they will all have that attribute, but given it's constant
>> will save repetition to put it in the driver. Conversely it might complicate the handling
>> of subsequent attribute_groups so I'd probably favour adding relevant documentation lines
>> and leaving it up to the drivers to implement this attribute.
>>
>> Thus we'd require (within reason) all drivers to have illuminance0 and name.
> 
> Why have a name attribute when you can just use the name of the device
> itself instead?  Isn't that what it is there for?
Could do, though I'm not entirely sure all bus types are implementing a name
attribute (I may be wrong, but I don't think spi does for example though it might
have gone in with the recent device table stuff).  We could just specify that it
should be present for the device.

Jonathan

Re: [PATCH 2/2] als: add unique device-ids to the als device class

[Jean Delvare]

On Thu, 26 Nov 2009 10:06:46 -0800, Greg KH wrote:
> On Thu, Nov 26, 2009 at 05:19:13PM +0000, Jonathan Cameron wrote:
> > > 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?
> > > 
> > Makes sense given we want all drivers to support some form of identification.
> > We could do it by stating they will all have that attribute, but given it's constant
> > will save repetition to put it in the driver. Conversely it might complicate the handling
> > of subsequent attribute_groups so I'd probably favour adding relevant documentation lines
> > and leaving it up to the drivers to implement this attribute.
> > 
> > Thus we'd require (within reason) all drivers to have illuminance0 and name.
> 
> Why have a name attribute when you can just use the name of the device
> itself instead?  Isn't that what it is there for?

Can you please clarify what you call "the name of the device"?

Can you also please explain what problem you think we are trying to
solve?

> confused,

Me even more ;)

-- 
Jean Delvare

[PATCH 2/2] als: add unique device-ids to the als device class

[Amit Kucheria]

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.

V2 - Updated the header to reflect the change is signature of
als_device_register(). Thanks to Jean for the review.

V1 - Add capability to als core to create unique ids

Signed-off-by: Amit Kucheria <amit.kucheria@verdurent.com>
Acked-by: Jean Delvare <khali@linux-fr.org>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/als/als_sys.c   |   48 ++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/als_sys.h |    2 +-
 2 files changed, 46 insertions(+), 4 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)
 {
-	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);
 
diff --git a/include/linux/als_sys.h b/include/linux/als_sys.h
index 500f300..41793f7 100644
--- a/include/linux/als_sys.h
+++ b/include/linux/als_sys.h
@@ -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__ */
-- 
1.6.3.3

Re: [PATCH 2/2] als: add unique device-ids to the als device class

[Greg KH]

On Thu, Nov 26, 2009 at 06:40:04PM +0000, Jonathan Cameron wrote:
> Greg KH wrote:
> > On Thu, Nov 26, 2009 at 05:19:13PM +0000, Jonathan Cameron wrote:
> >>> 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?
> >>>
> >> Makes sense given we want all drivers to support some form of identification.
> >> We could do it by stating they will all have that attribute, but given it's constant
> >> will save repetition to put it in the driver. Conversely it might complicate the handling
> >> of subsequent attribute_groups so I'd probably favour adding relevant documentation lines
> >> and leaving it up to the drivers to implement this attribute.
> >>
> >> Thus we'd require (within reason) all drivers to have illuminance0 and name.
> > 
> > Why have a name attribute when you can just use the name of the device
> > itself instead?  Isn't that what it is there for?
> Could do, though I'm not entirely sure all bus types are implementing a name
> attribute (I may be wrong, but I don't think spi does for example though it might
> have gone in with the recent device table stuff).  We could just specify that it
> should be present for the device.

ah, sorry, I was thinking of the name of the actual device, which is the
bus id here.

Nevermind, I'll go back to feeling stupid...

greg k-h